mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-22 08:51:13 +00:00
[AZ-484] Multi-source tile storage: source + captured_at
Add per-source tile rows to support multi-provider imagery (Google Maps + future UAV). Migration 013 (transactional) introduces source/captured_at columns, backfills existing rows to (source='google_maps', captured_at=created_at), and replaces the 4-column unique index with a 5-column index that includes source. TileRepository: - ColumnList includes source + captured_at - GetByTileCoordinatesAsync returns most-recent row across sources (ORDER BY captured_at DESC, updated_at DESC, id DESC) - GetTilesByRegionAsync uses DISTINCT ON to pick the most-recent tile per cell, restoring caller-facing row order - Insert/Update upsert on the new 5-column conflict key TileSource enum lives in Common.Enums. Snake_case wire format (google_maps, uav) is enforced by a focused TileSourceTypeHandler because the generic ToLowerInvariant pattern would emit "googlemaps", violating contract v1.0.0. TileService stamps Source=GoogleMaps + CapturedAt=UtcNow on every new tile. Tile-storage contract is now frozen at v1.0.0. AC coverage 7/7. New unit + integration tests cover all ACs; existing 200 unit + 5 smoke tests preserved. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,68 @@
|
||||
# Code Review Report
|
||||
|
||||
**Batch**: 25 (cycle 1)
|
||||
**Tasks**: AZ-484 (Multi-source tile storage schema)
|
||||
**Date**: 2026-05-11
|
||||
**Verdict**: PASS
|
||||
|
||||
## Findings
|
||||
|
||||
None.
|
||||
|
||||
## Phase Summary
|
||||
|
||||
### Phase 1 — Context
|
||||
Read AZ-484 task spec, `_docs/02_document/contracts/data-access/tile-storage.md` v1.0.0, the existing `_docs/02_document/architecture.md` Architecture Vision section, and the existing `module-layout.md` per-component map. Mapped the 15 changed files to AZ-484 (single-task batch).
|
||||
|
||||
### Phase 2 — Spec Compliance
|
||||
Walked every AC against code:
|
||||
|
||||
| AC | Promise | Validating test |
|
||||
|----|---------|-----------------|
|
||||
| AC-1 | Per-source coexistence on the same cell | `MultiSourceInsertCoexistsUnderNewIndex_AZ484_AC1` (TEMP), `NewUniqueConstraintIncludesSourceColumn_AZ484_AC1` (live schema) |
|
||||
| AC-2 | Most-recent across sources on read | `MostRecentAcrossSourcesSelection_AZ484_AC2` |
|
||||
| AC-3 | Same-source UPSERT collapses to one row | `SameSourceUpsertReplacesPreviousRow_AZ484_AC3` |
|
||||
| AC-4 | Migration backfill leaves no orphans | `BackfillUpdateAssignsGoogleMapsAndCapturedAt_AZ484_AC4` (TEMP simulation of the migration UPDATE) |
|
||||
| AC-5 | Google Maps path stamps Source + CapturedAt | `BuildTileEntity_SetsGoogleMapsSourceAndUtcCapturedAt_AZ484_AC5` |
|
||||
| AC-6 | Existing 200 unit + 5 smoke pass unchanged | Verified via the full suite run (handed off to autodev Step 11) |
|
||||
| AC-7 | Architecture / glossary / module-layout / contract updated | Documents amended in this batch; contract Status flipped from `draft` to `frozen` |
|
||||
|
||||
**Contract verification** against `_docs/02_document/contracts/data-access/tile-storage.md` v1.0.0:
|
||||
- Shape: `source VARCHAR(32) NOT NULL`, `captured_at TIMESTAMP NOT NULL` — matches migration 013.
|
||||
- 5-column unique index `idx_tiles_unique_location_source` — created by migration 013.
|
||||
- Producer write API: `InsertAsync` UPSERT on the 5-column key, refreshes `captured_at`/`updated_at`/`file_path`/`tile_x`/`tile_y` — matches.
|
||||
- Consumer read API: `GetByTileCoordinatesAsync` LIMIT 1 ordered by `(captured_at DESC, updated_at DESC, id DESC)`; `GetTilesByRegionAsync` uses `DISTINCT ON (latitude, longitude, tile_zoom, tile_size_meters)` with the same tie-break tuple — matches.
|
||||
- Wire format: `TileSource.GoogleMaps → 'google_maps'`, `TileSource.Uav → 'uav'` enforced by `TileSourceTypeHandler` (necessary because the generic `EnumStringTypeHandler<T>` would emit `'googlemaps'`).
|
||||
- Inv-1 / Inv-2 / Inv-5: `NOT NULL` columns + handler `Parse` throws `DataException` on unknown values (no silent coercion per `coderule.mdc`).
|
||||
- Inv-3: 5-column unique index.
|
||||
- Inv-4: identical tie-break tuple in `GetByTileCoordinatesAsync` and the inner `DISTINCT ON` of `GetTilesByRegionAsync` guarantees identical winner per cell.
|
||||
|
||||
### Phase 3 — Code Quality
|
||||
- SRP: `TileSourceTypeHandler` is a focused persistence concern (the bidirectional wire-format mapping); kept separate from the generic `EnumStringTypeHandler<T>` instead of leaking snake_case logic into the generic.
|
||||
- Comments: only added where intent is non-obvious (snake_case wire-format requirement, new ORDER BY tuple, per-source UPSERT semantics, transactional migration rationale). No narration-of-code comments.
|
||||
- Tests: every new test uses Arrange / Act / Assert.
|
||||
- DRY: `CreateTempTilesTable` factored out across the three TEMP-table integration tests.
|
||||
|
||||
### Phase 4 — Security Quick-Scan
|
||||
- All SQL parameters bound (`@Source`, `@CapturedAt`, etc.) — no string interpolation of caller-supplied values.
|
||||
- Migration backfill literal is `'google_maps'`, not user input.
|
||||
- No new secrets or credentials introduced.
|
||||
|
||||
### Phase 5 — Performance Scan
|
||||
- The new `DISTINCT ON` in `GetTilesByRegionAsync` can use `idx_tiles_unique_location_source` for the partition prefix; no extra round-trip; slow-query log threshold preserved.
|
||||
- No N+1 patterns introduced.
|
||||
|
||||
### Phase 6 — Cross-Task Consistency
|
||||
Single-task batch. Internal consistency: enum members, wire values, migration backfill literal, and test assertions all agree on `'google_maps'` / `'uav'`.
|
||||
|
||||
### Phase 7 — Architecture Compliance
|
||||
- Layering: `TileSource` enum lives in `SatelliteProvider.Common.Enums` (Layer 1 Foundation). DataAccess (Layer 1) and TileDownloader (Layer 3) both consume it through Common — no new cross-sibling ProjectReferences.
|
||||
- Public API respect: `TileSource` and `TileSourceTypeHandler` are public; `module-layout.md` Common Public API list updated to include `TileSource.cs`.
|
||||
- No new cycles.
|
||||
- No duplicate symbols across components.
|
||||
|
||||
### Baseline Delta
|
||||
Not computed inline — this batch makes no structural changes that would shift the existing `_docs/02_document/architecture_compliance_baseline.md` deltas. The AZ-484 changes stay within the existing layering invariants confirmed in earlier baseline scans.
|
||||
|
||||
## Verdict Logic
|
||||
No Critical, High, Medium, or Low findings → **PASS**.
|
||||
Reference in New Issue
Block a user