Support for clientlets (partial application of common parameters in operation groups)

[xirzec]

Today when we create operation groups, it might look something like

interface OperationGroup {
   doFoo(param1: string, param2: string, options: FooOptions): Promise<ReturnShape>;
   doBar(param1: string, param2: string, options: BarOptions): Promise<ReturnShape>;
   doBaz(param1: string, param2: string, options: BazOptions): Promise<ReturnShape>;
}

Often, it would be more convenient to allow the consumer to pass param1 and param2 when retrieving the operation group, such as:

await client.operationGroup(param1, param2).doFoo(options);
// Parameterized operation groups can also be cached to be used later:
const clientlet = client.operationGroup(param1, param2);
await clientlet.doBar();

[sarangan12]

@xirzec In the above example, I have added 2 more APIs below:

interface OperationGroup {
   doFoo(param1: string, param2: string, options?: FooOptions): Promise<ReturnShape>;
   doBar(param1: string, param2: string, options?: BarOptions): Promise<ReturnShape>;
   doBaz(param1: string, param2: string, options?: BazOptions): Promise<ReturnShape>;
   doXyz(param3: string, options?: XyzOptions): Promise<ReturnShape>;
   doAbc(options?: AbcOptions): Promise<ReturnShape>;
}

const clientlet = client.operationGroup(param1, param2, param3);
await clientlet.doBar();
await clientlet.doXyz();
await clientlet.doAbc();

With the above design, I agree the code to call doBar & doXyz are optmized. But, the code to call doXyz does not achieve much. Also, the code to call doAbc does not achieve anything. Am I missing something?

Also, let me take a specific example of skillsets.ts, Within the skillSets, we have APIs such as listIndexers and getIndexer.

  createOrUpdate(skillsetName: string, skillset: SearchIndexerSkillset, options?: SkillsetsCreateOrUpdateOptionalParams): Promise<SkillsetsCreateOrUpdateResponse>;
  delete(skillsetName: string, options?: SkillsetsDeleteOptionalParams): Promise<coreHttp.RestResponse>;
  get(skillsetName: string, options?: SkillsetsGetOptionalParams): Promise<SkillsetsGetResponse>;
  list(options?: SkillsetsListOptionalParams): Promise<SkillsetsListResponse>;
  create(skillset: SearchIndexerSkillset, options?: SkillsetsCreateOptionalParams): Promise<SkillsetsCreateResponse>;

const skillset_og = searchServiceClient.skillsets(
                                          skillsetName, //For createOrUpdate/delete/get APIs
                                          skillset, //For createOrUpdate/create APIs                                          
                       )
skillset_og.create();
skillset_og.list();
skillset_og.delete();

In the above code, the create API is unique that it gets a different skillset object in each call. So, it does not make sense to set it at the beginning, even when a similar object could be called in createOrUpdate API. The same argument could be applied to the skillsetName parameter. So, how will this feature help the customer?

[xirzec]

I agree that this doesn't help much for listing/manipulating resources. I think I see it more as a way to bind state for operations that are all scoped to a particular resource.

So to me a better analogue would be if say SearchClient didn't take an index name, but looked more like:

const client = new SearchClient(endpoint, credentials);
const index = client.index("MySearchIndex");
const results = await index.search("some search text");

So instead of splitting operation groups into multiple clients, we simply parameterize them into dynamic ones.

[xirzec]

Also curious what @bterlson and @joheredi think

[sarangan12]

Also, with the latest suggestion from Brian and Michael Nash in the Arch Board review, We could achieve the same functionality that Jeff mentioned in his last comment. But, in order to achieve this, we should do the code changes in in Modelerfour. I am working with autorest crew and will provide updates on the expected timelines soon.

[joheredi]

I like the idea of having these clientlets one thing I would like to set clear before we start working on this, is how we plan to gather the information about which parameters the clientlets would get.

Do we expect these clientlet parameters to be called out in the swagger explicitly? Or do we want to be smart in the generators and try to find the common parameters to hoist?

I think having this explicitly called out in the swagger is a better approach as it takes out a lot of guessing and would allow us to achieve more consistent results.

[joheredi]

I agree that this doesn't help much for listing/manipulating resources. I think I see it more as a way to bind state for operations that are all scoped to a particular resource.

So to me a better analogue would be if say SearchClient didn't take an index name, but looked more like:

const client = new SearchClient(endpoint, credentials);
const index = client.index("MySearchIndex");
const results = await index.search("some search text");

So instead of splitting operation groups into multiple clients, we simply parameterize them into dynamic ones.

This is cool! If there are methods in the operation group that don't need any of the "common" parameters, we could have a clientlet override that takes no parameters and only has the methods that don't take those parameters, when passing common parameters you get all.

for example:

interface OperationGroup {
   doFoo(param1: string, param2: string, options?: FooOptions): Promise<ReturnShape>;
   doBar(param1: string, param2: string, options?: BarOptions): Promise<ReturnShape>;
   doBaz(param1: string, param2: string, options?: BazOptions): Promise<ReturnShape>;
   doXyz(param3: string, options?: XyzOptions): Promise<ReturnShape>;
   doAbc(options?: AbcOptions): Promise<ReturnShape>;
}

would turn into:

Typescript Playground (link)

interface Client {
    foo(param1: string, param2: string): {
            doFoo(options?: FooOptions): Promise<ReturnShape>;
            doBar(options?: BarOptions): Promise<ReturnShape>;
            doBaz(options?: BazOptions): Promise<ReturnShape>;
            doXyz(param3: string, options?: XyzOptions): Promise<ReturnShape>;
            doAbc(options?: AbcOptions): Promise<ReturnShape>;
    },
    foo(): {
            doXyz(param3: string, options?: XyzOptions): Promise<ReturnShape>;
            doAbc(options?: AbcOptions): Promise<ReturnShape>;
    }
}

declare const client: Client;
client.foo("a", "b").doBar();
client.foo("a", "b").doAbc();
client.foo().doAbc();

[xirzec]

I think having this explicitly called out in the swagger is a better approach as it takes out a lot of guessing and would allow us to achieve more consistent results.

I think we definitely need configurability in any case, so we should have good x-ms properties to support it.

Out of the box I think we could maybe default to using the operation group pattern with this and do a simple greedy algorithm like:

1. Take the first operation in the group and record its parameters as candidate group parameters
2. For the remaining 2..n operations in the group, look at their parameters and compare positionally to the candidate group parameters. If one parameter doesn't match, discard that parameter and all following parameters from the candidate group parameters
3. Exit early if the candidate group parameters reach length 0

[xirzec]

The above will at worst give us what we have today (an operation group with no parameters) and at best give us some easy wins. We can construct overloads like you show above by using special directives

[bterlson]

I agree that we will likely want overrides, but I believe C# is being successful inferring this kind of thing based on parameters. I think their algorithm is similar to what Jeff suggests.

For the overall shape, for:

  createOrUpdate(skillsetName: string, skillset: SearchIndexerSkillset, options?: SkillsetsCreateOrUpdateOptionalParams): Promise<SkillsetsCreateOrUpdateResponse>;
  delete(skillsetName: string, options?: SkillsetsDeleteOptionalParams): Promise<coreHttp.RestResponse>;
  get(skillsetName: string, options?: SkillsetsGetOptionalParams): Promise<SkillsetsGetResponse>;
  list(options?: SkillsetsListOptionalParams): Promise<SkillsetsListResponse>;
  create(skillset: SearchIndexerSkillset, options?: SkillsetsCreateOptionalParams): Promise<SkillsetsCreateResponse>;

IIUC the C# approach is something like:

interface Operations {
  create,
  list,
  skillset(id): {
    get,
    createOrUpdate,
    delete
  }
}

The drawback of this approach is that, when these are subresources, the operations like create and list conflict with properties that might exist on the parent resource. I suspect that is why there's the cosmos approach:

interface Operations {
  skillsets: {
    create,
    list,
  }
  skillset(id): {
    get,
    createOrUpdate,
    delete
  }
}

I like the second one a lot, and it has precedence in Cosmos which gets pretty good user feedback.

[xirzec]

+1 on the second more nuanced approach