diff --git a/data/v2/build.py b/data/v2/build.py index 4103037fb..963cc5fe9 100644 --- a/data/v2/build.py +++ b/data/v2/build.py @@ -89,6 +89,66 @@ 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 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: @@ -679,6 +739,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]), @@ -751,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=int(info[10]) if info[10] != "" else None, - 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) @@ -786,23 +835,8 @@ 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[8]) if info[8] != "" 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]), - 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=effect_id, - move_effect_chance=int(info[9]) if info[9] != "" 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 a6532ee1f..9c4b863e3 100644 --- a/pokemon_v2/test_models.py +++ b/pokemon_v2/test_models.py @@ -173,3 +173,106 @@ 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 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. + """ + + 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} + + 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_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, 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")