Provide design for PATCH operation models from TypeSpec #2492
Comments
annelo-msft
added
v3
annelo-msft
changed the title
|
lmazuel
added
WS: Code Generation
Epic: Model Generation
|
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, "">>; We shall decide how we could handle this case in dotnet. |
|
Move out of GA as this is not high priority for now |
|
We will use |
|
@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. |
|
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. |
|
ContentSafety also contains the patch API. Currently, we only generate protocal methods for them. |
|
@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: |
|
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.
|
Thanks @annelo-msft to follow up on this.
|
|
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. |
|
Scenario 1: normal patch operation with all the properties from required to optional 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 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? |
|
@annelo-msft This is an example that same model 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. |
|
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. |
|
Hi @pshao25, I have a clarifying question - I am looking at the model definition for 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 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. |
|
@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.
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!
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, The added And I just realized in this case we cannot set |
If a property is needed to format the request URI, then I believe it needs to be required even on a PATCH model.
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. |
|
duplicate with #3648 |
lirenhe
changed the title
|
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 Azure/azure-sdk#3520 (comment).
There is a relevant document from @JeffreyRichter as well.
The text was updated successfully, but these errors were encountered: