search

openstreetmap / openstreetmap-website

The Rails application that powers OpenStreetMap
https://www.openstreetmap.org/
GNU General Public License v2.0
2.17k stars 910 forks source link

Order changeset elements for consistent pagination #5209

Closed tomhughes closed 1 week ago

tomhughes commented 2 weeks ago

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 commented 1 week ago

This was suggested in https://github.com/openstreetmap/openstreetmap-website/pull/4571#issuecomment-1994856482 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 (https://github.com/openstreetmap/openstreetmap-website/pull/5205#issuecomment-2346879691), 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 commented 1 week ago

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 commented 1 week ago

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

AntonKhorev commented 1 week ago

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.