Azure-sdk-for-go: Generated interfaces for the various services are not self-complete

[amshuman-kr]

Bug Report

• import path of package in question, e.g. .../services/XXX/mgmt/*/XXX/XXXapi

• SDK version master (and older releases)
  

• What happened?
  The API interface abstractions for the various services are not self-complete and cannot be used without using the corresponding concrete implementation structure. For example, the CreateOrUpdate function in the interface VirtualMachinesClientAPI returns the struct VirtualMachinesCreateOrUpdateFuture which cannot be used in any meaningful way without calling the GetResult function which takes the concrete implementation struct VirtualMachinesClient instead of the corresponding interface VirtualMachinesClientAPI. Also, the GetResult function uses the following two aspects of the concrete implementation struct VirtualMachinesClient which are not part of the VirtualMachinesClientAPI interface abstraction.

• VirtualMachinesClient struct implements the autorest.Sender which is used in the calls to future.DoneWithContext and autorest.DecorateSender and future.GetResult.

• VirtualMachinesClient supports the method CreateOrUpdateSender which is not part of the corresponding VirtualMachinesClientAPI interface.
  The generated interfaces for other services (and the methods in them) also seem to have the same problem which prevents them being used consistently without referring to the corresponding concrete implementation. This also makes it difficult to generate and use mock implementation for these interfaces in unit test cases.

• What did you expect or want to happen?
  The generated API interfaces for the various services are self-consistent and can be used without referring to the corresponding implementation structs except while instantiating the interface implementation.

• How can we reproduce it?
  N/A

• Anything we should know about your environment.
  N/A

Also, please let me know if this is the wrong repo to raise this issue and if I should raise it on the repo where the code generation happens (Azure/autorest, perhaps?).

[ArcturusZhang]

Hi @amshuman-kr thanks for this issue!

Indeed now the SDKs in services directory have this issue because the API interfaces are introduced after the structure of the generated SDK is confirmed. To change those, quite big breaking changes will be introduced to all of the packages in the services directory.

@jhendrixMSFT Could you also provide some insights? For instance, are there some minor features in track 2 that could also be backported to track 1?

[amshuman-kr]

@ArcturusZhang Thanks for the quick response!

To change those, quite big breaking changes will be introduced to all of the packages in the services directory.

Couldn't making the following changes to the generated service API interfaces solve this without breaking compatibility?

1. Make the VirtualMachinesClientAPI extend the autorest.Sender interface.
2. Add the CreateOrUpdateSender method to the VirtualMachinesClientAPI interface.
3. Change the type of the client parameter from VirtualMachinesClient struct to VirtualMachinesClientAPI interface in the GetResult method in the struct VirtualMachinesCreateOrUpdateFuture

Please note that the above changes keep the VirtualMachinesClient struct unchanged and makes only minimal change to the GetResult method in VirtualMachinesCreateOrUpdateFuture which would be backward compatible.

Something like this will need to be done for all the methods in all the generated service API interfaces and the generated future structs, of course.

[ArcturusZhang]

@amshuman-kr well, this solution has some problems.
First is that it does not make much sense to let VirtualMachinesClientAPI extend the Sender interface. But to solve the problem, maybe we could let it slide.
The biggest issue is that the third point will cause a cyclic import error. In this case, the computeapi is referencing a lot of models in the compute package, therefore the compute package cannot import computeapi package any more. If we are trying to keep the package structure, compute package cannot use anything from the computeapi. But we could put the changing of package structure under consideration.

[amshuman-kr]

First is that it does not make much sense to let VirtualMachinesClientAPI extend the Sender interface. But to solve the problem, maybe we could let it slide.

Given the current situation, I can't think of any way to avoid this other than changing the GetResult method even further (such as accepting client/sender as an additional parameter or embedding the client as a field in the VirtualMachinesCreateOrUpdateFuture).

The biggest issue is that the third point will cause a cyclic import error. In this case, the computeapi is referencing a lot of models in the compute package, therefore the compute package cannot import computeapi package any more. If we are trying to keep the package structure, compute package cannot use anything from the computeapi. But we could put the changing of package structure under consideration.

Is anyone currently using the XXXapi interfaces considering that they are not self-complete? Would the alternative of generating a fresh set of self-complete interfaces in the XXX package in a separate interfaces.go file be acceptable? That way the only existing code that would be touched would be the type of the client parameter in the GetResult function in VirtualMachinesCreateOrUpdateFuture.

[jhendrixMSFT]

I have a few ideas on how to implement this. Here are some prototypes.

Change FooFuture types to interfaces.
https://github.com/jhendrixMSFT/azure-sdk-for-go/commit/c086ae5b19d8e226c863afa96e5405017e125a25

While it's technically a breaking change, I think this is pretty compatible. It would also be the easiest way for callers to mock the future. The downside is that it affects all public surface area that uses long-running operations.

Change FooAPI interface types to return FooFuture interfaces.
https://github.com/jhendrixMSFT/azure-sdk-for-go/commit/b39713a791ed2b4476884bcb943b4b7b75974608

This is definitely a breaking change as the clients no longer satisfy the API interface (note the commented-out check on line 60) , you have to use the Adapter() funcs (GCP uses this strategy). The only benefit it has over the first is that it's pay-for-play, i.e. it only touches the mocking APIs so if you're not using them your code is not affected.

I need to think about this some more and discuss with the team. Please feel free to share your thoughts as well.

I'll also point out that for our track 2 SDK we already return interfaces for futures (called pollers in track 2), see https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/arm/compute/2020-09-30/armcompute#VirtualMachinesClient.BeginCreateOrUpdate for an example.

[jhendrixMSFT]

A variant to the second option would be to generate this as a new set of interfaces side-by-side with the existing ones.

[jhendrixMSFT]

A colleague suggested another approach which I think is best, and shouldn't be a breaking change unless you're creating your own FooFuture interface types.

https://github.com/jhendrixMSFT/azure-sdk-for-go/commit/b36fda9a3c13e12e89a1f3a75e1ea541d9a2eccb

This changes the FooFuture.Result() method to a func field on the FooFuture type; it also embeds the autorest.FutureAPI interface instead of the autorest.Future concrete type.

This way you can replace the definition of FooFuture.Result and the underlying implementation of FutureAPI with your own mocking constructs.

[amshuman-kr]

A colleague suggested another approach which I think is best, and shouldn't be a breaking change unless you're creating your own FooFuture interface types.

jhendrixMSFT@b36fda9

This changes the FooFuture.Result() method to a func field on the FooFuture type; it also embeds the autorest.FutureAPI interface instead of the autorest.Future concrete type.

This way you can replace the definition of FooFuture.Result and the underlying implementation of FutureAPI with your own mocking constructs.

Yes, this indeed seems to solve the problem without breaking existing functionality.

[jhendrixMSFT]

@alexeldeib I know you were interested in this in the past.

[amshuman-kr]

Thanks @jhendrixMSFT, @ArcturusZhang for so quickly addressing this and also making a release! ❤️