Add support for modular LRO (new pr is #1905)

[MaryGao]

The new pr is here: #1905.
fixes #1857

If we have a modular operation which is tagged as LRO, we need to generate two functions for it.

Non-LRO function

export async function createOrReplace(
  context: Client,
  role: string,
  name: string,
  options: CreateOrReplaceOptions = { requestOptions: {} }
): Promise<User>

Relevant LRO functions:

export async function beginCreateOrReplace(
  context: Client,
  role: string,
  name: string,
  options: CreateOrReplaceOptions = { requestOptions: {} }
): Promise<SimplePollerLike<OperationState<User>, User>> 

export async function beginCreateOrReplaceAndWait(
  context: Client,
  role: string,
  name: string,
  options: CreateOrReplaceOptions = { requestOptions: {} }
): Promise<User> 

Type conversion in beginCreateOrReplace

Regarding the method beginCreateOrReplace the return type is SimplePollerLike<OperationState<User>, User> so we need somehow to convert the RLC helper returns SimplePollerLike<OperationState<RelevantRawHttpResponse>, RelevantRawHttpResponse> to it.

Convert the underlying RelevantRawHttpResponse to User.

My idea here is to leverage the LRO option processResult to do this conversion, but we still need to change the original signiture to match the return type somehow.

Proposal

To solve above type mismatch issue I use as any to cast it considering it is only used internally.

export async function beginCreateOrReplace(
  context: Client,
  role: string,
  name: string,
  options: CreateOrReplaceOptions = { requestOptions: {} }
): Promise<SimplePollerLike<OperationState<User>, User>> {
  const initialResponse = await _createOrReplaceSend(
    context,
    role,
    name,
    options
  );
  const poller = (await getLongRunningPoller(context, initialResponse, {
    processResult: (result) => {
      return _createOrReplaceDeserialize(
        result as CreateOrReplaceLogicalResponse
      ) as any;
    },
    intervalInMs: 0
  })) as SimplePollerLike<OperationState<User>, User>;

  return poller;

User-side code

https://github.com/Azure/autorest.typescript/blob/d0aa833f8b45ced08b7a049560fb053c530d96a5/packages/typespec-ts/test/integration/lroModular.spec.ts

Error handling

Like createOrReplace the exception would be thrown by isUnexpected helper.

@deyaaeldeen @joheredi Could you help review above design?

Other bugs found to be fixed:

• allowInsecureConnection option set in StandardClient is not working in operation level
• createStandard in src/api/StandardContext.ts has undefined parameter
• self-referenced model issue

[joheredi]

Great job on this!

I have two main points to share:

1. I don't think we should create the begin__AndWait operations. The reason is, it makes our API more complicated but doesn't really help our users a lot. It might save them a line of code, but it makes our API confusing because it has two methods doing the same thing. We don't do this in Track2 Data-Plane libraries, and we should avoid it here too.

2. I was thinking about using the getLongRunning poller helper, but it might not work for all our needs. It was made for Swagger, which isn't great at describing long operations. There might be things we need to do that it can't handle.

For example, I'm not sure if it can handle @pollingOperation. We need to make sure we can handle all the LRO scenarios listed here.

Instead of using the RLC helper, I suggest making a private function for each LRO operation, like this:

function _getCreateOrReplaceLongRunningPoller(context: Client, initialResponse: CreateOrReplace200Response | CreateOrReplace201Response | CreateOrReplaceLogicalResponse | CreateOrReplaceDefaultResponse, options?: CreateHttpPollerOptions<CreateOrReplaceLogicalResponse, OperationState<CreateOrReplaceLogicalResponse>): Promise<SimplePollerLike<OperationState<CreateOrReplaceLogicalResponse>, CreateOrReplaceLogicalResponse>>  {
	// Individual implementation for this operation
}

If the current helper can handle all the LRO situations we need and doesn't need extra code for each operation, we can still use it. But as far as I know, it can't do everything we need, and it would need to have a significant amount of runtime code to handle those scenarios, which will impact bundle size.

I'm also not opposed to a hybrid approach where we use the helper for the scenario we currently support and generate the private functions for the new scenarios.

A similar idea applies to Pagination when we start working on that. But in pagination we do have runtime code generated per operation, so we won't be able to use the helper at all there.

[MaryGao]

@deyaaeldeen Could you share your ideas also?

@joheredi Thanks for your insights!

1. I like the idea to not create begin__AndWait because I found for customers it would be more intuitive to call the operation directly - createOrReplace. Do you think we should have the other beginXXXX which returns a poller?

I don't think we should create the begin__AndWait operations.

2. Not sure we mixed some questions together:

   • Does the current getLongRunning helper support all cases/our needs?

   Mary: I think the answer is NO. Currently the helper only supports basic cases and there are a lot of customizations not supported yet.

   • Do we need to support most LRO cases both in modular and RLC?

   Mary: I think the answer would be YES. Both users could somehow call LRO operation successfully, the difference would be in RLC they may need some cusomizaiton code with our helper function(?).

   • How do we implement LRO in moduler?

   Mary: two options here, one is private helper for specific operation _getCreateOrReplaceLongRunningPoller, the other is RLC common helper. Personally I would prefer later one based on the understanding the question 2.

I was thinking about using the getLongRunning poller helper

[deyaaeldeen]

We decided to generate this operation in Track 2 management-plane libraries to accommodate customers who are accustomed to the Track 1 pattern of waiting operations. If this generator is meant for a different segment of customers, I would strongly be in favor of removing this operation.

[deyaaeldeen]

did you consider generating an overload that can resume from a serialized poller state? We have an example for one in @azure/ai-language-text.

[MaryGao]

Let me sync with the detailed cases when you are back.

[deyaaeldeen]

nitpick: should we remove resumeFrom and updateIntervalInMs from the options bag first?

[MaryGao]

Moved, just double confirm why we prefer to remove them first? does that mean we may add them back in future?

[deyaaeldeen]

I don't understand this line, could you please explain?

[MaryGao]

This is mainly for isUnexpected helper function for polling operation to find original url and then check the relevant status code.

I generate this by referrng the RLC helper function and notice that we have below header set:

autorest.typescript/packages/rlc-common/src/static/pollingContent.ts

Lines 75 to 76 in 648ae3d

	lroResponse.rawResponse.headers["x-ms-original-url"] =
	initialResponse.request.url;

[MaryGao]

will close this considering it's out of date and re-open a new one for modular LRO