diff --git a/app/core/store_catalog_pipeline.py b/app/core/store_catalog_pipeline.py index 6b2cbf1..3506836 100644 --- a/app/core/store_catalog_pipeline.py +++ b/app/core/store_catalog_pipeline.py @@ -539,6 +539,7 @@ def stage_6_images(row: Dict[str, Any], *, enabled: bool = True) -> Dict[str, An return row try: from app.core.catalog_engine import catalog_engine + from app.services import produce_reference from app.services.image_search import find_all_image_urls # "Own Products" is a bucket, not a brand, so it must not enter the @@ -547,15 +548,41 @@ def stage_6_images(row: Dict[str, Any], *, enabled: bool = True) -> Dict[str, An # name, and _select_best_images derives its distinctive tokens from the # whole name ({toor, dal}), which is exactly right for a commodity. brand = row.get("brand") or "" - if brand == OWN_PRODUCTS_BRAND: + is_produce = brand == OWN_PRODUCTS_BRAND + if is_produce: brand = "" + product_name = row.get("product_name") or "" + + # A reviewed photo beats anything a search can find, so take it and stop. + # + # NOT merged into the candidate list below: _select_best_images ranks by + # whether the URL text names the product, and a Commons filename names + # the cultivar ("Honeycrisp.jpg" for Apple). The curated image would + # score zero relevance, sort below any searched URL containing "apple", + # and index 0 - the one that becomes image_url - would still be wrong. + # + # Only the bucket may ask. `produce_reference.lookup` falls back through + # shorter leading prefixes, so "Apple Cider Vinegar" would resolve to + # "apple" and be handed a photo of fruit. + if is_produce: + curated = produce_reference.image_url(product_name) + if curated: + row["image_url"] = curated + row["image_urls"] = [curated] + return row + + # produce=True suppresses the commodity-hint table (which turns "Apple" + # into "Apple fruit juice"), skips the packaged-goods databases, and + # drops the "-plant -tree -fish -botanical" Wikimedia exclusion block + # that fights every produce query. Without it this stage recreates the + # exact defect the Own Products image repair exists to undo. candidates = find_all_image_urls( - row.get("product_name") or "", brand=brand or None, max_results=24 + product_name, brand=brand or None, max_results=24, produce=is_produce ) if candidates: best = catalog_engine._select_best_images( - candidates, row.get("product_name") or "", brand, max_images=10 + candidates, product_name, brand, max_images=10 ) if best: row["image_urls"] = list(best) diff --git a/app/services/produce_reference.py b/app/services/produce_reference.py index e6d179a..7e2d969 100644 --- a/app/services/produce_reference.py +++ b/app/services/produce_reference.py @@ -126,9 +126,46 @@ def usda_fdc_id(product_name: str) -> Optional[int]: return entry.get("usda_fdc_id") if entry else None +# Words that make a name a DIFFERENT PRODUCT from the commodity it starts with, +# rather than a variety of it. +# +# `lookup` falls back through shorter leading prefixes, which is what lets +# "Mango Totapuri" and "Banana Robusta" find their base commodity - varieties of +# the same thing, correctly sharing one photo. The same fallback turns "Coconut +# Oil" into "Coconut" and "Apple Cider Vinegar" into "Apple", and a bottle of +# oil is not a variety of coconut. Handing it a photo of the raw fruit is the +# same class of error as the apple-juice image this table was built to fix, just +# pointing the other way. +# +# No row in the catalogue trips this today - all 159 names are exact keys - so +# this guards the case a future upload introduces, which is precisely when +# nobody would be looking. +_DERIVED_PRODUCT_WORDS = frozenset({ + "oil", "vinegar", "juice", "squash", "syrup", "jam", "jelly", "sauce", + "ketchup", "puree", "paste", "pickle", "powder", "flour", "atta", "rava", + "flakes", "chips", "crisps", "candy", "extract", "essence", "milkshake", + "smoothie", "dried", "fried", "roasted", "pickled", "canned", "frozen", +}) + + def image_url(product_name: str) -> Optional[str]: + """The reviewed photo for a commodity, or None. + + Stricter than `lookup` on purpose, and only here: `usda_fdc_id` keeps the + plain prefix fallback, because this guard is about not showing a misleading + PICTURE. Nutrition for a derived product is refused further upstream by the + category dispatch in `nutrition_data_service`. + """ entry = lookup(product_name) - return entry.get("image_url") if entry else None + if not entry: + return None + + matched = _normalize(entry.get("product_name") or "") + leftover = set(_normalize(product_name).split()) - set(matched.split()) + if leftover & _DERIVED_PRODUCT_WORDS: + return None + + return entry.get("image_url") def all_entries() -> Dict[str, Dict[str, Any]]: diff --git a/scripts/repair_brand_images.py b/scripts/repair_brand_images.py index 1a01099..f315c57 100644 --- a/scripts/repair_brand_images.py +++ b/scripts/repair_brand_images.py @@ -76,6 +76,7 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[1])) from app.infrastructure.settings import DB_HOST, DB_NAME # noqa: E402 from app.services.brand_registry import resolve_parent_brand # noqa: E402 from app.services.generic_products import OWN_PRODUCTS_BRAND # noqa: E402 +from app.services import produce_reference # noqa: E402 from app.services.vector_store import ( # noqa: E402 _connect, _list_brand_table_suffixes, @@ -440,18 +441,56 @@ def _is_own_products(brand: str) -> bool: return _sanitize_name(brand or "") == _sanitize_name(OWN_PRODUCTS_BRAND) -def _search_replacement(product_name: str, brand: str, max_results: int) -> List[str]: - """Validated image URLs for one product, best first. +def _search_replacement(product_name: str, brand: str, max_results: int, + timeout: int = 8) -> List[str]: + """Image URLs for one product, best first. - find_all_image_urls validates every candidate internally, so what comes back - is live by construction. Ranking then reuses the same _select_best_images - the ingestion pipeline uses, so a repaired row is ranked by the rules + For a loose commodity in the Own Products bucket the answer is already known + and already reviewed - see the curated branch below. Everything else is + searched, then ranked with the same _select_best_images the ingestion + pipeline uses, so a repaired row is ranked by the rules tests/test_image_selection.py already pins. + + On the searched path `find_all_image_urls(validate=True)` probes each + candidate, but it falls back to returning UNVALIDATED candidates when every + probe fails (image_search.py, the "validation killed everything" branch). So + a URL from here is usually live, not live by construction. """ key = _search_key(product_name, brand) if key in _SEARCH_CACHE: return _SEARCH_CACHE[key] + # THE REVIEWED IMAGE WINS, AND IT HAS TO SHORT-CIRCUIT TO DO SO. + # + # These photos were picked by hand and signed off on a contact sheet. + # Sending one through the rest of this function destroys it twice over. The + # `_names_product` corroboration gate at the end keeps only URLs containing + # a distinctive word of the product title, and the reviewed Commons photo + # for "Apple" is `Honeycrisp.jpg` - a Commons filename names the cultivar, + # not the catalogue word, so the CORRECT image is precisely the one that + # gets rejected. Even past that, ranking sorts any searched URL containing + # "apple" above it, and index 0 is what becomes `image_url`. + # + # Gating on the bucket is not a shortcut either: `produce_reference.lookup` + # walks shorter and shorter leading prefixes, so "Apple Cider Vinegar" misses + # twice and then hits "apple". This branch must never see another brand's + # rows. + if _is_own_products(brand): + curated = produce_reference.image_url(product_name) + if curated: + # A failed probe WARNS and keeps the URL, rather than falling back + # to a search. upload.wikimedia.org throttles a fast sweep and + # validate_image_url_live treats 429 exactly like 404, so a + # transient rate-limit would otherwise swap a reviewed photo for a + # searched one - silently, and at scale. That is the exact failure + # this table exists to prevent. A genuinely dead URL is a fix in + # produce_reference.json, and this warning is what asks for it. + if not _url_is_live(curated, timeout): + logger.warning(" curated image did not verify, using anyway " + "%s -> %s", product_name[:38], curated[:70]) + _SEARCH_CACHE[key] = [curated] + return [curated] + from app.services.image_search import find_all_image_urls # `stage_6_images` already blanks the brand for this bucket @@ -581,7 +620,7 @@ def repair(cur, conn, brands: List[str], apply: bool, timeout: int, workers: int totals["unrepairable"] += 1 continue - found = _search_replacement(name, suffix, max_results) + found = _search_replacement(name, suffix, max_results, timeout) if not found: logger.info(" no replacement found %s", name[:58]) unrepairable.append(f"{suffix}/{row['image_id']} ({name})") diff --git a/tests/test_produce_image_search.py b/tests/test_produce_image_search.py index 5f25916..f8f7d1c 100644 --- a/tests/test_produce_image_search.py +++ b/tests/test_produce_image_search.py @@ -161,3 +161,237 @@ def test_the_bucket_is_recognised_under_either_spelling(): assert _is_own_products("own_products") is True assert _is_own_products("Own Products") is True assert _is_own_products("Amul") is False + + +# --------------------------------------------------------------------------- +# The curated table reaches the two write paths +# --------------------------------------------------------------------------- +# Building 159 reviewed images and then not wiring them in is the failure this +# section exists to prevent. Until these tests existed, `produce_reference. +# image_url` had NO runtime consumer at all: the repair script re-searched every +# row, and the ingestion pipeline never even passed `produce=True`. So the +# contact sheet showed one set of images and the code would have written another. + +CURATED_COMMODITY = "Apple" +UNCURATED_COMMODITY = "Avocado" # genuinely absent from produce_reference.json +BUCKET = "own_products" # repair() passes the TABLE SUFFIX, not a name + + +@pytest.fixture +def repair_module(monkeypatch): + """The repair script with its process-lifetime search cache emptied. + + `_SEARCH_CACHE` is module-level and never cleared, so without this the first + test to run would answer for every test after it. + """ + from scripts import repair_brand_images + + monkeypatch.setattr(repair_brand_images, "_SEARCH_CACHE", {}) + monkeypatch.setattr(repair_brand_images, "_url_is_live", lambda url, timeout: True) + return repair_brand_images + + +@pytest.fixture +def no_search(monkeypatch): + """Record calls to the search, and fail loudly if one was not expected.""" + calls = [] + + def _fake(title, brand=None, country_hint=None, validate=True, + max_results=24, produce=False): + calls.append({"title": title, "brand": brand, "produce": produce}) + return [] + + monkeypatch.setattr(image_search, "find_all_image_urls", _fake) + return calls + + +def test_the_repair_returns_the_reviewed_image(repair_module, no_search): + from app.services import produce_reference + + found = repair_module._search_replacement(CURATED_COMMODITY, BUCKET, 8) + + assert found == [produce_reference.image_url(CURATED_COMMODITY)] + + +def test_the_repair_does_not_search_when_it_already_has_the_answer( + repair_module, no_search): + """The short-circuit is the behaviour, not an optimisation. + + A curated URL that went through the rest of `_search_replacement` would be + thrown away by the `_names_product` corroboration gate: the reviewed Commons + photo for Apple is `Honeycrisp.jpg`, and a Commons filename names the + cultivar, not the catalogue word. + """ + repair_module._search_replacement(CURATED_COMMODITY, BUCKET, 8) + + assert no_search == [], "the curated branch must return before searching" + + +def test_a_pack_size_still_finds_the_commodity(repair_module, no_search): + from app.services import produce_reference + + assert repair_module._search_replacement("Apple 1kg", BUCKET, 8) == [ + produce_reference.image_url(CURATED_COMMODITY)] + assert no_search == [] + + +def test_a_variety_shares_its_commodity_image(repair_module, no_search): + """`Mango Totapuri` and `Mango Alphonso` are the same fruit.""" + from app.services import produce_reference + + assert repair_module._search_replacement("Mango Totapuri 1kg", BUCKET, 8) == [ + produce_reference.image_url("Mango")] + + +def test_an_uncurated_commodity_falls_through_to_a_produce_search( + repair_module, no_search): + repair_module._search_replacement(UNCURATED_COMMODITY, BUCKET, 8) + + assert len(no_search) == 1 + assert no_search[0]["produce"] is True + assert no_search[0]["brand"] == "", "the bucket name must not enter the query" + + +def test_a_branded_product_never_consults_the_curated_table( + repair_module, no_search): + """`lookup` falls back through shorter leading prefixes, so asking it about + every brand's rows would put a photo of fruit on Apple-branded anything.""" + repair_module._search_replacement("Britannia Good Day 200g", "britannia", 8) + + assert len(no_search) == 1 + assert no_search[0]["produce"] is False + + +def test_a_curated_url_that_fails_its_probe_is_used_anyway( + repair_module, no_search, monkeypatch, caplog): + """upload.wikimedia.org throttles a fast sweep and the probe treats 429 + exactly like 404. Falling back to a search here would let a transient + rate-limit quietly replace reviewed photos with searched ones.""" + from app.services import produce_reference + + monkeypatch.setattr(repair_module, "_url_is_live", lambda url, timeout: False) + + with caplog.at_level("WARNING"): + found = repair_module._search_replacement(CURATED_COMMODITY, BUCKET, 8) + + assert found == [produce_reference.image_url(CURATED_COMMODITY)] + assert no_search == [], "a failed probe must not trigger a search" + assert "did not verify" in caplog.text + + +# --------------------------------------------------------------------------- +# The derived-product guard +# --------------------------------------------------------------------------- +@pytest.mark.parametrize("name", [ + "Coconut Oil", "Apple Cider Vinegar", "Tomato Ketchup", "Corn Flour", +]) +def test_a_derived_product_does_not_inherit_the_raw_commodity_photo(name): + """The prefix fallback that correctly resolves `Mango Totapuri` to `Mango` + also resolves `Coconut Oil` to `Coconut`. A bottle of oil is not a variety + of coconut, and showing the raw fruit for it is the apple-juice error + pointing the other way.""" + from app.services import produce_reference + + assert produce_reference.image_url(name) is None + + +def test_the_guard_does_not_touch_nutrition_matching(): + """`usda_fdc_id` keeps the plain fallback on purpose - the guard is about + not showing a misleading picture, and nutrition has its own dispatch.""" + from app.services import produce_reference + + assert produce_reference.usda_fdc_id("Apple 1kg") == 171688 + assert produce_reference.usda_fdc_id("Coconut Oil") is not None + + +# --------------------------------------------------------------------------- +# Stage 6 of the ingestion pipeline +# --------------------------------------------------------------------------- +@pytest.fixture +def stage_6(monkeypatch): + """`stage_6_images` plus a recorder on the ranking it must not reach.""" + from app.core import catalog_engine as catalog_engine_module + from app.core import store_catalog_pipeline + + ranked = [] + + def _fake_rank(urls, title, brand, max_images=20): + ranked.append(title) + return list(urls) + + monkeypatch.setattr(catalog_engine_module.catalog_engine, + "_select_best_images", _fake_rank) + return store_catalog_pipeline, ranked + + +def _produce_row(name): + from app.services.generic_products import OWN_PRODUCTS_BRAND + return {"product_name": name, "brand": OWN_PRODUCTS_BRAND} + + +def test_an_uploaded_commodity_gets_the_reviewed_image(stage_6, no_search): + from app.services import produce_reference + pipeline, ranked = stage_6 + + row = pipeline.stage_6_images(_produce_row(CURATED_COMMODITY)) + + expected = produce_reference.image_url(CURATED_COMMODITY) + assert row["image_url"] == expected + assert row["image_urls"] == [expected] + assert no_search == [] + + +def test_the_reviewed_image_is_not_put_through_the_ranking(stage_6, no_search): + """`_select_best_images` scores a URL by whether it names the product, so + `Honeycrisp.jpg` scores zero and any searched URL containing "apple" sorts + above it. Index 0 is what becomes `image_url`, so merging rather than + short-circuiting would leave the wrong image on the card.""" + pipeline, ranked = stage_6 + + pipeline.stage_6_images(_produce_row(CURATED_COMMODITY)) + + assert ranked == [] + + +def test_uploaded_produce_with_no_curated_entry_still_gets_produce_mode( + stage_6, no_search): + """The fix that was missing entirely: this stage never passed `produce=`, so + every new commodity was searched against the packaged-goods databases with + the "-plant -tree -fish" exclusion block that fights produce queries.""" + pipeline, ranked = stage_6 + + pipeline.stage_6_images(_produce_row(UNCURATED_COMMODITY)) + + assert len(no_search) == 1 + assert no_search[0]["produce"] is True + assert no_search[0]["brand"] is None, "the bucket must not enter the query" + + +def test_an_uploaded_branded_product_is_unchanged(stage_6, no_search): + pipeline, ranked = stage_6 + + pipeline.stage_6_images({"product_name": "Britannia Good Day 200g", + "brand": "Britannia"}) + + assert len(no_search) == 1 + assert no_search[0]["produce"] is False + assert no_search[0]["brand"] == "Britannia" + + +def test_a_row_that_already_has_images_is_left_alone(stage_6, no_search): + pipeline, ranked = stage_6 + row = dict(_produce_row(CURATED_COMMODITY), + image_urls=["https://example.com/already.jpg"]) + + assert pipeline.stage_6_images(row)["image_urls"] == [ + "https://example.com/already.jpg"] + assert no_search == [] + + +def test_images_can_still_be_switched_off(stage_6, no_search): + pipeline, ranked = stage_6 + + row = pipeline.stage_6_images(_produce_row(CURATED_COMMODITY), enabled=False) + + assert "image_url" not in row + assert no_search == []