catalogue images

repositories/catalogueColumns_test.go

@@ -83,3 +83,74 @@ func TestCoreColumnsAreAlwaysSelectedPlainly(t *testing.T) {
 		t.Errorf("core columns should lead the select list, got:\n%s", got)
 	}
 }
+
+/*
+Where product photos come from.
+
+The owning team records what its image search found, per product, in
+`image_url` and `image_urls`. Most of those are EXTERNAL — bigbasket, amazon, a
+shop's own CDN — and only a subset were ever mirrored into our bucket. Reading
+the bucket alone found photos for 2 of britannia's 6 products while they had
+URLs for all 6.
+*/
+
+func TestJsonImageListIsRead(t *testing.T) {
+	// The shape their API returns today.
+	raw := `["https://www.bigbasket.com/media/uploads/p/l/270729_21-britannia.jpg",` +
+		`"https://m.media-amazon.com/images/I/71n1Q3cQL3L.jpg"]`
+	got := parseImageList(raw)
+	if len(got) != 2 || got[0] != "https://www.bigbasket.com/media/uploads/p/l/270729_21-britannia.jpg" {
+		t.Fatalf("json list not parsed: %#v", got)
+	}
+}
+
+// The same column stored as a Postgres text[] rather than jsonb. Which one it
+// is belongs to the owning team, and this side should not break when it moves.
+func TestPostgresArrayImageListIsRead(t *testing.T) {
+	got := parseImageList(`{"https://a.example/1.jpg","https://b.example/2.jpg"}`)
+	if len(got) != 2 || got[1] != "https://b.example/2.jpg" {
+		t.Fatalf("pg array not parsed: %#v", got)
+	}
+}
+
+func TestEmptyImageListsAreNotPhotos(t *testing.T) {
+	for _, raw := range []string{"", "  ", "{}", "[]", "null"} {
+		if got := parseImageList(raw); len(got) != 0 {
+			t.Errorf("%q should yield no photos, got %#v", raw, got)
+		}
+	}
+}
+
+// A malformed value must cost the photos, not the product.
+func TestMalformedImageListDoesNotBreakTheRow(t *testing.T) {
+	if got := parseImageList(`["unterminated`); len(got) != 0 {
+		t.Errorf("expected no photos from malformed json, got %#v", got)
+	}
+}
+
+func TestTheListedUrlWinsAndDuplicatesCollapse(t *testing.T) {
+	row := catalogueProductRow{
+		ImageID:   "britannia_britannia_marie_gold_250g",
+		ImageURL:  "https://a.example/1.jpg",
+		ImageURLs: `["https://a.example/1.jpg","https://a.example/2.jpg"]`,
+	}
+	got := imagesFor("britannia", row)
+	if len(got) != 2 {
+		t.Fatalf("the primary url repeated in the list should appear once: %#v", got)
+	}
+	if got[0] != "https://a.example/1.jpg" || got[1] != "https://a.example/2.jpg" {
+		t.Errorf("order should follow image_urls: %#v", got)
+	}
+}
+
+// With no URLs recorded, the bucket listing is still consulted — it is the only
+// source for anything ingested before these columns existed.
+func TestNoRecordedUrlsFallsBackToTheBucket(t *testing.T) {
+	row := catalogueProductRow{ImageID: "cadbury_cadbury_5_star_5_gm"}
+	// db.GetImages returns nil when the image store was never initialised,
+	// which is the case here — the point is that it does not panic and does not
+	// invent a photo.
+	if got := imagesFor("cadbury", row); len(got) != 0 {
+		t.Errorf("expected no photos without an image store, got %#v", got)
+	}
+}

repositories/catalogueRepository.go

@@ -1,6 +1,7 @@
 package repositories
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
 	"log"
@@ -69,6 +70,24 @@ var catalogueOptionalColumns = []struct{ Name, Present, Absent string }{
 	{"search_query", "search_query", "NULL::text AS search_query"},
 	{"created_at", "created_at", "NULL::timestamptz AS created_at"},
 	{"updated_at", "updated_at", "NULL::timestamptz AS updated_at"},
+
+	// Where the photos actually are.
+	//
+	// The catalogue pipeline records what it found per product: `image_url` for
+	// the primary and `image_urls` for the rest. Most are EXTERNAL — bigbasket,
+	// amazon, a shop's own CDN — because stage 6 searches the web and only some
+	// results get mirrored into our bucket.
+	//
+	// This side ignored both columns and instead listed bucket objects under
+	// `daily/brands/{brand}/{image_id}/`, so a product showed a photo only if it
+	// happened to have been mirrored. Measured on britannia: 2 of 6 products had
+	// images here while the owning team had URLs for all 6.
+	//
+	// Cast to text and parsed by hand: the column is an array, and GORM's raw
+	// scan silently drops slice-kind destination fields — the same reason
+	// providers, highlights and nutrients are read this way.
+	{"image_url", "image_url", "NULL::text AS image_url"},
+	{"image_urls", "image_urls::text AS image_urls", "NULL::text AS image_urls"},
 }
 
 // columnsFor builds the SELECT list for one table from the columns it has.
@@ -118,6 +137,65 @@ type catalogueProductRow struct {
 	SearchQuery  string
 	CreatedAt    time.Time
 	UpdatedAt    time.Time
+	ImageURL     string
+	ImageURLs    string
+}
+
+// imagesFor decides which photos a catalogue product carries.
+//
+// The pipeline's own `image_urls` first, then its single `image_url`, and the
+// bucket listing last. That order is deliberate: the owning team records what
+// it found for every product, and only a subset of those files were ever
+// mirrored into our bucket — so listing the bucket alone finds photos for a
+// minority of the catalogue and reports the rest as having none.
+//
+// The bucket stays as the fallback rather than being dropped. It is the only
+// source for anything ingested before these columns existed, and it is the one
+// source that cannot rot: a external URL is somebody else's CDN and will
+// eventually 404, at which point the card falls through to the next photo.
+func imagesFor(brand string, row catalogueProductRow) []string {
+	seen := make(map[string]bool)
+	var out []string
+	add := func(url string) {
+		url = strings.TrimSpace(url)
+		if url == "" || seen[url] {
+			return
+		}
+		seen[url] = true
+		out = append(out, url)
+	}
+
+	for _, url := range parseImageList(row.ImageURLs) {
+		add(url)
+	}
+	add(row.ImageURL)
+
+	if len(out) == 0 {
+		return db.GetImages(brand, row.ImageID)
+	}
+	return out
+}
+
+// parseImageList reads the list whichever way it was stored.
+//
+// A Postgres text[] casts to `{a,b}` and a jsonb column to `["a","b"]`, and the
+// column type is the owning team's to change. Handling both here costs four
+// lines and removes a whole class of "it worked until they migrated".
+func parseImageList(raw string) []string {
+	raw = strings.TrimSpace(raw)
+	if raw == "" || raw == "{}" || raw == "[]" || raw == "null" {
+		return nil
+	}
+	if strings.HasPrefix(raw, "[") {
+		var urls []string
+		if err := json.Unmarshal([]byte(raw), &urls); err == nil {
+			return urls
+		}
+		// Falls through on a malformed value rather than dropping the row.
+		log.Printf("catalogue: could not parse image_urls as JSON: %.120s", raw)
+		return nil
+	}
+	return models.ParsePGArray(raw)
 }
 
 func (row catalogueProductRow) toModel(brand string) models.CatalogueProduct {
@@ -129,7 +207,7 @@ func (row catalogueProductRow) toModel(brand string) models.CatalogueProduct {
 		Description:  row.Description,
 		Category:     row.Category,
 		ImageID:      row.ImageID,
-		Images:       db.GetImages(brand, row.ImageID),
+		Images:       imagesFor(brand, row),
 		Size:         row.Size,
 		VariantKey:   row.VariantKey,
 		ProductSKU:   row.ProductSKU,