Skip to content

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

@lovelydinosaur
Copy link
Contributor

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.

@lovelydinosaur
Copy link
Contributor

Gotcha - thanks for clarifying.

@lovelydinosaur
Copy link
Contributor

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