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

Ability to add a protocol method to a HLC SDK #1970

Closed
lmazuel opened this issue Feb 11, 2022 · 8 comments
Closed

Ability to add a protocol method to a HLC SDK #1970

lmazuel opened this issue Feb 11, 2022 · 8 comments
Assignees
Labels
blocking-release Blocks release v3 Version 3 of AutoRest C# generator.

Comments

@lmazuel
Copy link
Member

lmazuel commented Feb 11, 2022

There are two approaches:

  • Rebase a SDK on top of DPG, which makes new generation add protocol methods natively
  • Find a way to generate some methods in protocol mode and expose them

Solution needs to be cost effective, since this is designed to scale fast new methods that don't necessarly requires immediate wrapping with strong design.

@lmazuel lmazuel added v3 Version 3 of AutoRest C# generator. blocking-release Blocks release labels Feb 11, 2022
@annelo-msft
Copy link
Member

Just FYI, we are currently working on this to solve this issue for storage: Azure/azure-sdk-for-net#25626
I'm working on the Core pieces, and @ShivangiReja is working on the changes to the generator.

We are currently working on the second approach (selectively adding a protocol method).
We would like to address the first approach as well, but may not do so this sprint.

@annelo-msft
Copy link
Member

What we've done is make it possible to configure an OperationId and the generator will add a protocol method for that operation, in an existing HLC SDK. @lmazuel, we have not addressed your first bullet point. Is this sufficient, or is this issue asking for us to do both bullet points?

@ShivangiReja
Copy link
Member

Hi @lmazuel, for this task we will already have HLC SDK generated and we need to generate protocol methods on the top of that. I've some questions about the expectations of this task:

  • Do we need to generate public protocol methods or internal protocol methods which will be called by public APIs?
  • Do we need to generate all the methods as protocol methods aur just some methods based on the some configuration?

@lmazuel
Copy link
Member Author

lmazuel commented Mar 11, 2022

What we've done is make it possible to configure an OperationId and the generator will add a protocol method for that operation, in an existing HLC SDK. @lmazuel Laurent Mazuel FTE, we have not addressed your first bullet point. Is this sufficient, or is this issue asking for us to do both bullet points?

One of the two, so we're good :)

Do we need to generate public protocol methods or internal protocol methods which will be called by public APIs?

Public protocol method, based on some configuration like @annelo-msft just said

Do we need to generate all the methods as protocol methods aur just some methods based on the some configuration?

This would be unfortunate that in order to expose one method of the Swagger as protocol, we are required to expose all of them. So just some methods based on configuration.

We should aim to a world where we always generate protocol methods:

  • Some are internal, because we're HLC-ing them
  • Some public as-is
    But that's outside of the scope of this issue, and long term vision

@ShivangiReja
Copy link
Member

ShivangiReja commented Mar 11, 2022

Based on @lmazuel's comment we need to generate public protocol methods. For storage we have generated internal protocol methods in RestClient following #1970 (comment) approach.

@annelo-msft - I've some questions:

  • Where should we generate public protocol methods based on the configuration? should we generate public DPGClient and only generate those methods in DPGClient which are mentioned in the config?
    • If we want to generate public protocol methods in DPGClient, then we can not use DPGClient name same as we generate today, otherwise it will conflict with HLCClient name.
  • Should we keep generating internal protocol methods in RestClient for storage?
    • If yes, then we have to use different config for generating public protocol methods than protocol-method-list otherwise, it will generate internal protocol methods in RestClient as well as public protocol methods for storage.

@annelo-msft
Copy link
Member

Where should we generate public protocol methods based on the configuration?

What would it take to generate them directly onto the HLCClient? Is that possible if the HLCClient is hand-written with an inner RestClient?

If that doesn't make any sense, then we could just add them to the DPG inner client, rename it (it's internal anyway, so the name isn't super important) -- maybe <service>ProcotolClient and call through from the HLCClient to the DPG Client protocol methods, e.g.

public virtual Response GetProduct(RequestContent content, RequestContext context = null) => _protocolClient.GetProduct(content, context);

Should we keep generating internal protocol methods in RestClient for storage?

I think we should ... would it be possible to update the protocol-method-list so we could specify whether each method is internal or public?

@ShivangiReja
Copy link
Member

ShivangiReja commented Mar 12, 2022

What would it take to generate them directly onto the HLCClient?

Sounds good to me!

Here is what I think based on your thoughts -

  • Keep protocol-method-list config and generate all the protocol methods listed in protocol-method-list in RestClient.
  • For HLC SDK, if public-clients: true it generates public client with public methods in it. So we can leverage public-clients: true and add public protocol methods listed in protocol-method-list config in public HLC generated client. Most of the public clients are handwritten for HLC SDK, if that's the case for any SDK then dev will have to handwrite the protocol methods and call the RestClient, just like HLC methods.

Some benefits of using this approach are:

  • We will follow the same approach of generating and using protocol methods like HLC methods.
  • We will not have to introduce any new classes. We will use the existing HLC public client to generate public protocol methods and RestClient to generate internal protocol methods.
  • We will not have to change anything in protocol-method-list config.
  • We will not have to change anything for storage.

@annelo-msft
Copy link
Member

Since only a few non-GA libraries are using public-clients: true, we'll postpone doing that part of the work until we have a clear customer need for it.

It is common for hand-written HLC clients to call through to methods on the RestClient. Since we already have that functionality (to add protocol methods to the inner RestClient), I'll close this issue for now and we can revisit it if we find another approach is needed.

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

No branches or pull requests

3 participants