Move effect build order is incorrect (fixes #1663)

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)
 

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")