Demo and fix broken pagination #7889

sonthonaxrk · 2021-03-31T08:48:29Z

Originally discussed in #7888

This demonstrates how the pagination is broken if someone tries to order by non_unique_field,id. The only case where it does work is when the offset_cutoff is the same as the page_size.

$ pytest tests/test_cursor_pagination.py -v

tests/test_cursor_pagination.py::test_filtered_items_are_paginated[page_size_divisor_of_offset] FAILED                                             [ 20%]
tests/test_cursor_pagination.py::test_filtered_items_are_paginated[page_size_multiple_of_offset] FAILED                                            [ 40%]
tests/test_cursor_pagination.py::test_filtered_items_are_paginated[page_size_uneven_divisor_of_offset] FAILED                                      [ 60%]
tests/test_cursor_pagination.py::test_filtered_items_are_paginated[page_size_uneven_multiple_of_offset] FAILED                                     [ 80%]
tests/test_cursor_pagination.py::test_filtered_items_are_paginated[page_size_same_as_offset] PASSED                                                [100%]

sonthonaxrk · 2021-03-31T14:59:30Z

tests/test_pagination.py

-        assert next is None
-        assert previous_url is not None
-        assert next_url is None
-


This test was a bit too complicated to test with the MockQueryset.

sonthonaxrk · 2021-03-31T15:00:05Z

tests/test_pagination.py

@@ -884,7 +919,7 @@ def test_cursor_pagination_with_page_size_negative(self):

        assert previous == [4, 4, 5, 6, 7]
        assert current == [7, 7, 7, 7, 7]
-        assert next == [8, 9, 9, 9, 9]  # Paging artifact
+        #assert next == [8, 9, 9, 9, 9]  # Paging artifact


I'm pretty sure these 'paging artefacts' have been fixed by this.

sonthonaxrk · 2021-03-31T15:01:16Z

tests/test_pagination.py

+
+            return _statements
+
+        return lambda item: operator_map[q_object.connector](l(item) for l in _parse(q_object))


I'm not a massive fan of 'MockQuerysets' because we're really reimplementing stuff that's handled by the database. But I tried to keep the pattern of tests as they are.

tomchristie · 2021-04-01T09:18:07Z

Okay, so this PR falls a long way into our "no" territory.

You've spent time and care on this, so I'll try to help explain some of why our outlook on this as maintainers differs to yours.

This demonstrates how the pagination is broken if someone tries to order by non_unique_field,id.

So, the pull request doesn't demonstrate much to me because it's a ~430 line pull request that'd require a huge amount of time to fully review, for a use case that doesn't really meet the documented constraints of the CursorPagination.

To give some contrast, here's a one-liner pull request, that seemed pretty reasonable (use deque instead of list, for performance reasons) which we accepted, and then reverted once it turned out that it actually broke some existing usages for unexpected reasons.

Consider the amount of time we need to spend in order to fully assess if a one line pull request, with a clear motivation. Then contrast it with what's being requested here. I'd hope that'd help make it more clear that large scoped pull requests like this just aren't realistic, particularly when the motivation and understanding of the trade-offs it introduces haven't first been adequately addressed in discussion.

If you're invested in putting time into this, then the best route is as a third party package, which we would then link to from the main docs... https://www.django-rest-framework.org/api-guide/pagination/#third-party-packages

sonthonaxrk · 2021-04-02T13:43:43Z

@tomchristie I get that it is a little bit subtle and doesn't happen in all cases; while I also understand that you probably have 100s of inexperienced developers vying for your time who have no idea what they're doing.

I know this stuff is taxing to read. As per the project guidelines, I submitted a pull request, first with the failing test, then another commit with the fix. I even linked the individual commits (for the problem, and the fix). But I don't think anyone even took the time to examine how it's broken (because it is hard work and people have priorities). I feel that I've received a knee-jerk response that's essentially: it's not a problem; and that even if it was, it is not; and that I should read the documentation.

I totally get the tedium of managing a package that's used by thousands, and how APIs are fragile. I've managed packages too. However, it would have been nice to at least have had the reaction "yes this is bug, but we can't fix it because of x". With the utmost respect, I'm actually working on a project that you previously consulted on, and this is an edge case that I think you were personally caught out by (and partly why I felt that I should contribute back).

If there's no recognition that is a 'wontfix' or a 'bug', what's the point in me making a 3rd party package? Someone might encounter this issue, but all they'll see is two maintainers denying this is a problem.

If it would help, I'm more than happy to spend more time trying to make a shorter pull request that reduces the number of testing changes. I'm also more than happy to jump of a meets/zoom/skype etc to pair with you to help bring you through the problem.

The only reason that I added more commits was that my change broke the old tests (because the tests seem to both run as mocks and with real objects). Because I was told that I was 'using it wrong' and not reading the documentation, I started to doubt myself, and spent a few hours verifying that my change was in fact okay. I haven't meaningfully changed the tests. In fact I think I fixed a few subtle bugs that are labeled 'paging artefact'.

tomchristie · 2021-04-02T14:11:06Z

Ok noted. I'm too busy to have a second look over this right now, but can take a re-review next week sometime.

I guess the helpful thing from my POV would be starting with an absolutely minimal PR, that demonstrates a breakage with as absolutely small a change footprint as possible. (Although I realise that's asking for more work on your side)

sonthonaxrk · 2021-04-02T14:17:36Z

Thanks @tomchristie, I really appreciate that. I apologise for getting frustrated there. I also understand that contributing to open source takes time, and that problems often languish for months before they can be merged

In regards to the breakage, I tried to keep it small (while being comprehensive). There's only one test there, I just I would be able to write clearer code if the problem was encapsulated in a single file.

https://github.com/sonthonaxrk/django-rest-framework/blob/29d8796b1d96cbe77ecd81663ee7afbace0229e0/tests/test_cursor_pagination.py

But I'll try and condense it down ASAP into test_pagination.py.

Personally, I think it's worth at least investigating further. Paginated endpoints are often critical for clients who want to fetch all of their data, and a breakage particularly in that use case can have major consequences for businesses.

Demo broken pagination

2375f6c

sonthonaxrk force-pushed the tests/broken-pagination branch from 3ee55cd to 6bcd2f8 Compare March 31, 2021 09:22

sonthonaxrk changed the title ~~Demo broken pagination~~ Demo and fix broken pagination Mar 31, 2021

sonthonaxrk force-pushed the tests/broken-pagination branch from 6bcd2f8 to 75dc267 Compare March 31, 2021 09:24

Fix broken pagination

9408b43

sonthonaxrk force-pushed the tests/broken-pagination branch from 75dc267 to 9408b43 Compare March 31, 2021 09:30

Fix normal tests

2171081

sonthonaxrk commented Mar 31, 2021

View reviewed changes

rollo-b2c2 added 4 commits March 31, 2021 16:27

pep8

91f9677

Python3.5 compat

99bd5b0

More pep8

9412ed1

isort

29d8796

sonthonaxrk force-pushed the tests/broken-pagination branch from 2f7acbf to 29d8796 Compare March 31, 2021 18:15

tomchristie closed this Apr 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Demo and fix broken pagination #7889

Demo and fix broken pagination #7889

Uh oh!

sonthonaxrk commented Mar 31, 2021 •

edited by tomchristie

Loading

Uh oh!

sonthonaxrk Mar 31, 2021

Uh oh!

sonthonaxrk Mar 31, 2021

Uh oh!

sonthonaxrk Mar 31, 2021

Uh oh!

tomchristie commented Apr 1, 2021 •

edited

Loading

Uh oh!

sonthonaxrk commented Apr 2, 2021 •

edited

Loading

Uh oh!

tomchristie commented Apr 2, 2021

Uh oh!

sonthonaxrk commented Apr 2, 2021 •

edited

Loading

Uh oh!

Uh oh!


		return _statements

		return lambda item: operator_map[q_object.connector](l(item) for l in _parse(q_object))

Uh oh!

Demo and fix broken pagination #7889

Demo and fix broken pagination #7889

Uh oh!

Conversation

sonthonaxrk commented Mar 31, 2021 • edited by tomchristie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonthonaxrk Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

sonthonaxrk Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

sonthonaxrk Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

tomchristie commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonthonaxrk commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomchristie commented Apr 2, 2021

Uh oh!

sonthonaxrk commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sonthonaxrk commented Mar 31, 2021 •

edited by tomchristie

Loading

tomchristie commented Apr 1, 2021 •

edited

Loading

sonthonaxrk commented Apr 2, 2021 •

edited

Loading

sonthonaxrk commented Apr 2, 2021 •

edited

Loading