JsonNumber implementation and tests

[kasiabulat]

I implemented JsonNumber methods using string to keep the actual value (and added more tests for it). I also added more equals-related methods to JsonNumber, JsonBoolean and JsonString.

cc: @joperezr @bartonjs @ericstj @ahsonkhan @terrajobst

[joperezr]

question: will this roundtrip when using exponential notation?

[joperezr]

Would it also make sense to implement IComperable?

[ahsonkhan]

cc @JamesNK

[JamesNK]

cc @JamesNK

I don't have any context here. Could you point me at a spec/proposal/issue that discusses what these APIs are for

[ahsonkhan]

I don't have any context here. Could you point me at a spec/proposal/issue that discusses what these APIs are for

This is still in the initial prototype/design phase and maybe @joperezr / @kasiabulat have some doc/spec they can point you to (if we don't have one yet, it might be good to write one up at some point).

We are starting to fill in implementation details and then go through the API review/design in corefx within the next month or so (before @kasiabulat's internship ends), so there is no issue yet.

Essentially, we are looking to create a modifiable, dictionary-backed Json DOM API to complement the readonly JsonDocument/JsonElement, with type hierarchy consisting of the base type JsonNode (rather than a single union type JsonElement).
The user should be able to build up a structured in-memory representation of the JSON payload and modify it. That includes, remove, add, and update (similar to Newtonsoft.Json's JToken).

The WritableJsonApiTests.cs contains a bunch of usage scenarios that we want to enable with these types. For example:

corefx/src/System.Text.Json/tests/WritableJsonApiTests.cs

Lines 19 to 42 in 25109c3

	public static void TestCreatingJsonObject()
	{
	var developer = new JsonObject
	{
	{ "name", "Kasia" },
	{ "age", 22 },
	{ "is developer", true },
	{ "null property", (JsonNode) null }
	};
	Assert.IsType<JsonString>(developer["name"]);
	var developerNameCasted = developer["name"] as JsonString;
	Assert.Equal("Kasia", developerNameCasted.Value);
	Assert.IsType<JsonNumber>(developer["age"]);
	var developerAgeCasted = developer["age"] as JsonNumber;
	Assert.Equal(22, developerAgeCasted.GetInt32());
	Assert.IsType<JsonBoolean>(developer["is developer"]);
	var isDeveloperCasted = developer["is developer"] as JsonBoolean;
	Assert.Equal(true, isDeveloperCasted.Value);
	Assert.Null(developer["null property"]);
	}

I cc'd you primarily to get your eyes on it, and to get your insights early (if applicable), especially to avoid common pitfalls or design mistakes that you may have encountered (and to get your general opinion on the design). If you see any big rocks or have significant concerns, it would be great to discuss and address them before we have gone too deep with the current approach.

[kasiabulat]

I changed methods into one-liners and added more tests, but still need to add some for Get/TryGet for different types (correct and incorrect examples). Personally, I think it should be possible to GetInt after setting something that fits into int and not possible if it doesn't etc. And in current implementation I forget the information of what exact type was stored.

[ahsonkhan]

Consider adding "throw argumentexception" validation to a copy of JsonWriterHelper.ValidateNumber (with Debug.Asserts changed to actual checks).

The length check can go there as well:
https://github.com/dotnet/corefx/blob/9d6729a940952dcc14dc8683218684f4da29198a/src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.cs#L137-L148

[ahsonkhan]

I would suspect these strings to generally be small (say < 256 characters). Consider instead, stackalloc a span and use the Span<byte> overload of Encoding.UTF8.GetBytes to avoid the allocation on .NET Core 3.0 (and use the arraypool for larger length strings).
https://github.com/dotnet/corefx/blob/ef5b086fc5f134e3244d0bca10d70db8055fa1c6/src/System.Runtime/ref/System.Runtime.cs#L7683

OR

Validate the value as chars.

Something to consider to optimize the implementation a bit (but that can be deferred to later, if you like).

[ahsonkhan]

Question: Would escaped characters be supported? Especially since the Utf8JsonWriter escapes the + character by default (since it uses JavascriptEncoder.Default).

Console.WriteLine(double.Parse("25.1e+1")); // valid - 251
Console.WriteLine(double.Parse("25.1e\\u002B1")); // Fails

[kasiabulat]

I added decimal support and addressed other minor issues. There are still some questions left to discuss, but I am merging this PR and I will create one to the main corefx fork.

[kasiabulat]

I created the issue for my project: https://github.com/dotnet/corefx/issues/39922