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

Don't strip empty query params when paginating #4260

Merged
merged 1 commit into from
Aug 12, 2016

Conversation

npars
Copy link
Contributor

@npars npars commented Jul 13, 2016

Description

Fixes issue with pagination when the url contains empty query params. The previous implementation would strip out any empty query params from the next and previous urls. This PR fixes the query parsing and ensures that empty query params are preserved.

This possibly fixes #3328.

- Add `keep_blank_values=True` to `urlparse.parse_qs` calls
@codecov-io
Copy link

codecov-io commented Jul 13, 2016

Current coverage is 91.23%

Merging #4260 into master will not change coverage

@@             master      #4260   diff @@
==========================================
  Files            52         52          
  Lines          5776       5776          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           5270       5270          
  Misses          506        506          
  Partials          0          0          

Powered by Codecov. Last updated by 06751f8...bf3ed2a

@tomchristie
Copy link
Member

The previous implementation would strip out any empty query params from the next and previous urls.

Why is that a problem?
The current behavior as describes sounds more desirable to me on first sight.

Closing for now, but may consider reopening given sufficient cause.

@npars
Copy link
Contributor Author

npars commented Jul 14, 2016

The request object received by the View treats empty query params and missing query params differently:

GET http://testserver/?filter=

(Pdb) repr(request.query_params.get('filter'))
"u''"

GET http://testserver/

(Pdb) repr(request.query_params.get('filter'))
'None'

This means that when paginating the next and previous pages could contain a completely different data set from the current query if the API is relying on this behaviour.

We currently have an implementation that uses this behaviour for passing search strings into the API. An empty search string is considered valid and returns different results than when the search param is not provided at all. As a result of this only the first page from DRF is valid, all subsequent pages contain the incorrect dataset.

@tomchristie tomchristie reopened this Jul 14, 2016
@tomchristie
Copy link
Member

Gotcha - thanks for clarifying.

@tomchristie
Copy link
Member

Okeydokes - this looks good to me. Thanks for your work on it! 😄

Closes #4392
Closes #4393

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cursor Pagination doesn't play well with query parameters
3 participants