Skip to content

Convert Newtonsoft.Json metadata properties in ConvertFrom-Json #7308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
iSazonov merged 1 commit into from
Jul 25, 2018
Merged

Convert Newtonsoft.Json metadata properties in ConvertFrom-Json #7308

iSazonov merged 1 commit into from
Jul 25, 2018

Conversation

@adamgauthier
Copy link
Contributor

@adamgauthier adamgauthier commented Jul 18, 2018

PR Summary

Use MetadataPropertyHandling.Ignore in JsonSerializerSettings when Deserializing instead of the default (MetadataPropertyHandling.Default). Documentation on this option can be found here and here. Basically Newtonsoft.Json specific metadata properties like $id/$type/$ref are by default read and used by Newtonsoft.Json to deserialize into types, use the same instance for different properties, etc.

ConvertFrom-Json deserializes into the generic type PSCustomObject, I think it doesn't make sense that these specific properties would not be converted as they are part of the json object. This restrict people from using these properties if they are not using Newtonsoft.Json, but even more likely, it restrict them from manipulating the entire raw json data of an object serialized with the library (which is probably what they want to do, otherwise they would be using the library directly to deserialize).

I don't know how "breaking" of a change this is considered to be, I would assume not many people rely on the current behavior, but it is changing with this PR.

Fixes #7307

PR Checklist

@adamgauthier adamgauthier force-pushed the convert-from-json-metadata branch from e1850b4 to 778fcf2 Compare July 18, 2018 01:55
@dantraMSFT
Copy link
Contributor

dantraMSFT commented Jul 18, 2018

@louistio Would you mind rewording the title and the description for this PR. The associated issue states that the metaproperties are not being converted yet the PR title appears to state that they should be ignored.

Thanks,

@adamgauthier adamgauthier changed the title Ignore Newtonsoft.Json metadata properties in ConvertFrom-Json Convert Newtonsoft.Json metadata properties in ConvertFrom-Json Jul 18, 2018
@adamgauthier
Copy link
Contributor Author

adamgauthier commented Jul 18, 2018 •
edited
Loading

@dantraMSFT Yes my bad, it is confusing, this PR stated to ignore them as in to tell the serializer not to do anything special with them, not to ignore them as in not convert them. I changed the title now. (I would also amend the commit message but I know you guys don't like rewriting history while a review is active)

@iSazonov iSazonov requested a review from markekraus July 20, 2018 03:51
@iSazonov iSazonov self-assigned this Jul 20, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 20, 2018

@louistio Seems the header should be "Don't convert ..."

iSazonov
iSazonov approved these changes Jul 20, 2018
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a comment

markekraus
markekraus approved these changes Jul 20, 2018
View reviewed changes
Copy link
Contributor

@markekraus markekraus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments are not blocking. Otherwise LGTM

@markekraus
Copy link
Contributor

markekraus commented Jul 20, 2018

Upon reviewing the issue, I do not believe this to be a breaking change as this appears to be a regression from 5.1. The behavior in 5.1 was to serialize the meta properties as object properties which is now broekn in 6.0. This PR corrects that regression and IMO is not a breaking change.

@adamgauthier
Copy link
Contributor Author

adamgauthier commented Jul 20, 2018

@iSazonov I think there is confusion about the naming. The issue is that some values are not converted from json to PSCustomObject, because Newtonsoft.Json reads them and thinks they are metadata properties only useful for deserialization. This change makes Newtonsoft.Json "ignore" those properties so they are converted correctly from json to PSCustomObject. I'm not sure how naming this PR "Don't convert" would make much sense.

@iSazonov
Copy link
Collaborator

iSazonov commented Jul 24, 2018

@dantraMSFT Do you have any comments?

@iSazonov iSazonov merged commit e34554c into PowerShell:master Jul 25, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 25, 2018

@louistio Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@dantraMSFT dantraMSFT Awaiting requested review from dantraMSFT

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

+1 more reviewer

@markekraus markekraus markekraus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ConvertFrom-Json does not convert Newtonsoft.Json specific metadata properties in root object

4 participants

@adamgauthier @dantraMSFT @iSazonov @markekraus