annelo-msft
commented
2 years ago - Can we have models that are used in both create and update, but that allow you to set the property for create but not for update?
Closed annelo-msft closed 4 months ago
annelo-msft
commented
2 years ago 
JeffreyRichter
commented
2 years ago Yes. PACTH can be used for both create and update; POST/PUT isn't even necessary for service to support at all. So, the 1st PATCH creates and further PATCHes update. We tell services that if a PATCH body includes a property that is "create-only" to accept it if its value matches what's already there (the retry scenario) else to fail the request with a 400.
Azure API Stewardship: Guidelineshttps://aka.ms/azapi/guidance | Review process https://aka.ms/azapi | Recordingshttps://aka.ms/azapi/teams | Calendarhttps://aka.ms/azapi/calendar | Join the Interest Grouphttps://aka.ms/azapi/join-interest-group
Azure SDK Architecture: Guidelineshttps://azure.github.io/azure-sdk/general_introduction.html | Review process https://aka.ms/azsdk/archreview/process | Recordingshttps://aka.ms/azsdk/archreview/recordings | Calendarhttps://aka.ms/azsdk/archreview/calendar | Join the Interest Grouphttps://aka.ms/azsdk/archreview/join-interest-group
From: Anne Thompson @.> Sent: Thursday, August 4, 2022 12:51:01 PM To: Azure/autorest.csharp @.> Cc: Jeffrey Richter @.>; Mention @.> Subject: Re: [Azure/autorest.csharp] Provide design for PATCH operation models from Cadl (Issue #2492)
— Reply to this email directly, view it on GitHubhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAzure%2Fautorest.csharp%2Fissues%2F2492%23issuecomment-1205702165&data=05%7C01%7Cjeffreyr%40microsoft.com%7Cb64bbccde081467a76c808da7652a90a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637952394640444818%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=9Ir4a%2F4JGD0ZjmQP5rF0VGxaMyiehlm1t1VIcEtm6Ds%3D&reserved=0, or unsubscribehttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAARLJP6XOH3CZHEZV5ENROLVXQNKLANCNFSM54W6D6MA&data=05%7C01%7Cjeffreyr%40microsoft.com%7Cb64bbccde081467a76c808da7652a90a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637952394640444818%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=wsMUAW3D3dAuJkkUnwAQu8voqzv%2FG34y5tp1wEzvdmc%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>
lirenhe
commented
1 year ago CADL is defining the merge-patch body in https://github.com/Azure/cadl-azure/blob/main/packages/cadl-azure-core/lib/operations.cadl#L49-L64
Foundations.ResourceCreateOrUpdateModel applies to the resource type, inside the definition, CADL remove properties with read visibility and make all the other required properties as optional.
TypeScript plan to generate the model type as
interface TResourceMergeAndPatch = Partial<Exclude<TResource, "">>; interface TResourcePatch = Exclude<TResource, "">;
We shall decide how we could handle this case in dotnet.
lirenhe
commented
1 year ago Move out of GA as this is not high priority for now
chunyu3
commented
1 year ago We will use DynamicData to implement patch operation and operation with merge-patch content-type. DynamicData will be supported in Azure.Core end of June (maybe). We will delay supporting patch operation and merge-patch until DynamicData is released in Azure.Core.
annelo-msft
commented
1 year ago @chunyu3, what is your timeline for needing to support this? We intend to release DynamicData in the next Core release, but it will not have patch support in that release.
lirenhe
commented
1 year ago I found a couple of patch operations in another service (LoadTest): https://github.com/Azure/azure-rest-api-specs/blob/feature/loadtesting/specification/loadtestservice/routes.tsp
Though we could still generate protocal methods for the user but that may impact the usablilty, as this is a missing feature, I hope the Patch support could be ready ASAP.
cc @mikekistler @m-nash
lirenhe
commented
1 year ago ContentSafety also contains the patch API. Currently, we only generate protocal methods for them.
m-nash
commented
1 year ago @lirenhe can you write up what code a customer would have to do for various patching scenarios to work with protocol methods? I think this would help everyone understand our current state and how good or bad it currently is.
lirenhe
commented
1 year ago @lirenhe can you write up what code a customer would have to do for various patching scenarios to work with protocol methods? I think this would help everyone understand our current state and how good or bad it currently is.
I am not the expert in this area, but below are what are service teams shows how to use the protocol patch operation:
annelo-msft
commented
1 year ago Thank you for these, @lirenhe!
Since DynamicData has released, I'd like to start sketching out this design now. It would be good for me to get a holistic understanding of how PATCH operations show up in our generated libraries.
lirenhe
commented
1 year ago Thank you for these, @lirenhe!
Since DynamicData has released, I'd like to start sketching out this design now. It would be good for me to get a holistic understanding of how PATCH operations show up in our generated libraries.
- Are these two libraries - Azure.AI.ContentSafety and Azure.Developer.LoadTesting the only two we're currently tracking that have PATCH operations, or are there others?
- Are they primarily used for CreateOrUpdate operations, or do they show up in any other ways?
- Is our goal to generate convenience methods for these, or is the expectation that they will continue to be protocol-only?
Thanks @annelo-msft to follow up on this.
Yes, those are the 2 libraries we found that use PATCH operations. I am not aware of other service using this now.
As I know, they are used only in CreateOrUpdate operations, so I think we could just support 'application/merge-patch+json'. Not sure whether we should also support 'application/json-patch+json' today. @m-nash?
Our goal is to generate convenience methods for PATCH operation. Today, we asked service team to use protocol and do not do any customization mainly because we don't know how the convenience method would look like.
mikekistler
commented
1 year ago The API Stewardship Board discourages 'application/json-patch+json'. There are some teams that support it so it should be "possible" but not "easy" -- so that teams must make a conscious choice to use it.
+1 on convenience methods for PATCH.
pshao25
commented
1 year ago Scenario 1: normal patch operation with all the properties from required to optional
model Test {
required: string;
optional?: string;
}
@patch op method(@body body: Test): void;Expected: we could send any combination of properties like:
var test = new Test();
test.Optional = "a"; // Not set required property
var response = someClass.Method(test); // OK.Scenario 2: Same model in different places
model Test {
required: string;
optional?: string;
}
@patch op patch(@body body: Test): Test;
@post op post(@body body: Test): void;Expected:
var response = someClass.patch(new Test() {Optional = "a"}); // The input is OK. But output if return `{optional: "b"}` should fail because it should contain `required`
var response = someClass.post(new Test() {Optional = "a"}); // Should fail because it should contain `required`Thanks for this use-case, @pshao25! Can you help me understand what a corresponding API is in an existing service library? It will help me understand the use case better if I can understand where a service is using this.
Another piece of information that would be helpful for the design is the intended use case for such a model.
For example, in the above, what is the user doing on a POST vs. on a PATCH? Is POST creation of a new model and PATCH is an update? Can PATCH be used to create a new instance of the resource on the service, or not?
pshao25
commented
1 year ago @annelo-msft This is an example that same model TextBlocklist is used in both get and patch. So 1) when calling getTextBlocklist, if response payload does not contain required property of TextBlocklist, call should fail, 2) when calling createOrUpdateTextBlocklist, every properties in TextBlocklist could leave empty because it is patch.
For model in both post and patch, I didn't find a real case. But I think it makes sense that POST is creation of a new model and PATCH is an update. Or we could think this way, if you think same model shouldn't be in both of post and patch, then compiler should block this, at least linter should block this. If we don't block this, then it means this scenario makes sense.
annelo-msft
commented
1 year ago That is a great example, thank you @pshao25! I will try to get back to you soon with a proposal for how we can address it.
annelo-msft
commented
1 year ago Hi @pshao25, I have a clarifying question - I am looking at the model definition for TextBlocklist in the models file here: https://github.com/Azure/azure-rest-api-specs/blob/14d24d17491d8c2bde24532cb8cc2d663c0ffd9f/specification/cognitiveservices/ContentSafety/models.tsp#L134-L147
How can you tell that a property of the model is required for GET and not for PATCH?
From their REST API documents, it looks like blocklistName is needed by URI, so should be taken by the model constructor to create the request, but not be written to the body by the serialize operation. Specifically, I am suggesting that the client's service methods would have the following signatures:
public virtual Response<TextBlocklist> CreateOrUpdateTextBlocklist(TextBlocklist blocklist, CancellationToken cancellationToken = default);
public virtual Response CreateOrUpdateTextBlocklist(string blocklistName, RequestContent content, RequestContext context = null);
public virtual Task<Response<TextBlocklist>> CreateOrUpdateTextBlocklistAsync(TextBlocklist blocklist, CancellationToken cancellationToken = default);
public virtual Task<Response> CreateOrUpdateTextBlocklistAsync(string blocklistName, RequestContent content, RequestContext context = null);Do you know if there are any cases where we have required properties that aren't used in the URI? I would like to understand these to make sure our design accounts for those. I think it is possible, we just need to understand the different cases to call out in the generator.
Update: I added an example of this to the proposal PR, here.
This illustrates the case where a property is not required (or allowed?) in the Merge Patch request body, but is needed by the model to format the request URI, so should be required in the model constructor. The important piece here is that properties that shouldn't appear in the Merge Patch JSON need to be in the initial JSON that is parsed to create the MutableJsonElement. If they are added after the initial creation, they will go on the changelist and be written out in the WritePatch operation.
pshao25
commented
1 year ago @annelo-msft I'm not sure if that is the correct signature. I'm contacting typespec team to figure out some expected behavior about this scenario. Anyway the signature is not my emphasis in this issue.
How can you tell that a property of the model is required for GET and not for PATCH?
Because when a model is used in a PATCH operation, all of its properties automatically become optional, right? You could set any part of them and leave any required property unset. That is PATCH!
Do you know if there are any cases where we have required properties that aren't used in the URI?
I haven't find one. Once I got one, I'll let you know. But I believe it is very easy to understand this case. For example,
@doc("Text Blocklist.")
@resource("text/blocklists")
model TextBlocklist {
@key("blocklistName")
blocklistName: string;
description?: string;
count: int;
}
createOrUpdateTextBlocklist is ResourceCreateOrUpdate<TextBlocklist>;The added count could leave empty in a PATCH body like {"description": "d"} so that only description is updated. Could you update your example with this "though made up, very common I think" model?
And I just realized in this case we cannot set count to null because a required property cannot be deleted.
annelo-msft
commented
1 year ago You could set any part of them and leave any required property unset.
If a property is needed to format the request URI, then I believe it needs to be required even on a PATCH model.
Do you know if there are any cases where we have required properties that aren't used in the URI? I haven't find one.
I would be interested to know this. I can imagine that there are cases where some properties are required for GET and not for PATCH, for example a creation time, or some value set by the service that the user is intended to read and not update. For these, we would make these read-only properties on the model, so the user cannot modify them for a PATCH operation. For GET operations, they would be populated by the internal serialization constructor, not a public constructor that the end-user has access to.
In general, that would be the pattern, and I think it works for all pairs of GET/PATCH operations, by the following logic:
I think this will only run into problems if where there are multiple input operations using the same model, and where those input operations have different (required/optional) or (read-only/read-write) for the same property. If we run across cases where this is happening, it would be good to understand why they exist -- i.e. what specific scenarios they are addressing -- so we can make sure our model design reflects the user intent.
chunyu3
commented
12 months ago duplicate with https://github.com/Azure/autorest.csharp/issues/3648
pshao25
commented
4 months ago We will track in #3648
The August MVP target service has PATCH operations. We need to determine whether we will provide special model support for PATCH operations.
This may have already been decided in https://github.com/Azure/azure-sdk/issues/3520#issuecomment-1061373039. There is a relevant document from @JeffreyRichter as well.