-
Notifications
You must be signed in to change notification settings - Fork 10
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
support commas in data_attrs query param #488
Conversation
c98824c
to
b07f433
Compare
But, when we discussed this, you said the ," is still seen as a comma and parsed early on? |
@joeribekker Yes, but then I realized that we still can treat |
8bfcc82
to
47bd81f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, looks good overall 👍
* only one filtering expression is allowed | ||
|
||
If you want to use several filtering expressions, just use this `data_attr` several times in the query string. | ||
Example: `data_attr=height__exact__100&data_attr=naam__icontains__boom` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicky, but ideally explode: true
would be specified here to indicate that it should be used like ?data_attr=...&data_attr=...
(see https://swagger.io/docs/specification/v3_0/serialization/#query-parameters)
Although looking at it, apparently explode: true
is considered the default by OAS3, so I'm not sure if our schema is correct in that case 🤔. Might be something to investigate in another issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it to the OAS and created issue to investigate all filters #498
Fixes #472
We use comma as a separator between
data_attrs
valuesIf we want to support commas inside values we can consider several options:
Remove several value groups, separated by commas, so comma won't be a reserved character here
Before:
?data_attrs=naam__icontains__anna,naam__icontains__Advies
After:
?data_attrs=naam__icontains__anna&data_attrs=naam__icontains__Advies, support
This is my personal favorite option, but it will break current API
Change separator from comma to other character, for example to
|
.Before:
?data_attrs=naam__icontains__anna,naam__icontains__Advies
After:
?data_attrs=naam__icontains__anna|naam__icontains__Advies, support
Also a nice option, but also breaks current API
Escape comma in the value part or use other char instead of comma in the value
Here comma in the value is replaced with
\,
Before:
?data_attrs=naam__icontains__anna,naam__icontains__Advies
After:
?data_attrs=naam__icontains__anna,naam__icontains__Advies\, support
This options is implemented in the PR, since it doesn't break the API
Use some regex magic to define when comma is used as separator and when it's used as a part of value.
Let's not do it, since it's very tricky and unclear what happens
What do you think is the best way to implement it?
Should we introduce the breaking change? The next big release already contains a breaking change, since we remove v1 endpoints
@joeribekker @stevenbal @SonnyBA @Coperh let's discuss it