Update JsonData in Experimental#32613
Conversation
ghost
added
the
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs
Show resolved
Hide resolved
sdk/core/Azure.Core.Experimental/tests/public/Azure.Core.Experimental.Tests.Public.csproj
Show resolved
Hide resolved
sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs
Show resolved
Hide resolved
sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs
Show resolved
Hide resolved
annelo-msft
requested review from
JoshLove-msft,
ellismg and
jsquire
and removed request for
JoshLove-msft
sdk/core/Azure.Core.Experimental/tests/public/Azure.Core.Experimental.Tests.Public.csproj
Outdated
Show resolved
Hide resolved
| // because apparently the type system has decided the IEnumerable | ||
| // is holding JsonData, not dynamic, so the DMO callback is never called | ||
| // and the explicit cast to int is not visible b/c JsonData is internal. | ||
| //IEnumerable value = (IEnumerable)data; |
There was a problem hiding this comment.
I believe this may be due to using IEnumerable which is non-generic and assumes object for the type. Does using IEnumerable<dynamic> as the cast work?
There was a problem hiding this comment.
It does not ... it fails with a RuntimeBinderException whereas just IEnumerable fails with an InvalidCastException.
| public static implicit operator float? (Azure.Core.Dynamic.JsonData json) { throw null; } | ||
| public static implicit operator float (Azure.Core.Dynamic.JsonData json) { throw null; } | ||
| public static implicit operator string (Azure.Core.Dynamic.JsonData json) { throw null; } | ||
| public static bool operator !=(Azure.Core.Dynamic.JsonData? left, int right) { throw null; } |
There was a problem hiding this comment.
Don't we want equality operators for the same set of primitives that we have the casts for?
There was a problem hiding this comment.
These will be addressed in a follow-up PR. My goal was to merge this to make something available early for CloudMachine to start working with.
| var json = JsonData.Parse("{}"); | ||
| dynamic jsonData = json; | ||
| jsonData.a = JsonData.EmptyObject(); | ||
| jsonData.a = JsonData.Parse("{}"); |
There was a problem hiding this comment.
If we need empty, we should somehow add specific API for it as opposed to asking users to parse {}
There was a problem hiding this comment.
This is only for internal test purposes. It simulates what would be returned by a protocol method response without providing a client. Per our requirement that customers will modify JsonData instances only for round-trips, I don't think we need a formal API to create an empty instance. I can wrap the method in the test to make the intent more clear - that the caller would obtain the JsonData instance from a client method.
| public void CannotGetMemberOnArray() | ||
| { | ||
| dynamic data = JsonDataTestHelpers.CreateFromJson("[1, 2, 3]"); | ||
| Assert.Throws<InvalidOperationException>(() => { var x = data.Property; }); |
There was a problem hiding this comment.
We should think what's the right exception type to throw here.
There was a problem hiding this comment.
Cool, thanks! In some places, we are also throwing JsonException, and I don't think it is currently clear when we throw which one or why.
|
@annelo-msft looks like this change also is the reason some of the purview tests are failing see https://dev.azure.com/azure-sdk/internal/_build/results?buildId=2055180&view=logs&j=19ae9ca6-cc95-59cd-da1c-d5a57cd99bf5&t=d4ee6916-5fd3-538d-4187-7e8f2bed8e1c&l=49. It looks like it is caused by the move of JsonData to a new namespace. |
7 participants
Addresses #30717.
This PR does the following:
ToDynamic()extension method to BinaryData, to enableresponse.Content.ToDynamic()The following will be handled in a later PR: