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

Order changeset elements for consistent pagination #5209

Merged

Conversation

tomhughes
Copy link
Member

Changeset elements in the history view are being paginated but no order is being specified so stepping through the pages may not give consistent results and may skip elements or show them twice - this is currently visible in the form of occasional test failures in test_show_paginated_element_links when it doesn't show elements in the order create_list created them.

@AntonKhorev
Copy link
Collaborator

This was suggested in #4571 (comment) but I haven't done it because we don't have appropriate indexes. This requires reading all changeset node/ways/relations and sorting them. We also have counts of changeset elements but at least counting doesn't require sorting.

Also if you want the pagination here to be closer to like it is for traces, diary entries etc (#5205 (comment)), that's done with sorting by some id. We can't sort changeset elements just by id because ids are not unique, you can have several versions of the same element. That's why this PR sorts by (id, version) but it's not quite the same as in other places. What would have made it nearly the same is something like #4660.

@tomhughes
Copy link
Member Author

Well what I mostly want in the short term is for the tests to be reliable, but the reason they're not reliable is that the web site is not reliable - next and previous may not step through the objects reliably.

As we're paginating this data with a full count of pages we're already reading it all anyway so having to sort it doesn't change much.

I don't care much what the order is so long as it's stable which I'm not sure timestamp+version is (though adding id as a third element would guarantee stability).

@mmd-osm
Copy link
Contributor

mmd-osm commented Sep 16, 2024

Timestamp + version is not stable. cgimap does mass inserts/updates, where elements have the exact same timestamp down to the µs.

@AntonKhorev AntonKhorev merged commit f90c7c4 into openstreetmap:master Sep 18, 2024
24 checks passed
@AntonKhorev
Copy link
Collaborator

we're already reading it all anyway so having to sort it doesn't change much

I can't check this because I don't know what's the distribution of requested changesets and their timings. Let's merge this and see. Maybe everything is going to work fast enough, then we don't need to bother with extra indexes.

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

Successfully merging this pull request may close these issues.

4 participants