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

Operations not generated as expected from TypeSpec #3344

Closed
catalinaperalta opened this issue May 3, 2023 · 15 comments
Closed

Operations not generated as expected from TypeSpec #3344

catalinaperalta opened this issue May 3, 2023 · 15 comments
Assignees
Labels
DPG v3 Version 3 of AutoRest C# generator.

Comments

@catalinaperalta
Copy link
Member

catalinaperalta commented May 3, 2023

Opening this issue that the Content Safety team mentioned in teams:

TypeSpec link: https://github.com/Azure/azure-rest-api-specs-pr/pull/9275

For this operation in typespec:

 @summary("Create Or Update Text Blocklist")
  @doc("Updates a text blocklist, if blocklistName does not exist, create a new blocklist.")
  @convenientAPI
  createOrUpdateTextBlocklist is Azure.Core.ResourceCreateOrUpdate<TextBlocklist>;

Generated code has no overload to take both blocklistName and TextBlocklist(or description defined in TextBlocklist), only has following:

public virtual Response CreateOrUpdateTextBlocklist(string blocklistName, RequestContent content, RequestContext context = null)

@mengaims can provide more information on what is expected here.

@catalinaperalta catalinaperalta added the v3 Version 3 of AutoRest C# generator. label May 3, 2023
@catalinaperalta
Copy link
Member Author

@m-nash any suggestions here would be appreciated

@catalinaperalta
Copy link
Member Author

catalinaperalta commented May 3, 2023

Another operation that is not generated as expected:

This method should return TextBlockItem instead of TextBlocklist:

public virtual Pageable<TextBlocklist> GetTextBlocklistItems(string blocklistName, int? maxCount = null, int? skip = null, int? maxpagesize = null, CancellationToken cancellationToken = default)

The corresponding typespec code is:

 @summary("Get All BlockItems By blocklistName")
  @doc("Get all blockItems in a text blocklist")
  listTextBlocklistItems is Azure.Core.ResourceList<
    TextBlockItem,
    ListQueryParametersTrait<StandardListQueryParameters>
  >;

@mengaims
Copy link

mengaims commented May 4, 2023

Opening this issue that the Content Safety team mentioned in teams:

For this operation in typespec:

 @summary("Create Or Update Text Blocklist")
  @doc("Updates a text blocklist, if blocklistName does not exist, create a new blocklist.")
  @convenientAPI
  createOrUpdateTextBlocklist is Azure.Core.ResourceCreateOrUpdate<TextBlocklist>;

Generated code has no overload to take both blocklistName and TextBlocklist(or description defined in TextBlocklist), only has following:

public virtual Response CreateOrUpdateTextBlocklist(string blocklistName, RequestContent content, RequestContext context = null)

@mengaims can provide more information on what is expected here.

See my manual fix for the expected generation:
Azure/azure-sdk-for-net@9825c37

@mengaims
Copy link

mengaims commented May 4, 2023

Another operation that is not generated as expected:

This method should return TextBlockItem instead of TextBlocklist:

public virtual Pageable<TextBlocklist> GetTextBlocklistItems(string blocklistName, int? maxCount = null, int? skip = null, int? maxpagesize = null, CancellationToken cancellationToken = default)

The corresponding typespec code is:

 @summary("Get All BlockItems By blocklistName")
  @doc("Get all blockItems in a text blocklist")
  listTextBlocklistItems is Azure.Core.ResourceList<
    TextBlockItem,
    ListQueryParametersTrait<StandardListQueryParameters>
  >;

The same, see may manual fix for the expected generation:
Azure/azure-sdk-for-net@84919ed

@lirenhe lirenhe added the DPG label May 4, 2023
@chunyu3
Copy link
Member

chunyu3 commented May 5, 2023

Azure.Core.ResourceList<
TextBlockItem,
ListQueryParametersTrait<StandardListQue

as the tsp definition, the operation will take blocklistName as path parameter and model TextBlocklist as the body parameter,
So codegen will generate following protocol method:

public virtual Response CreateOrUpdateTextBlocklist(string blocklistName, RequestContent content, RequestContext context = null)

If you enable to generate convenience method, codegen will generate convenience method

public virtual Response CreateOrUpdateTextBlocklist(string blocklistName, TextBlocklist content, CancellationToken cancellationToken = default)

So the operation generated is as expected according to the tsp definition.

@mengaims
Copy link

mengaims commented May 5, 2023

Thanks @chunyu3 for the update! I've checked latest generated code, only the first method is generated and the the second one was not, although I have convenient decorator in tsp:
@summary("Create Or Update Text Blocklist") @doc("Updates a text blocklist, if blocklistName does not exist, create a new blocklist.") @convenientAPI createOrUpdateTextBlocklist is Azure.Core.ResourceCreateOrUpdate<TextBlocklist>;
Could you help have a check?

@pshao25
Copy link
Member

pshao25 commented May 8, 2023

@catalinaperalta @mengaims
createOrUpdateTextBlocklist is Azure.Core.ResourceCreateOrUpdate<TextBlocklist>; is a patch operation. So we don't generate convenience method. It is a known issue.

listTextBlocklistItems is Azure.Core.ResourceList<TextBlockItem, ListQueryParametersTrait<StandardListQueryParameters>>; This issue we will fix.

@mengaims
Copy link

mengaims commented May 9, 2023

Add @heaths for awareness of these two issues.

@mengaims
Copy link

mengaims commented May 9, 2023

@pshao25, how could sdk user initialize RequestContent if only this method is provided?
public virtual Response CreateOrUpdateTextBlocklist(string blocklistName, RequestContent content, RequestContext context = null)

And for the second issue, what would be the ETA?

@catalinaperalta
Copy link
Member Author

Thanks @chunyu3 for the update! I've checked latest generated code, only the first method is generated and the the second one was not, although I have convenient decorator in tsp: @summary("Create Or Update Text Blocklist") @doc("Updates a text blocklist, if blocklistName does not exist, create a new blocklist.") @convenientAPI createOrUpdateTextBlocklist is Azure.Core.ResourceCreateOrUpdate<TextBlocklist>; Could you help have a check?

Did your generation improve after removing @convenientAPI?

@chunyu3
Copy link
Member

chunyu3 commented May 10, 2023

Did your generation improve after removing @convenientAP

As @pshao25 said, this is patch operation, and now we will not auto-gen convenience method for patch operation.
we plan to support this in future, and I will discuss with our arch about the schedule. Thanks.

@pshao25
Copy link
Member

pshao25 commented May 10, 2023

@mengaims There are sample docs generated in the Docs folder right near the generated code telling you how to create a RequestContent. You could take it as a reference.

@mengaims
Copy link

Thanks @chunyu3 for the update! I've checked latest generated code, only the first method is generated and the the second one was not, although I have convenient decorator in tsp: @summary("Create Or Update Text Blocklist") @doc("Updates a text blocklist, if blocklistName does not exist, create a new blocklist.") @convenientAPI createOrUpdateTextBlocklist is Azure.Core.ResourceCreateOrUpdate<TextBlocklist>; Could you help have a check?

Did your generation improve after removing @convenientAPI?

@catalinaperalta The convenientAPI is not the root cause of it, I've tried the typespec without this decorator and things are the same. As Crystal mentioned, this is expected behavior of the generation tool now. But I think from the service point of view, we would need a better method for customer to use. And I will continue this discussion to get knowledge about how to customize it or if manual work is required here.

@chunyu3
Copy link
Member

chunyu3 commented May 11, 2023

File a sperate issue #3386 for incorrect list operation issue.

@pshao25
Copy link
Member

pshao25 commented May 11, 2023

createOrUpdateTextBlocklist is Azure.Core.ResourceCreateOrUpdate<TextBlocklist>; is a patch operation. We will not implement convenience method until Mid June.

listTextBlocklistItems is Azure.Core.ResourceList<TextBlockItem, ListQueryParametersTrait<StandardListQueryParameters>>; is tracked at #3386

So close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DPG v3 Version 3 of AutoRest C# generator.
Projects
None yet
Development

No branches or pull requests

5 participants