From 42d9fe1b52111af271ec6b589db005aefd4b1b2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miquel=20Ra=C3=AFch?= Date: Thu, 20 Aug 2026 10:39:02 +0200 Subject: [PATCH] fix: make API list ordering deterministic 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. --- pokemon_v2/api.py | 2 +- pokemon_v2/models.py | 24 +++++++++++++-- pokemon_v2/serializers.py | 32 +++++++++++--------- pokemon_v2/tests.py | 62 +++++++++++++++++++++++++++++++++++++-- 4 files changed, 101 insertions(+), 19 deletions(-) diff --git a/pokemon_v2/api.py b/pokemon_v2/api.py index 75039128c..b04d577b1 100644 --- a/pokemon_v2/api.py +++ b/pokemon_v2/api.py @@ -1021,7 +1021,7 @@ def get(self, request: Request, pokemon_id: int) -> Response: "encounterconditionvaluemap_set__encounter_condition_value", "encounterpokemondetail_set", ) - .order_by("location_area_id", "version_id", "encounter_slot_id") + .order_by("location_area_id", "version_id", "encounter_slot_id", "pk") ) grouped_data: list[dict[str, Any]] = [] diff --git a/pokemon_v2/models.py b/pokemon_v2/models.py index 7998e1a7c..a6b34e672 100644 --- a/pokemon_v2/models.py +++ b/pokemon_v2/models.py @@ -1,10 +1,12 @@ # pyright: reportIncompatibleVariableOverride=false from __future__ import annotations -from typing import Any +from typing import Any, ClassVar, TypeVar from django.db import models -from typing_extensions import override +from typing_extensions import Self, override + +_ModelT = TypeVar("_ModelT", bound=models.Model) __all__: tuple[str, ...] = ( "Ability", @@ -165,6 +167,7 @@ "PalPark", "PalParkArea", "PalParkAreaName", + "PokeApiManager", "PokeApiModel", "PokeathlonStat", "PokeathlonStatName", @@ -230,7 +233,24 @@ ############################ +class PokeApiManager(models.Manager[_ModelT]): + """Default manager ordering every queryset by primary key, so that the API + serializes lists deterministically. Call sites needing another order + override it with ``order_by()``. + + 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 unaffected. + """ + + @override + def get_queryset(self) -> models.QuerySet[_ModelT]: + return super().get_queryset().order_by("pk") + + class PokeApiModel(models.Model): + objects: ClassVar[PokeApiManager[Self]] = PokeApiManager() + class Meta: abstract = True diff --git a/pokemon_v2/serializers.py b/pokemon_v2/serializers.py index 430c1c385..fb85c66cb 100644 --- a/pokemon_v2/serializers.py +++ b/pokemon_v2/serializers.py @@ -1053,7 +1053,7 @@ def get_method_rates(self, obj: LocationAreaEncounterRate) -> ReturnList[ReturnD rates = ( LocationAreaEncounterRate.objects.filter(location_area=obj, encounter_method__isnull=False) .select_related("encounter_method", "version") - .order_by("encounter_method_id") + .order_by("encounter_method_id", "version_id", "pk") ) grouped_rates: list[dict[str, Any]] = [ { @@ -1081,7 +1081,7 @@ def get_encounters(self, obj: LocationArea) -> ReturnList[ReturnDict[str, Any]]: "encounterconditionvaluemap_set", "encounterpokemondetail_set", ) - .order_by("pokemon_id", "version_id") + .order_by("pokemon_id", "version_id", "encounter_slot_id", "pk") ) grouped_data: list[dict[str, Any]] = [] @@ -1597,7 +1597,7 @@ def get_held_by_pokemon(self, obj: Item) -> ReturnList[ReturnDict[str, Any]]: pokemon_items = ( PokemonItem.objects.filter(item=obj) .select_related("pokemon", "version") - .order_by("pokemon_id", "version_id") + .order_by("pokemon_id", "version_id", "pk") ) grouped_data: list[dict[str, Any]] = [ { @@ -1739,7 +1739,9 @@ class Meta: @extend_schema_field(BerryFlavorBerryMapSerializer(many=True)) def get_berries_with_flavor(self, obj: BerryFlavor) -> ReturnList[ReturnDict[str, Any]]: flavor_map_objects = ( - 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", "pk") ) return cast( "ReturnList[ReturnDict[str, Any]]", @@ -2629,7 +2631,7 @@ def get_pokemon_form_sprites(self, obj: PokemonForm) -> dict[str, Any]: @extend_schema_field(PokemonFormTypeSerializer(many=True)) def get_pokemon_form_types(self, obj: PokemonForm) -> ReturnList[ReturnDict[str, Any]]: - form_types = PokemonFormType.objects.filter(pokemon_form=obj).select_related("type").order_by("slot") + form_types = PokemonFormType.objects.filter(pokemon_form=obj).select_related("type").order_by("slot", "pk") if form_types: return cast( @@ -2638,7 +2640,7 @@ def get_pokemon_form_types(self, obj: PokemonForm) -> ReturnList[ReturnDict[str, ) # Fall back to parent Pokemon's types if no form-specific types exist - pokemon_types = PokemonType.objects.filter(pokemon=obj.pokemon).select_related("type").order_by("slot") + pokemon_types = PokemonType.objects.filter(pokemon=obj.pokemon).select_related("type").order_by("slot", "pk") return cast( "ReturnList[ReturnDict[str, Any]]", PokemonTypeSerializer(pokemon_types, many=True, context=self.context).data, @@ -2946,7 +2948,7 @@ def get_pokemon_moves(self, obj: Pokemon) -> list[dict[str, Any]]: pokemon_moves = ( PokemonMove.objects.filter(pokemon=obj, move__isnull=False) .select_related("move", "version_group", "move_learn_method") - .order_by("move__id", "version_group_id") + .order_by("move__id", "version_group_id", "move_learn_method_id", "level", "pk") ) vg_cache: dict[int, Any] = {} @@ -2990,7 +2992,7 @@ def get_pokemon_held_items(self, obj: Pokemon) -> list[dict[str, Any]]: pokemon_items = ( PokemonItem.objects.filter(pokemon=obj, item__isnull=False) .select_related("item", "version") - .order_by("item__id", "version_id") + .order_by("item__id", "version_id", "pk") ) version_cache: dict[int, Any] = {} @@ -3033,7 +3035,7 @@ def get_past_pokemon_abilities(self, obj: Pokemon) -> list[dict[str, Any]]: past_abilities = ( PokemonAbilityPast.objects.filter(pokemon=obj, generation__isnull=False) .select_related("generation", "ability") - .order_by("generation_id") + .order_by("generation_id", "slot", "pk") ) final_data: list[dict[str, Any]] = [] @@ -3061,7 +3063,7 @@ def get_past_pokemon_stats(self, obj: Pokemon) -> list[dict[str, Any]]: past_stats = ( PokemonStatPast.objects.filter(pokemon=obj, generation__isnull=False) .select_related("generation", "stat") - .order_by("generation_id") + .order_by("generation_id", "stat_id", "pk") ) final_data: list[dict[str, Any]] = [] @@ -3086,7 +3088,7 @@ def get_past_pokemon_stats(self, obj: Pokemon) -> list[dict[str, Any]]: @extend_schema_field(PokemonTypeSerializer(many=True)) def get_pokemon_types(self, obj: Pokemon) -> ReturnList[ReturnDict[str, Any]]: - types = PokemonType.objects.filter(pokemon=obj).select_related("type").order_by("slot") + types = PokemonType.objects.filter(pokemon=obj).select_related("type").order_by("slot", "pk") return cast( "ReturnList[ReturnDict[str, Any]]", PokemonTypeSerializer(types, many=True, context=self.context).data, @@ -3097,7 +3099,7 @@ def get_past_pokemon_types(self, obj: Pokemon) -> list[dict[str, Any]]: past_types = ( PokemonTypePast.objects.filter(pokemon=obj, generation__isnull=False) .select_related("generation", "type") - .order_by("generation_id", "slot") + .order_by("generation_id", "slot", "pk") ) final_data: list[dict[str, Any]] = [] @@ -3361,7 +3363,7 @@ class Meta: @extend_schema_field(EvolutionChainLinkSerializer) def build_chain(self, obj: EvolutionChain) -> dict[str, Any]: - pokemon_objects = PokemonSpecies.objects.filter(evolution_chain=obj).order_by("order") + pokemon_objects = PokemonSpecies.objects.filter(evolution_chain=obj).order_by("order", "pk") summary_data = cast( "ReturnList[ReturnDict[str, Any]]", PokemonSpeciesSummarySerializer(pokemon_objects, many=True, context=self.context).data, @@ -3535,7 +3537,9 @@ class Meta: @extend_schema_field(PokemonDexNumberSerializer(many=True)) def get_pokedex_entries(self, obj: Pokedex) -> ReturnList[ReturnDict[str, Any]]: entries = ( - PokemonDexNumber.objects.filter(pokedex=obj).select_related("pokemon_species").order_by("pokedex_number") + PokemonDexNumber.objects.filter(pokedex=obj) + .select_related("pokemon_species") + .order_by("pokedex_number", "pokemon_species_id", "pk") ) return cast( "ReturnList[ReturnDict[str, Any]]", diff --git a/pokemon_v2/tests.py b/pokemon_v2/tests.py index 9b525a341..80e4c4903 100644 --- a/pokemon_v2/tests.py +++ b/pokemon_v2/tests.py @@ -1309,6 +1309,7 @@ def setup_pokemon_species_data( is_legendary=False, is_mythical=False, order=1, + pk=None, ): generation = generation or cls.setup_generation_data(name="gen for " + name) @@ -1321,6 +1322,7 @@ def setup_pokemon_species_data( pokemon_habitat = pokemon_habitat or cls.setup_pokemon_habitat_data(name="pkm hbtt for " + name) pokemon_species = PokemonSpecies.objects.create( + pk=pk, name=name, generation=generation, evolves_from_species=evolves_from_species, @@ -1573,10 +1575,11 @@ def setup_pokemon_item_data(cls, pokemon=None, item=None, version=None, rarity=5 return pokemon_item @classmethod - def setup_pokemon_move_data(cls, pokemon, move, version_group, level=0, order=1): - move_learn_method = cls.setup_move_learn_method_data(name="mv lrn mthd for pkmn") + def setup_pokemon_move_data(cls, pokemon, move, version_group, level=0, order=1, move_learn_method=None, pk=None): + move_learn_method = move_learn_method or cls.setup_move_learn_method_data(name="mv lrn mthd for pkmn") pokemon_move = PokemonMove.objects.create( + pk=pk, pokemon=pokemon, version_group=version_group, move=move, @@ -2686,6 +2689,29 @@ def test_growth_rate_api(self): "{}{}/pokemon-species/{}/".format(TEST_HOST, API_V2, pokemon_species.pk), ) + def test_reverse_relation_lists_are_ordered_by_pk(self): + # Lists coming straight from a reverse relation have no order_by() of + # their own, so they fall back to the manager ordering by pk. The pks + # are inserted out of order, since an unordered query returns them in + # insertion order and would pass either way. + growth_rate = self.setup_growth_rate_data(name="grth rt for ordering") + species = [ + self.setup_pokemon_species_data( + pk=pk, + growth_rate=growth_rate, + name="pkmn spcs for ordering {}".format(pk), + ) + for pk in (30, 10, 20) + ] + + response = self.client.get("{}/growth-rate/{}/".format(API_V2, growth_rate.pk)) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual( + [entry["name"] for entry in response.data["pokemon_species"]], + [entry.name for entry in sorted(species, key=lambda entry: entry.pk)], + ) + # Location Tests def test_location_api(self): location = self.setup_location_data(name="base lctn") @@ -4591,6 +4617,38 @@ def test_pokemon_moves_with_non_contiguous_version_group_and_method_ids(self): "{}{}/move-learn-method/{}/".format(TEST_HOST, API_V2, pokemon_move.move_learn_method.pk), ) + def test_pokemon_moves_version_group_details_are_deterministically_ordered(self): + # A pokemon can learn the same move in the same version group more than + # once, so these rows tie on every other order_by() field and fall back + # to the pk. The pks are inserted out of order on purpose. + pokemon_species = self.setup_pokemon_species_data(name="pkmn spcs for mv ordering") + pokemon = self.setup_pokemon_data(pokemon_species=pokemon_species, name="pkmn for mv ordering") + self.setup_pokemon_sprites_data(pokemon=pokemon) + self.setup_pokemon_cries_data(pokemon, latest=True, legacy=True) + + move = self.setup_move_data(name="mv for mv ordering") + version_group = self.setup_version_group_data(name="ver grp for mv ordering") + move_learn_method = self.setup_move_learn_method_data(name="mv lrn mthd for mv ordering") + + for pk, order in ((30, 3), (10, 1), (20, 2)): + self.setup_pokemon_move_data( + pk=pk, + pokemon=pokemon, + move=move, + version_group=version_group, + move_learn_method=move_learn_method, + level=5, + order=order, + ) + + response = self.client.get("{}/pokemon/{}/".format(API_V2, pokemon.pk), headers={"host": "testserver"}) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual( + [detail["order"] for detail in response.data["moves"][0]["version_group_details"]], + [1, 2, 3], + ) + def test_pokemon_form_api(self): pokemon_species = self.setup_pokemon_species_data() pokemon = self.setup_pokemon_data(pokemon_species=pokemon_species)