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

[Epic] Design for PATCH operation models from TypeSpec #3648

Open
5 tasks
pshao25 opened this issue Aug 9, 2023 · 2 comments
Open
5 tasks

[Epic] Design for PATCH operation models from TypeSpec #3648

pshao25 opened this issue Aug 9, 2023 · 2 comments
Assignees
Labels

Comments

@pshao25
Copy link
Member

pshao25 commented Aug 9, 2023

Tasks

Preview Give feedback

Related tasks

Preview Give feedback
  1. Client CodeGen
    pshao25
  2. Client CodeGen
    pshao25
@chunyu3
Copy link
Member

chunyu3 commented Aug 14, 2023

duplicate with #2492

@pshao25
Copy link
Member Author

pshao25 commented Jan 3, 2024

Original design

A model only in patch request

According to api guidelines,

  • PUT request/response, PATCH response, GET response, and POST request/response could share the same schema.
  • PATCH request schema should contain all the same fields with no required fields.

All the properties in the PATCH request of generated SDK should be optional.

Case 1: The properties in spec are all optional

@resource("testV2")
model TestV2 {
  optionalProperty?: string;
}

interface DynamicData {
  patch is ResourceCreateOrUpdate<TestV2>;
}

Option 1 (Don't change name): It follows the api guidance so just go with the design in Azure/azure-sdk-for-net#37341
Option 2 (Change name): Still change name to e.g. TestV2Update, for the purpose of 1) be consistent with case 2 below, 2) service driven scenario below.

Case 2: Still have required properties in spec because response could have the required properties

@resource("testV2")
model TestV2 {
  requiredProperty: string;
  optionalProperty?: string;
}

interface DynamicData {
  patch is ResourceCreateOrUpdate<TestV2>;
}

This means input is {optionalProperty: xxx} and response is {optionalProperty: xxx, requiredProperty: xxx}.

Option 1: We may need a linter in emitter to block required properties in spec. And let users to choose one of:

  1. Declare a new model with only optional properties as input in client.net.tsp
  2. Use @visibility("read") to exclude required properties.

Option2: We think it is valid spec by implicitly creating a new model e.g. TestV2Update with requiredProperty changing to optional.

A model in both patch request and other operations

@resource("testV2")
model TestV2 {
  optionalProperty?: string;
}

interface DynamicData {
  patch is ResourceCreateOrUpdate<TestV2>;
  put is ResourceCreateOrReplace<TestV2>;
  get is ResourceRead<TestV2>;
}

Option 1: block this by linter "same model cannot be both in the patch request and other operations". Again, let users to choose one of:

  1. Declare a new model with only optional properties as input in client.net.tsp
  2. Use e.g. @visibility("create") to exclude required properties. We don't support this now.

Option 2: generate normal TestV2 to PUT and GET, and a new model TestV2Update to PATCH with patch pattern.

Service driven story

Case 1: first has a PATCH and then has a PUT/GET

// V1
patch is ResourceCreateOrUpdate<TestV2>;

// V2
patch is ResourceCreateOrUpdate<TestV2>;
put is ResourceCreateOrReplace<TestV2>;

Case 2: first has a PUT/GET and then has a PATCH

// V1
put is ResourceCreateOrReplace<TestV2>;

// V2
patch is ResourceCreateOrUpdate<TestV2>;
put is ResourceCreateOrReplace<TestV2>;

Considering both of these two scenarios, it might be necessary to have a new model e.g. TestV2Update to patch operation.

@pshao25 pshao25 changed the title Design for PATCH operation models from TypeSpec [Epic] Design for PATCH operation models from TypeSpec Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants