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

api surface revamp for query parameters #1494

Closed
baywet opened this issue Apr 4, 2022 · 6 comments · Fixed by #1520
Closed

api surface revamp for query parameters #1494

baywet opened this issue Apr 4, 2022 · 6 comments · Fixed by #1520
Assignees
Labels
Csharp Pull requests that update .net code generator Issues or improvements relater to generation capabilities. Go Java TypeScript Pull requests that update Javascript code

Comments

@baywet
Copy link
Member

baywet commented Apr 4, 2022

The current generation process results in the following request executor methods. (this is an exhaustive sample)

PostAsync(requestBody, 
   q => {
       q.Select = new string[] {"displayName"};
       q.Count = true;
   },
   h => { h.Add("ConsistencyLevel", "eventual"); },
   o => { o.Add(new RetryHandlerOptions()); },
   responseHandler);

Except for Go which uses a wrapper class because the language doesn't support overloads any other way than through naming conventions.

This solution is a problem as query parameters are not always described, which means the parameter is not always added. If a parameter is described in a later generation, and if consumers don't/can't use named parameters, this will end-up as a breaking change. And we don't want to have to major rev for such minor changes.

This issue proposes to use a configuration wrapper object for: query parameters, requestHeaders, and requestOptions. And leave the body and the response handler outside of that wrapper object. This would have the following advantage:

  • adding query parameters is no longer a breaking change
  • closer to idiomatic usage in TS/java/Go
  • easier access in dotnet (fewer actions)
  • easier access to the request body and response handler parameters in go (currently included in the wrapper) if no option/header/query param needs to be specified.

Example usage:

PostAsync(requestBody, config => {
   config.Query.Select = new string[] {"displayName"};
   config.Query.Count = true;
   config.Headers.Add("ConsistencyLevel", "eventual");
   config.Options.Add(new RetryHandlerOptions());
}, responseHandler);

We have considered this alternative (removing the action) but it was ruled out so the generation can pre-populate some values in the future if required.

PostAsync(requestBody, new() {
   Query = new () {
      Select = new string[] {"displayName"},
      Count = true,
   }),
   Headers = new() {
      { "ConsistencyLevel", "eventual"},
   },
   Options = new() {
      { new RetryHandlerOptions()) }
   }
}, responseHandler);
@baywet baywet added TypeScript Pull requests that update Javascript code Go Java generator Issues or improvements relater to generation capabilities. labels Apr 4, 2022
@baywet baywet self-assigned this Apr 4, 2022
@baywet baywet added this to Kiota Apr 4, 2022
@baywet baywet moved this to Todo in Kiota Apr 4, 2022
@nikithauc
Copy link
Contributor

In TypeScript, the convention is to use an interface for config options and the config option could go in abstractions. Example:

interface config{
header: Headers,
options: RequestOptions
}

@baywet baywet moved this from Todo to In Progress in Kiota Apr 11, 2022
@baywet baywet added the Csharp Pull requests that update .net code label Apr 11, 2022
@baywet
Copy link
Member Author

baywet commented Apr 11, 2022

@nikithauc I think both can be achieved, I'll start by restructuring things to a single parameter using a class with properties like described above. And through your work in #1013 you can replace that class by an interface in a refiner to provide a better experience to end users.

@nikithauc
Copy link
Contributor

Curious on what is the purpose of using a class here?

@baywet
Copy link
Member Author

baywet commented Apr 12, 2022

Because most languages do not allow object notation to infer to a type instance and/or require a class to implement the interface. The only exception to date being typescript and maybe ruby/Python

@nikithauc
Copy link
Contributor

I would create another issue separate from the models interface work so that this is tracked. The scopes for the issues vary.

@baywet
Copy link
Member Author

baywet commented Apr 13, 2022

sure, go ahead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Csharp Pull requests that update .net code generator Issues or improvements relater to generation capabilities. Go Java TypeScript Pull requests that update Javascript code
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants