azaion/satellite-provider
1
0
Fork 0
mirror of https://github.com/azaion/satellite-provider.git synced 2026-06-22 21:11:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

[AZ-376] [AZ-378] [AZ-379] [AZ-380] Repo cleanup: dead code, logger discipline, ColumnList consts
ci/woodpecker/push/01-test Pipeline was successful
Details
ci/woodpecker/push/02-build-push Pipeline was successful
Details

Browse Source 
Batch 23 of refactor 03-code-quality-refactoring (4 tasks, 5 SP):

- AZ-376 (C23): Delete unused FindExistingTileAsync from
  ITileRepository / TileRepository. No callers; method also took the
  obsolete `version` arg removed by C06/AZ-357.
- AZ-378 (C25): Repository _logger discipline.
  TileRepository.GetTilesByRegionAsync now emits LogWarning when the
  query exceeds SlowQueryThresholdMs (500 ms). RegionRepository and
  RouteRepository drop the unused ILogger<TRepo> field, parameter, and
  using; Program.cs DI registrations updated.
- AZ-379 (C26): Extract `private const string ColumnList` per repo
  (TileRepository, RegionRepository, RouteRepository); SELECTs use
  $@"SELECT {ColumnList} FROM ..." (C# 10+ const interpolation).
  INSERT/UPDATE/DELETE unchanged; route_points single-site SELECT left
  inline.
- AZ-380 (C27): Delete dead alias GeoUtils.CalculatePolygonDiagonalDistance.

Tests: +9 new (RepositoryRefactorTests x8, GeoUtilsRefactorTests x1)
covering each AC via reflection / file-content assertions; pattern
mirrors ToolingConfigurationTests (b22) and AcceptanceCriteriaRT2Tests
(b19). Unit suite 181 -> 190, all green. dotnet format clean.

Code review: PASS_WITH_WARNINGS (3 Low findings, all informational or
out-of-scope for this batch). See
_docs/03_implementation/reviews/batch_23_review.md.

Cumulative review counter 2/3; next K=3 review fires after batch 24.

Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
2026-05-11 04:57:49 +03:00
parent 534ab41b8e
commit 6099d1c86b
15 changed files with 475 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files
_docs/03_implementation/reviews/batch_23_review.md
+90
View File
@@ -0,0 +1,90 @@
# Code Review Report
**Batch**: 23
**Tasks**: AZ-376 (C23), AZ-378 (C25), AZ-379 (C26), AZ-380 (C27)
**Date**: 2026-05-11
**Verdict**: PASS_WITH_WARNINGS
## Findings
| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Low | Maintainability | `SatelliteProvider.DataAccess/Repositories/RouteRepository.cs:38` | `route_points` column list left inline (single use) |
| 2 | Low | Spec-Gap | `SatelliteProvider.DataAccess/Repositories/TileRepository.cs:69` | `EARTH_CIRCUMFERENCE_METERS`, `TILE_SIZE_PIXELS`, `111000.0` literals still in `GetTilesByRegionAsync` |
| 3 | Low | Spec-Gap | task ACs reference "37 unit + 5 smoke" | Stale test count in task ACs |
### Finding Details
**F1: `route_points` column list left inline (single use)** (Low / Maintainability)
- Location: `SatelliteProvider.DataAccess/Repositories/RouteRepository.cs:38-46`
- Description: `GetRoutePointsAsync` keeps its 8-column list inline rather than extracting a second const. AZ-379 task spec is explicit ("Per repository: extract the column list once" — singular) and the column list is used at exactly one site, so extracting would not reduce duplication.
- Suggestion: leave as-is. If a future SELECT also needs the route_points columns, extract `RoutePointColumnList` then.
- Task: AZ-379
**F2: Earth-related literals still in `GetTilesByRegionAsync`** (Low / Spec-Gap)
- Location: `SatelliteProvider.DataAccess/Repositories/TileRepository.cs:58-67, 90`
- Description: `EARTH_CIRCUMFERENCE_METERS`, `TILE_SIZE_PIXELS`, and `111000.0` are still local constants/literals. These are the explicit target of **AZ-377 (C24)**, which is in batch 24 and has dependency AZ-371 ✓. Out of scope for batch 23.
- Suggestion: addressed in batch 24 by AZ-377.
- Task: AZ-377 (out of scope here)
**F3: Stale test count in task ACs** (Low / Spec-Gap)
- Location: all four task spec files (`AZ-376_*.md`, `AZ-378_*.md`, `AZ-379_*.md`, `AZ-380_*.md`) — AC-3
- Description: each AC-3 quotes "37 unit + 5 smoke tests stay green". Pre-`/document`-era figure; current count is 190 unit tests after this batch (was 181 before). Same finding as F1 of batch_22_review (carried over).
- Suggestion: defer to refactor Phase 7 documentation sweep (batch 22 review already noted this).
- Task: shared
## Phase Notes
**Phase 1 — Context loading**: read `_docs/04_refactoring/03-code-quality-refactoring/list-of-changes.md` (C23, C25, C26, C27), four task specs, and the current source under review.
**Phase 2 — Spec compliance**: every AC has a corresponding test in `RepositoryRefactorTests.cs` or `GeoUtilsRefactorTests.cs`:
- AZ-376 AC-1 ↔ `TileRepository_DoesNotExposeFindExistingTileAsync_AZ376_AC1`
- AZ-376 AC-2/AC-3 ↔ build + 190/190 unit run (covers AC-2 build success and AC-3 tests-green)
- AZ-378 AC-1 ↔ `TileRepository_KeepsAndUsesLogger_AZ378_AC1`
- AZ-378 AC-2 ↔ `RegionRepository_HasNoUnusedLoggerParameter_AZ378_AC2` + `RouteRepository_HasNoUnusedLoggerParameter_AZ378_AC2`
- AZ-378 AC-3 ↔ 190/190 unit run
- AZ-379 AC-1 ↔ `*_DefinesColumnListConstantOnce_AZ379_AC1` (one per repo)
- AZ-379 AC-2 ↔ `RepositoryColumnLists_ContainExpectedColumns_AZ379_AC2` (column tokens preserved). Strict byte-for-byte SQL is structurally guaranteed by const interpolation; smoke run (deferred to test-run skill) will validate end-to-end DB interaction.
- AZ-379 AC-3 ↔ 190/190 unit run
- AZ-380 AC-1 ↔ `GeoUtils_DoesNotExposeCalculatePolygonDiagonalDistance_AZ380_AC1`
- AZ-380 AC-2/AC-3 ↔ build + 190/190 unit run
**Phase 3 — Code quality**:
- New `SlowQueryThresholdMs = 500` constant has a brief one-line use site; tunable later via config (deferred to a separate ticket if telemetry becomes formal).
- `Stopwatch.StartNew()` + log on threshold breach is the cheapest visible-only-when-slow pattern; no perf overhead in the hot path beyond a `long` comparison.
- `private const string ColumnList` placed at the top of each repository class; `$@"..."` interpolation requires C# 10+ const-interpolated strings (project targets net8.0, fully supported).
- Removed `using Microsoft.Extensions.Logging` from `RegionRepository.cs` and `RouteRepository.cs` after dropping the field.
**Phase 4 — Security quick-scan**:
- `ColumnList` interpolation is `private const string` defined in source code (not user input). Same trust boundary as the original inline `const string sql`. Dapper parameterization preserved for all actual values. No SQL-injection vector introduced.
- Slow-query log emits `latitude`, `longitude`, `sizeMeters`, `zoomLevel` — geographic coordinates, not PII; consistent with existing API logging.
- No hardcoded secrets. No new external input paths.
**Phase 5 — Performance scan**:
- Net positive: `GetTilesByRegionAsync` now self-reports slow queries. Hot path adds one `Stopwatch.GetTimestamp` start + one comparison + the existing await; negligible.
- ColumnList interpolation is compile-time; zero runtime cost.
- AZ-376 deletes a database round-trip method (no callers, but if reflection ever found it, it's gone now). Minor surface reduction.
**Phase 6 — Cross-task consistency**:
- AZ-376 (delete method) and AZ-378 (logger field) and AZ-379 (SELECT constants) all touch `TileRepository.cs`; applied in topological order avoiding interaction. Final state merges cleanly.
- DI registrations in `Program.cs` updated atomically with `RegionRepository` and `RouteRepository` constructor changes — no DI breakage.
- No conflicting patterns: all repos still use Dapper + raw SQL; constructor shape now varies (TileRepository keeps logger, others don't), justified by per-repo logger-usage decision in AZ-378.
**Phase 7 — Architecture compliance**:
- All edits stay within `SatelliteProvider.DataAccess/**` (DataAccess Owns) and `SatelliteProvider.Common/Utils/GeoUtils.cs` (Common Owns). `Program.cs` is WebApi Owns and is allowed to import DataAccess.
- No new cross-component imports. No new cyclic deps. No duplicate symbols introduced.
- `module-layout.md` Public API surface for ITileRepository changed (one method removed). All external consumers (none use the deleted method per grep) unaffected.
## Baseline Delta
`_docs/02_document/architecture_compliance_baseline.md` exists. No new Architecture findings introduced; F2 from baseline (logger fields not used) is partially **Resolved** for `RegionRepository` and `RouteRepository` (deleted) and **Carried over** as a deliberate-use case for `TileRepository`.
| Status | Finding |
|--------|---------|
| Resolved | F-baseline: `RegionRepository._logger` unused — deleted (AZ-378) |
| Resolved | F-baseline: `RouteRepository._logger` unused — deleted (AZ-378) |
| Carried over (now justified) | F-baseline: `TileRepository._logger` unused — now used by `GetTilesByRegionAsync` slow-query warning (AZ-378) |
## Verdict
**PASS_WITH_WARNINGS** — three Low findings, all informational or out-of-scope; zero Critical / High / Medium.