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

JsonObject should support property order manipulation #102932

Closed
Tracked by #100159
eiriktsarpalis opened this issue May 31, 2024 · 11 comments · Fixed by #103645
Closed
Tracked by #100159

JsonObject should support property order manipulation #102932

eiriktsarpalis opened this issue May 31, 2024 · 11 comments · Fixed by #103645
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@eiriktsarpalis
Copy link
Member

The JsonObject class implements IDictionary<string, JsonNode?>, however as of today the type does permit any manipulation of the order of its properties (even though order is being tracked internally). While this is consistent with the semantics of JSON where order of properties is not significant, it is often the case that one wants to arrange the order out of human readability concerns. For example, it is common for JSON schema docs to place the $schema and $id properties at the start of the object.

We should update JsonObject so that properties can be inserted at specific locations.

API Proposal

namespace System.Text.Json.Nodes;

public sealed partial class JsonObject : IDictionary<string, JsonNode?>
{
+    public int IndexOf(string key, out JsonNode? value);
+    public void Insert(int index, string key, JsonNode? value);
+    public void RemoveAt(int index);
}

API Usage

// Adds or moves the $id property to the start of the object
JsonObject schema = ...;
switch (schema.IndexOf("$id", out JsonNode? idValue))
{
    case < 0: // $id property missing
       idValue = (JsonNode)"https://example.com/schema";
       schema.Insert(0, "$id", idValue);
       break;

    case 0: // $id property already at the start of the object
        break; 

    case int index: // $id exists but not at the start of the object
        schema.RemoveAt(index);
        schema.Insert(0, "$id", idValue);
}

Alternative Designs

We can't have JsonObject implement IList<KeyValuePair<string, JsonNode?> (at least not without explicit implementation) since the base JsonNode class already exposes an indexer for ints with different signature and semantics.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 31, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label May 31, 2024
@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone May 31, 2024
@eiriktsarpalis eiriktsarpalis self-assigned this May 31, 2024
@eiriktsarpalis eiriktsarpalis added the api-ready-for-review API is ready for review, it is NOT ready for implementation label May 31, 2024
@gregsdennis
Copy link
Contributor

This is good in order to support serialization scenarios.

Note that JSON Schema is set up to process these keywords first, regardless of where they appear in a document.

@eiriktsarpalis eiriktsarpalis added the blocking Marks issues that we want to fast track in order to unblock other important work label May 31, 2024
@mwadams
Copy link
Contributor

mwadams commented May 31, 2024

I feel this is the wrong place to support this.

For me, it is a serialization concern not a JSON Object model concern.

In some ways it is a thin-end-of-the-wedge problem; as soon as we start to suggest that JSON objects have a notion of "property ordering" outside of the serialized representation, people will start depending on it.

We already see subtle bugs from people depending on the fact that Dictionary<K,V> has remarkably stable ordering and I worry that this enters the same realm.

@eiriktsarpalis
Copy link
Member Author

Unlike Dictionary, the JsonOject implementation uses a well-defined notion of property ordering. This proposal is simply a matter of making this available to users, should they have need for it.

@mwadams
Copy link
Contributor

mwadams commented Jun 1, 2024

So JsonObject isn't a model of a JSON object, it is intrinsically a model of a serialized representation of a JSON object. This lack of separation of concerns is where my discomfort lies.

I would have less discomfort if that was front-and-centre - i.e. this is not a JsonObject it is really a SerializedJsonObject.

(And the fact it is sometimes not-yet-serialized is a laziness optimization 😀)

@eiriktsarpalis
Copy link
Member Author

So JsonObject isn't a model of a JSON object, it is intrinsically a model of a serialized representation of a JSON object. This lack of separation of concerns is where my discomfort lies.

Every model of a JSON object necessarily encapsulates a notion of property ordering, whether intentional or incidental. I wouldn't consider that a departure from the spirit of the JSON specification though; it is not that objects should not be orderable but rather that different permutations of the same object must be semantically equivalent. The latter should still be the case (cf. the implementation of the DeepEquals method etc).

@mwadams
Copy link
Contributor

mwadams commented Jun 1, 2024

This is true. And also true of e.g. dictionary enumeration. But we are careful to conceptually separate "regular dictionary" and "ordered dictionary" in the type system.

The conceptual change with this API is that we are transitioning from the former to the latter.

Just for clarity - I'm not wholly opposed, just "uncomfortable".

@eiriktsarpalis
Copy link
Member Author

Like I said, unlike Dictionary JsonObject guarantees order of enumeration. It is very much implemented as an ordered dictionary.

@terrajobst
Copy link
Member

terrajobst commented Jun 11, 2024

Video

  • We should make the int-based indexer on JsonNode work to return the JsonNode
    • We don't need to out the property from IndexOf then
  • We should also implement IList<KeyValuePair<string, JsonNode?>>
namespace System.Text.Json.Nodes;

public sealed partial class JsonObject : IDictionary<string, JsonNode?>, IList<KeyValuePair<string, JsonNode?>>
{
    public int IndexOf(string key);
    public void Insert(int index, string key, JsonNode? value);
    public void RemoveAt(int index);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 11, 2024
@KennethHoff
Copy link

We should also implement IList<KeyValuePair<string, JsonNode?>>

Based on the discussion, wouldn't it be clearer to say: "We should also explicitly implement IList<KeyValuePair<string, JsonNode?>>" to make it clear that it's not public API

@eiriktsarpalis
Copy link
Member Author

In addition to the APIs as approved #102932 (comment) I propose the inclusion of the following methods:

public sealed partial class JsonObject : IDictionary<string, JsonNode?>, IList<KeyValuePair<string, JsonNode?>>
{
    public KeyValuePair<string, JsonNode?> GetAt(int index);
    public void SetAt(int index, JsonValue? value);
    public void SetAt(int index, string propertyName, JsonValue? value);
}

These APIs reflect the API surface in the newly added OrderedDictionary and solve a use case that would otherwise require making an explicit cast of the object to an IList instance.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants