-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Core] Introduce JsonData type in Azure.Core #30717
Comments
I guess the obvious question is why this belongs in Azure.Core. There doesn't seem to be anything specific to Azure here, and seems like it could go in the BCL, similar to BinaryData. Also, how does this fit in with the JsonNode type introduced in System.Text.Json? |
Some historical context - when we originally designed Another thing pushing the custom time at the time were schedule concerns from our end (ameliorated now as they've shipped) and there was also at the time some concern about having a type that was Net Standard 2.0 compatible (or whatever the minimum version of NS that the Azure SDKs are targeting these days). |
Ah that makes sense given the timelines. JsonNode was added pretty recently. Regarding the minimum version considerations, since STJ is shipped as an OOB package, I wonder if we could consider upgrading the central repo dependency considering that this is a BCL library that has very strong guarantees when it comes to compatibility. That is, if this type actually addresses our needs from the SDK side. |
@KrzysztofCwalina, do you have a clear set of requirements for your current need for JsonData? Would JsonNode suffice for those purposes? |
Does JsonNode support dynamic? I don't think so, but if it does, we could use it. The requirement is to be able to do: Response response = client.Get();
dynamic json = JsonData.Parse(response.Content);
Console.WriteLine(json.Foo);
Console.WriteLine(json.Foo.Bar);
Console.WritelIne(json);
Console.WriteLine(((int)json) + 5);
Console.WriteLine(json[5]); // not sure if it's possible, if not we need to invent a way to deal with arrays. The main work needed, given that we have the prototype, is to:
|
It doesn't look like it does, but it has indexers so you could do things like: JsonNode json = JsonNode.Parse(response.ContentStream);
Console.WriteLine(json["foo"]);
Console.WriteLine(json["foo"]["bar"]); It also has a bunch of implicit operators so I think you can do: Console.WriteLine(((int)json) + 5); And indexing into an array is supported: Console.WriteLine(json[5]); |
The casts and array accessors look good, but I think the property access syntax is so much messier. We might want to do a user study to compare these two. |
JsonNode usage example with TextAnalytics - https://gist.github.com/JoshLove-msft/34897c9f5d3250e8b23167ce5c01c2b6 |
Per conversation with @KrzysztofCwalina, the DPG user experience will come down to:
His observation is that anonymous types are the clear winner over JsonNode on the input side. There were several cons here:
Most problems that JsonData has around lack of Intellisense and compiler safety are shared by JsonNode, although you do get compile-time checks on JsonNode usage that are deferred to runtime with JsonData. Overall, the thinking behind JsonData is that with this approach the code, once written, best expresses the user's intent. There remains to be an in-depth analysis of complex types and advanced scenarios. |
Agreed that anonymous types are easier for input, especially when not roundtripping. I don't see how using JsonNode for return types would preclude using anonymous types for input. |
@JoshLove-msft, I made the same point. @KrzysztofCwalina's perspective was that we either embrace JsonNode for the full round-trip, or we go with the JsonData/Merge/anonymous type story. It's true that anonymous types could still be used for input with JsonNode, but if round-tripping requires using JsonNode for input, it's not clear why we would write DPG docs and samples using JsonNode for output and not input. So the argument was largely driven around the usability of anonymous types over JsonNode for input, not the comparison of JsonNode to JsonData. As we mentioned earlier, @KrzysztofCwalina did also feel that we need more analysis of JsonData/anonymous types vs. JsonNode for complex types and advanced scenarios - that's work I haven't done yet that remains to be filled in before either approach would be greenlighted. |
I think it depends on the scenario. For the initial request you can use anonymous types or JsonNode. When you get back a JsonNode, and you need to send it back to the service, you would use JsonNode. I think the usability is better with JsonNode when you are working with a response from the service that you either want to send as-is or mutate. For the initial request, anonymous types are easier. |
Sure. I think the overarching concern there is that we'd like consistency in the documentation around DPG, and not to have to special case things too much, since DPG is going to be advanced enough as it is. |
I wonder if making JsonNode more anonymous type friendly would be an option? |
But thinking about this more, I'm not sure I understand the argument around anonymous types for input. For JsonData, I assume we would use anonymous types for input only + JsonData for round-tripping, right? Or am I missing the end-to-end flow here? Do you have an example of RequestContent.Merge works? Nevermind, found this from an email thread: // ***
// 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); I think I understand the argument now. When using JsonData, we always use anonymous types for input, even for round-tripping. I think this will be sort of complicated to use the Merge API for nested models though. For instance, let's say you wanted to change one property of a sub model, do users need to specify all other properties, or would the Merge just ignore any missing properties? i.e. how would I delete a property vs how would I set a value to null? Or do we not need that type of semantics (we don't need to let users remove properties)? |
I believe it should follow the HTTP PATCH semantics - keep the existing data and overwrite anything that is provided when calling Therre's precedent for this kind of pattern if you consider C# and F# records and the |
@JoshLove-msft, this makes me curious about the BinaryData API. Do you know why some conversions to BinaryData are handled as constructors and some as FromXx static methods? |
I believe there are static methods and constructors for each conversion with the exception of streams. The static methods allow you to use async. IIRC we didn't add a stream constructor because it would seem to imply that we are wrapping the stream, when in reality, we would be consuming it and storing the bytes in memory. |
This makes sense - I suppose we don't really need the ability to remove properties for DPG as you would generally be getting and then updating data with the same JSON schema. |
Can you say more about why we had the constructors? Neither JsonNode nor JsonDocument have constructors, so I'm wondering what the semantics of the constructor imply that is different from the Parse method, to understand whether we would want a constructor taking object on JsonData. @ellismg, do you know the .NET thinking around this? |
IIRC the thinking was that constructors are more discoverable, but we also needed at least some static methods to support async operations, i.e. FromStreamAsync. |
What @JoshLove-msft said tracks with my understanding. |
Released in https://www.nuget.org/packages/Azure.Core/1.33.0 |
API Change Needed
Yes
Background and motivation
The JsonData type provides a dynamic layer for accessing JSON and is intended to improve the developer experience, over JsonDocument which we recommend to users today, when working with protocol methods. This type will improve usability for raw JSON that comes back from protocol methods as BinaryData.
Goals
Scenarios
Pageable<dynamic>
Samples
Notes
In the past, we could not finalize the prototype of JsonData in Azure.Core.Experimental because we tried to make it read-write. If we scope the problem to just parsing responses, we think we can provide it as a useful type for customers of DPG clients.
API / Feature Proposal
Creation
Serialization/Deserialization
Usage as output model
If current node is a leaf node (not an array or an object)
If current node is an array node
If current node is an object
Other
Prior Description
Time Constraints and Dependencies
If possible, we would like to have this type by late September (@KrzysztofCwalina to comment on timeline needs)
Stakeholders
@KrzysztofCwalina, DPG customers
The text was updated successfully, but these errors were encountered: