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-client] array params methods should use append instead of set #31

Closed
warmans opened this issue Mar 2, 2018 · 3 comments · Fixed by #38
Closed

[api-client] array params methods should use append instead of set #31

warmans opened this issue Mar 2, 2018 · 3 comments · Fixed by #38
Labels
enhancement Request for a change or improvement of existing functionality help wanted This issue is open for contributions in form of PR or specification

Comments

@warmans
Copy link
Contributor

warmans commented Mar 2, 2018

here is an example swagger parameter definition:

{
  "type": "array",
  "items": {
    "type": "string"
  },
  "name": "filters",
  "in": "query"
}

and here is what gets generated for that parameter

  listThings(filters: string[], options?: HttpOptions): Observable<models.V2RuleList> {
    const path = `/api/v2/things`;
    options = {...this.options, ...options};

    if (filters) {
      options.params = options.params.set('filters', String(filters));
    }
    return this.sendRequest<models.V2ThingList>('GET', path, options);
  }

The filters array ends up being joined with commas.

What should happen:

The generated code for the param should look more like this:

    if (filters) {
      filters.forEach(v => {
        options.params = options.params.append('filters', v);
      });
    }

This is unfortunately arguable since apparently there is not a clear standard. However multiple params with the same name is the most and reliable approach AFAIK so it should probably be used.

The functionality could be hidden behind a flag for backwards compatibility i.e. it uses set by default but append if --append-array-params is supplied during generation.

@vmasek vmasek added the enhancement Request for a change or improvement of existing functionality label Mar 3, 2018
@vmasek vmasek changed the title Array Params Should use Append [api-client] array params methods should use append instead of set Mar 3, 2018
@vmasek vmasek added the help wanted This issue is open for contributions in form of PR or specification label Mar 3, 2018
@vmasek
Copy link
Member

vmasek commented Mar 3, 2018

I think the best will be to have the option that will switch the set for append as you proposed

@warmans
Copy link
Contributor Author

warmans commented Mar 5, 2018

OK I'll probably work on a PR for this later today.

@vmasek
Copy link
Member

vmasek commented Mar 5, 2018

In beta branch I have refactored the code and split it to separate functions, I am also planning to add linting. You can wait till it is in master so I wouldn't mess up your work with conflicts.

vmasek pushed a commit that referenced this issue Mar 12, 2018
* Default to relative path instead of localhost for domain.

* Remove window declare (not needed). Use shorter syntax for setting default address

* Implement default hostname in generator instead of template

* Fixes to domain generation

* Added commnets to generator

* Undo unintended change to template

* Fixed import path to APIClient

* Fixed #31 - array parameters not being added correctly. Rebuilt generated test code.

* Update parser.ts

* test(gcloud-firestore): any corrected to string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Request for a change or improvement of existing functionality help wanted This issue is open for contributions in form of PR or specification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants