Ensure CursorPagination respects nulls in the ordering field by ddelange · Pull Request #8912 · encode/django-rest-framework (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits9 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Note: Before submitting this pull request, please review our contributing guidelines.
Description
When some records contain a null for the ordering field:
- 1.year = null
- 2.year = null
- 3.year = 1995
- 4.year = 2000
Currently, if we cursor paginate with ?page_size=1&ordering=-year (the nulls will come first), we'll paginate: 1, 4, 3.
Expected behaviour: 1, 2, 4, 3.
The problem lies in the __lt/__gt filter, which implicitly filters out nulls altogether.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please make sure that the tests also pass and no regression introduced?
Hi @auvipy 👋
I saw the MockQuerySet (tests/test_pagination.py) related errors introduced by this PR, I will look into it soon.
There are also a number of errors that do not look related to this PR (isoformat datetime discrepancies etc). Could you have a look?
the CI is stable in main branch, if you fiix it, I can look into the others for sure. btw which issue you are trying to fix?
I haven't created an issue for this PR. Would you like me to create one?
the CI is stable in main branch
ok interesting, that these can be side-effects of this PR 🤔
_____________________ TestNaiveDateTimeField.test_outputs ______________________
tests/test_fields.py:674: in test_outputs
assert self.field.to_representation(output_value) == expected_output, \
E AssertionError: output value: datetime.datetime(2001, 1, 1, 13, 0)
E assert '2001-01-01T13:00:00-06:00' == '2001-01-01T13:00:00'
E - 2001-01-01T13:00:00
E + 2001-01-01T13:00:00-06:00
E ? ++++++
_________________ TestRegularFieldMappings.test_field_options __________________
tests/test_model_serializer.py:219: in test_field_options
self.assertEqual(repr(TestSerializer()), expected)
E AssertionError: "Test[316 chars]rue, max_value=9223372036854775807, min_value=[491 chars]')))" != "Test[316 chars]rue, required=False)\n default_field = Inte[302 chars]')))"
E Diff is 1073 characters long. Set self.maxDiff to None to see it.
you can describe that in description section too. and yes those are side-effects of this PR
you can describe that in description section too
what exactly would you like me to add to the current description of the bug?
yes those are side-effects of this PR
both tests/test_fields.py and tests/test_model_serializer.py are green on this branch locally btw (mac w/ cpython 3.11) 🤔
I haven't created an issue for this PR. Would you like me to create one?
the CI is stable in main branch
ok interesting, that these can be side-effects of this PR 🤔
_____________________ TestNaiveDateTimeField.test_outputs ______________________ tests/test_fields.py:674: in test_outputs assert self.field.to_representation(output_value) == expected_output, \ E AssertionError: output value: datetime.datetime(2001, 1, 1, 13, 0) E assert '2001-01-01T13:00:00-06:00' == '2001-01-01T13:00:00' E - 2001-01-01T13:00:00 E + 2001-01-01T13:00:00-06:00 E ? ++++++_________________ TestRegularFieldMappings.test_field_options __________________ tests/test_model_serializer.py:219: in test_field_options self.assertEqual(repr(TestSerializer()), expected) E AssertionError: "Test[316 chars]rue, max_value=9223372036854775807, min_value=[491 chars]')))" != "Test[316 chars]rue, required=False)\n default_field = Inte[302 chars]')))" E Diff is 1073 characters long. Set self.maxDiff to None to see it.
found out these errors also occur on master, they just don't fail tox overall 👍
I fixed the existing pagination tests, will add a new test that covers nulls in the ordering field.
thanks, and will be looking forward to more relevant tests
hi @auvipy 👋 any chance you could cut a new release any time soon?
I'm waiting for Original creators response. I hope that will happen soon
max-muoto added a commit to max-muoto/django-rest-framework that referenced this pull request
auvipy pushed a commit that referenced this pull request
we have to revert this for some issues
thanks for the ping, #9381 makes total sense.
2 participants
