Skip to content
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.

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

Add JsonObject ordering and extension data support #51717

Merged
merged 4 commits into from
Apr 28, 2021

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Apr 22, 2021

As a follow-up to #51025 add the features:

  • Ability to have JsonObject extension properties via:
    [JsonExtensionData] public JsonObject ExtensionData {get; set;}
    • Done in a linker-friendly way so the source-gen effort will be able to trim JsonNode out.
    • Today only (I)Dictionary<string, JsonElement> is supported as an extension property. Since JsonElement is read-only, it can't be effectively used for modifying extension data, so supporting JsonObject will enable that scenario.
  • Deterministic ordering semantics so subsequent Remove() and Add() do not change the ordering of existing items. New items are appended. It is possible now to add methods like "AddBefore(node)" and "AddAfter(node)" if desired.
    • Internally the items are held by a List<> but once a threshold is hit an additional Dictionary<> is created for fast lookup by property name. Once the dictionary is created, both are kept in sync. Enumeration is always done by the list for deterministic ordering.

Minor items:

  • Converter refactoring of the System.Object converter to detect whether to use JsonElement or JsonNode. Previously, the JsonNode converter handled this and the System.Object converter only knew about JsonElement. That could cause issues for users that ask for the converter for System.Object.
  • Additional test coverage.
  • Misc feedback from original PR.

Fixes #51572
(no longer using an IDictionary field; instead Dictionary<> used).

@steveharter steveharter added this to the 6.0.0 milestone Apr 22, 2021
@steveharter steveharter self-assigned this Apr 22, 2021
@steveharter steveharter requested a review from jozkee as a code owner April 22, 2021 22:47
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Apr 22, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

As a follow-up to #51025 add the features:

  • Ability to have extension properties by of type JsonObject of the form:
    [JsonExtensionData] public JsonObject ExtensionData {get; set;}
    • Done in a linker-friendly way so the source-gen effort will be able to trim JsonNode out.
  • Deterministic ordering semantics so subsequent Remove() and Add() do not change the ordering of existing items. New items are appended. It is possible now to add methods like "AddBefore(node)" and "AddAfter(node)" if desired.
    • Internally the items are held by a List<> but once a threshold is hit an additional Dictionary<> is created for fast lookup by property name. Once the dictionary is created, both are kept in sync. Enumeration is always done by the list for deterministic ordering.

Minor items:

  • Converter refactoring of the System.Object converter to detect whether to use JsonElement or JsonNode. Previously, the JsonNode converter handled this and the System.Object converter only knew about JsonElement. That could cause issues for users that ask for the converter for System.Object.
  • Additional test coverage.
  • Misc feedback from original PR.

Fixes #51572
(no longer using an IDictionary field; instead Dictionary<> used).

Author: steveharter
Assignees: steveharter
Labels:

area-System.Text.Json

Milestone: 6.0.0

Comment on lines 268 to 271
if (_dictionary != null)
{
return _dictionary.ContainsKey(propertyName);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't seem like the dictionary can be null if this method is called.

Copy link
Member

Choose a reason for hiding this comment

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

Also, if this cannot be changed into an assert perhaps rewrite to return _dictionary?.ContainsKey(propertyName);

Copy link
Member Author

Choose a reason for hiding this comment

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

_dictionary can be null here whenever the threshold isn't hit yet. E.g.:

JsonObject jObject = JsonNode.Parse("{}").AsObject();
bool exists = jObject.ContainsKey("FOO");

@steveharter steveharter merged commit a0c1fca into dotnet:main Apr 28, 2021
@steveharter steveharter deleted the NodeFeatures branch April 28, 2021 15:02
@ghost ghost locked as resolved and limited conversation to collaborators May 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Blazor WASM] - Investigate 6% size regression in System.Collections.dll.br
4 participants