azaion/satellite-provider

Fork 0

mirror of https://github.com/azaion/satellite-provider.git synced 2026-06-21 17:01:14 +00:00

Files

T

Oleksandr Bezdieniezhnykh 6099d1c86b

ci/woodpecker/push/01-test Pipeline was successful

Details

ci/woodpecker/push/02-build-push Pipeline was successful

Details

[AZ-376] [AZ-378] [AZ-379] [AZ-380] Repo cleanup: dead code, logger discipline, ColumnList consts

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>

2026-05-11 04:57:49 +03:00

6.8 KiB

Raw Blame History

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.

6.8 KiB Raw Blame History