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

feat: collect multiple Qs paramaters to list #122

Merged

Conversation

SergeTupchiy
Copy link
Contributor

@SergeTupchiy SergeTupchiy commented Mar 7, 2024

The goal is to support query strings like: clientid=a&clientid=b.
However, we probably need to apply it conditionally since it's not backward-compatible: previous implementation was silently dropping all besides the last value, while the new one will collect all values to the list.

fun({K, V}, MapAcc) ->
maps:update_with(
K,
fun([_|_] = OldV) -> [V | OldV];
Copy link
Contributor

@savonarola savonarola Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I remember (from my times being a "web developer" 😅), the common convention understood by many frameworks is to add [] when we want an array, that is, converting a[]=1&a[]=2&a[]=3 to #{a => [1,2,3]} (and a[b]=1&a[c]=2 to #{a => #{b => 1, c => 2}}).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current implementation actually corresponds to the convention in OpenAPI 3 with style = form and explode = true.

That said, if the plan is to rely on API spec later, and for example taking only the last element if the respective spec says that the parameter is not actually an array, then it sounds fine I would say.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I remember (from my times being a "web developer" 😅), the common convention understood by many frameworks is to add [] when we want an array, that is, converting a[]=1&a[]=2&a[]=3 to #{a => [1,2,3]} (and a[b]=1&a[c]=2 to #{a => #{b => 1, c => 2}}).

this is mind blowing

Copy link
Contributor Author

@SergeTupchiy SergeTupchiy Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think repeating QS params (without []) to represent lists is perfectly fine. My only concern is backward compatibility, which can be solved either in EMQX or probably in minirest itself (but we would need to pass an option to parse repeated params to lists).

Or we can simply ignore it: if someone is sending us repeated params while we don't expect it, it's probably OK to respond with 400 Bad Request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how I propose to solve backward compatibility: https://github.com/SergeTupchiy/emqx/blob/EMQX-11900-multiple-clientid-username-clients-API/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl#L463-L520

Drawbacks:

  • maybe a bit complex/over-engineered (for a quite simple task)

Advantages:

  • it relies on hocon types to decide on how to wrap/unwrap Qs params, no need to introduce any new meta field to differentiate arrays from single QS params.
  • developers can continue defining QS params schemas as before or they can simply add hocon:array(_):
hoconsc:mk(hoconsc:array(binary()), #{
   in => query,
   required => false,
   desc => <<"Client ID">>
})}
  • can handle more complex types like union(array(Type), SingleType)
  • respects custom converters, so that it's possible to define an array with a custom encoding, e. g. to parse coma separated lists

@SergeTupchiy SergeTupchiy force-pushed the collect-multiple-qs-params-to-list branch from e5dd658 to dd54aba Compare March 14, 2024 16:46
@SergeTupchiy SergeTupchiy marked this pull request as ready for review March 14, 2024 17:00
@SergeTupchiy SergeTupchiy merged commit b169cab into emqx:master Mar 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants