Make MutableJsonDocument internal shared source #38159
Conversation
|
API change check API changes are not detected in this pull request. |
annelo-msft
requested review from
hallipr,
weshaggard,
benbp,
JoshLove-msft,
christothes and
KrzysztofCwalina
as code owners
|
|
||
| void IModelJsonSerializable<SimplePatchModel>.Serialize(Utf8JsonWriter writer, ModelSerializerOptions options) | ||
| { | ||
| _element.WriteTo(writer, options.Format.ToString()); |
There was a problem hiding this comment.
@m-nash, should ModelSerializationOptions.Format have an implicit cast to string?
There was a problem hiding this comment.
I don't think it makes sense to have this on options as that will have more than just a Format on it. For format implicit to string we were just following out Extensible Enum pattern. Often times I have found that adding implicit ToString adds more confusion that it helps, but lets discuss if you feel differently here.
annelo-msft
changed the title
| #pragma warning disable AZC0020 // Avoid using banned types in libraries | ||
| private readonly MutableJsonElement _element; | ||
|
|
||
| // Note: A child patch model doesn't have a public constructor. |
There was a problem hiding this comment.
We have public constructor currently. Is there any reason we change this?
There was a problem hiding this comment.
My thinking is that, for Patch models, we need a way to connect the MutableJsonElement in the child model to the MutableJsonElement in the parent model, so that they point to the same root MutableJsonDocument. Without this, they don't access the same changelist and writing the Patch JSON would be a lot more complicated.
We could make it so that nested models have a public constructor but can't be set on the parent (shown here), but if you can't set the child model, then I don't know why you would have a public constructor on the child model - callers could create an instance of it but then not do anything with it, which seems like it could be frustrating to users.
There may be times when a nested model is also an input model on a service method - when that happens the model will need to have a public constructor, and we will need to make the parent model not settable so we can connect the MutableJsonElements. It would be ideal if the generator could detect that case and generate the model with the public constructor when that happens.
…msft/azure-sdk-for-net into core-make-mjdoc-internal
| public partial class ChildPatchModel | ||
| { | ||
| #pragma warning disable AZC0020 // Avoid using banned types in libraries | ||
| private readonly MutableJsonElement _element; |
There was a problem hiding this comment.
If a model is output and roundtrip only, I think storing all the data in MJE is fine. But what will we do for models that are also input models? Should these have two modes: one where data is stored in regular fields (as we do today) and one where the data is stored in MJE?
There was a problem hiding this comment.
I'll see if I can add some benchmarks to see how favorable that approach is.
There was a problem hiding this comment.
Per annelo-msft#9, MJD serialization is comparable to standard model serialization, except when we modify the change list, since it's essentially a pass-through to JsonDocument when there are no changes. I'll spend some time looking at whether we can optimize the case where we access the change list.
sdk/core/Azure.Core/tests/public/PatchModels/ChildPatchModel.Serialization.cs
Outdated
Show resolved
Hide resolved
sdk/core/Azure.Core/tests/public/PatchModels/ChildPatchModel.Serialization.cs
Outdated
Show resolved
Hide resolved
sdk/core/Azure.Core/tests/public/PatchModels/ChildPatchModel.Serialization.cs
Outdated
Show resolved
Hide resolved
| protected override string JsonPayload => """ | ||
| { | ||
| "id": "abc", | ||
| "child": {"a": "aa", "b": "bb"} |
There was a problem hiding this comment.
I would add an extra property here that isn't defined in the model to validate that portion of the serialization expectations as well.
There was a problem hiding this comment.
That feels out of scope for this PR - is there a reason it is needed here?
There was a problem hiding this comment.
Since we are defining a new model template that the generator would need to create I think its important to make sure it not only enables the new functionality but is compatible with all the existing functionality.
Its pretty simple to do just add "extra": "stuff" to the payload and in your expected overload you just check if format is J then that should be there.
Example change to expected https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core/tests/public/ModelSerializationTests/ModelXTests.cs#L42-L43.
Example change to verification https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core/tests/public/ModelSerializationTests/ModelXTests.cs#L54-L58
Problem description
Ultimately, we plan to make MutableJsonDocument and related types public types in Azure.Core.
In the short term, however, we are moving them to internal shared source to enable .NET generated libraries to use the types to generate patch models for the JSON Merge Patch work.
What's in this PR
This PR does the following:
What's left to do
Their are more patch model cases that aren't achievable today without the addition of helper types described in #37341, including:
Types to support these cases will be added in subsequent PRs.