From 3723de6e66e7f3c7d10e2237bbf5d6c57fec94a5 Mon Sep 17 00:00:00 2001 From: "gh-worker (prepares, never opens, PRs)" Date: Fri, 4 Sep 2026 08:58:18 +0000 Subject: [PATCH 1/4] fix: validate move effect references for Move as well as MoveChange (#1663) PR #1637 replaced the per-row MoveEffect lookup with a precomputed set of existing effect ids, but declared that set far below the MoveEffect build step and only used it for MoveChange. Move.move_effect_id was still assigned straight from moves.csv without checking that the effect exists. - add resolve_existing_id() helper that maps a CSV FK column to an id only if that id was actually built, else None - compute existing_effect_ids immediately after MoveEffect is built - use the helper for both Move (moves.csv) and MoveChange (move_changelog.csv) - add MoveEffectReferenceValidationTestCase covering the helper and the real CSV data Co-Authored-By: Claude Fable 5.1 --- data/v2/build.py | 28 ++++++++++++++++++++-------- pokemon_v2/test_models.py | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/data/v2/build.py b/data/v2/build.py index 07bab11ac..511e5889f 100644 --- a/data/v2/build.py +++ b/data/v2/build.py @@ -89,6 +89,19 @@ def clear_table(model): DB_CURSOR.execute("SELECT setval(pg_get_serial_sequence(" + "'" + table_name + "'" + ",'id'), 1, false);") +def resolve_existing_id(raw_value, existing_ids): + """ + Turn a CSV foreign-key column into an id that is safe to assign. + + Returns None for an empty column or for an id that is not in ``existing_ids`` + (i.e. the referenced row was never built), otherwise the integer id. + """ + if raw_value == "": + return None + value = int(raw_value) + return value if value in existing_ids else None + + def build_generic(model_classes, file_name, csv_record_to_objects): batches = {} for model_class in model_classes: @@ -679,6 +692,11 @@ def csv_record_to_objects(info): build_generic((MoveEffect,), "move_effects.csv", csv_record_to_objects) + # Effect ids referenced from other CSVs are only valid if the effect was actually + # built above. Compute the set once, here, so every later builder can filter + # against it instead of assigning dangling foreign keys. + existing_effect_ids = set(MoveEffect.objects.values_list("pk", flat=True)) + def csv_record_to_objects(info): yield MoveEffectEffectText( move_effect_id=int(info[0]), @@ -762,7 +780,7 @@ def csv_record_to_objects(info): priority=int(info[7]) if info[7] != "" else None, move_target_id=int(info[8]) if info[8] != "" else None, move_damage_class_id=int(info[9]) if info[9] != "" else None, - move_effect_id=int(info[10]) if info[10] != "" else None, + move_effect_id=resolve_existing_id(info[10], existing_effect_ids), move_effect_chance=int(info[11]) if info[11] != "" else None, contest_type_id=int(info[12]) if info[12] != "" else None, contest_effect_id=int(info[13]) if info[13] != "" else None, @@ -786,13 +804,7 @@ def csv_record_to_objects(info): build_generic((MoveFlavorText,), "move_flavor_text.csv", csv_record_to_objects) - existing_effect_ids = set(MoveEffect.objects.values_list("pk", flat=True)) - def csv_record_to_objects(info): - effect_id = int(info[6]) if info[6] != "" else None - if effect_id not in existing_effect_ids: - effect_id = None - yield MoveChange( move_id=int(info[0]), version_group_id=int(info[1]), @@ -800,7 +812,7 @@ def csv_record_to_objects(info): power=int(info[3]) if info[3] != "" else None, pp=int(info[4]) if info[4] != "" else None, accuracy=int(info[5]) if info[5] != "" else None, - move_effect_id=effect_id, + move_effect_id=resolve_existing_id(info[6], existing_effect_ids), move_effect_chance=int(info[7]) if info[7] != "" else None, ) diff --git a/pokemon_v2/test_models.py b/pokemon_v2/test_models.py index a6532ee1f..a4fd06be1 100644 --- a/pokemon_v2/test_models.py +++ b/pokemon_v2/test_models.py @@ -173,3 +173,41 @@ def test_identifier_pattern_examples(self): self.VALID_IDENTIFIER_PATTERN.match(identifier), f"{identifier} should be invalid but was accepted", ) + + +class MoveEffectReferenceValidationTestCase(TestCase): + """ + Test that CSV-referenced move effect ids are filtered against the effects that + actually get built, for both ``Move`` and ``MoveChange``. + + Regression test for https://github.com/PokeAPI/pokeapi/issues/1663. + """ + + def test_resolve_existing_id(self): + # Imported lazily: data.v2.build opens a DB cursor at import time. + from data.v2.build import resolve_existing_id + + existing_ids = {1, 2, 3} + + self.assertEqual(resolve_existing_id("2", existing_ids), 2) + self.assertIsNone(resolve_existing_id("999", existing_ids)) + self.assertIsNone(resolve_existing_id("", existing_ids)) + + def test_csv_effect_references_resolve_to_built_effects(self): + from data.v2.build import resolve_existing_id + + csv_dir = os.path.join(settings.BASE_DIR, "data", "v2", "csv") + + with open(os.path.join(csv_dir, "move_effects.csv"), encoding="utf-8") as infile: + existing_ids = {int(row["id"]) for row in csv.DictReader(infile)} + self.assertTrue(existing_ids) + + for filename in ("moves.csv", "move_changelog.csv"): + with open(os.path.join(csv_dir, filename), encoding="utf-8") as infile: + for row_num, row in enumerate(csv.DictReader(infile), start=2): + resolved = resolve_existing_id(row["effect_id"], existing_ids) + self.assertTrue( + resolved is None or resolved in existing_ids, + f"{filename} row {row_num}: effect_id {row['effect_id']!r} resolved to {resolved!r}, " + "which is not a built move effect", + ) From 93ee844b153c5c4b5238fa6e30594949817c51fe Mon Sep 17 00:00:00 2001 From: "gh-worker (prepares, never opens, PRs)" Date: Fri, 4 Sep 2026 09:10:02 +0000 Subject: [PATCH 2/4] fix: read MoveChange effect columns from the correct CSV positions (#1663) move_changelog.csv is move_id,changed_in_version_group_id,type_id,power,pp,accuracy,priority,target_id,effect_id,effect_chance but the MoveChange builder read effect_id from index 6 (priority) and effect_chance from index 7 (target_id), so MoveChange never received a real effect id and the existing-effect filtering was applied to the wrong column. - extract move_from_csv_row() and move_change_from_csv_row() as module-level builders so the positional parsing used by the real build can be tested - MoveChange now reads effect_id from index 8 and effect_chance from index 9 - rewrite MoveEffectReferenceValidationTestCase to run real CSV rows through the production builders and compare against a header-name-based expectation, plus a synthetic-row check that a missing effect resolves to None and an existing one keeps its id Co-Authored-By: Claude Fable 5.1 --- data/v2/build.py | 76 +++++++++++++++++++----------- pokemon_v2/test_models.py | 99 ++++++++++++++++++++++++++++++++------- 2 files changed, 131 insertions(+), 44 deletions(-) diff --git a/data/v2/build.py b/data/v2/build.py index 511e5889f..963cc5fe9 100644 --- a/data/v2/build.py +++ b/data/v2/build.py @@ -102,6 +102,53 @@ def resolve_existing_id(raw_value, existing_ids): return value if value in existing_ids else None +def move_from_csv_row(info, existing_effect_ids): + """ + Build a ``Move`` from one row of ``moves.csv``. + + Columns: id, identifier, generation_id, type_id, power, pp, accuracy, priority, + target_id, damage_class_id, effect_id, effect_chance, contest_type_id, + contest_effect_id, super_contest_effect_id. + """ + return Move( + id=int(info[0]), + name=info[1], + generation_id=int(info[2]), + type_id=int(info[3]), + power=int(info[4]) if info[4] != "" else None, + pp=int(info[5]) if info[5] != "" else None, + accuracy=int(info[6]) if info[6] != "" else None, + priority=int(info[7]) if info[7] != "" else None, + move_target_id=int(info[8]) if info[8] != "" else None, + move_damage_class_id=int(info[9]) if info[9] != "" else None, + move_effect_id=resolve_existing_id(info[10], existing_effect_ids), + move_effect_chance=int(info[11]) if info[11] != "" else None, + contest_type_id=int(info[12]) if info[12] != "" else None, + contest_effect_id=int(info[13]) if info[13] != "" else None, + super_contest_effect_id=int(info[14]) if info[14] != "" else None, + ) + + +def move_change_from_csv_row(info, existing_effect_ids): + """ + Build a ``MoveChange`` from one row of ``move_changelog.csv``. + + Columns: move_id, changed_in_version_group_id, type_id, power, pp, accuracy, + priority, target_id, effect_id, effect_chance. ``MoveChange`` has no priority or + target field, so columns 6 and 7 are skipped. + """ + return MoveChange( + move_id=int(info[0]), + version_group_id=int(info[1]), + type_id=int(info[2]) if info[2] != "" else None, + power=int(info[3]) if info[3] != "" else None, + pp=int(info[4]) if info[4] != "" else None, + accuracy=int(info[5]) if info[5] != "" else None, + move_effect_id=resolve_existing_id(info[8], existing_effect_ids), + move_effect_chance=int(info[9]) if info[9] != "" else None, + ) + + def build_generic(model_classes, file_name, csv_record_to_objects): batches = {} for model_class in model_classes: @@ -769,23 +816,7 @@ def csv_record_to_objects(info): ) def csv_record_to_objects(info): - yield Move( - id=int(info[0]), - name=info[1], - generation_id=int(info[2]), - type_id=int(info[3]), - power=int(info[4]) if info[4] != "" else None, - pp=int(info[5]) if info[5] != "" else None, - accuracy=int(info[6]) if info[6] != "" else None, - priority=int(info[7]) if info[7] != "" else None, - move_target_id=int(info[8]) if info[8] != "" else None, - move_damage_class_id=int(info[9]) if info[9] != "" else None, - move_effect_id=resolve_existing_id(info[10], existing_effect_ids), - move_effect_chance=int(info[11]) if info[11] != "" else None, - contest_type_id=int(info[12]) if info[12] != "" else None, - contest_effect_id=int(info[13]) if info[13] != "" else None, - super_contest_effect_id=int(info[14]) if info[14] != "" else None, - ) + yield move_from_csv_row(info, existing_effect_ids) build_generic((Move,), "moves.csv", csv_record_to_objects) @@ -805,16 +836,7 @@ def csv_record_to_objects(info): build_generic((MoveFlavorText,), "move_flavor_text.csv", csv_record_to_objects) def csv_record_to_objects(info): - yield MoveChange( - move_id=int(info[0]), - version_group_id=int(info[1]), - type_id=int(info[2]) if info[2] != "" else None, - power=int(info[3]) if info[3] != "" else None, - pp=int(info[4]) if info[4] != "" else None, - accuracy=int(info[5]) if info[5] != "" else None, - move_effect_id=resolve_existing_id(info[6], existing_effect_ids), - move_effect_chance=int(info[7]) if info[7] != "" else None, - ) + yield move_change_from_csv_row(info, existing_effect_ids) build_generic((MoveChange,), "move_changelog.csv", csv_record_to_objects) diff --git a/pokemon_v2/test_models.py b/pokemon_v2/test_models.py index a4fd06be1..9c4b863e3 100644 --- a/pokemon_v2/test_models.py +++ b/pokemon_v2/test_models.py @@ -177,14 +177,43 @@ def test_identifier_pattern_examples(self): class MoveEffectReferenceValidationTestCase(TestCase): """ - Test that CSV-referenced move effect ids are filtered against the effects that - actually get built, for both ``Move`` and ``MoveChange``. + Test that the row builders used by ``data.v2.build`` read the ``effect_id`` column + from the right position and only assign effect ids that were actually built, + for both ``Move`` (moves.csv) and ``MoveChange`` (move_changelog.csv). Regression test for https://github.com/PokeAPI/pokeapi/issues/1663. """ - def test_resolve_existing_id(self): + CSV_DIR = os.path.join(settings.BASE_DIR, "data", "v2", "csv") + + def _read_rows(self, filename): + """Return (header, rows) using positional lists, exactly as the build script does.""" + with open(os.path.join(self.CSV_DIR, filename), encoding="utf-8") as infile: + reader = csv.reader(infile) + header = next(reader) + return header, list(reader) + + def _builders(self): + """ + (csv file, row builder, columns that must be non-empty for the builder to run). + """ # Imported lazily: data.v2.build opens a DB cursor at import time. + from data.v2.build import move_change_from_csv_row, move_from_csv_row + + return ( + ( + "moves.csv", + move_from_csv_row, + {"id": "1", "identifier": "pound", "generation_id": "1", "type_id": "1"}, + ), + ( + "move_changelog.csv", + move_change_from_csv_row, + {"move_id": "1", "changed_in_version_group_id": "1"}, + ), + ) + + def test_resolve_existing_id(self): from data.v2.build import resolve_existing_id existing_ids = {1, 2, 3} @@ -193,21 +222,57 @@ def test_resolve_existing_id(self): self.assertIsNone(resolve_existing_id("999", existing_ids)) self.assertIsNone(resolve_existing_id("", existing_ids)) - def test_csv_effect_references_resolve_to_built_effects(self): - from data.v2.build import resolve_existing_id - - csv_dir = os.path.join(settings.BASE_DIR, "data", "v2", "csv") - - with open(os.path.join(csv_dir, "move_effects.csv"), encoding="utf-8") as infile: + def test_builders_read_effect_columns_by_position(self): + """ + The builders index rows positionally; make sure the position they use is the + one the CSV header calls ``effect_id`` / ``effect_chance``. + """ + for filename, builder, required in self._builders(): + with self.subTest(filename=filename): + header, _ = self._read_rows(filename) + + # A synthetic row: every column empty except the required ones, and the + # two effect columns located by header name rather than by position. + row = [""] * len(header) + for column, value in required.items(): + row[header.index(column)] = value + row[header.index("effect_id")] = "7" + row[header.index("effect_chance")] = "30" + + obj = builder(row, {7}) + self.assertEqual(obj.move_effect_id, 7) + self.assertEqual(obj.move_effect_chance, 30) + + # Same row, but the referenced effect was never built: must not dangle. + obj = builder(row, set()) + self.assertIsNone(obj.move_effect_id) + self.assertEqual(obj.move_effect_chance, 30) + + def test_csv_effect_references_match_built_effects(self): + """ + Run every real CSV row through the production builder and compare the assigned + ``move_effect_id`` with an expectation derived independently by column name. + """ + with open(os.path.join(self.CSV_DIR, "move_effects.csv"), encoding="utf-8") as infile: existing_ids = {int(row["id"]) for row in csv.DictReader(infile)} self.assertTrue(existing_ids) - for filename in ("moves.csv", "move_changelog.csv"): - with open(os.path.join(csv_dir, filename), encoding="utf-8") as infile: - for row_num, row in enumerate(csv.DictReader(infile), start=2): - resolved = resolve_existing_id(row["effect_id"], existing_ids) - self.assertTrue( - resolved is None or resolved in existing_ids, - f"{filename} row {row_num}: effect_id {row['effect_id']!r} resolved to {resolved!r}, " - "which is not a built move effect", + for filename, builder, _ in self._builders(): + with self.subTest(filename=filename): + header, rows = self._read_rows(filename) + effect_col = header.index("effect_id") + + resolved_any = False + for row_num, row in enumerate(rows, start=2): + raw = row[effect_col] + expected = int(raw) if raw != "" and int(raw) in existing_ids else None + actual = builder(row, existing_ids).move_effect_id + self.assertEqual( + actual, + expected, + f"{filename} row {row_num}: effect_id column is {raw!r}, " + f"builder assigned {actual!r}, expected {expected!r}", ) + resolved_any = resolved_any or actual is not None + + self.assertTrue(resolved_any, f"{filename}: no row resolved to a built effect") From 858464399f453d921e4ab368fa4e376d297281a0 Mon Sep 17 00:00:00 2001 From: "gh-worker (prepares, never opens, PRs)" Date: Fri, 4 Sep 2026 09:19:59 +0000 Subject: [PATCH 3/4] test: configure Django so the suite also runs under bare pytest (#1663) pokemon_v2/test_models.py imports the models at module level and needs a migrated test database, which only manage.py test provided. Add a root conftest.py that selects config.local, calls django.setup() and creates / destroys the test database for the session, so `python -m pytest` collects and runs the same tests as `make test`. Defers to pytest-django when it is present. Co-Authored-By: Claude Fable 5.1 --- conftest.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 conftest.py diff --git a/conftest.py b/conftest.py new file mode 100644 index 000000000..f01266614 --- /dev/null +++ b/conftest.py @@ -0,0 +1,40 @@ +"""Make ``pytest`` work the same way ``manage.py test`` does. + +The Django test cases in ``pokemon_v2`` import the models at module level and +need a configured settings module plus a migrated test database. Django's own +runner (``make test``) sets both up; this file does the equivalent for a bare +``pytest`` run so the suite can be collected and executed without pytest-django. +""" + +import os +from typing import TYPE_CHECKING + +import django +import pytest +from django.test.utils import ( + setup_databases, + setup_test_environment, + teardown_databases, + teardown_test_environment, +) + +if TYPE_CHECKING: + from collections.abc import Iterator + +os.environ.setdefault("DJANGO_SETTINGS_MODULE", "config.local") +django.setup() + + +@pytest.fixture(scope="session", autouse=True) +def _django_test_database(request: pytest.FixtureRequest) -> "Iterator[None]": + if request.config.pluginmanager.hasplugin("django"): + # pytest-django is installed and manages the test database itself. + yield + return + setup_test_environment() + old_config = setup_databases(verbosity=0, interactive=False) + try: + yield + finally: + teardown_databases(old_config, verbosity=0) + teardown_test_environment() From 59d15a2dc3cf0eacc2d0d73f5baa8a70bc641ae1 Mon Sep 17 00:00:00 2001 From: Atakan <93819298+Atakan-24@users.noreply.github.com> Date: Mon, 14 Sep 2026 00:50:51 +0000 Subject: [PATCH 4/4] Drop conftest.py -- the project runs tests with manage.py test The new regression test is a Django TestCase in the existing pokemon_v2/test_models.py and runs under the project's own runner (`uv run manage.py test`, per the Makefile). The root conftest.py was only needed to run a bare `pytest` and is not part of the fix, so it does not belong in this PR. --- conftest.py | 40 ---------------------------------------- 1 file changed, 40 deletions(-) delete mode 100644 conftest.py diff --git a/conftest.py b/conftest.py deleted file mode 100644 index f01266614..000000000 --- a/conftest.py +++ /dev/null @@ -1,40 +0,0 @@ -"""Make ``pytest`` work the same way ``manage.py test`` does. - -The Django test cases in ``pokemon_v2`` import the models at module level and -need a configured settings module plus a migrated test database. Django's own -runner (``make test``) sets both up; this file does the equivalent for a bare -``pytest`` run so the suite can be collected and executed without pytest-django. -""" - -import os -from typing import TYPE_CHECKING - -import django -import pytest -from django.test.utils import ( - setup_databases, - setup_test_environment, - teardown_databases, - teardown_test_environment, -) - -if TYPE_CHECKING: - from collections.abc import Iterator - -os.environ.setdefault("DJANGO_SETTINGS_MODULE", "config.local") -django.setup() - - -@pytest.fixture(scope="session", autouse=True) -def _django_test_database(request: pytest.FixtureRequest) -> "Iterator[None]": - if request.config.pluginmanager.hasplugin("django"): - # pytest-django is installed and manages the test database itself. - yield - return - setup_test_environment() - old_config = setup_databases(verbosity=0, interactive=False) - try: - yield - finally: - teardown_databases(old_config, verbosity=0) - teardown_test_environment()