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

TypeSpec C# emitter doesn't produce LRO GetOperationStatus API #3964

Closed
devlie opened this issue Nov 21, 2023 · 6 comments · Fixed by #3981
Closed

TypeSpec C# emitter doesn't produce LRO GetOperationStatus API #3964

devlie opened this issue Nov 21, 2023 · 6 comments · Fixed by #3981
Assignees
Labels
DPG/RLC v2.1 Post Gallium work DPG GA-Required v3 Version 3 of AutoRest C# generator.
Milestone

Comments

@devlie
Copy link
Member

devlie commented Nov 21, 2023

Describe the issue or request
We are converting our Swagger to TypeSpec, and we have 2 LRO APIs, ImportDevices and ImportUpdate that each has corresponding polling API GetOperationStatus.

The API is present in old Swagger and the method was generated for our shipped SDK.

The API is ported to TypeSpec and the generated Swagger also has it, but it's not auto-generated by C# emitter.

When compiling, the backward compat checker flags them as missing:

C:\Users\leolie\.nuget\packages\microsoft.dotnet.apicompat\5.0.0-beta.20467.1\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : MembersMustExist : Member 'public Azure.Response Azure.IoT.DeviceUpdate.DeviceManagementClient.GetOperationStatus(System.String, System.Nullable<Azure.ETag>, Azure.RequestContext)'
does not exist in the implementation but it does exist in the contract. [D:\repos\azure-sdk-for-net\sdk\deviceupdate\Azure.IoT.DeviceUpdate\src\Azure.IoT.DeviceUpdate.csproj::TargetFramework=netstandard2.0]

C:\Users\leolie\.nuget\packages\microsoft.dotnet.apicompat\5.0.0-beta.20467.1\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : MembersMustExist : Member 'public System.Threading.Tasks.Task<Azure.Response> Azure.IoT.DeviceUpdate.DeviceManagementClient.GetOperationStatusAsync(System.String, System.Nullable<Az
ure.ETag>, Azure.RequestContext)' does not exist in the implementation but it does exist in the contract. [D:\repos\azure-sdk-for-net\sdk\deviceupdate\Azure.IoT.DeviceUpdate\src\Azure.IoT.DeviceUpdate.csproj::TargetFramework=netstandard2.0]

C:\Users\leolie\.nuget\packages\microsoft.dotnet.apicompat\5.0.0-beta.20467.1\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : MembersMustExist : Member 'public Azure.Response Azure.IoT.DeviceUpdate.DeviceUpdateClient.GetOperationStatus(System.String, System.Nullable<Azure.ETag>, Azure.RequestContext)' does
 not exist in the implementation but it does exist in the contract. [D:\repos\azure-sdk-for-net\sdk\deviceupdate\Azure.IoT.DeviceUpdate\src\Azure.IoT.DeviceUpdate.csproj::TargetFramework=netstandard2.0]

C:\Users\leolie\.nuget\packages\microsoft.dotnet.apicompat\5.0.0-beta.20467.1\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : MembersMustExist : Member 'public Azure.Operation Azure.IoT.DeviceUpdate.DeviceUpdateClient.StartImportUpdate(Azure.WaitUntil, Azure.Core.RequestContent, Azure.RequestContext)' does
 not exist in the implementation but it does exist in the contract. [D:\repos\azure-sdk-for-net\sdk\deviceupdate\Azure.IoT.DeviceUpdate\src\Azure.IoT.DeviceUpdate.csproj::TargetFramework=netstandard2.0]

Here's the link to generated code

@devlie devlie added v3 Version 3 of AutoRest C# generator. DPG labels Nov 21, 2023
@archerzz
Copy link
Member

@devlie It's by design that operations referred by @pollingOperation will be omitted. But I can see the use case here.

We actually don't use the operation specified in @pollingOperation. As you can see here:

Note that, for many APIs, the Url of the StatusMonitor (or resource GET) endpoint can easily be determined from the values in the initial request and response. This pattern is often followed when linking operations in OpenAPI3 specifications.

So can you try to use other ways to define your LRO? Like https://azure.github.io/typespec-azure/docs/libraries/azure-core/reference/interfaces#Azure.Core.LongRunningResourceAction That doesn't require a @pollingOperation so that your API will be generated.

@devlie
Copy link
Member Author

devlie commented Nov 22, 2023

Doesn't LongRunningResourceAction requires the POST method to return StatusMonitor object in the 202 response body? Our API doesn't do that - we simply return Operation-Location header which points to the Operation resource.

@KrzysztofCwalina
Copy link
Member

@devlie, aside compat, is there a reason why the users would need GetOperationStatus?

The user can call Operation.UpdateStatus to get the current status, can't she/he?

@chunyu3 chunyu3 added the DPG/RLC v2.1 Post Gallium work label Nov 29, 2023
@archerzz
Copy link
Member

archerzz commented Nov 29, 2023

@devlie, aside compat, is there a reason why the users would need GetOperationStatus?

The user can call Operation.UpdateStatus to get the current status, can't she/he?

@KrzysztofCwalina There are cases that users preserve the operation ID, and later on invoke explicit GetOperationStatus. For some reason, they cannot keep the original Operation instances.
Last week, I happened to be contacted by another developer trying to migrate from track1 mgmt sdk to track2. And RecoveryServices has the same pattern: https://github.com/Azure/azure-rest-api-specs/blob/a3f9d294bcdd93bdf5a475bbd6d67f3f5caf503b/specification/recoveryservicesbackup/resource-manager/Microsoft.RecoveryServices/stable/2023-04-01/bms.json#L1520

One solution is the LRO rehydration (#2158) which is not available yet.

@archerzz archerzz added this to the 2023-12 milestone Nov 29, 2023
@devlie
Copy link
Member Author

devlie commented Dec 1, 2023

@KrzysztofCwalina, sorry for the late response. It depends on what kind of LRO method DPG emits - if it only emits a blocking method (start LRO and wait till done), then exposing GetOperationStatus API is not useful; if it also emits non-blocking method (start LRO and return immediately), then obviously GetOperationStatus API is a must.

In theory, a service can show intermediate progress (e.g. % complete) in the Operation resource, so if DPG only emits blocking call, then user who wants to know/show progress cannot do that.

Specifically for our service, the only intermediate progress we show is we expose the updateId of the update being imported in the middle of the operation (the importer doesn't know it because the update can be authored by a different party).

If I were to handwrite our SDK, I would write both blocking and non-blocking LRO methods.

@archerzz
Copy link
Member

After discussion, we decided to adopt the following strategy:

  • We should generate every operation under namespaces marked with @service
    • @service means it's part of REST API.
  • So, if developer wants to expose an LRO polling operation, define it under a namespace marked with @service
  • Otherwise, define it under a namespace without @service

archerzz added a commit that referenced this issue Dec 13, 2023
* feat(dpg): generate polling operation by default

Polling operation can be both public and internal. If we by default do not
generate polling operations, then there is no way for developers to generate
public polling operations. We can use `@access` to hide polling operations, or
define LRO in other ways so that polling operation will not be defined.

- change emitter to emit polling operation by default
- regen test/samples
- add test case for lro polling operation in namespace without @service

resolve #3964

---------

Co-authored-by: Mingzhe Huang (from Dev Box) <mingzhehuang@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DPG/RLC v2.1 Post Gallium work DPG GA-Required v3 Version 3 of AutoRest C# generator.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants