ben.stull
a851d3587c
Addresses adversarial-review findings: a canonical file carrying a stray Shopify-signature name (e.g. SEO Title) no longer misdetects as Shopify and silently drops canonical Type / corrupts the weight unit (kg->g). A canonical-distinctive column (Description/Variant Cost/Variant Weight/Google Product Category/Variant Volume/Tax ID/Position) now vetoes detection, so detection leans conservative (under-detection warns; over-detection corrupts). Also closes the dual-named-column shadow (Body (HTML)+Description) and the stale-weight-unit-on-clear case. Tests cover all three. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
276 lines
15 KiB
Markdown
276 lines
15 KiB
Markdown
# products domain — developer notes
|
||
|
||
DOC-4 (SD-0002 §11): the import/export spine as built in SLICE-5, extended by
|
||
SLICE-6–8. Operator-facing material is in [`OPERATIONS.md`](./OPERATIONS.md);
|
||
the spec is SD-0002 in the content repo.
|
||
|
||
## Layout and pipeline
|
||
|
||
`backend/app/domains/products/` is layered like the rest of the app
|
||
(main → domains → platform, enforced by import-linter):
|
||
|
||
- `models.py` — canonical row model + the column registry.
|
||
- `codec.py` — bytes → `ParsedFile`; file-level gates only.
|
||
- `validate.py` — rows → `CanonicalProduct` blocks + per-row errors.
|
||
- `diff.py` — catalog × canonical products → apply plan + preview records.
|
||
- `serialize.py` — the export half: `CatalogProduct` snapshot → canonical CSV
|
||
(the inverse of `codec`/`validate`). DB-free, like `diff.py`.
|
||
- `repo.py` — SQL only: catalog snapshot, draft/run CRUD, apply primitives.
|
||
Never commits or rolls back.
|
||
- `service.py` — use-case orchestration; owns every transaction boundary and
|
||
emits the TEL events via `backend/app/platform/telemetry.py`.
|
||
|
||
```mermaid
|
||
flowchart LR
|
||
subgraph validate_path [import_validate]
|
||
A[parse_csv] --> B[build_products] --> D[compute_diff] --> E[(import_draft)]
|
||
C[load_catalog] --> D
|
||
end
|
||
subgraph confirm_path [confirm_draft]
|
||
E --> F[re-derive: parse → build → diff] --> G{fingerprint match?}
|
||
G -- yes --> H[one-transaction apply<br/>run + products + delete draft]
|
||
G -- no --> I[PreviewStale<br/>draft kept]
|
||
end
|
||
```
|
||
|
||
Confirm re-derives everything from the draft's stored `file_bytes` against the
|
||
live catalog, then checks the fingerprint — so what lands is exactly what the
|
||
preview showed, or the confirm refuses (`preview_stale`). A confirm with no
|
||
adds and no updates refuses with `nothing_to_apply`.
|
||
|
||
## Canonical model and blank-vs-absent
|
||
|
||
`models.py` is the one model every dialect maps to (INV-17). `KNOWN_COLUMNS`
|
||
is the registry header detection, unknown-column warnings, and validation all
|
||
read; `CLEAR_DEFAULTS` holds the reset values for clearable fields
|
||
(`status`, `published`, `product_type`, `tags`).
|
||
|
||
The §6.5.1 cell semantics, as implemented:
|
||
|
||
- **Absent column** → the field never enters `fields{}` → untouched by diff
|
||
and apply (never a change).
|
||
- **Present-but-empty cell** → `fields[name] = None` (an explicit clear) →
|
||
resolved at diff time to its `CLEAR_DEFAULTS` entry, or NULL where none
|
||
exists.
|
||
- **The position exception:** a cleared `Variant Position` has no
|
||
`CLEAR_DEFAULTS` entry — `diff.resolved_variant_fields` resolves it to the
|
||
variant's 1-based file order within its product, never to NULL.
|
||
|
||
Option names live both in `CanonicalProduct.option_names` (the values) and in
|
||
`fields{}` (the file-presence marker the diff needs for the absent-vs-clear
|
||
distinction).
|
||
|
||
## Dialects (INV-17, SLICE-8)
|
||
|
||
A file is normalized to canonical names **at the codec boundary**, so every stage
|
||
downstream of `parse_csv` (validate → diff → apply) is dialect-agnostic — it only
|
||
ever sees canonical cells. Two dialects exist: `canonical` and `shopify`.
|
||
|
||
`dialect_shopify.py` is the Shopify adapter and the source of truth for the
|
||
mapping (mirrored by `tests/test_products_dialect_shopify.py` and
|
||
`tests/fixtures/shopify-export.csv`):
|
||
|
||
- **Detection** — `is_shopify_header()` returns `shopify` iff the header carries a
|
||
Shopify-only *signature* column (`Body (HTML)`, `Variant Grams`, `Cost per item`,
|
||
`Gift Card`, a `Google Shopping / …` column, …) **and no canonical-distinctive
|
||
column**. A canonical-distinctive name — one Shopify renames away (`Description`,
|
||
`Variant Cost`, `Variant Weight`, `Google Product Category`) or a canonical-only
|
||
column Shopify lacks (`Variant Volume`, `Variant Tax ID 1/2`, `Variant Position`) —
|
||
**vetoes** Shopify detection. Detection is by signature, not an exact header set, so
|
||
extra market/region columns don't defeat it; the veto biases it conservative, because
|
||
under-detection is safe (Shopify-named columns are warned as not-imported) while
|
||
over-detection corrupts (canonical `Type` / `Variant Weight Unit` would be dropped).
|
||
So a hybrid or ambiguous header falls through to `canonical`, which maps shared
|
||
columns identically and warns the rest — it never misparses (§7.4).
|
||
- **Mapping** — `map_shopify_header(header)` returns `(mapped, not_imported)`:
|
||
`mapped[i]` is the canonical name for column `i` (or `None` when it has no
|
||
canonical home), and `not_imported` is the warning list surfaced as the draft's
|
||
`unknown_columns`. The rule per column, in order: in `SHOPIFY_RENAME` →
|
||
canonical name; in `SHOPIFY_DROP` (`Type`, `Variant Weight Unit`) → not imported;
|
||
in `KNOWN_COLUMNS` → pass through; else → not imported (this last branch absorbs
|
||
the unbounded `… / <Market>` price columns without enumerating them).
|
||
- **The two name-collision overrides** are why `SHOPIFY_DROP` exists: Shopify's
|
||
`Type` is a free-text category, but canonical `Type` is *structural*
|
||
(`standalone`/kits), so it is warned rather than folded in; and Shopify's
|
||
`Variant Weight Unit` is dropped because the `Variant Grams` → `Variant Weight`
|
||
rename is the weight source.
|
||
- **The one value transform** lives in `parse_csv`: when a Shopify row has a mapped
|
||
`Variant Weight` (from `Variant Grams`), the codec synthesizes
|
||
`Variant Weight Unit = "g"`, since Shopify grams are unitless integers.
|
||
|
||
`detect_dialect()` in `codec.py` is the INV-17 seam; a new dialect is a new adapter
|
||
module plus a branch there, with no change to validate/diff/apply.
|
||
|
||
## Error granularity
|
||
|
||
`validate.py` never raises on a row problem: every violation is recorded as a
|
||
merchant-language `RowError` and **poisons its whole product block** — the
|
||
product previews as `kind="error"` and is excluded from apply, while parsing
|
||
continues so one pass yields a complete accounting (BUC-1a). On apply, error
|
||
rows are recorded per line in `import_run_error`; `rows_errored` on the run is
|
||
the **error-row count**, while the preview's errors tile counts error
|
||
**products** — the two numbers legitimately differ.
|
||
|
||
File-level problems (`not_csv`, `missing_required_column`, `too_many_rows`,
|
||
`file_too_large`) raise `FileRejected` in `codec.py` instead: no draft is
|
||
created. The BFF adds an early 413 for oversized uploads (`main.py`).
|
||
|
||
## INV-11 mechanics
|
||
|
||
`compute_diff` makes **one walk** that produces two views of the same
|
||
computation: the typed apply `plan` (resolved natives — `Decimal`, `bool`,
|
||
lists) that `confirm_draft` executes, and the JSON-safe preview `records`
|
||
stored as draft JSONB and served verbatim to the SPA. Because both derive from
|
||
the same walk they cannot diverge. The `fingerprint` is
|
||
`sha256(json.dumps(records, sort_keys=True, separators=(",", ":")))`; a
|
||
mismatch at confirm means the catalog drifted since preview → `PreviewStale`
|
||
(409 `preview_stale`, draft kept for re-validation). The whole apply — run row,
|
||
product/variant/image writes, error rows, draft delete — is one transaction;
|
||
any exception rolls it back and emits TEL-6.
|
||
|
||
## Export & the round-trip (SLICE-6)
|
||
|
||
`serialize.py` is the export half of "one codec, two directions": it turns the
|
||
`CatalogProduct` snapshot (the same one `repo.load_catalog` builds for the diff
|
||
engine) back into canonical CSV, writing exactly the columns `codec.py` /
|
||
`validate.py` parse, in the §6.5.1 row grammar — `HEADER` is the full canonical
|
||
column set; product fields + option names sit on the first row; one variant per
|
||
row; images interleave; a product with more images than variants emits
|
||
image-only rows.
|
||
|
||
`repo.export_catalog` returns the status-filtered snapshot list (sorted by
|
||
handle, deterministic); the `service.export_catalog` generator streams
|
||
`serialize.catalog_to_csv` over it and emits TEL-3 (`catalog_exported`) once the
|
||
stream is exhausted. The BFF wraps it in a `StreamingResponse`; an empty
|
||
(filtered) catalog raises `EmptyCatalog` **eagerly** → `409 empty_catalog`
|
||
before any bytes stream.
|
||
|
||
**INV-12** (`diff(catalog, import(export(catalog))) = ∅`) is locked two ways: a
|
||
property test (`test_products_serialize.py`) runs the real
|
||
export→parse→diff loop over 200 generated **text-field** catalogs and asserts
|
||
every product is `unchanged`; the `e2e_roundtrip_noop` browser scenario does the
|
||
same through the UI (export download → re-upload → all-unchanged preview, import
|
||
disabled). Numeric/`Decimal` fields — the property test's deliberate blind spot
|
||
(string-form vs value-identity) — get explicit round-trip unit tests.
|
||
|
||
## Image pipeline (SLICE-7)
|
||
|
||
### `platform/objectstore`
|
||
|
||
A two-adapter port (`backend/app/platform/objectstore/`):
|
||
|
||
- `local.py` — writes blobs under a configurable local directory; used in
|
||
tests and local dev (`ECOMM_OBJECTSTORE_KIND=local`).
|
||
- `gcs.py` — wraps `google-cloud-storage`; bucket name comes from
|
||
`ECOMM_OBJECTSTORE_BUCKET` (`ECOMM_OBJECTSTORE_KIND=gcs`). Auth is the
|
||
VM's service-account ADC — no credential bytes in the overlay or secrets.
|
||
|
||
Object keys for product images follow the prefix `product-images/` (the
|
||
`provision-bucket` gesture also sets a lifecycle rule on `import-drafts/` for
|
||
forward-compat, even though the draft blob remains BYTEA this slice — see
|
||
seams below).
|
||
|
||
### `platform/images`
|
||
|
||
`backend/app/platform/images.py` decodes an uploaded or fetched image byte
|
||
string and produces up to four renditions stored in the objectstore:
|
||
|
||
- **`original`** — stored verbatim after the resolution bar passes.
|
||
- **`thumb`** — 150 px on the shorter side, WebP.
|
||
- **`card`** — 400 px on the shorter side, WebP.
|
||
- **`detail`** — 800 px on the shorter side, WebP.
|
||
|
||
**Resolution bar (`MIN_IMAGE_SHORT_SIDE = 500`):** images whose shorter side
|
||
is below 500 px are rejected (Q-3). This is the only hard gate; oversized
|
||
images are scaled down without rejection. All renditions share the same
|
||
objectstore key prefix (`product-images/<image_id>/`).
|
||
|
||
### `domains/products/imagefetch.py` — the fetch phase
|
||
|
||
After `confirm_draft` commits, the run enters `fetching_images` (if it has
|
||
any pending image URLs) and a `ThreadPoolExecutor(max_workers=4)` processes
|
||
them concurrently:
|
||
|
||
- **SSRF guard (INV-18, extended):** only `http`/`https` schemes are
|
||
permitted; the resolved IP must be public (RFC-1918, loopback, link-local,
|
||
and multicast ranges are blocked); redirects are followed only within the
|
||
same guard — a redirect to a private IP is rejected mid-chain.
|
||
- **Bounds:** ≤ 20 MB response body, ≤ 30 s per fetch.
|
||
- **Per-image presence/idempotency check (`claim_image_for_fetch`):** before
|
||
processing, each `CatalogImage` row is checked so an already-handled row is
|
||
skipped. This is a presence guard adequate for the **single-worker,
|
||
in-process** model — the deployed app runs one uvicorn worker, recovery runs
|
||
at startup over prior-process runs, and a fresh confirm always targets a new
|
||
run, so two phase invocations never process the same run's images
|
||
concurrently. It is **not** a cross-process lock. A true multi-worker claim
|
||
would need a distinct in-flight status transition (`pending → fetching` with
|
||
`RETURNING`, or `SELECT … FOR UPDATE SKIP LOCKED`) — that is the future seam
|
||
if the fetch phase ever moves multi-process or onto a queue (§6.9).
|
||
- **Startup recovery scan:** on process start the app scans for runs stuck in
|
||
`fetching_images` and re-enqueues their pending images (emits TEL-5). This
|
||
covers crash/restart scenarios without a separate scheduler. On a mid-fetch
|
||
process restart (e.g. a deploy) the phase is interrupted and the run is left
|
||
resumable: pending images stay `pending`, fetched ones stay `fetched`, and
|
||
the startup scan completes the run. Any pool-closed error in an in-flight
|
||
worker at shutdown is benign — the uncommitted image stays `pending` and is
|
||
re-fetched on resume. Mid-flight interruption is tolerated by construction
|
||
(§6.9).
|
||
- **Progress tracking:** the run row carries `image_progress`,
|
||
`image_counts`, and `image_outcomes` (updated per image); these fields feed
|
||
the run-detail UI panel.
|
||
|
||
On completion the phase sets the run status to `complete` and emits TEL-4.
|
||
|
||
### Image-serving route
|
||
|
||
`GET /api/products/images/{id}/{rendition}` — storefront-authorized,
|
||
streams the objectstore blob. The response sets
|
||
`Cache-Control: public, immutable, max-age=31536000` (the object key encodes
|
||
the image id, so the URL is stable for the lifetime of the image). INV-16:
|
||
an image belongs to exactly one storefront; the route enforces storefront
|
||
scope before reading from the objectstore.
|
||
|
||
### Hosted-URL recognition and INV-12 over images
|
||
|
||
`domains/products/hosted.py` provides a predicate that recognises whether an
|
||
image URL is already served by this app (i.e., a `GET /api/products/images/…`
|
||
URL at the current `APP_URL`). The diff pre-pass `_resolve_hosted_images`
|
||
uses it to convert hosted URLs back to their `CatalogImage` FK before hashing
|
||
the diff fingerprint — this closes the INV-12 round-trip guarantee over
|
||
image columns (a re-imported export previews as all-unchanged even when
|
||
images are hosted here).
|
||
|
||
## Named seams (what later slices replace)
|
||
|
||
- **`import_draft.file_bytes` remains BYTEA (deferred past SLICE-7).** Moving
|
||
the draft blob to object storage was scoped out of SLICE-7. The media
|
||
bucket holds only `product-images/` objects this slice; the `provision-bucket`
|
||
script sets the `import-drafts/` lifecycle rule for forward-compat so the
|
||
migration will be clean when it lands.
|
||
- **`codec.detect_dialect` → Shopify (SLICE-8).** Today it always returns
|
||
`"canonical"`; SLICE-8 recognizes Shopify's exact header set here (INV-17).
|
||
- **Run-status complete shortcut → shipped in SLICE-7.**
|
||
`confirm_draft` now inserts the run as `applying`, then transitions to
|
||
`fetching_images` (if the run has pending image URLs) or directly to
|
||
`complete`. The fetch phase fills `image_progress` / `image_counts` /
|
||
`image_outcomes` and sets `complete` when done.
|
||
|
||
## Test map
|
||
|
||
| File | Covers |
|
||
| --- | --- |
|
||
| `backend/tests/test_products_codec.py` | file-level gates: parse, caps (INV-18), required columns, dialect |
|
||
| `backend/tests/test_products_validate.py` | every §6.5.1 row-error rule, one fixture each |
|
||
| `backend/tests/test_products_diff.py` | classification, blank-vs-absent, option matching, fingerprint |
|
||
| `backend/tests/test_products_serialize.py` | serializer grammar + INV-12 property test (round-trip no-op over generated catalogs) + decimal round-trip |
|
||
| `backend/tests/test_products_export.py` | status-filtered snapshot, streamed export, EmptyCatalog, TEL-3 |
|
||
| `backend/tests/test_products_service.py` | draft lifecycle: validate/preview/discard, expiry, TEL-1 |
|
||
| `backend/tests/test_products_invariants.py` | INV-10 (never deletes), INV-14 (storefront isolation), apply transactionality, TEL-6 |
|
||
| `backend/tests/test_products_endpoints.py` | §6.4 API scenarios + auth/storefront gates |
|
||
| `e2e/tests/import-preview-confirm.spec.ts` | happy path: upload → preview → confirm → history |
|
||
| `e2e/tests/import-errors.spec.ts` | actionable row errors at preview and on the run report |
|
||
| `e2e/tests/import-file-rejected.spec.ts` | file-level rejection, picker stays live, no trace |
|
||
| `e2e/tests/import-cancel.spec.ts` | cancel at preview leaves no trace |
|
||
| `e2e/tests/export-download.spec.ts` | export downloads canonical CSV, status filter respected |
|
||
| `e2e/tests/roundtrip-noop.spec.ts` | export → re-import → all-unchanged, import disabled (PUC-10) |
|