search

Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.18k stars 4.54k forks source link

[Core] Add JSON Merge Patch support to JsonData #27402

Open mikekistler opened 2 years ago

mikekistler commented 2 years ago

Library name

core

Please describe the feature.

To support JSON Merge Patch methods, .NET should implement a PatchBuilder.

An update method can then support JSON Merge Patch by accepting a RequestContent body.

Work items

azure-sdk commented 2 years ago

Label prediction was below confidence level 0.6 for Model:ServiceLabels: 'Event Hubs:0.17217015,Extensions:0.10761291,Service Bus:0.101956904'

annelo-msft commented 1 year ago

Described verbally at minute 9 of Recording[MS INTERNAL ONLY]

m-nash commented 1 year ago

@KrzysztofCwalina It looks like our plan of record was to skip convenience methods for PATCH which would allow customers to give us a merge patch payload since RequestContent can accept anything.

It also sounds like in a second iteration we wanted to create a PatchableBuilder helper which would allow customers to easily create the merge patch json and the result of .Build would be a RequestContent so it could go straight into the protocol method.

Is that correct and do we have any initial design of PatchableBuilder? If not I will put together some options here.

I also wanted to ask if we wanted to do a hybrid convenience method for patch that looked like this

//protocol shape
public Response Patch(RequestContent content, RequestContext context = default) { }

//convenience shape
public Response<Model> Patch(RequestContent content, CancellationToken token = default) { }

It seems deserializing the response should be simple enough and would still be a valuable convenience for customers while at the same time allow the same flexibility with RequestContent as the input vs some PatchModel.

KrzysztofCwalina commented 1 year ago

When I investigated the possibility of shipping strongly typed (model based) PATCH APIs last time, I ran into an issue of very high complexity of implementation of such PATCH-enabled models. Since then, we (@annelo-msft) implemented a general-purpose library that can track changes and easily generate JSON PATCH payloads. Given that, I wonder if we should not revisit our POR to expose all PATCH APIs as simple/raw RequestContent.

The new general purpose API is basically a mutable JsonDocument-like API. I think it makes is trivial (relatively) to implement patchable models, e.g. 

public class Model {
    MutableJsonDocument _doc;

    internal Model(BinaryData content) => _doc = MutableJsonDocument.Parse(content);

    public string Foo {
        get => _doc["foo"]; // we could cache this, at the cost of all writes going into both places (the cache and _doc)
        set => _doc["foo"] = value;
    }

    internal void WritePatch(Stream stream) => _doc.WriteTo(stream, 'p'); // this write JSON PATCH

@annelo-msft, what do you think?

annelo-msft commented 1 year ago

Yes, I think this is achievable with MutableJsonDocument.

KrzysztofCwalina commented 1 year ago

@tg-msft, thoughts?

Basically MutableJsonDocument takes care of all the complicated tracking of complicated object graphs that killed my investigation into patchable models. Note that a simple model with just primitive properties is very simple without MJD. The difficulty starts once nodes in the middle of a complicated graph get mutated. Anne knows what I am talking about :-)

m-nash commented 1 year ago

@KrzysztofCwalina what about the comment around returning a full Response<Model> vs Response? PATCH convenience methods can still take advantage of the strongly typed response but not a strongly typed model for request?

KrzysztofCwalina commented 1 year ago

PATCH convenience methods can still take advantage of the strongly typed response but not a strongly typed model for request?

I probably don't understand something, but to me the PATCh convineince method would take advantage of the strongly typed model:

Model m = client.Get(...);
m.A = 5;
client.Patch(m);
m-nash commented 1 year ago

Spoke offline with @KrzysztofCwalina and we decided for the short term we will keep protocol only for PATCH until we flush out some of the details around using the MutableJsonDocument object.

/cc @srnagar @lmazuel

annelo-msft commented 1 year ago

This is being requested with some priority from the generator team: https://github.com/Azure/autorest.csharp/issues/2492#issuecomment-1583894515

A prerequisite to this work -- at least in the approach we are currently considering -- is completion of the initial release of DynamicData.

@tg-msft, @KrzysztofCwalina FYI

github-actions[bot] commented 4 months ago

Hi @mikekistler, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.

LuChen-Microsoft commented 3 months ago

Hi our ACS Chat team got the similar issue here : Teams discussion link

The ACS Chat team is working on the implementation of a new property called retention policy. Retention Policy is an optional property in the Chat thread and we are trying to allow users to update it to null.

We have this Update class to which we've added the new optional property called DataRetention: azure-sdk-for-net/sdk/communication/Azure.Communication.Chat/src/Generated/Models/UpdateChatThreadRequest.cs at 7e3911296bfd2c9860cb5016da67aab2ba0c5376 · Azure/azure-sdk-for-net (github.com)

The API expects it to be serialized to "DataRetention": null when the user wants to overwrite it to null. However when the user passes it as null like below, it doesn't get serialized and it stays the same:

            var updateOptionsWithNullRetentionPolicy = new UpdateChatThreadPropertiesOptions();
            updateOptionsWithNullRetentionPolicy.RetentionPolicy = null;

            await chatThreadClient.UpdatePropertiesAsync(updateOptionsWithNullRetentionPolicy);
            var updateResponseWithNullRetentionPolicy = await chatThreadClient.GetPropertiesAsync();
            Assert.IsNull(updateResponseWithNullRetentionPolicy.Value.RetentionPolicy); ---> FAILS, stays the same

This is how the autogenerated serialization of the update method is currently implemented, which doesn't work as it skips the serialization of the DataRetention to null when the customer passes it as null:

internal HttpMessage CreateUpdateChatThreadPropertiesRequest(string chatThreadId, string topic, IDictionary<string, string> metadata, ChatRetentionPolicy retentionPolicy)
{
    var message = _pipeline.CreateMessage();
    var request = message.Request;
    request.Method = RequestMethod.Patch;
    var uri = new RawRequestUriBuilder();
    uri.AppendRaw(_endpoint, false);
    uri.AppendPath("/chat/threads/", false);
    uri.AppendPath(chatThreadId, true);
    uri.AppendQuery("api-version", _apiVersion, true);
    request.Uri = uri;
    request.Headers.Add("Accept", "application/json");
    request.Headers.Add("Content-Type", "application/merge-patch+json");
    UpdateChatThreadRequest updateChatThreadRequest = new UpdateChatThreadRequest()
    {
        Topic = topic,
    };
    if (metadata != null)
    {
        foreach (var value in metadata)
        {
            updateChatThreadRequest.Metadata.Add(value);
        }
    }
    var model = updateChatThreadRequest;
    var content = new Utf8JsonRequestContent();
    content.JsonWriter.WriteObjectValue(model);
    request.Content = content;
    return message;
}