fix: make API list ordering deterministic - #1642
Conversation
|
Hi @FallenDeity Can you chime in and review this PR? Maybe I'm wrong but your current re-implementation is already preserving a custom-order of the fields, am I wrong? On my PC when building the current If this PR enforces the order or a specific sorting strategy we can merge it in! @MiquelRForgeFlow Thanks for the PR! |
Naramsim
left a comment
Naramsim
left a comment
There was a problem hiding this comment.
I trust the tests on this PR. I checked it out locally and everything seems to run
|
I remember trying to do this years ago. One of those ones where you see a solution and realise "damn, it was that easy?" Will make reviewing the pokeapi-data diffs in the future far easier 🙏 |
i did not do anything for order in my prior pr it was mainly just serializer query optimization, I have a few thoughts on why the same order is maintained few of the serializers already have order_by where now pk has been added as a fallback and also we freshly load the data in sqlite/postgres before serving which might tend to return the rows in same physical order its defined in the csv since there is no db reorg happening this change is limited to the nested json lists and makes the behaviour consistent like this for example now it will call the query set on the manager with the ditto change i am working on it should make diff viewing a lot easier yeah awesome work :) few reviews
- BerryFlavorMap.objects.filter(berry_flavor=obj, potency__gt=0).select_related("berry").order_by("potency")
+ BerryFlavorMap.objects.filter(berry_flavor=obj, potency__gt=0)
.select_related("berry")
.order_by("potency", "berry_id")
)also add a comment in the manager that if distinct() is used the fields for both order_by() and distinct() must be same https://stackoverflow.com/questions/9795660/postgresql-distinct-on-with-different-order-by |
Most ORM queries had no explicit ORDER BY, so the database was free to
return rows in any order. The same dataset could then be serialized
differently on every build, turning each api-data regeneration into a
huge diff of pure reordering noise.
Give PokeApiModel a default manager that orders by primary key. Managers
are inherited from abstract bases and reverse relation managers derive
from the model's default manager, so this covers plain queries, reverse
relations and prefetches at once, including the declarative reverse
fields (Generation, GrowthRate, PokemonColor, PokemonShape,
PokemonHabitat) that no serializer call site could order.
Add tie-breakers to the partial orderings that left equal keys undefined,
ending every order_by() with the primary key: pokemon moves and held
items, location area and pokemon encounters, past abilities, stats and
types, form and pokemon types, berry flavors, pokedex entries and
encounter method rates. The few calls left untouched already order by
order_by("id"), which is the primary key.
Document in the manager that a distinct(*fields) call has to spell out a
matching order_by(), since PostgreSQL requires the leading ORDER BY
expressions to match the DISTINCT ON ones. A plain distinct() is fine,
because the primary key is already part of the selected row.
Add two regression tests that enforce the ordering, one for a declarative
reverse relation and one for rows tying on the whole ORDER BY. Both insert
rows with primary keys out of order, so both fail without this fix.
345e774 to
e0e0362
Compare
|
Thanks for the reviews! All three points addressed in the amended commit:
|
4 participants
Most ORM queries had no explicit ORDER BY, so the database was free to return rows in any order. The same dataset could then be serialized differently on every build, turning each api-data regeneration into a huge diff of pure reordering noise.
Give PokeApiModel a default manager that orders by primary key. Managers are inherited from abstract bases and reverse relation managers derive from the model's default manager, so this covers plain queries, reverse relations and prefetches at once, including the declarative reverse fields (Generation, GrowthRate, PokemonColor, PokemonShape, PokemonHabitat) that no serializer call site could order.
Add tie-breakers to the partial orderings that left equal keys undefined, ending every order_by() with the primary key: pokemon moves and held items, location area and pokemon encounters, past abilities, stats and types, form and pokemon types, berry flavors, pokedex entries and encounter method rates. The few calls left untouched already order by order_by("id"), which is the primary key.
Document in the manager that a distinct(*fields) call has to spell out a matching order_by(), since PostgreSQL requires the leading ORDER BY expressions to match the DISTINCT ON ones. A plain distinct() is fine, because the primary key is already part of the selected row.
Add two regression tests that enforce the ordering, one for a declarative reverse relation and one for rows tying on the whole ORDER BY. Both insert rows with primary keys out of order, so both fail without this fix.
No migration needed; call sites needing another order still override it.
AI coding assistance disclosure
Of course, the PokeApiManager was AI idea. Also used to check nothing is left out.
Contributor check list