Skip to content

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

Merged
Ilya (iSazonov) merged 1 commit into
PowerShell:masterfrom
adamgauthier:convert-from-json-metadata
Jul 25, 2018
Merged

Convert Newtonsoft.Json metadata properties in ConvertFrom-Json#7308
Ilya (iSazonov) merged 1 commit into
PowerShell:masterfrom
adamgauthier:convert-from-json-metadata

Conversation

@adamgauthier

Copy link
Copy Markdown
Contributor

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

@dantraMSFT

Copy link
Copy Markdown
Contributor

Adam Gauthier (@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 Adam Gauthier (adamgauthier) changed the title Ignore Newtonsoft.Json metadata properties in ConvertFrom-Json Convert Newtonsoft.Json metadata properties in ConvertFrom-Json Jul 18, 2018
@adamgauthier

Adam Gauthier (adamgauthier) commented Jul 18, 2018

Copy link
Copy Markdown
Contributor Author

Dan Travison (@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

Copy link
Copy Markdown
Collaborator

Adam Gauthier (@louistio) Seems the header should be "Don't convert ..."

@iSazonov Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Leave a comment

@markekraus Mark Kraus (markekraus) left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

Ilya (@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
Copy Markdown
Collaborator

Dan Travison (@dantraMSFT) Do you have any comments?

@iSazonov
Ilya (iSazonov) merged commit e34554c into PowerShell:master Jul 25, 2018
@iSazonov

Copy link
Copy Markdown
Collaborator

Adam Gauthier (@louistio) Thanks for your contribution!

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
…Shell#7308)

 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

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