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

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

Open
2 of 5 tasks
mikekistler opened this issue Mar 8, 2022 · 12 comments
Open
2 of 5 tasks

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

mikekistler opened this issue Mar 8, 2022 · 12 comments
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. feature-request This issue requires a new behavior in the product in order be resolved.
Milestone

Comments

@mikekistler
Copy link
Member

mikekistler commented Mar 8, 2022

Library name

core

Please describe the feature.

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

  • PatchBuilder will be implemented in core.
  • PatchBuilder will accept name / values and "build" a RequestContent.
  • Names can be "dotted" to set nested fields arbitrarily deep
  • Builder methods will allow fields to be "added" or "removed" (set to null)
  • "build" might be generic to allow checking that patch structure conforms to a particular class

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

Work items

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Mar 8, 2022
@azure-sdk
Copy link
Collaborator

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

@jsquire jsquire added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Mar 8, 2022
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Mar 8, 2022
@annelo-msft
Copy link
Member

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

@annelo-msft annelo-msft added Zinc Semester feature-request This issue requires a new behavior in the product in order be resolved. labels Aug 24, 2022
@m-nash
Copy link
Member

m-nash commented Feb 10, 2023

@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
Copy link
Member

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
Copy link
Member

Yes, I think this is achievable with MutableJsonDocument.

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Feb 10, 2023

@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
Copy link
Member

m-nash commented Feb 10, 2023

@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
Copy link
Member

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
Copy link
Member

m-nash commented Feb 13, 2023

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

@jsquire jsquire changed the title [FEATURE REQ] Add core support for JSON Merge Patch methods [Core] Add JSON Merge Patch support to JsonData Mar 14, 2023
@annelo-msft
Copy link
Member

annelo-msft commented Jun 9, 2023

This is being requested with some priority from the generator team: Azure/autorest.csharp#2492 (comment)

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

Copy link

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.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 11, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 11, 2024
@LuChen-Microsoft
Copy link
Member

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;
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. feature-request This issue requires a new behavior in the product in order be resolved.
Projects
None yet
Development

No branches or pull requests

7 participants