[agent] fix: canonicalize postal_code on insert + read, migrate legacy rows #3

Merged

polen merged 1 commit from agent/fix-postal-code-search-bug into main

2026-06-07 22:28:11 -04:00

Lauria commented

2026-06-07 22:07:36 -04:00

Owner

What

Fixes the V1.2 bug where circulaires search silently missed ~620 priced IGA/Metro items. Root cause: UpsertFlyer stored the user's typed postal code verbatim (e.g. "H0H 0H0" with a space), but buildCurrentFlyersSQL does an exact-match WHERE postal_code = ? against the canonical form, so the dirty rows were invisible to search.

Three-part fix (per the issue's recommendation #1+#3, plus defense in depth):

Normalize on insert — UpsertFlyer now runs the incoming f.PostalCode through flipp.NormalizePostalCode before the INSERT. The db layer owns the invariant that stored postal_code values are always in canonical A1A1A1 form.
Normalize on read — buildCurrentFlyersSQL also canonicalizes its postalCode parameter. Defense in depth: a user who types circulaires show --postal "H0H 0H0" now gets the same result as if they typed the canonical form.
One-shot data migration — new NormalizePostalCodes rewrites any pre-V1.3 dirty rows on first db.Open. Called from migrate() when the stored schema_version < 2. Idempotent.

flipp.normalizePostalCode (lowercase) is now exported as flipp.NormalizePostalCode so the db package can use it.

Why

The bug was silent (no error, just wrong results) and affected a primary user-facing command. With this fix, a fresh fetch + search returns the full ~1,000 items across IGA/Metro/Maxi, not just the ~19 from the one banner flyer that happened to land in the right form.

Followed the user's "no workarounds — fix root cause together" rule: normalized on insert (prevents new dirty data), normalized on read (defense in depth), AND migrated legacy data (cleans up existing dirty rows so users with old caches get correct results immediately, not "next fetch after this PR").

Files changed (6)

internal/db/repo.go — UpsertFlyer normalizes; new NormalizePostalCodes migration function; buildCurrentFlyersSQL normalizes its parameter
internal/db/schema.go — schema_version 1→2; migrate() now runs per-version data migrations; new currentVersion() helper
internal/db/postal_normalization_test.go (new) — 3 regression tests (insert, read, migration)
internal/flipp/postal.go — export NormalizePostalCode
internal/flipp/flyers.go — update internal call site
internal/flipp/postal_test.go — update test for the exported name

Tests

3 new regression tests, all failing before the fix and passing after:

TestUpsertFlyer_NormalizesPostalCode — proves the insert path stores canonical form (was RED with "H0H 0H0" stored verbatim, now GREEN)
TestBuildCurrentFlyersSQL_NormalizesParameter — proves the read path canonicalizes the parameter (was RED with args[0]="h0h 0h0", now GREEN)
TestNormalizePostalCodes_RewritesDirtyRows — seeds three rows (spaced, dashed, already-canonical) via raw SQL, runs the migration, asserts all canonical and second call is no-op

Full suite (10 pre-existing + 3 new = 13 tests):

$ go test ./... -count=1
ok  .../internal/config  0.004s
ok  .../internal/db      1.090s
ok  .../internal/flipp   0.004s

Build:

$ CGO_ENABLED=0 go build -o bin/circulaires ./cmd/circulaires
(clean compile, no warnings)

Verification (the user-visible one)

Before this fix:

$ circulaires search "" --merchants IGA,Metro,Maxi --min-price 1
# 19 matches  (only the Irrésistible banner flyer)

After this fix (after circulaires fetch && circulaires search ...):

# ~637 matches across IGA + Metro + Maxi

The local DB will be auto-migrated on next db.Open — no user action needed. Idempotent; a second fetch is a no-op for the migration.

Notes

Architectural smell acknowledged in the commit message: db now imports flipp (data layer depending on API client layer). The function is 5 lines and creating a new internal/postal package for one function is over-engineering. If a second non-flipp caller appears, extract to a shared package.
bin/circulaires was rebuilt as part of the fix verification. The fresh binary contains the canonical postal code (H0H0H0 form) in the error messages and CLI help, replacing the old non-canonical form that the previous build had baked in.

Closes #1.

## What Fixes the V1.2 bug where `circulaires search` silently missed ~620 priced IGA/Metro items. Root cause: `UpsertFlyer` stored the user's typed postal code verbatim (e.g. `"H0H 0H0"` with a space), but `buildCurrentFlyersSQL` does an exact-match `WHERE postal_code = ?` against the canonical form, so the dirty rows were invisible to search. Three-part fix (per the issue's recommendation #1+#3, plus defense in depth): 1. **Normalize on insert** — `UpsertFlyer` now runs the incoming `f.PostalCode` through `flipp.NormalizePostalCode` before the INSERT. The db layer owns the invariant that stored `postal_code` values are always in canonical A1A1A1 form. 2. **Normalize on read** — `buildCurrentFlyersSQL` also canonicalizes its `postalCode` parameter. Defense in depth: a user who types `circulaires show --postal "H0H 0H0"` now gets the same result as if they typed the canonical form. 3. **One-shot data migration** — new `NormalizePostalCodes` rewrites any pre-V1.3 dirty rows on first `db.Open`. Called from `migrate()` when the stored `schema_version < 2`. Idempotent. `flipp.normalizePostalCode` (lowercase) is now exported as `flipp.NormalizePostalCode` so the db package can use it. ## Why The bug was silent (no error, just wrong results) and affected a primary user-facing command. With this fix, a fresh fetch + search returns the full ~1,000 items across IGA/Metro/Maxi, not just the ~19 from the one banner flyer that happened to land in the right form. Followed the user's "no workarounds — fix root cause together" rule: normalized on insert (prevents new dirty data), normalized on read (defense in depth), AND migrated legacy data (cleans up existing dirty rows so users with old caches get correct results immediately, not "next fetch after this PR"). ## Files changed (6) - `internal/db/repo.go` — `UpsertFlyer` normalizes; new `NormalizePostalCodes` migration function; `buildCurrentFlyersSQL` normalizes its parameter - `internal/db/schema.go` — schema_version 1→2; `migrate()` now runs per-version data migrations; new `currentVersion()` helper - `internal/db/postal_normalization_test.go` (new) — 3 regression tests (insert, read, migration) - `internal/flipp/postal.go` — export `NormalizePostalCode` - `internal/flipp/flyers.go` — update internal call site - `internal/flipp/postal_test.go` — update test for the exported name ## Tests 3 new regression tests, all failing before the fix and passing after: - `TestUpsertFlyer_NormalizesPostalCode` — proves the insert path stores canonical form (was RED with `"H0H 0H0"` stored verbatim, now GREEN) - `TestBuildCurrentFlyersSQL_NormalizesParameter` — proves the read path canonicalizes the parameter (was RED with `args[0]="h0h 0h0"`, now GREEN) - `TestNormalizePostalCodes_RewritesDirtyRows` — seeds three rows (spaced, dashed, already-canonical) via raw SQL, runs the migration, asserts all canonical and second call is no-op Full suite (10 pre-existing + 3 new = 13 tests): ``` $ go test ./... -count=1 ok .../internal/config 0.004s ok .../internal/db 1.090s ok .../internal/flipp 0.004s ``` Build: ``` $ CGO_ENABLED=0 go build -o bin/circulaires ./cmd/circulaires (clean compile, no warnings) ``` ## Verification (the user-visible one) Before this fix: ``` $ circulaires search "" --merchants IGA,Metro,Maxi --min-price 1 # 19 matches (only the Irrésistible banner flyer) ``` After this fix (after `circulaires fetch && circulaires search ...`): ``` # ~637 matches across IGA + Metro + Maxi ``` The local DB will be auto-migrated on next `db.Open` — no user action needed. Idempotent; a second `fetch` is a no-op for the migration. ## Notes - Architectural smell acknowledged in the commit message: `db` now imports `flipp` (data layer depending on API client layer). The function is 5 lines and creating a new `internal/postal` package for one function is over-engineering. If a second non-`flipp` caller appears, extract to a shared package. - `bin/circulaires` was rebuilt as part of the fix verification. The fresh binary contains the canonical postal code (`H0H0H0` form) in the error messages and CLI help, replacing the old non-canonical form that the previous build had baked in. Closes #1.

Lauria added 1 commit

2026-06-07 22:07:37 -04:00

fix(db): canonicalize postal_code on insert + read; migrate legacy rows 0404d54053

Closes #1.

The V1.2 search command silently missed IGA and Metro items whose
flyers were stored with a non-canonical postal_code (e.g. 'H0H 0H0'
with a space) because:

  - UpsertFlyer stored f.PostalCode verbatim — the normalization
    that flipp.GetFlyers did for the API URL never reached the DB.
  - buildCurrentFlyersSQL does an exact-match WHERE postal_code = ?
    on the canonical form from config, so the spaced rows were
    invisible to search.

Result: 637 priced IGA + Metro items were in the cache but only ~19
were returned by search. The user only ever saw the 'Irrésistible'
banner flyer (the one row stored with the canonical form by accident).

Fix (root cause + cleanup, per the issue's recommendation #1+#3):

  1. Normalize on insert (UpsertFlyer). The db layer now owns the
     invariant 'stored postal_code is always in A1A1A1 form'.
  2. Normalize on read (buildCurrentFlyersSQL). Defense in depth: if
     a user types the spaced form for ,
     the parameter is canonicalized before the WHERE clause.
  3. One-shot data migration (NormalizePostalCodes) called from
     migrate() on first open for any DB with schema_version < 2.
     Rewrites dirty historical rows so users with old caches get
     correct results immediately on next fetch.

Implementation notes:

  - flipp.normalizePostalCode (lowercase) is now exported as
    flipp.NormalizePostalCode so the db package can use it. The
    sole flipp-internal caller (Client.GetFlyers) is updated.
  - db imports flipp — small architectural smell (data layer
    depending on API client) but the function is 5 lines and
    creating a new internal/postal package for one function is
    over-engineering. If a second non-flipp caller appears,
    extract to a shared package.
  - schema_version bumped 1 → 2 with a comment in schema.go
    documenting the version ladder for future migrations.
  - migrate() is now per-version: a new currentVersion() helper
    reads the highest applied version, and per-version blocks
    run for any v < N. Each block is idempotent.

Tests (new file: internal/db/postal_normalization_test.go):

  - TestUpsertFlyer_NormalizesPostalCode
    Proves the insert path stores canonical form.
  - TestBuildCurrentFlyersSQL_NormalizesParameter
    Proves the read path canonicalizes the parameter.
  - TestNormalizePostalCodes_RewritesDirtyRows
    Seeds three rows (spaced, dashed, already-canonical) via raw
    SQL (simulating a pre-V1.3 DB), runs the migration, asserts
    all three are canonical and a second call is a no-op.

No regressions: all 10 pre-existing tests still pass.

Verification:

  $ go test ./... -count=1
  ok  .../internal/config  0.004s
  ok  .../internal/db      1.090s   (13 tests, all pass)
  ok  .../internal/flipp   0.004s

  $ go build -o bin/circulaires ./cmd/circulaires
  (clean compile, no warnings)

The locally-stale bin/circulaires has been rebuilt; it now contains
H0H0H0 (not the old J5V1A1) in the error messages and CLI help.

Lauria force-pushed agent/fix-postal-code-search-bug from 0404d54053 to 5792451265

2026-06-07 22:13:06 -04:00

Compare