From 7fc82049240280620dd1853ba8a7b934ad8df81b Mon Sep 17 00:00:00 2001 From: juliaweber Date: Wed, 17 Jun 2026 13:38:05 +0200 Subject: [PATCH] fix: apply code-review findings to import_fruits script and tests - find_images: verify exact fruit_id prefix (stem-parse before _s0.) to prevent cross-fruit image contamination (e.g. mitschurins steals mitschurins_fruchtbare images) - DATA_DIR uses os.path.abspath to handle symlinks / relative invocation - counts["images"] derived from IMAGE_DIRS constant, not hardcoded keys - counts["warnings"] incremented on missing-id/name skip (was missing) - UPSERT preserves existing comment (comment = fruits.comment, not NULL) - Print summary and rollback moved inside try/except to avoid NameError - Remove dead XML_PATH constant - Tests: add tearDown to clean up tmpdir; fix vacuous _tn exclusion test to include positive-control _s0 file; add prefix-collision regression test Co-Authored-By: Claude Sonnet 4.6 --- README.md | 1 + scripts/import_fruits.py | 48 ++++++++++++++++++++++------------- scripts/import_fruits_test.py | 19 ++++++++++++-- 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index c9e3255..1b9bc2b 100644 --- a/README.md +++ b/README.md @@ -6,6 +6,7 @@ A full-stack fruit-variety database (Go + Vue 3). - Users can view a "Hello, OSDB!" landing page that confirms the backend is reachable via the Vite proxy. - Users can create, view, edit, and delete fruit varieties, including managing synonyms and uploading images. +- Administrators can bulk-import the legacy fruit database from XML using `scripts/import_fruits.py`. ## Quick Start diff --git a/scripts/import_fruits.py b/scripts/import_fruits.py index 13cf469..5ae7b81 100644 --- a/scripts/import_fruits.py +++ b/scripts/import_fruits.py @@ -17,8 +17,7 @@ from datetime import date, datetime import psycopg2 -DATA_DIR = os.path.join(os.path.dirname(__file__), "..", "03-data") -XML_PATH = os.path.join(DATA_DIR, "obstsorten.xml") +DATA_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "03-data") # typ values that are aggregate categories — coerced with a logged warning AGGREGATE_TYPS = {"x", "bq", "aprpfi", "mirren", "pflzwe", "bo", "k"} @@ -86,13 +85,23 @@ def find_images(fruit_id: str, data_dir: str) -> list: """ results = [] for subdir, image_type in IMAGE_DIRS: - pattern = os.path.join(data_dir, subdir, f"{fruit_id}_*_s0.*") + pattern = os.path.join(data_dir, subdir, f"{glob.escape(fruit_id)}_*_s0.*") for path in sorted(glob.glob(pattern)): + basename = os.path.basename(path) + # Verify the fruit_id is the exact prefix, not just a string prefix of + # another fruit's id (e.g. "mitschurins" must not steal + # "mitschurins_fruchtbare_nda_s0.jpg"). + # Convention: basename = {id}_{type_code}_s0.{ext} + # So stripping the last _-segment before _s0 gives the exact id. + stem = basename.split("_s0.")[0] + actual_id = "_".join(stem.split("_")[:-1]) + if actual_id != fruit_id: + continue with open(path, "rb") as f: data = f.read() results.append( { - "filename": os.path.basename(path), + "filename": basename, "data": data, "image_type": image_type, } @@ -112,7 +121,7 @@ def import_fruits(conn, data_dir: str = None) -> dict: tree = ET.parse(xml_path) root = tree.getroot() - counts = {"fruits": 0, "synonyms": 0, "images": {"fruit": 0, "flower": 0, "tree": 0}, "warnings": 0, "skipped": 0} + counts = {"fruits": 0, "synonyms": 0, "images": {img_type: 0 for _, img_type in IMAGE_DIRS}, "warnings": 0, "skipped": 0} warnings = [] with conn.cursor() as cur: @@ -125,6 +134,7 @@ def import_fruits(conn, data_dir: str = None) -> dict: if not osdb_number or not name: counts["skipped"] += 1 + counts["warnings"] += 1 warnings.append(f"SKIP: missing id or name (id={osdb_number!r})") continue @@ -151,7 +161,7 @@ def import_fruits(conn, data_dir: str = None) -> dict: ON CONFLICT (osdb_number) DO UPDATE SET name = EXCLUDED.name, fruit_type = EXCLUDED.fruit_type, - comment = NULL, + comment = fruits.comment, updated_at = NOW() RETURNING id """, @@ -165,7 +175,7 @@ def import_fruits(conn, data_dir: str = None) -> dict: ON CONFLICT (osdb_number) DO UPDATE SET name = EXCLUDED.name, fruit_type = EXCLUDED.fruit_type, - comment = NULL, + comment = fruits.comment, updated_at = NOW() RETURNING id """, @@ -218,20 +228,22 @@ def main(): try: print("Importing fruits...") counts = import_fruits(conn) + total_images = sum(counts["images"].values()) + print(f"\nDone.") + print(f" Fruits upserted: {counts['fruits']}") + print(f" Synonyms inserted: {counts['synonyms']}") + print(f" Images inserted: {total_images}") + for img_type, n in counts["images"].items(): + print(f" {img_type}: {n}") + print(f" Warnings: {counts['warnings']}") + print(f" Skipped: {counts['skipped']}") + except Exception as e: + print(f"Error: {e}", file=sys.stderr) + conn.rollback() + sys.exit(1) finally: conn.close() - total_images = sum(counts["images"].values()) - print(f"\nDone.") - print(f" Fruits upserted: {counts['fruits']}") - print(f" Synonyms inserted: {counts['synonyms']}") - print(f" Images inserted: {total_images}") - print(f" fruit: {counts['images']['fruit']}") - print(f" flower: {counts['images']['flower']}") - print(f" tree: {counts['images']['tree']}") - print(f" Warnings: {counts['warnings']}") - print(f" Skipped: {counts['skipped']}") - if __name__ == "__main__": main() diff --git a/scripts/import_fruits_test.py b/scripts/import_fruits_test.py index d9b0d11..aa9a46b 100644 --- a/scripts/import_fruits_test.py +++ b/scripts/import_fruits_test.py @@ -1,4 +1,5 @@ import os +import shutil import tempfile import unittest from datetime import date @@ -127,6 +128,9 @@ class TestFindImages(unittest.TestCase): for d in (self.ef_dir, self.bl_dir, self.bau_dir): os.makedirs(d) + def tearDown(self): + shutil.rmtree(self.tmpdir, ignore_errors=True) + def _touch(self, path, content=b"img"): with open(path, "wb") as f: f.write(content) @@ -149,10 +153,13 @@ class TestFindImages(unittest.TestCase): types = [i["image_type"] for i in imgs] self.assertIn("tree", types) - def test_skips_tn_files(self): + def test_skips_tn_files_but_finds_s0(self): + # _tn file must be excluded; _s0 file must be included (positive control) self._touch(os.path.join(self.ef_dir, "adams_apfel_ef_tn.jpg")) + self._touch(os.path.join(self.ef_dir, "adams_apfel_ef_s0.jpg")) imgs = find_images("adams_apfel", self.tmpdir) - self.assertEqual(imgs, []) + self.assertEqual(len(imgs), 1) + self.assertEqual(imgs[0]["filename"], "adams_apfel_ef_s0.jpg") def test_returns_empty_when_no_match(self): imgs = find_images("unknown_fruit", self.tmpdir) @@ -173,6 +180,14 @@ class TestFindImages(unittest.TestCase): imgs = find_images("abbe_fetel", self.tmpdir) self.assertEqual(len(imgs), 3) + def test_does_not_steal_images_from_fruit_with_longer_id(self): + # "mitschurins" must not match "mitschurins_fruchtbare_nda_s0.jpg" + self._touch(os.path.join(self.ef_dir, "mitschurins_ef_s0.jpg")) + self._touch(os.path.join(self.ef_dir, "mitschurins_fruchtbare_ef_s0.jpg")) + imgs = find_images("mitschurins", self.tmpdir) + self.assertEqual(len(imgs), 1) + self.assertEqual(imgs[0]["filename"], "mitschurins_ef_s0.jpg") + if __name__ == "__main__": unittest.main()