diff --git a/repositories/catalogueColumns_test.go b/repositories/catalogueColumns_test.go new file mode 100644 index 0000000..7830e80 --- /dev/null +++ b/repositories/catalogueColumns_test.go @@ -0,0 +1,85 @@ +package repositories + +import ( + "strings" + "testing" +) + +// The bug: a brand table one column short of the old eighteen-column check was +// not degraded, it was INVISIBLE — missing from getbrands and "Unknown brand" +// on every read. 19 of the owning team's 35 brands were reachable; the other 16 +// existed and could not be seen from this side at all. + +func columnSet(names ...string) map[string]bool { + have := make(map[string]bool, len(names)) + for _, n := range names { + have[n] = true + } + return have +} + +var everyColumn = func() map[string]bool { + names := append([]string{}, catalogueCoreColumns...) + for _, c := range catalogueOptionalColumns { + names = append(names, c.Name) + } + return columnSet(names...) +}() + +func TestAFullTableSelectsEveryRealColumn(t *testing.T) { + got := columnsFor(everyColumn) + if strings.Contains(got, "NULL::") { + t.Errorf("a complete table should select no NULL stand-ins:\n%s", got) + } + for _, want := range []string{"product_sku", "providers::text AS providers", "fssai_license"} { + if !strings.Contains(got, want) { + t.Errorf("missing %q from:\n%s", want, got) + } + } +} + +// The exact shape that used to vanish: everything but one enrichment column. +func TestATableMissingOneColumnIsStillReadable(t *testing.T) { + have := columnSet() + for name := range everyColumn { + have[name] = true + } + delete(have, "fssai_license") + + got := columnsFor(have) + if !strings.Contains(got, "NULL::text AS fssai_license") { + t.Errorf("absent column should be selected as NULL:\n%s", got) + } + if strings.Contains(got, ", fssai_license,") { + t.Errorf("must not select a column the table does not have:\n%s", got) + } + if !strings.Contains(got, "product_name") { + t.Errorf("the rest of the table must still be read:\n%s", got) + } +} + +// A NULL stand-in has to carry the same cast as the real column, or the scan +// destination changes type between one brand table and the next. +func TestNullStandInsKeepTheColumnType(t *testing.T) { + bare := columnsFor(columnSet("id", "product_name")) + for _, want := range []string{ + "NULL::text AS providers", + "NULL::text AS highlights", + "NULL::text AS nutrients", + "NULL::timestamptz AS created_at", + "NULL::timestamptz AS updated_at", + } { + if !strings.Contains(bare, want) { + t.Errorf("missing %q from:\n%s", want, bare) + } + } +} + +// Core columns are never substituted — without an id there is nothing to order +// or address rows by, which is why a table lacking them is skipped outright. +func TestCoreColumnsAreAlwaysSelectedPlainly(t *testing.T) { + got := columnsFor(columnSet("id", "product_name")) + if !strings.HasPrefix(got, "id, product_name") { + t.Errorf("core columns should lead the select list, got:\n%s", got) + } +} diff --git a/repositories/catalogueRepository.go b/repositories/catalogueRepository.go index 2983d83..8ac7f00 100644 --- a/repositories/catalogueRepository.go +++ b/repositories/catalogueRepository.go @@ -40,6 +40,63 @@ const catalogueProductColumns = `id, product_name, title, description, category, variant_key, product_sku, sku_source, price_range, providers::text AS providers, fssai_license, highlights::text AS highlights, nutrients::text AS nutrients, search_query, created_at, updated_at` +// catalogueCoreColumns is the minimum a `brand_*` table must have to be worth +// reading: an id to order and address rows by, and a name to show. Everything +// else is enrichment, and a table missing some of it is still a perfectly good +// catalogue of products. +var catalogueCoreColumns = []string{"id", "product_name"} + +// catalogueOptionalColumns is selected when present and replaced with NULL when +// it is not, so one absent column costs that column rather than the whole brand. +// +// The expression is carried next to the name because three of these are text[] +// and need casting; the NULL stand-in has to be cast the same way or the scan +// destination changes type from one table to the next. +var catalogueOptionalColumns = []struct{ Name, Present, Absent string }{ + {"title", "title", "NULL::text AS title"}, + {"description", "description", "NULL::text AS description"}, + {"category", "category", "NULL::text AS category"}, + {"image_id", "image_id", "NULL::text AS image_id"}, + {"size", "size", "NULL::text AS size"}, + {"variant_key", "variant_key", "NULL::text AS variant_key"}, + {"product_sku", "product_sku", "NULL::text AS product_sku"}, + {"sku_source", "sku_source", "NULL::text AS sku_source"}, + {"price_range", "price_range", "NULL::text AS price_range"}, + {"providers", "providers::text AS providers", "NULL::text AS providers"}, + {"fssai_license", "fssai_license", "NULL::text AS fssai_license"}, + {"highlights", "highlights::text AS highlights", "NULL::text AS highlights"}, + {"nutrients", "nutrients::text AS nutrients", "NULL::text AS nutrients"}, + {"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"}, +} + +// columnsFor builds the SELECT list for one table from the columns it has. +// +// This is the fix for brands going missing. Discovery used to demand all +// eighteen columns — `HAVING COUNT(DISTINCT column_name) = 18` — so a brand +// table written by a newer pipeline, with one column renamed or not yet added, +// was not merely degraded but INVISIBLE: absent from `getbrands`, and "Unknown +// brand" on every read. Measured against the live catalogue, 19 of the owning +// team's 35 brands were reachable and the other 16 could not be seen from this +// side at all. +// +// Selecting NULL for what is absent turns that into the smaller, honest +// failure: the brand appears, its products list, and the fields nobody wrote +// come back empty. +func columnsFor(have map[string]bool) string { + parts := make([]string, 0, len(catalogueCoreColumns)+len(catalogueOptionalColumns)) + parts = append(parts, catalogueCoreColumns...) + for _, col := range catalogueOptionalColumns { + if have[col.Name] { + parts = append(parts, col.Present) + continue + } + parts = append(parts, col.Absent) + } + return strings.Join(parts, ", ") +} + // catalogueProductRow mirrors catalogueProductColumns for scanning; array // columns land here as their raw Postgres text[] literal. type catalogueProductRow struct { @@ -118,10 +175,11 @@ func NewCatalogueRepository(db *gorm.DB) CatalogueRepository { // // Two things it checks that a name alone would not: // -// - the table has every column `catalogueProductColumns` selects. A -// `brand_*` table of a different shape would error on read, which is -// exactly the `brand_sakthi` failure — a table that exists in the map and -// cannot be queried. +// - the table has the CORE columns — an id and a product name. It used to +// demand all eighteen, which turned "this table is a little different" into +// "this brand does not exist": 16 of 35 brands were unreachable that way. +// Anything beyond the core is now selected as NULL when absent, so a table +// of a different shape is read rather than hidden. // - discovery returning nothing falls back to the literal map, so a // permissions problem on information_schema cannot take the catalogue dark. // @@ -130,8 +188,27 @@ var ( brandTablesMu sync.RWMutex brandTablesData map[string]string brandTablesAt time.Time + + // The columns each discovered table actually has, filled by + // discoverBrandTables and read by columnsForTable. Kept beside the table map + // and refreshed with it, so the two can never describe different schemas. + brandColumnsMu sync.RWMutex + brandColumnsData map[string]map[string]bool ) +// columnsForTable is the SELECT list for one table, or the full fixed list when +// discovery has not run — which is the built-in fallback case, where the tables +// are the known-good ones and every column is present by definition. +func columnsForTable(table string) string { + brandColumnsMu.RLock() + have := brandColumnsData[table] + brandColumnsMu.RUnlock() + if have == nil { + return catalogueProductColumns + } + return columnsFor(have) +} + const brandTablesTTL = 5 * time.Minute func (r *catalogueRepository) brandTables() map[string]string { @@ -159,34 +236,76 @@ func (r *catalogueRepository) brandTables() map[string]string { return discovered } -// discoverBrandTables lists `brand_*` tables that carry every column the reader -// needs, in one query. +// discoverBrandTables lists every `brand_*` table that can be read at all, and +// records which columns each one actually has. +// +// It asks for the CORE columns only. The previous version demanded all eighteen +// — `HAVING COUNT(DISTINCT column_name) = 18` — which sounds like a safety check +// and behaves like a filter: a table whose pipeline had renamed one column, or +// not written it yet, failed the count and disappeared from the product +// entirely. It was not listed by `getbrands` and every read of it answered +// "Unknown brand", so from this side the brand did not exist. +// +// That is how 19 of the owning team's 35 brands were visible. The missing ones +// were not broken and not empty; they were a column short of a test that had no +// need to be that strict, and nothing anywhere said so. +// +// The per-table column set is read in the same query and handed to columnsFor, +// which substitutes NULL for anything absent — so a table can now be missing +// enrichment without being missing. func (r *catalogueRepository) discoverBrandTables() (map[string]string, error) { - required := []string{ - "id", "product_name", "title", "description", "category", "image_id", "size", - "variant_key", "product_sku", "sku_source", "price_range", "providers", - "fssai_license", "highlights", "nutrients", "search_query", "created_at", "updated_at", - } - // `IN (?)` with a slice rather than `= ANY(?)` with a driver array type: // GORM expands the former itself, and the latter would pull in lib/pq for // one call in a codebase that has no other use for it. - var names []string + var rows []struct { + TableName string + ColumnName string + } err := r.db.Raw(` - SELECT c.table_name + SELECT c.table_name, c.column_name FROM information_schema.columns c WHERE c.table_schema = 'public' AND c.table_name LIKE 'brand\_%' - AND c.column_name IN (?) - GROUP BY c.table_name - HAVING COUNT(DISTINCT c.column_name) = ? - ORDER BY c.table_name`, - required, len(required), - ).Scan(&names).Error + ORDER BY c.table_name`).Scan(&rows).Error if err != nil { return nil, err } + // Group the columns by table, then keep the tables that have the core set. + byTable := make(map[string]map[string]bool) + for _, row := range rows { + if byTable[row.TableName] == nil { + byTable[row.TableName] = make(map[string]bool) + } + byTable[row.TableName][row.ColumnName] = true + } + + var names []string + columns := make(map[string]map[string]bool, len(byTable)) + for table, have := range byTable { + usable := true + for _, core := range catalogueCoreColumns { + if !have[core] { + usable = false + break + } + } + if !usable { + // Named individually. A brand dropped here is a brand nobody can + // reach, and silence about it is what made the last one take a + // round trip through two teams to find. + log.Printf("catalogue: skipping %q — it lacks one of the core columns %v", table, catalogueCoreColumns) + continue + } + names = append(names, table) + columns[table] = have + } + sort.Strings(names) + + brandColumnsMu.Lock() + brandColumnsData = columns + brandColumnsMu.Unlock() + out := make(map[string]string, len(names)) for _, table := range names { brand := strings.TrimPrefix(table, "brand_") @@ -297,7 +416,7 @@ func (r *catalogueRepository) getProductsForBrand(brand, category, keyword strin var rows []catalogueProductRow dataQuery := fmt.Sprintf( `SELECT %s FROM %s WHERE %s ORDER BY id LIMIT ? OFFSET ?`, - catalogueProductColumns, table, whereClause, + columnsForTable(table), table, whereClause, ) dataArgs := append(append([]interface{}{}, args...), pagesize, offset) if err := r.db.Raw(dataQuery, dataArgs...).Scan(&rows).Error; err != nil { @@ -335,7 +454,7 @@ func (r *catalogueRepository) getProductsAllBrands(category, keyword string, pag table := tables[brand] var rows []catalogueProductRow - dataQuery := fmt.Sprintf(`SELECT %s FROM %s WHERE %s ORDER BY id`, catalogueProductColumns, table, whereClause) + dataQuery := fmt.Sprintf(`SELECT %s FROM %s WHERE %s ORDER BY id`, columnsForTable(table), table, whereClause) if err := r.db.Raw(dataQuery, args...).Scan(&rows).Error; err != nil { // Skip the brand rather than abandoning the merge. This aborted on // the first failure, so one absent table returned 500 for a browse @@ -404,7 +523,7 @@ func (r *catalogueRepository) GetProductBySKU(brand, sku string) (*models.Catalo var row catalogueProductRow query := fmt.Sprintf( `SELECT %s FROM %s WHERE product_sku = ? LIMIT 1`, - catalogueProductColumns, table, + columnsForTable(table), table, ) result := r.db.Raw(query, sku).Scan(&row) if result.Error != nil { @@ -431,7 +550,7 @@ func (r *catalogueRepository) GetProductByID(brand string, id int64) (*models.Ca var row catalogueProductRow query := fmt.Sprintf( `SELECT %s FROM %s WHERE id = ? LIMIT 1`, - catalogueProductColumns, table, + columnsForTable(table), table, ) result := r.db.Raw(query, id).Scan(&row) if result.Error != nil {