fix(tradein/cian): sanitize valuation cache writes (047 UNIQUE + 026/044 cols) #523

Merged

lekss361 merged 1 commit from feat/cian-history-sanitize into main

2026-05-24 13:44:39 +00:00

lekss361 commented

2026-05-24 13:39:18 +00:00

Owner

What

Sanitize Cian valuation persistence — 3 latent bugs identified in audit before activating history scrapers (next PRs):

047 migration — add UNIQUE(listing_id, change_time) to offer_price_history. Without it the existing ON CONFLICT DO NOTHING in cian_detail.save_detail_enrichment only suppresses PK conflicts and silently inserts dupes. Idempotent via DO $$ pg_constraint guard (matches 045 style).
_save_to_cache — fill price_rub / accuracy (mig 026 original columns, currently NULL for Cian rows) and low_price / high_price (mig 044 sale band, sourced from sale_price_from/to).
_save_to_cache — accept and persist house_id / listing_id FK (mig 044). On cache refresh COALESCE(EXCLUDED, existing) preserves previously-set canonical link. Plumbed through estimate_via_cian_valuation with default None — current estimator.py callsite unchanged.

Why now

Blocking-precursor for PR 2 (cian-detail + cian-newbuilding admin endpoints) and PR 4 (backfill task). Both will write to offer_price_history and need the UNIQUE for ON CONFLICT to work; PR 4 will pass house_id/listing_id from canonical matching.

Files

tradein-mvp/backend/data/sql/047_cian_history_sanitize.sql — NEW
tradein-mvp/backend/app/services/scrapers/cian_valuation.py — _save_to_cache / _load_from_cache / estimate_via_cian_valuation

Test plan

ruff clean on touched files (verified)
column count INSERT=27 / VALUES=27 (verified by pre-push review)
migration idempotent — re-run no-op (DO block guards on pg_constraint)
_load_from_cache round-trip: sale_price_from/to populated from low_price/high_price
post-merge: smoke /estimate request, verify new row in external_valuations has price_rub, accuracy, low_price, high_price non-NULL

Follow-up PRs:

PR 2: /admin/scrape/cian-detail + /admin/scrape/cian-newbuilding endpoints (unblock 404 sections on /scrapers/cian)
PR 3: surface chart_data + rent + change on /trade-in
PR 4: Celery backfill task

## What Sanitize Cian valuation persistence — 3 latent bugs identified in audit before activating history scrapers (next PRs): 1. **047 migration** — add `UNIQUE(listing_id, change_time)` to `offer_price_history`. Without it the existing `ON CONFLICT DO NOTHING` in `cian_detail.save_detail_enrichment` only suppresses PK conflicts and silently inserts dupes. Idempotent via `DO $$ pg_constraint` guard (matches 045 style). 2. **`_save_to_cache`** — fill `price_rub` / `accuracy` (mig 026 original columns, currently NULL for Cian rows) and `low_price` / `high_price` (mig 044 sale band, sourced from `sale_price_from/to`). 3. **`_save_to_cache`** — accept and persist `house_id` / `listing_id` FK (mig 044). On cache refresh `COALESCE(EXCLUDED, existing)` preserves previously-set canonical link. Plumbed through `estimate_via_cian_valuation` with default None — current `estimator.py` callsite unchanged. ## Why now Blocking-precursor for PR 2 (cian-detail + cian-newbuilding admin endpoints) and PR 4 (backfill task). Both will write to `offer_price_history` and need the UNIQUE for ON CONFLICT to work; PR 4 will pass `house_id`/`listing_id` from canonical matching. ## Files - `tradein-mvp/backend/data/sql/047_cian_history_sanitize.sql` — NEW - `tradein-mvp/backend/app/services/scrapers/cian_valuation.py` — `_save_to_cache` / `_load_from_cache` / `estimate_via_cian_valuation` ## Test plan - ruff clean on touched files (verified) - column count INSERT=27 / VALUES=27 (verified by pre-push review) - migration idempotent — re-run no-op (DO block guards on `pg_constraint`) - `_load_from_cache` round-trip: `sale_price_from/to` populated from `low_price/high_price` - post-merge: smoke `/estimate` request, verify new row in `external_valuations` has `price_rub`, `accuracy`, `low_price`, `high_price` non-NULL Follow-up PRs: - PR 2: `/admin/scrape/cian-detail` + `/admin/scrape/cian-newbuilding` endpoints (unblock 404 sections on /scrapers/cian) - PR 3: surface chart_data + rent + change on /trade-in - PR 4: Celery backfill task

lekss361 added 1 commit 2026-05-24 13:39:19 +00:00

fix(tradein/cian): sanitize valuation cache writes (047 UNIQUE, fill 026+044 cols) d04ee38347

- 047_cian_history_sanitize.sql: dedup existing rows then add
  UNIQUE(listing_id, change_time) so ON CONFLICT DO NOTHING in
  cian_detail.save_detail_enrichment suppresses real duplicates,
  not just PK collisions. Idempotent via DO $$ pg_constraint guard.
- cian_valuation._save_to_cache: fill price_rub/accuracy (mig 026),
  low_price/high_price (mig 044), house_id/listing_id (mig 044).
  COALESCE on house_id/listing_id preserves existing FK on cache refresh.
- cian_valuation._load_from_cache: restore low_price/high_price from DB
  into sale_price_from/sale_price_to so callers get the full band.
- estimate_via_cian_valuation: add house_id/listing_id kwargs (default None),
  pass through to _save_to_cache. No callsite changes needed (defaults preserve
  existing behavior; PR 4 can supply IDs when context is available).

lekss361 commented

2026-05-24 13:44:31 +00:00

Author

Owner

Deep Code Review — verdict

Summary

Status: APPROVE
Files: 2 (P0: 1 migration, P1: 1 scraper)
Lines: +56 / -1 · SHA verified: d04ee38

Verified

Column count 27=27 (INSERT vs VALUES) — recounted manually:

INSERT cols: source/cache_key/address + total_area/rooms_count/floor/total_floors + repair_type/deal_type + price_rub/accuracy + low_price/high_price + house_id/listing_id + sale_price_rub/sale_accuracy + rent_price_rub/rent_accuracy + chart/chart_change_pct/chart_change_direction + external_house_id/filters_hash + raw_payload/fetched_at/expires_at = 27
VALUES expressions: 'cian_valuation' + :ck + :addr + CAST(:ta) + :rc/:fl/:tf + :rt/:dt + CAST(:sp)/CAST(:sa) + CAST(:lp)/CAST(:hp) + CAST(:hid)/CAST(:lid) + CAST(:sp)/CAST(:sa) + CAST(:rp)/CAST(:ra) + CAST(:ch)/CAST(:ccp)/:ccd + :ehi/:fh + CAST(:raw)/NOW()/NOW()+INTERVAL = 27
:sp/:sa correctly bound twice (legacy mig 026 price_rub/accuracy + new mig 029 sale_price_rub/sale_accuracy) — intentional per PR description.

Migration 047 idempotency — DELETE-then-ALTER wrapped in single BEGIN/COMMIT. DO guards on pg_constraint (matches 045 style). DELETE ... WHERE a.id > b.id keeps lowest-id of each dup group. Re-run is a no-op. Auto-applied via .forgejo/workflows/deploy-tradein.yml (for sql_file in $(ls -1 backend/data/sql/*.sql | sort)) — safe with psql ON_ERROR_STOP.

ON CONFLICT semantics — Existing INSERT INTO offer_price_history ... ON CONFLICT DO NOTHING in cian_detail.save_detail_enrichment:255 is bare (no target). Before this PR, no UNIQUE existed besides PK (bigserial → never conflicts), so the clause was dead. After 047 the new offer_price_history_listing_change_uq is suppressed by the bare DO NOTHING. Confirmed correct.

Asymmetric UPDATE — house_id/listing_id use COALESCE(EXCLUDED, existing) (canonical link is sticky); other cols use bare EXCLUDED.x (refresh). Matches description.

psycopg v3 — All numeric/jsonb/bigint bound via CAST(:x AS type), no ::type shorthand. No SQL injection surface (cache_key is sha256; address from authenticated estimator).

Findings

LOW — test break (not CI-gated): tradein-mvp/backend/tests/test_cian_valuation.py:336-347 builds cached_row plain dict without low_price/high_price keys. After this PR _load_from_cache calls _parse_num(row["low_price"]) which will raise KeyError on plain dict (real RowMapping would also raise — but real DB will have the columns). No CI gate on pytest in tradein workflows, so won't block deploy. Easy follow-up: add "low_price": None, "high_price": None to mock dict.

Note (not blocker) — latent risk for PR4: Partial unique index external_valuations_canonical_uniq_idx (mig 044) on (source, house_id, listing_id) WHERE both NOT NULL. Current ON CONFLICT (source, cache_key) doesn't target it. When PR4 starts filling house_id+listing_id from canonical matching, two requests for the same listing with different (area, rooms, floor) → different cache_keys → same (source, house_id, listing_id) → INSERT raises unhandled IntegrityError. Address in PR4 (e.g. dedupe by canonical key first, or use a different ON CONFLICT target). Current PR is unaffected because callsite in estimator.py:566 still passes house_id=None/listing_id=None.

Positive

Backward-compat: new kwargs default None, no breaking signature change.
Migration deps documented (-- Dependencies: 023...).
Round-trip symmetry: _load_from_cache SELECT adds low_price/high_price; _save_to_cache populates them.
Clean follow-up plan documented in PR body.

Verdict

APPROVE — no production risk, 1 cosmetic test-only break to address in follow-up, 1 documented latent risk for PR4 to handle. Merging.

## Deep Code Review — verdict ### Summary - Status: APPROVE - Files: 2 (P0: 1 migration, P1: 1 scraper) - Lines: +56 / -1 · SHA verified: d04ee38 ### Verified **Column count 27=27** (INSERT vs VALUES) — recounted manually: - INSERT cols: source/cache_key/address + total_area/rooms_count/floor/total_floors + repair_type/deal_type + price_rub/accuracy + low_price/high_price + house_id/listing_id + sale_price_rub/sale_accuracy + rent_price_rub/rent_accuracy + chart/chart_change_pct/chart_change_direction + external_house_id/filters_hash + raw_payload/fetched_at/expires_at = **27** - VALUES expressions: 'cian_valuation' + :ck + :addr + CAST(:ta) + :rc/:fl/:tf + :rt/:dt + CAST(:sp)/CAST(:sa) + CAST(:lp)/CAST(:hp) + CAST(:hid)/CAST(:lid) + CAST(:sp)/CAST(:sa) + CAST(:rp)/CAST(:ra) + CAST(:ch)/CAST(:ccp)/:ccd + :ehi/:fh + CAST(:raw)/NOW()/NOW()+INTERVAL = **27** - `:sp`/`:sa` correctly bound twice (legacy mig 026 `price_rub`/`accuracy` + new mig 029 `sale_price_rub`/`sale_accuracy`) — intentional per PR description. **Migration 047 idempotency** — DELETE-then-ALTER wrapped in single BEGIN/COMMIT. DO $$ guards on `pg_constraint` (matches 045 style). `DELETE ... WHERE a.id > b.id` keeps lowest-id of each dup group. Re-run is a no-op. Auto-applied via `.forgejo/workflows/deploy-tradein.yml` (`for sql_file in $(ls -1 backend/data/sql/*.sql | sort)`) — safe with `psql ON_ERROR_STOP`. **ON CONFLICT semantics** — Existing `INSERT INTO offer_price_history ... ON CONFLICT DO NOTHING` in `cian_detail.save_detail_enrichment:255` is bare (no target). Before this PR, no UNIQUE existed besides PK (bigserial → never conflicts), so the clause was dead. After 047 the new `offer_price_history_listing_change_uq` is suppressed by the bare DO NOTHING. Confirmed correct. **Asymmetric UPDATE** — `house_id`/`listing_id` use `COALESCE(EXCLUDED, existing)` (canonical link is sticky); other cols use bare `EXCLUDED.x` (refresh). Matches description. **psycopg v3** — All numeric/jsonb/bigint bound via `CAST(:x AS type)`, no `::type` shorthand. No SQL injection surface (cache_key is sha256; address from authenticated estimator). ### Findings **LOW — test break (not CI-gated):** `tradein-mvp/backend/tests/test_cian_valuation.py:336-347` builds `cached_row` plain dict without `low_price`/`high_price` keys. After this PR `_load_from_cache` calls `_parse_num(row["low_price"])` which will raise `KeyError` on plain dict (real `RowMapping` would also raise — but real DB will have the columns). No CI gate on pytest in tradein workflows, so won't block deploy. Easy follow-up: add `"low_price": None, "high_price": None` to mock dict. **Note (not blocker) — latent risk for PR4:** Partial unique index `external_valuations_canonical_uniq_idx` (mig 044) on `(source, house_id, listing_id) WHERE both NOT NULL`. Current `ON CONFLICT (source, cache_key)` doesn't target it. When PR4 starts filling `house_id`+`listing_id` from canonical matching, two requests for the same listing with different `(area, rooms, floor)` → different cache_keys → same `(source, house_id, listing_id)` → INSERT raises unhandled IntegrityError. Address in PR4 (e.g. dedupe by canonical key first, or use a different ON CONFLICT target). Current PR is unaffected because callsite in `estimator.py:566` still passes `house_id=None`/`listing_id=None`. ### Positive - Backward-compat: new kwargs default `None`, no breaking signature change. - Migration deps documented (`-- Dependencies: 023...`). - Round-trip symmetry: `_load_from_cache` SELECT adds `low_price/high_price`; `_save_to_cache` populates them. - Clean follow-up plan documented in PR body. ### Verdict **APPROVE** — no production risk, 1 cosmetic test-only break to address in follow-up, 1 documented latent risk for PR4 to handle. Merging.