Skip to content
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

Make JsonData Immutable #31203

Closed
wants to merge 10 commits into from
Closed

Conversation

annelo-msft
Copy link
Member

Modifications to JsonData by @jsquire.

Addresses #30717

@azure-sdk
Copy link
Collaborator

API change check

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

Azure.Core.Experimental

{
WriteTo(writer);
}
return new BinaryData(memoryStream.ToArray());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about?

Suggested change
return new BinaryData(memoryStream.ToArray());
return new BinaryData(memoryStream.GetBuffer().AsMemory(0, (int)stream.Position));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how the BinaryData FromStream method constructs the instance after copying into a separate MemoryStream - https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/System.Memory.Data/src/BinaryData.cs#L179

I think the reason we didn't type sniff for MemoryStream and attempt to extract the ArraySegment is that we wanted the semantics to be consistent across different streams.

Copy link
Member Author

@annelo-msft annelo-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsquire, do you know why we implement IDynamicMetaObjectProvider instead of inheriting from DynamicObject?

@jsquire
Copy link
Member

jsquire commented Sep 30, 2022

@jsquire, do you know why we implement IDynamicMetaObjectProvider instead of inheriting from DynamicObject?

I do not. @ellismg would probably have insight.

@ellismg
Copy link
Member

ellismg commented Sep 30, 2022

I do not. @ellismg would probably have insight.

I can't recall the exact reason, but if I had to hazard a guess, it would be that we didn't want to inherit all of the public methods from DynamicObject on to our public surface area and into intellisense. Suspect that's also why we decided to explicitly implement the interface.

@annelo-msft
Copy link
Member Author

I can't recall the exact reason, but if I had to hazard a guess, it would be that we didn't want to inherit all of the public methods from DynamicObject on to our public surface area and into intellisense. Suspect that's also why we decided to explicitly implement the interface.

We're investigating making the JsonData API fully internal, so I revisited this question.

It looks like all the TryXx methods on DynamicObject only get called after an attempt to access the dynamic property/conversion/method etc on the underlying typed instance fails and throws an exception, which would degrade performance. So, I think we will still want to implement IDynamicMetaObjectProvider even if the API is internal.

There's also the following comment in the DynamicObject reference docs, supporting this hypothesis:

if you need to manage DLR fast dynamic dispatch caching, create your own implementation of the IDynamicMetaObjectProvider interface.

See: https://learn.microsoft.com/en-us/dotnet/api/system.dynamic.dynamicobject?view=net-7.0#remarks

@annelo-msft
Copy link
Member Author

Closing in favor of #31769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants