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 <noreply@anthropic.com>
This commit is contained in:
@@ -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 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.
|
- 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
|
## Quick Start
|
||||||
|
|
||||||
|
|||||||
+30
-18
@@ -17,8 +17,7 @@ from datetime import date, datetime
|
|||||||
|
|
||||||
import psycopg2
|
import psycopg2
|
||||||
|
|
||||||
DATA_DIR = os.path.join(os.path.dirname(__file__), "..", "03-data")
|
DATA_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "03-data")
|
||||||
XML_PATH = os.path.join(DATA_DIR, "obstsorten.xml")
|
|
||||||
|
|
||||||
# typ values that are aggregate categories — coerced with a logged warning
|
# typ values that are aggregate categories — coerced with a logged warning
|
||||||
AGGREGATE_TYPS = {"x", "bq", "aprpfi", "mirren", "pflzwe", "bo", "k"}
|
AGGREGATE_TYPS = {"x", "bq", "aprpfi", "mirren", "pflzwe", "bo", "k"}
|
||||||
@@ -86,13 +85,23 @@ def find_images(fruit_id: str, data_dir: str) -> list:
|
|||||||
"""
|
"""
|
||||||
results = []
|
results = []
|
||||||
for subdir, image_type in IMAGE_DIRS:
|
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)):
|
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:
|
with open(path, "rb") as f:
|
||||||
data = f.read()
|
data = f.read()
|
||||||
results.append(
|
results.append(
|
||||||
{
|
{
|
||||||
"filename": os.path.basename(path),
|
"filename": basename,
|
||||||
"data": data,
|
"data": data,
|
||||||
"image_type": image_type,
|
"image_type": image_type,
|
||||||
}
|
}
|
||||||
@@ -112,7 +121,7 @@ def import_fruits(conn, data_dir: str = None) -> dict:
|
|||||||
tree = ET.parse(xml_path)
|
tree = ET.parse(xml_path)
|
||||||
root = tree.getroot()
|
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 = []
|
warnings = []
|
||||||
|
|
||||||
with conn.cursor() as cur:
|
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:
|
if not osdb_number or not name:
|
||||||
counts["skipped"] += 1
|
counts["skipped"] += 1
|
||||||
|
counts["warnings"] += 1
|
||||||
warnings.append(f"SKIP: missing id or name (id={osdb_number!r})")
|
warnings.append(f"SKIP: missing id or name (id={osdb_number!r})")
|
||||||
continue
|
continue
|
||||||
|
|
||||||
@@ -151,7 +161,7 @@ def import_fruits(conn, data_dir: str = None) -> dict:
|
|||||||
ON CONFLICT (osdb_number) DO UPDATE
|
ON CONFLICT (osdb_number) DO UPDATE
|
||||||
SET name = EXCLUDED.name,
|
SET name = EXCLUDED.name,
|
||||||
fruit_type = EXCLUDED.fruit_type,
|
fruit_type = EXCLUDED.fruit_type,
|
||||||
comment = NULL,
|
comment = fruits.comment,
|
||||||
updated_at = NOW()
|
updated_at = NOW()
|
||||||
RETURNING id
|
RETURNING id
|
||||||
""",
|
""",
|
||||||
@@ -165,7 +175,7 @@ def import_fruits(conn, data_dir: str = None) -> dict:
|
|||||||
ON CONFLICT (osdb_number) DO UPDATE
|
ON CONFLICT (osdb_number) DO UPDATE
|
||||||
SET name = EXCLUDED.name,
|
SET name = EXCLUDED.name,
|
||||||
fruit_type = EXCLUDED.fruit_type,
|
fruit_type = EXCLUDED.fruit_type,
|
||||||
comment = NULL,
|
comment = fruits.comment,
|
||||||
updated_at = NOW()
|
updated_at = NOW()
|
||||||
RETURNING id
|
RETURNING id
|
||||||
""",
|
""",
|
||||||
@@ -218,20 +228,22 @@ def main():
|
|||||||
try:
|
try:
|
||||||
print("Importing fruits...")
|
print("Importing fruits...")
|
||||||
counts = import_fruits(conn)
|
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:
|
finally:
|
||||||
conn.close()
|
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__":
|
if __name__ == "__main__":
|
||||||
main()
|
main()
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
import os
|
import os
|
||||||
|
import shutil
|
||||||
import tempfile
|
import tempfile
|
||||||
import unittest
|
import unittest
|
||||||
from datetime import date
|
from datetime import date
|
||||||
@@ -127,6 +128,9 @@ class TestFindImages(unittest.TestCase):
|
|||||||
for d in (self.ef_dir, self.bl_dir, self.bau_dir):
|
for d in (self.ef_dir, self.bl_dir, self.bau_dir):
|
||||||
os.makedirs(d)
|
os.makedirs(d)
|
||||||
|
|
||||||
|
def tearDown(self):
|
||||||
|
shutil.rmtree(self.tmpdir, ignore_errors=True)
|
||||||
|
|
||||||
def _touch(self, path, content=b"img"):
|
def _touch(self, path, content=b"img"):
|
||||||
with open(path, "wb") as f:
|
with open(path, "wb") as f:
|
||||||
f.write(content)
|
f.write(content)
|
||||||
@@ -149,10 +153,13 @@ class TestFindImages(unittest.TestCase):
|
|||||||
types = [i["image_type"] for i in imgs]
|
types = [i["image_type"] for i in imgs]
|
||||||
self.assertIn("tree", types)
|
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_tn.jpg"))
|
||||||
|
self._touch(os.path.join(self.ef_dir, "adams_apfel_ef_s0.jpg"))
|
||||||
imgs = find_images("adams_apfel", self.tmpdir)
|
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):
|
def test_returns_empty_when_no_match(self):
|
||||||
imgs = find_images("unknown_fruit", self.tmpdir)
|
imgs = find_images("unknown_fruit", self.tmpdir)
|
||||||
@@ -173,6 +180,14 @@ class TestFindImages(unittest.TestCase):
|
|||||||
imgs = find_images("abbe_fetel", self.tmpdir)
|
imgs = find_images("abbe_fetel", self.tmpdir)
|
||||||
self.assertEqual(len(imgs), 3)
|
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__":
|
if __name__ == "__main__":
|
||||||
unittest.main()
|
unittest.main()
|
||||||
|
|||||||
Reference in New Issue
Block a user