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

filters for arrays #45

Merged
merged 1 commit into from
Aug 11, 2021
Merged

filters for arrays #45

merged 1 commit into from
Aug 11, 2021

Conversation

regiscamimura
Copy link
Contributor

No description provided.

@stevelacey
Copy link
Contributor

@regiscamimura what's happening here?

@regiscamimura
Copy link
Contributor Author

regiscamimura commented Aug 10, 2021

The previous version of the ListAPI would support array parameters like ?oneToMany=1&oneToMany=2, and there was specific logic to workaround those filters and it would produce a query using the __in modifier, so one_to_many__in=[1,2]. With the adoption of the django-url-filter, the syntax is different, we would do ?oneToMany__in=1,2, which we can do and use. This change is to preserve the ?oneToMany=1&oneToMany=2 format, so that gets translated to django-url-filter syntax and we can use that feature.

@stevelacey
Copy link
Contributor

stevelacey commented Aug 10, 2021

Alright, cool, it might be worth doing something like this instead so we're not string building just for django-url-filter to then parse it back to what it started as.

self.q_objects.add(Q(**{f"{key}__in": self.bundle[key]}), Q.AND)

I copied this from the search fields stuff above and maybe it's not quite right; but generally any approach where we apply the filter ourselves so we don't have to push it through the url filter parser is probably better.

Might be worth checking over line 84-90 while you're here if you have a moment, because they don't look right, is and being ignored? And is the or vs and grouping correct?

@wadewilliams
Copy link
Contributor

Why should we preserve the ?oneToMany=1&oneToMany=2 format?

There should be one-- and preferably only one --obvious way to do it.

@regiscamimura
Copy link
Contributor Author

regiscamimura commented Aug 10, 2021

I realize that we parsing and then django-url-filter parses it back, yeah. But I think it's preferably to not apply the filter ourselves after all, first, we're adding django-url-filter so we don't need to reinvent the wheel, secondly, it's good that we do not add special logic for this and that case, so I would keep the way it is.

===

Preserve the format because it complies with usual query string format for multiple parameters, syntax sugar and such. Besides, it would be an update that doesn't breaks current applications (you know which) using the framework.

@regiscamimura
Copy link
Contributor Author

refs #48

@stevelacey
Copy link
Contributor

Why should we preserve the ?oneToMany=1&oneToMany=2 format?

That’s the format that axios will use when you pass an arrays of things in, my preference is we don’t support commas, it has just come for free in django url filter.

@wadewilliams wadewilliams merged commit f94f706 into master Aug 11, 2021
@wadewilliams wadewilliams deleted the bug/array-filters branch August 11, 2021 06:10
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.

3 participants