Add extension data support to Json serializer#37578
Add extension data support to Json serializer#37578steveharter wants to merge 11 commits intodotnet:masterfrom
Conversation
This partially addresses https://github.com/dotnet/corefx/issues/36024. For the intial version, only `Dictionary<string, [some concrete type]>` is supported.
steveharter
changed the title
|
/azp run corefx-ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
cc @bartonjs |
cc @MarcoRossignoli since this may affect your PR: #37586 |
|
|
||
| string keyName = Encoding.UTF8.GetString(unescapedPropertyName); | ||
|
|
||
| // Currently we don't apply any naming policy. If we do, we'd have to pass it onto the JsonDocument. |
There was a problem hiding this comment.
So there are two different things that can have a naming policy applied to it.
- The dictionary key
- The JSON content
Json.NET has an extension point for changing the dictionary key. On the other hand the Json.NET serializer doesn't modify the extension data content. An approach I took with the serializer is JToken (i.e. JsonElement) is already JSON, and the serializer always writes it unchanged.
There was a problem hiding this comment.
On the other hand the Json.NET serializer doesn't modify the extension data content. An approach I took with the serializer is JToken (i.e. JsonElement) is already JSON, and the serializer always writes it unchanged.
I am fine with this and it would remove the need to pass in the serializer options to the JsonDocument.
@rynowak, @KrzysztofCwalina - what do you think? Is it necessary for the "overflow" data to honor the serializer options?
@JamesNK, what about the read/deserialize side of things or is that not relevant here?
There was a problem hiding this comment.
Here's the settings not on the reader\writer but could be applied:
- Naming on serialize (not in reader options, plus would be a new option on JsonSerializerOptions)
- IgnoreNulls (not in reader options)
Here's the settings on reader\writer options that could applied (they should be applied IMO but currently aren't):
- AllowTrailingCommas
- MaxDepth
- ReadCommentHandling
- WriteIndented
There was a problem hiding this comment.
For now, pending future discussion, I will pass in the JsonReaderOptions\JsonWriterOptions like we do for other calls into the reader\writer. I will not add any support for property naming or IgnoreNull at this time.
In the future, can we make sure that the PRs are isolated to a single change? Refactoring/bug fixes should be in separate PRs to make it easier to review each independently. Reviewing the |
The other changes were related except for some of the "ignore" changes - however even that the new code I added needed to use ignore and refactor a bit to share that ignore logic with the new dictionary and existing object code. For the indent, I try to do the indent\moving code on the second commit, which I did partially here. |
steveharter
force-pushed
the
Overflow
branch
8 times, most recently
from
8a297d8 to
946c161
Compare
|
Closing and replacing this with #37690 due to issues with corrupt branch history. |
6 participants
Adds the overflow or data extension support via the new JsonExtensionDataAttribute
Other changes:
Open issues:
JsonSerializerOptionsto theJsonDocumentand have theJsonDocumentuse theJsonSerializerOptionsreader options (pretty printing, comment support, etc), plus two possible serializer-only options:ExtensionDataPropertyNamingproperty.