JsonObject implementation and tests

[kasiabulat]

I added JsonObject implementation and tests.

addresses: #39922
cc: @joperezr @bartonjs @ericstj @ahsonkhan @terrajobst @JamesNK @stephentoub

[ahsonkhan]

Some initial feedback.

[ahsonkhan]

I wouldn't have expected DuplicatePropertyNameHandling to show up within JsonObject's API surface, but rather within the options passed in to the type that creates JsonObjects from some json payload (say JsonDocumentOptions).

Do we think there will be a need for this setting when creating a JsonObject itself via the ctor? What scenario would it be used for?

[JamesNK]

+1

People who are manually creating a JsonObject in code can choose whether to use o.Add("name", "value") or o["name"] = "value". That choice is basically the equivalent of duplicating property handling.

[kasiabulat]

So actually, my Add behaves as specified in DuplicatePropertyNameHandling, to be consistent. So while indexer will always replace values, Add will either throw / replace or ignore (defaultly would behave just as indexer, but I can change the default behavior to exceptions).

Maybe we could have those options both in JsonObject and passed to parsing method? I think people might want to specify the behavior they want also when creating objects from C# - what this API is designed for?

[ahsonkhan]

I think people might want to specify the behavior they want also when creating objects from C# - what this API is designed for?

Can you give an example/sample where you think it would be useful to have DuplicatePropertyNameHandling when creating JsonObject?

[kasiabulat]

For example something containing interactions with user?

var properties = new JsonObject(DuplicatePropertyNameHandling.Error);

while(PropertiesReady())
{
    string userProvidedPropertyName = Console.ReadLine();
    string userProvidedPropertyValue = Console.ReadLine();
    try
    {
        properties.Add(userProvidedPropertyName, userProvidedPropertyValue);
    }
    catch (ArgumentException)
    {
        Console.WriteLine(string.Format("Property name `{0}` already exists. Please provide properties with unique names.", userProvidedPropertyName));
    }
}
Mailbox.SendData(properties.AsJsonElement());

[JamesNK]

IMO JsonObject should implement IDictionary<string, JsonNode> and JsonArray should implement IList<JsonNode>

[kasiabulat]

JsonArray is planned to implement IList, but we decided not to make JsonObject implement IDictionary, because it would be then possible to pass it for methods that accepts a dictionary, which we didn't find useful and desired. @bartonjs ?

[joperezr]

@kasiabulat are there any methods from IDictionary that we would be missing if we wanted to implement that? Are there any Sort methods in IDictionary?

[kasiabulat]

There are no extension methods accepting IDictionary / IList instead of IEnumerable. LINQ queries work faster with IList and that's why making JsonArray implement it seemed valuable. No queries work faster with IDictionary - that's also why we thought implementing it by JsonObject would be unnecessary.

To implement IDictionary we are lacking:

• IsReadOnly
• Keys (but we have PropertyNames),
• Values (but we have PropertyValues)
• Clear()
• CopyTo(T[], Int32)
• Contains(T)
• ContainsKey(TKey) (but we have ContainsProperty)
• TryGetValue(TKey, TValue) (but we have (Try)GetPropertyValue).

[JamesNK]

FYI use explicit implementation for the dictionary APIs people will likely never use - https://softwareengineering.stackexchange.com/questions/136319/whats-the-difference-between-implementing-an-interface-explicitly-or-implicitly

[bartonjs]

I think my point was that unless there were compelling extension methods or other specific use cases that it probably didn't need to implement the IDictionary interface. e.g. that one should be able to say "It implements IDictionary and IReadOnlyDictionary because [reason]" where "[reason]" is something more concrete than "because it's logically a dictionary" or "because it already had all the right methods".

Same result, different presentation: If no one will ever use the fact that it's an I(ReadOnly)Dictionary then it didn't need to assert the interface.

[JamesNK]

Amoungst other benefits, IDictionary allows a collection initializer to be used:

var o = new JsonObject
{
    ["Name"] = "James",
    ["Radness"] = 98,
}

[bartonjs]

Amoungst other benefits, IDictionary allows a collection initializer to be used:

That doesn't require IDictionary. Just IEnumerable + suitable Add methods, which it already has 😄

[JamesNK]

+1

People who are manually creating a JsonObject in code can choose whether to use o.Add("name", "value") or o["name"] = "value". That choice is basically the equivalent of duplicating property handling.

[ahsonkhan]

Should this be returning a JsonNode instead, rather than throwing? What can I do if I want to get the value of a property that happens to be a number, string, array, etc.?

[kasiabulat]

There's a method GetPropertyValue as well, working for all types of nodes, GetJsonObjectPropertyValue is just accelerating method for accessing nested objects. We were thinking of adding GetJsonArrayPropertyValue as well.

[ahsonkhan]

This isn't generic ReadOnlySpan but rather a specific T (in this case char).

Suggested change

	/// <param name="propertyValue"><see cref="ReadOnlySpan{T}"/> value of the property to add.</param>
	/// <param name="propertyValue"><see cref="ReadOnlySpan{char}"/> value of the property to add.</param>

@mairaw, @rpetrusha - this is how we format the xml comments for a specific T on a generic type, correct?

[kasiabulat]

With "ReadOnlySpan{char}" I got the error "Type parameter declaration must be an identifier not a type."

[ahsonkhan]

I knew I had written type-specific generic within xml comments before. It should be uppercase.

Suggested change

	/// <param name="propertyValue"><see cref="ReadOnlySpan{T}"/> value of the property to add.</param>
	/// <param name="propertyValue"><see cref="ReadOnlySpan{Char}"/> value of the property to add.</param>

corefx/src/Common/src/CoreLib/System/Buffers/StandardFormat.cs

Lines 69 to 71 in 36f1a74

	/// <summary>
	/// Converts a <see cref="ReadOnlySpan{Char}"/> into a StandardFormat
	/// </summary>

[kasiabulat]

Ok, thanks, I'll fix it in next PR.