Ensure CursorPagination respects nulls in the ordering field

[ddelange]

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.

[auvipy]

can you please make sure that the tests also pass and no regression introduced?

[ddelange]

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?

[auvipy]

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?

[ddelange]

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.

[auvipy]

you can describe that in description section too. and yes those are side-effects of this PR

[ddelange]

you can describe that in description section too

what exactly would you like me to add to the current description of the bug?

[ddelange]

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) 🤔

[ddelange]

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 👍

[ddelange]

I fixed the existing pagination tests, will add a new test that covers nulls in the ordering field.

[auvipy]

thanks, and will be looking forward to more relevant tests

[ddelange]

hi @auvipy 👋 any chance you could cut a new release any time soon?

[auvipy]

I'm waiting for Original creators response. I hope that will happen soon

[auvipy]

we have to revert this for some issues

[ddelange]

thanks for the ping, #9381 makes total sense.