mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-21 17:21:13 +00:00
[AZ-357] Refactor C06: drop tile Version concept; cumulative review batches 7-9
AZ-357 — eliminate year-based tile cache expiry (LF-1): - Migration 012: drop 5-col unique index, dedupe by (lat,lon,zoom, size) keeping max(updated_at), add new 4-col unique index, make version column nullable + drop default. Column itself preserved per coderule (column drops require explicit confirmation; tracked in AZ-373 / C20). - TileEntity.Version, TileMetadata.Version, DownloadTileResponse. Version: int -> int? (HTTP shape preserved; field still in JSON). - TileService.DownloadAndStoreTilesAsync: drop currentVersion year computation and the .Where(t => t.Version == currentVersion) cache filter. BuildTileEntity: drop year arg; write Version=null. - TileRepository: ON CONFLICT now 4-col; lookup queries ORDER BY updated_at DESC instead of version DESC. - Tests: replace inverted BT02b with positive AZ357_AC1 (prior-year cached tile is reused). Add BuildTileEntity_ DoesNotPopulateVersion_AZ357 to enforce the no-write contract. - 69 unit + 5 smoke + 3 stub-contract integration tests pass. Cumulative code review (batches 7-9, 7 tasks): VERDICT=PASS. Report at _docs/03_implementation/reviews/batch_09_review.md. Zero Critical/High/Medium/Low findings. Architecture baseline remains clean. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -0,0 +1,84 @@
|
||||
# Batch 10 Report — Refactor 03 Phase 2 (continued)
|
||||
|
||||
Date: 2026-05-10
|
||||
Epic: AZ-350 (03-code-quality-refactoring)
|
||||
Status: ✅ Complete, pushed (after batch 11 commit, riding with 09 cumulative review)
|
||||
|
||||
## Scope (1 task / 5 SP)
|
||||
|
||||
| ID | C-ID | Title | Points | Component |
|
||||
|----|------|-------|--------|-----------|
|
||||
| AZ-357 | C06 | Drop tile `Version` concept; latest row wins; new migration | 5 | Services.TileDownloader + DataAccess |
|
||||
|
||||
Single-task batch — DB migration is higher risk and benefits from dedicated review focus.
|
||||
|
||||
## Changes
|
||||
|
||||
### Migration
|
||||
- **NEW** `SatelliteProvider.DataAccess/Migrations/012_DropTileVersionConstraint.sql`
|
||||
- Drops `idx_tiles_unique_location` (5-column).
|
||||
- Dedupes by 4-column key using `ROW_NUMBER() OVER (PARTITION BY ... ORDER BY updated_at DESC, id DESC)` — keeps latest row per cell, deterministic tie-break by id.
|
||||
- Recreates `idx_tiles_unique_location` on `(latitude, longitude, tile_zoom, tile_size_meters)`.
|
||||
- `ALTER COLUMN version DROP NOT NULL` and `DROP DEFAULT` so new rows can store NULL.
|
||||
- Column itself preserved (per coderule.mdc — no column drops without confirmation; covered by AZ-373 / C20 separately).
|
||||
|
||||
### Production
|
||||
- **MODIFIED** `SatelliteProvider.DataAccess/Models/TileEntity.cs`
|
||||
- `Version` changed from `int` → `int?` (matches the now-nullable column).
|
||||
- **MODIFIED** `SatelliteProvider.Common/DTO/TileMetadata.cs`
|
||||
- `Version` changed to `int?` to surface the nullable column to consumers (HTTP shape preserved per the task constraint — the field is still present in JSON).
|
||||
- **MODIFIED** `SatelliteProvider.Api/Program.cs` (`DownloadTileResponse`)
|
||||
- `Version` changed to `int?` for the same reason.
|
||||
- **MODIFIED** `SatelliteProvider.DataAccess/Repositories/TileRepository.cs`
|
||||
- `InsertAsync.ON CONFLICT` clause: 5-col → 4-col (drops `version`).
|
||||
- `GetByTileCoordinatesAsync`: `ORDER BY version DESC` → `ORDER BY updated_at DESC` (latest row wins per AC-1).
|
||||
- `GetTilesByRegionAsync`: `ORDER BY version DESC, latitude DESC, longitude ASC` → `ORDER BY latitude DESC, longitude ASC, updated_at DESC` (after migration there's at most 1 row per cell so version-ordering is meaningless; updated_at is the meaningful tie-break).
|
||||
- `FindExistingTileAsync` left untouched — slated for deletion in AZ-376 / C23.
|
||||
- **MODIFIED** `SatelliteProvider.Services.TileDownloader/TileService.cs`
|
||||
- Removed `var currentVersion = DateTime.UtcNow.Year;` and the `.Where(t => t.Version == currentVersion)` cache filter (root cause of LF-1: cache expiring on Jan 1).
|
||||
- `BuildTileEntity` signature: dropped the `currentVersion` parameter; now writes `Version = null`. New code never writes the deprecated year value.
|
||||
- All 3 call sites updated to drop the year argument.
|
||||
|
||||
### Tests
|
||||
- **MODIFIED** `SatelliteProvider.Tests/TileServiceTests.cs`
|
||||
- Replaced `DownloadAndStoreTilesAsync_IgnoresStaleVersionCachedTiles_BT02b` with `DownloadAndStoreTilesAsync_TreatsCachedTileFromPriorYearAsFresh_AZ357_AC1` — same setup with a `Version = Year - 1` row, but inverted assertion: the cached tile IS reused (not re-downloaded). Directly proves AC-1 (cache survives year boundary).
|
||||
- Added `BuildTileEntity_DoesNotPopulateVersion_AZ357` — captures the entity passed to `InsertAsync` and asserts `Version == null`. Enforces the "new code does not write to it" constraint.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Unit tests**: 69 / 69 passing (was 68 → +2 new AZ-357 tests, −1 inverted/replaced test = net +1).
|
||||
- **Integration smoke + full suite**: green. Container exits 0. The 20-point extended-route test ran 690 tiles end-to-end with the new schema applied to a fresh Postgres volume — exercises:
|
||||
- Insert path: writes `Version = null`, conflicts on the new 4-col key.
|
||||
- Read path: `GetTilesByRegionAsync` returns tiles ordered by `updated_at DESC`.
|
||||
- `GetOrDownloadTileAsync` cache-hit path: tile lookup uses `ORDER BY updated_at DESC`.
|
||||
|
||||
## Acceptance criteria coverage
|
||||
|
||||
| AC | Evidence |
|
||||
|----|----------|
|
||||
| **AC-1** Cache survives year boundary | Unit test `TreatsCachedTileFromPriorYearAsFresh_AZ357_AC1`: prior-year `Version` row reused; `InsertAsync` not called. |
|
||||
| **AC-2** Migration runs cleanly on populated tile data | (Partial) Migration applied successfully against an integration test DB during container startup. Dedupe SQL is correct by construction (`ROW_NUMBER OVER PARTITION BY ... ORDER BY updated_at DESC, id DESC`). **Not explicitly tested with pre-staged duplicates** — see "Known coverage gap" below. Consistent with how migration 004 (which used the same pattern) was originally verified. |
|
||||
| **AC-3** Upsert behaves on the new key | New `InsertAsync.ON CONFLICT (latitude, longitude, tile_zoom, tile_size_meters)` clause; integration suite re-runs identical (lat,lon,zoom,size) inserts during the route test (690 tiles processed without unique-violation errors). |
|
||||
| **AC-4** 37 unit + 5 smoke tests stay green | 69 unit + 5 smoke + 3 stub-contract green. |
|
||||
|
||||
### Known coverage gap (AC-2, partial)
|
||||
|
||||
The migration's dedupe DELETE has not been exercised against a pre-populated table containing rows that violate the new 4-column constraint. Reasons not addressed in this batch:
|
||||
|
||||
- The integration test stack starts with a fresh DB volume, so the migration runs against an empty table.
|
||||
- Inserting test duplicates *after* migration startup is impossible (the new constraint blocks it).
|
||||
- Adding a pre-init SQL injection (docker-compose `command:` or an init script in the postgres image) is out of scope for a 5 SP refactor and would touch CI tooling.
|
||||
|
||||
**Mitigation**: the SQL pattern (`ROW_NUMBER OVER PARTITION BY ... ORDER BY updated_at DESC, id DESC`) is well-understood and matches the established project precedent (migration 004 used a similar `DELETE...USING` pattern with no test). Production rollout should follow the spec's risk mitigation: capture pre-migration row counts, dry-run against a populated copy.
|
||||
|
||||
This gap is recorded in `_docs/_process_leftovers/` if user wants follow-up tracking; otherwise treat as accepted risk consistent with prior migrations.
|
||||
|
||||
## Behavior preservation
|
||||
|
||||
- **HTTP shape**: `DownloadTileResponse` still has `version` field. JSON output is `"version": null` for new tiles, `"version": 2025` (or other year) for tiles inserted before this migration. Consumers parsing as `int?` (most JSON libraries default to nullable) are unaffected; consumers parsing as `int` would need to handle null. None observed in the suite.
|
||||
- **Cache semantics**: stricter (cache survives year flip) — the *intended* behavior. The replaced test asserted the bug; the new test asserts the fix.
|
||||
|
||||
## Up next
|
||||
|
||||
- **Batch 11**: AZ-362 (idempotent POST contract for caller-supplied GUIDs, 3 SP) — Api + RegionProcessing + RouteManagement. Depends on AZ-353 (done in batch 8). This will be the next-and-final batch this session unless paused.
|
||||
- After batch 11, K=3 cumulative review trigger fires again (batches 10, 11, 12) — but only 2 batches new, so falls below threshold. Continue per user direction.
|
||||
@@ -0,0 +1,125 @@
|
||||
# Code Review Report — Cumulative (Batches 7+8+9)
|
||||
|
||||
**Batch**: 9 (cumulative; covers batches 7, 8, 9)
|
||||
**Tasks reviewed**: AZ-351, AZ-352, AZ-353, AZ-354, AZ-356, AZ-359, AZ-363
|
||||
**Date**: 2026-05-10
|
||||
**Mode**: Cumulative (Phases 1–7 on union of changed files since last review)
|
||||
**Verdict**: **PASS**
|
||||
**Critical**: 0 | **High**: 0 | **Medium**: 0 | **Low**: 0
|
||||
|
||||
## Files in scope
|
||||
|
||||
| File | Tasks | Change |
|
||||
|------|-------|--------|
|
||||
| `SatelliteProvider.Api/Program.cs` | AZ-351, AZ-353, AZ-354, AZ-356 | Modified (DI rewire, CORS validator, exception handler middleware, 501 stubs, removed per-endpoint try/catch) |
|
||||
| `SatelliteProvider.Api/GlobalExceptionHandler.cs` | AZ-353 | New |
|
||||
| `SatelliteProvider.Api/CorsConfigurationValidator.cs` | AZ-354 | New |
|
||||
| `SatelliteProvider.Services.RegionProcessing/RegionService.cs` | AZ-359 | Modified (catch ladder collapsed) |
|
||||
| `SatelliteProvider.Services.RegionProcessing/RegionFailureClassifier.cs` | AZ-359 | New |
|
||||
| `SatelliteProvider.Services.RegionProcessing/RegionRequestQueue.cs` | AZ-363 | Modified (counters removed) |
|
||||
| `SatelliteProvider.Services.RegionProcessing/SatelliteProvider.Services.RegionProcessing.csproj` | AZ-359 | Modified (`InternalsVisibleTo`) |
|
||||
| `SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs` | AZ-352 | Modified (empty catch removed; static→instance method) |
|
||||
| `SatelliteProvider.Services.RouteManagement/SatelliteProvider.Services.RouteManagement.csproj` | AZ-352 | Modified (`InternalsVisibleTo`) |
|
||||
| `SatelliteProvider.Tests/CorsConfigurationValidatorTests.cs` | AZ-354 | New (9 tests) |
|
||||
| `SatelliteProvider.Tests/GlobalExceptionHandlerTests.cs` | AZ-353 | New (3 tests) |
|
||||
| `SatelliteProvider.Tests/RegionFailureClassifierTests.cs` | AZ-359 | New (10 tests) |
|
||||
| `SatelliteProvider.Tests/RouteProcessingServiceTests.cs` | AZ-352 | New (4 tests) |
|
||||
| `SatelliteProvider.Tests/SatelliteProvider.Tests.csproj` | AZ-353 | Modified (added `SatelliteProvider.Api` ProjectReference for unit testing API-layer types) |
|
||||
| `SatelliteProvider.IntegrationTests/StubAndErrorContractTests.cs` | AZ-356, AZ-353 | New (3 integration tests) |
|
||||
| `SatelliteProvider.IntegrationTests/Program.cs` | AZ-356 | Modified (registers new test class in smoke + full suites) |
|
||||
|
||||
## Findings
|
||||
|
||||
**No findings of any severity.**
|
||||
|
||||
## Phase Summaries
|
||||
|
||||
### Phase 1 — Context loading
|
||||
Read all 7 task specs from `_docs/02_tasks/done/`, `_docs/02_document/architecture_compliance_baseline.md`, `_docs/02_document/module-layout.md`, plus changed source files.
|
||||
|
||||
### Phase 2 — Spec compliance
|
||||
|
||||
| Task | AC | Evidence |
|
||||
|------|----|----------|
|
||||
| AZ-351 (C01) | Logger resolved via DI as `ILogger<DatabaseMigrator>` | `Program.cs:96` — `app.Services.GetRequiredService<ILogger<DatabaseMigrator>>()` |
|
||||
| AZ-352 (C02) AC-1 | Empty catch removed; warning logged with structured fields | `RouteProcessingService.cs:610-628` — `_logger.LogWarning("...{FilePath}...", filePath)` |
|
||||
| AZ-352 (C02) AC-2 | Null-guard on `filePath` | `RouteProcessingService.cs:612` — `ArgumentNullException.ThrowIfNull(filePath)` |
|
||||
| AZ-352 unit tests | 4 tests cover valid name, malformed, non-numeric coords, null path | `RouteProcessingServiceTests` — 4 passing |
|
||||
| AZ-353 (C03) AC-1 | 5xx → sanitized `application/problem+json` with `correlationId` | `GlobalExceptionHandler.cs:30-53` + unit test `TryHandleAsync_WritesSanitizedProblemDetailsAndReturnsTrue_AC1` |
|
||||
| AZ-353 AC-2 | Full exception logged server-side with correlation ID | `GlobalExceptionHandler.cs:30-35` + unit test `..._LogsFullExceptionWithCorrelationId_AC2` |
|
||||
| AZ-353 AC-3 | 4xx framework errors NOT promoted to 5xx | `GlobalExceptionHandler.cs:22-26` (`BadHttpRequestException` branch) + unit test `..._BadHttpRequestException_HonorsFrameworkStatusAndDoesNotErrorLog_AC3` + integration `CreateRoute_InvalidPayload_Returns400_AZ353_AC3` |
|
||||
| AZ-354 (C04) AC-1 | Production with empty origins + no opt-in → throw | `CorsConfigurationValidator.cs:14-28` + unit test `..._ProductionWithEmptyOriginsAndNoOptIn_Throws_AC1` |
|
||||
| AZ-354 AC-2 | Non-Production with empty origins → no throw, warning emitted | `Program.cs:89-94` + unit tests for Dev/Staging/Local |
|
||||
| AZ-354 AC-3 | Explicit `AllowAnyOrigin=true` opt-in → no throw in Prod | Unit test `..._ProductionWithExplicitAllowAnyOrigin_DoesNotThrow_AC3` |
|
||||
| AZ-356 (C05) AC-1 | Stub endpoints return 501 ProblemDetails | `Program.cs:178-192` + integration tests `StubMgrs_Returns501`, `StubUpload_Returns501` |
|
||||
| AZ-356 OpenAPI | `.ProducesProblem(501)` declared on stub endpoints | `Program.cs:124, 129` |
|
||||
| AZ-359 (C07) AC-1 | Same observable failure-path messages preserved | `RegionFailureClassifier.cs:30-69` — string assertions in 7 unit tests |
|
||||
| AZ-359 AC-2 | RegionTests still green (timeout + rate-limit covered) | `RegionServiceTests.ProcessRegionAsync_DownloaderFailure_*` passing |
|
||||
| AZ-359 AC-3 | 37+ unit + 5 smoke green | 68 unit + 5 smoke + 3 stub-contract integration — all pass |
|
||||
| AZ-363 (C10) | Write-only counters removed | `RegionRequestQueue.cs` — no `_totalEnqueued`/`_totalDequeued` fields remain |
|
||||
|
||||
All ACs traced and demonstrably satisfied. No Spec-Gap findings.
|
||||
|
||||
### Phase 3 — Code quality
|
||||
|
||||
- **SOLID**: All three new types have a single, sharply-named responsibility. `GlobalExceptionHandler` handles 5xx sanitization; `CorsConfigurationValidator` validates CORS config; `RegionFailureClassifier` maps exceptions to a typed failure category. None depend on more than the minimum needed.
|
||||
- **Error handling**: AZ-352 fix is correct — the previously empty `catch {}` is gone; failures now log a warning with structured fields and return a sentinel `(-1, -1)`. AZ-359 fix collapses 9 catches into 1; same exception types still caught (no broadening or narrowing). AZ-353 explicitly preserves the framework's 4xx status — addresses a subtle prior-art trap where naive `IExceptionHandler` would have promoted all errors to 500.
|
||||
- **Naming**: `RegionFailureCategory`, `RegionFailureClassification`, `EnsureSafeForEnvironment`, `ShouldUsePermissivePolicy`, `ShouldWarnAboutPermissiveDefault` all read clearly from caller perspective.
|
||||
- **Complexity**: `RegionService.ProcessRegionAsync` shrinks ~50 LOC of duplicated catch handling to ~10 LOC. No new method exceeds 50 lines; no new cyclomatic-complexity hotspot.
|
||||
- **DRY**: Three meaningful DRY wins — (a) catch ladder collapse, (b) CORS rule centralized as testable constants/methods, (c) per-endpoint try/catch in `Program.cs` removed in favor of the global handler.
|
||||
- **Test quality**: Tests assert on specific status codes, message substrings, log capture (via mock `ILogger`), and content type. Not "no error thrown" tests.
|
||||
- **Dead code**: AZ-363 removed `_totalEnqueued`/`_totalDequeued`. `RegionRequestQueue` is now lean.
|
||||
|
||||
### Phase 4 — Security
|
||||
|
||||
- **Sanitized 5xx**: `Detail = "An unexpected error occurred. Use the correlationId to look up the server log entry."` — does not leak exception type, message, or stack trace to client. ✓
|
||||
- **Correlation ID**: `httpContext.TraceIdentifier` — connection-scoped string, not a sensitive value. Adequate for log correlation.
|
||||
- **BadHttpRequestException Detail echoes framework message** (e.g., `"Failed to bind parameter "double Latitude" from "' OR 1=1 --"."`). The echoed text is the user's own input, which they already possess; no server-side info is leaked. This is also standard ASP.NET behavior. Not a finding.
|
||||
- **CORS**: Hard-fail in Production for empty `AllowedOrigins` + missing opt-in. Closes the most common CORS misconfiguration in prod deployments.
|
||||
- **501 stubs**: ProblemDetails contain only the literal "not implemented" message; no internal info.
|
||||
- **No SQL injection / command injection / hardcoded secrets** introduced.
|
||||
|
||||
### Phase 5 — Performance
|
||||
|
||||
- All new code is pure, allocation-light computation in non-hot paths (validator runs once at startup; classifier runs once per failure; handler runs once per unhandled exception).
|
||||
- No new N+1 queries, no blocking I/O in async contexts, no O(n²) algorithms.
|
||||
|
||||
### Phase 6 — Cross-task consistency
|
||||
|
||||
- AZ-353, AZ-354, AZ-356 all touch `Program.cs`; their interactions are coherent: validator runs first (may throw), then DI registers handler + ProblemDetails, then `app.UseExceptionHandler()` activates the pipeline. No conflicting wiring.
|
||||
- All new ProblemDetails responses use `"application/problem+json"` content type consistently.
|
||||
- Logging style consistent across changes: structured fields with `{PascalCase}` placeholders, `_logger.LogError`/`LogWarning` with explicit exception when relevant.
|
||||
- Test conventions consistent: `// Arrange` / `// Act` / `// Assert` per project rule; FluentAssertions throughout.
|
||||
|
||||
### Phase 7 — Architecture
|
||||
|
||||
| Check | Result |
|
||||
|-------|--------|
|
||||
| Layer direction | All cross-component imports go Layer-3 → Layer-1 (Common/DataAccess) or Layer-4 → Layer-3/1. No upward imports. |
|
||||
| Public API respect | New types under each component live in the component's root namespace and are exposed as appropriate (`public sealed`, `public static`). No internal-of-other-component imports. |
|
||||
| New cyclic dependencies | None. The split (per `module-layout.md`) prevents cross-sibling project references; the new files all sit within their owning component. |
|
||||
| Duplicate symbols across components | None. `RegionFailureClassifier` is unique to RegionProcessing; `GlobalExceptionHandler` and `CorsConfigurationValidator` are unique to Api. |
|
||||
| Cross-cutting concerns mis-placed | None. CORS validation belongs in Api (depends on env name); 5xx handler belongs in Api (depends on ASP.NET); region failure classification belongs in RegionProcessing (region-specific business semantics). |
|
||||
|
||||
## Baseline delta
|
||||
|
||||
`architecture_compliance_baseline.md` lists 5 findings, all Resolved as of epic AZ-309. This cumulative review introduces 0 new architecture findings.
|
||||
|
||||
| Status | Count |
|
||||
|--------|-------|
|
||||
| Carried over | 0 |
|
||||
| Resolved | 5 (already by AZ-309) |
|
||||
| Newly introduced | 0 |
|
||||
|
||||
## Test results referenced
|
||||
|
||||
- **Unit**: 68 / 68 passing (was 35 pre-Phase 1; +10 RegionFailureClassifier, +9 CorsConfigurationValidator, +3 GlobalExceptionHandler, +4 RouteProcessingService, +7 carried from prior baseline migrations across batches 7+8 — TileService internals + others).
|
||||
- **Integration smoke**: 5 / 5 passing.
|
||||
- **Stub-contract integration**: 3 / 3 passing.
|
||||
- **Full integration suite**: green; container exits 0.
|
||||
|
||||
## Conclusion
|
||||
|
||||
**Verdict: PASS.** Phase 1 (Critical) closed cleanly. Phase 2 (Correctness) opened with AZ-359 also clean. Architecture baseline still at zero findings.
|
||||
|
||||
The implement skill may proceed to batch 10 without an auto-fix loop or user gate.
|
||||
Reference in New Issue
Block a user