dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.98k stars 4.66k forks source link

JsonElement implement standard interfaces #39384

Closed IS4Code closed 2 years ago

IS4Code commented 4 years ago

Background and Motivation

JsonElement, for example when provided for an object property, cannot be easily manipulated generally, without using its concrete type. It cannot be converted to any specific type without using methods like GetInt32 on the unboxed value, cannot be cloned and cannot be compared. This complicates code that uses it and harms generality, since it requires the code to reference JSON classes where previously it relied on declarative representation of JSON objects modelled via plain .NET types and it didn't need to touch any JSON deserialization at all (all being done behind the scenes).

The interfaces that should be implemented are IConvertible, ICloneable, and IEquatable<JsonElement>. IConvertible and ICloneable can be only explicitly implemented.

Proposed API

public readonly partial struct JsonElement : IConvertible, ICloneable, IEquatable<JsonElement>

Usage

The IConvertible implementation should behave like if the original raw string had been deserialized to the provided type (so even types like string[] or IDictionary<string, int> should be supported), and additionally, I think if the element type is a string, it should attempt to convert the string to the target type. This allows it to interact better with methods like Convert.ToInt32 etc.

Since the type already has a Clone method, it makes sense to also implement ICloneable despite it having limited uses. Still, if the value of a particular JsonElement could "expire" (as indicated by the description of its Clone method), it needs a generally applicable mechanism for cloning it by value. I am aware it boxes the result, but any user of ICloneable must be prepared for that anyway.

As for IEquatable<JsonElement>, it should probably compare numbers, booleans, null and strings by value, and arrays and objects by reference, and of course operator overloads and overriding Equals(object) is also expected.

ghost commented 2 years ago

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of the experimental issue cleanup initiative we are currently trialing in a limited number of areas. Please share any feedback you might have in the linked issue.

IS4Code commented 2 years ago

eiriktsarpalis commented 2 years ago

The IConvertible implementation should behave like if the original raw string had been deserialized to the provided type (so even types like string[] or IDictionary<string, int> should be supported), and additionally, I think if the element type is a string, it should attempt to convert the string to the target type. This allows it to interact better with methods like Convert.ToInt32 etc.

Why is this needed? JsonElement already exposes Get* methods for most primitive types. If you take a look at the types implementing IConvertible you'll see that it's restricted to numeric type. JsonElement is not a numeric type, but instead represents a node in the JSON DOM.

Since the type already has a Clone method, it makes sense to also implement ICloneable despite it having limited uses. Still, if the value of a particular JsonElement could "expire" (as indicated by the description of its Clone method), it needs a generally applicable mechanism for cloning it by value. I am aware it boxes the result, but any user of ICloneable must be prepared for that anyway.

Sure, but what is the use case for implementing ICloneable? My understanding is that we don't implement this interface in most new types cc @bartonjs

As for IEquatable, it should probably compare numbers, booleans, null and strings by value, and arrays and objects by reference, and of course operator overloads and overriding Equals(object) is also expected.

My understanding is that there is no universally accepted notion of equality for JSON trees. There is no such thing as "reference equality" for JSON objects or arrays stored in a UTF-8 encoded buffer. JsonElement exposes the ValueEquals methods that constrain themselves to character-by-character equality comparison: they are intentionally named ValueEquals for that reason. As such, I don't think that JsonElement should implement IEquatable.

ghost commented 2 years ago

This issue has been marked with the api-needs-work label. This may suggest that the proposal requires further refinement before it can be considered for API review. Please refer to our API review guidelines for a detailed description of the process.

When ready to submit an amended proposal, please ensure that the original post in this issue has been updated, following the API proposal template and examples as provided in the guidelines.

IS4Code commented 2 years ago

It's been a while, so I might not recall precisely the cases where these additions were useful, but here's some backstory.

System.Text.Json works as an "official" lightweight alternative to Json.NET, not as robust but sufficiently viable. The issue is that it is impossible to treat both JsonElement and JToken interchangeably, for example to be able to switch between the two engines when browsing JSON objects.

This would be a step towards reuniting them, to be able to produce a code that takes an object and expects to be able to extract something meaningful from it.

This works extremely well for Json.NET: JToken implements ICloneable, IEnumerable, even IDynamicMetaObjectProvider. JValue implements IEquatable, IFormattable, IComparable, IConvertible etc.

In comparison to this, JsonElement has the Get* methods for conversion, the Enumerate* methods for traversing, the Clone method, an indexer, so which standard interfaces that support these methods as well does it implement? None. If someone handed me the source code of JsonElement for evaluation, I'd tell them to learn about abstraction, or provide a very good reason why none of the interfaces were considered.

I believe this address the concerns generally, but to be specific:

Why is this needed? JsonElement already exposes Get* methods for most primitive types.

It feels weird to have a type that has lots of these standard conversion methods, yet nothing that works generically.

If you take a look at the types implementing IConvertible you'll see that it's restricted to numeric type. JsonElement is not a numeric type, but instead represents a node in the JSON DOM.

I don't see any mention in the documentation that would indicate such a restriction. It "defines methods that convert the value of the implementing reference or value type to a common language runtime type that has an equivalent value." To me, a JsonElement that is essentially a string, boolean, or numeric, qualifies. Additionally, bool, char, DateTime, and Enum are only somewhat numeric and DBNull and string are not numeric at all, yet they implement this interface as well. Additionally, the authors of Json.NET saw it appropriate to implement it too, so there is some precedent for it.

If the case of IConvertible is not convincing enough, perhaps another standard mechanism can be used, like TypeConverter support.

Sure, but what is the use case for implementing ICloneable? My understanding is that we don't implement this interface in most new types cc @bartonjs

Not sure why it is so, but in this case, it feels justified. As I said, it is the only way to prevent the instance to expire due to its JsonDocument. ICloneable works as a lightweight alternative to in-process serialization and deserialization. XmlNode also works the same way. Moreover, it feels very weird to have a case of a value type that should be immutable but actually isn't. The present of ICloneable could give that away.

My understanding is that there is no universally accepted notion of equality for JSON trees. There is no such thing as "reference equality" for JSON objects or arrays stored in a UTF-8 encoded buffer. JsonElement exposes the ValueEquals methods that constrain themselves to character-by-character equality comparison: they are intentionally named ValueEquals for that reason. As such, I don't think that JsonElement should implement IEquatable.

Yet there is a universally accepted notion of equality for JSON values (well perhaps not for fractional numbers), which is something that could be reflected in a way that has been the standard since .NET 1.

A .NET string is equal to itself and all other strings that represent the same sequence of char values (equivalence). An array is only equal to itself (identity). JsonElement can represent either of these, but why can't the same hold?

And of course there is a reference equality (identity) for JSON objects/arrays – if it represents the same position in the same buffer (JsonDocument), then it's (obviously) the same thing. Otherwise if it's False, True, Null, String or Number, I feels any sufficiently experienced programmer would tell you how equivalence works for these values.

There is also IStructuralEquatable which might be used for arrays, but I didn't intend to advocate for that.


I agree that these interfaces might not be something that should be used when dealing with actual (statically-typed) instances of JsonElement, but why not have an explicit implementations just in case? There are a lot of cases like this in the BCL.

And yes, using a value type with explicit interface implementations is fine:

void DoSomething<TValue>(TValue val) where TValue : IEquatable<TValue>, IConvertible, ICloneable

JValue works fine here, so why not JsonElement?

IS4Code commented 2 years ago

My goal for this idea was to make the JsonElement type manageable in dynamic/generic contexts, since without these interfaces it is completely opaque to any such code, despite behaving like a collection, or a value potentially equivalent to one expressed as a standard BCL type. Adding these (explicitly implemented) interfaces with almost trivial implementations is already a big step towards that. Interfaces on value types are useful too.

eiriktsarpalis commented 2 years ago

The issue is that it is impossible to treat both JsonElement and JToken interchangeably

JsonElement and JToken represent radically different approaches to representing a JSON DOM, and as such JsonElement was never intended as a drop-in replacement for JToken. STJ v6 will ship with JsonNode which models a writeable DOM that is more compatible with the functionality offered by JToken. That being said, it is also not intended as a drop-in replacement for JToken. In fact some features were deliberately cut, e.g. dynamic support since we regard this to be an "archived component": it stays in the shared framework for backward compatbility but otherwise we won't be adding it new types that we ship. I would say that the same applies to old interfaces like ICloneable and IConvertible.

Yet there is a universally accepted notion of equality for JSON values (well perhaps not for fractional numbers), which is something that could be reflected in a way that has been the standard since .NET 1.

Sure but JsonElement representations do not restrict themselves to values. Not all types need to be equatable, and JsonElement certainly doesn't need to implement IEquatable. If absolutely necessary you can always author a custom IEqualityComparer<JsonElement> implementation that contains the equality semantics of your liking.

An array is only equal to itself (identity)

I'm not sure what that means. If you're referring to reference equality I should point out that JsonElement does not encapsulate any array instances, it is merely wrapping an UTF8-encoded JSON document.

bartonjs commented 2 years ago

Sure, but what is the use case for implementing ICloneable? My understanding is that we don't implement this interface in most new types

We have some very clear guidance here:

eiriktsarpalis commented 2 years ago

Per feedback we don't believe any of the suggestions meet the bar for inclusion to JsonElement. Thank you for posting the sugesstions.