[For Discussion] Move JsonData to Core

[annelo-msft]

Context

• Addresses [Core] Introduce JsonData type in Azure.Core #30717
• Builds on discussions in Make JsonData Immutable #31203

Requirements

The main work needed, given that we have the prototype, is to:

• Remove mutating APIs.
• Decide how arrays work (see above)
  There is a test showing this capability in the old PR: CanAccessArrayValues
• Decide how casts work, e.g. var date = (DateTime)json; or cast to custom model: var model = (FooModel)json.Foo
  There is a proposal regarding an approach to enabling this in this commit. It suffers from the issue that it uses JsonSerializer.Deserialize<T> to do the conversion between JsonData and T, and we know that not all of our Azure SDK models are serializable via STJ.
• Decide how roundtripping would work. It will not be very easy/usable, but it should be possible.

Suggested approach (tracked separately by #31943):

// ***
// Round-trip: JsonData and Anonymous types
// ***
Response current = await client.GetKeyValueAsync("FontColor");
RequestContent updated = RequestContent.Merge(current, patch:
    new
    {
        value = "updated value";
    }
);
dynamic keyValue = new JsonData(current.Content);
Response putResponse = await client.PutKeyValueAsync((string)keyValue.key, updated, ContentType.ApplicationJson);

• Implement a good debugger visualizer

If this isn't "good" yet, let's clarify the ask.

• Update our protocol method docs
  We will file issues to track this.

Code sample

Per this comment:

The output of:

dynamic json = JsonData.Parse(new BinaryData(new
{
    Foo = new
    {
        Bar = "hi"
    }
}));
Console.WriteLine(json.Foo);
Console.WriteLine(json.Foo.Bar);
Console.WriteLine(json);

dynamic json = JsonData.Parse("5");
Console.WriteLine(((int)json) + 5);

dynamic json = JsonData.Parse("[1, 2, 3, 4, 5, 6]");
Console.WriteLine((int)json[5]);

is

{"Bar":"hi"}
hi
{"Foo":{"Bar":"hi"}}
10
6

[azure-sdk]

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Core
Azure.Core.Experimental

[KrzysztofCwalina]

If this isn't "good" yet, let's clarify the ask.

I worry that this view does not scale to large JSON payloads.

[annelo-msft]

A string cast is required here for this to succeed right now.

[annelo-msft]

A string cast is required here to succeed.

[annelo-msft]

Handle IDisposable properly.

[annelo-msft]

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs#L61

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.Parse.cs#L87

[annelo-msft]

Per @AlexanderSher:

I've created a small benchmark. It looks like disposing JsonDocument is quite important:
https://gist.github.com/AlexanderSher/251a035cfbbe539337e4a1c6504ff52b

[annelo-msft]

@AlexanderSher, does this address your concern on this? 795e2ff

[heaths]

Discussed with @KrzysztofCwalina offline, but I don't think we should automatically convert camelCase JSON properties to PascalCase .NET properties on responses. Requests don't get the same treatment, so this may be confusing or even misleading. Say you get a response back, want to update it, and then send it back (which you also can't since JsonData is effectively immutable):

var json = response.Content.ToDynamic();
var data = new
{
  Id = json.Id,
  Foo = "new foo",
};
client.UpdateFoo(Request.Create(data)); // throws RequestFailedException: 400 BadParameter or something because the properties are supposed to be camelCase.

I appreciate we want to be idiomatic, but as soon as customers are having to deal with raw JSON - via System.Text.Json like my samples were before vs. what you're prototyping here with JsonData (which is a remarkable improvement) - I don't think we should "auto-correct" anything. Python nor JS would since customers are working directly with the JSON data. I don't think C# / .NET should either.

Model classes give us this ability, and once we generate them (for Cadl or otherwise) we can and should do that translation because we can do so consistently.

/cc @tg-msft

[annelo-msft]

I guess I don't understand the thinking there. We do this in our public models, and the idea is that JsonData is a stand-in until we add those types. Ideally, the customer would have to change very little to move to any types we added to a library. It's not that they're dealing with raw Json (which they can do easily if they want) -- it's a layer over the Json (or Xml, or whatever) that makes it conform to a higher .NET standard for look-and-feel. Is consistency with anonymous types your primary concern here, because I agree that is not good. Are there any other considerations I'm not thinking of?

[annelo-msft]

Ah, I see that this is a question about round-tripping the type, my apologies for having missed that the first time. We are planning to provide a separate API to handle-round tripping, that may be able to address this. I am still curious to know if there are other considerations here beyond the inconsistency across input/output that we should also be aware of.

[heaths]

Assume with my example above that the JSON schema is indeed camelCase, as most services define (and should): the request would accept id and foo, not Id and Foo. However, by translating from camelCase to PascalCase we could give callers the impression that the properties are indeed Id and Foo.

This isn't just about roundtripping, though. It's about perception. If all the response properties are PascalCase, why wouldn't many think that request properties are too?

I appreciate this isn't "raw" JSON. That's not what I intended. But it's a very thin wrapper over raw JSON that is meant to provide a JSON-like interaction for the customer, yes? By morphing into something it's not just to seem idiomatic, we create this confusion of casing for customers. Why not just leave it with the exact shape - casing and all - as the service returns (response) and expects (request)?

[annelo-msft]

That's totally fair - all good points. It feels like the question hinges on how thin the wrapper is and how well we document and illustrate via samples what the type is actually doing. If, for example, it also wraps other formats like XML, it might be more clear that you can't make too many assumptions about what's underneath.

[heaths]

If this supports XML, I don't see my point changing: if an XML document uses camelCase or PascalCase (and in my experience it varies even more than with JSON, which is more often camelCase), we should leave casing as is to avoid confusion. If we start showing that customers can always use PascalCase for responses, why wouldn't they think they can use it for input as well? Or if they use the obj["foo"]["bar"] pattern it will also vary from obj.Foo.Bar. Changing cases like that just seems so jarring.

What benefit to the customer does translating the casing have? I can certainly see a lot of cons. The only pro is "it feels like C#", but they aren't working with generated types (C# or otherwise): they are working with thin wrappers over raw JSON, XML, etc.

[tg-msft]

Should ToDynamic take an optional ObjectSerailizer so folks can customize this? I feel like that would easily allow this to work for people who wanted it, but then we could get away with less magic by default if we didn't change the case.

[ghost]

Hi @annelo-msft. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.