diff --git a/SatelliteProvider.Api/Program.cs b/SatelliteProvider.Api/Program.cs index 3105850..20cb4c5 100644 --- a/SatelliteProvider.Api/Program.cs +++ b/SatelliteProvider.Api/Program.cs @@ -79,8 +79,8 @@ builder.Services.AddSwaggerGen(c => var app = builder.Build(); -var logger = app.Services.GetRequiredService>(); -var migrator = new DatabaseMigrator(connectionString, logger as ILogger); +var migratorLogger = app.Services.GetRequiredService>(); +var migrator = new DatabaseMigrator(connectionString, migratorLogger); if (!migrator.RunMigrations()) { throw new Exception("Database migration failed. Application cannot start."); diff --git a/SatelliteProvider.Services.RegionProcessing/RegionRequestQueue.cs b/SatelliteProvider.Services.RegionProcessing/RegionRequestQueue.cs index d44627d..e62c109 100644 --- a/SatelliteProvider.Services.RegionProcessing/RegionRequestQueue.cs +++ b/SatelliteProvider.Services.RegionProcessing/RegionRequestQueue.cs @@ -9,8 +9,6 @@ public class RegionRequestQueue : IRegionRequestQueue { private readonly Channel _queue; private readonly ILogger? _logger; - private int _totalEnqueued = 0; - private int _totalDequeued = 0; public RegionRequestQueue(int capacity, ILogger? logger = null) { @@ -25,7 +23,6 @@ public class RegionRequestQueue : IRegionRequestQueue public async ValueTask EnqueueAsync(RegionRequest request, CancellationToken cancellationToken = default) { - _totalEnqueued++; await _queue.Writer.WriteAsync(request, cancellationToken); } @@ -35,7 +32,6 @@ public class RegionRequestQueue : IRegionRequestQueue { if (_queue.Reader.TryRead(out var request)) { - _totalDequeued++; return request; } } diff --git a/SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs b/SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs index 691e779..d0ecb01 100644 --- a/SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs +++ b/SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs @@ -607,25 +607,23 @@ public class RouteProcessingService : BackgroundService return earthRadiusMeters * c; } - private static (int TileX, int TileY) ExtractTileCoordinatesFromFilename(string filePath) + internal (int TileX, int TileY) ExtractTileCoordinatesFromFilename(string filePath) { - try + ArgumentNullException.ThrowIfNull(filePath); + + var filename = Path.GetFileNameWithoutExtension(filePath); + var parts = filename.Split('_'); + + if (parts.Length >= 4 && parts[0] == "tile" && + int.TryParse(parts[2], out var tileX) && + int.TryParse(parts[3], out var tileY)) { - var filename = Path.GetFileNameWithoutExtension(filePath); - var parts = filename.Split('_'); - - if (parts.Length >= 4 && parts[0] == "tile") - { - if (int.TryParse(parts[2], out var tileX) && int.TryParse(parts[3], out var tileY)) - { - return (tileX, tileY); - } - } + return (tileX, tileY); } - catch - { - } - + + _logger.LogWarning( + "Could not extract tile coordinates from filename {FilePath}; expected pattern tile___", + filePath); return (-1, -1); } diff --git a/SatelliteProvider.Services.RouteManagement/SatelliteProvider.Services.RouteManagement.csproj b/SatelliteProvider.Services.RouteManagement/SatelliteProvider.Services.RouteManagement.csproj index a2b872e..5911051 100644 --- a/SatelliteProvider.Services.RouteManagement/SatelliteProvider.Services.RouteManagement.csproj +++ b/SatelliteProvider.Services.RouteManagement/SatelliteProvider.Services.RouteManagement.csproj @@ -19,4 +19,8 @@ + + + + diff --git a/SatelliteProvider.Tests/RouteProcessingServiceTests.cs b/SatelliteProvider.Tests/RouteProcessingServiceTests.cs new file mode 100644 index 0000000..5b2feb7 --- /dev/null +++ b/SatelliteProvider.Tests/RouteProcessingServiceTests.cs @@ -0,0 +1,99 @@ +using FluentAssertions; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Moq; +using SatelliteProvider.Common.Configs; +using SatelliteProvider.DataAccess.Repositories; +using SatelliteProvider.Services.RouteManagement; + +namespace SatelliteProvider.Tests; + +public class RouteProcessingServiceTests +{ + private static RouteProcessingService BuildSut(out Mock> loggerMock) + { + loggerMock = new Mock>(); + var routeRepo = new Mock(); + var regionRepo = new Mock(); + var serviceProvider = new Mock(); + var storageOptions = Options.Create(new StorageConfig()); + + return new RouteProcessingService( + routeRepo.Object, + regionRepo.Object, + serviceProvider.Object, + storageOptions, + loggerMock.Object); + } + + private static void VerifyWarningLogged(Mock> loggerMock, string substringInState) + { + loggerMock.Verify( + l => l.Log( + LogLevel.Warning, + It.IsAny(), + It.Is((state, _) => state.ToString()!.Contains(substringInState)), + It.IsAny(), + It.IsAny>()), + Times.AtLeastOnce); + } + + [Fact] + public void ExtractTileCoordinatesFromFilename_ValidName_ReturnsParsedCoordinates_AC1() + { + // Arrange + var sut = BuildSut(out _); + + // Act + var (x, y) = sut.ExtractTileCoordinatesFromFilename("/tiles/tile_1700000000_42_99.jpg"); + + // Assert + x.Should().Be(42); + y.Should().Be(99); + } + + [Fact] + public void ExtractTileCoordinatesFromFilename_MalformedName_LogsWarningAndReturnsSentinel_AC1() + { + // Arrange + var sut = BuildSut(out var loggerMock); + const string malformed = "/tmp/not_a_tile_filename.jpg"; + + // Act + var (x, y) = sut.ExtractTileCoordinatesFromFilename(malformed); + + // Assert + x.Should().Be(-1); + y.Should().Be(-1); + VerifyWarningLogged(loggerMock, "not_a_tile_filename"); + } + + [Fact] + public void ExtractTileCoordinatesFromFilename_TilePrefixWithNonNumericCoords_LogsWarningAndReturnsSentinel_AC1() + { + // Arrange + var sut = BuildSut(out var loggerMock); + const string nonNumeric = "/tiles/tile_1700000000_alpha_beta.jpg"; + + // Act + var (x, y) = sut.ExtractTileCoordinatesFromFilename(nonNumeric); + + // Assert + x.Should().Be(-1); + y.Should().Be(-1); + VerifyWarningLogged(loggerMock, "tile_1700000000_alpha_beta"); + } + + [Fact] + public void ExtractTileCoordinatesFromFilename_NullPath_PropagatesArgumentNullException_AC2() + { + // Arrange + var sut = BuildSut(out _); + + // Act + Action act = () => sut.ExtractTileCoordinatesFromFilename(null!); + + // Assert + act.Should().Throw(); + } +} diff --git a/_docs/02_tasks/_dependencies_table.md b/_docs/02_tasks/_dependencies_table.md index 618004b..e8161f1 100644 --- a/_docs/02_tasks/_dependencies_table.md +++ b/_docs/02_tasks/_dependencies_table.md @@ -30,9 +30,9 @@ Roadmap: `_docs/04_refactoring/03-code-quality-refactoring/analysis/refactoring_ | Task | C-ID | Title | Phase | Depends On | Points | Status | |------|------|-------|-------|-----------|--------|--------| -| AZ-351 | C01 | Fix null logger to DatabaseMigrator | 1 | — | 2 | To Do | -| AZ-352 | C02 | Replace empty catch in ExtractTileCoordinatesFromFilename | 1 | — | 2 | To Do | -| AZ-363 | C10 | Delete write-only counters in RegionRequestQueue | 1 | — | 1 | To Do | +| AZ-351 | C01 | Fix null logger to DatabaseMigrator | 1 | — | 2 | Done (In Testing) | +| AZ-352 | C02 | Replace empty catch in ExtractTileCoordinatesFromFilename | 1 | — | 2 | Done (In Testing) | +| AZ-363 | C10 | Delete write-only counters in RegionRequestQueue | 1 | — | 1 | Done (In Testing) | | AZ-356 | C05 | Stub endpoints return 501 | 1 | — | 2 | To Do | | AZ-354 | C04 | Strict CORS by default | 1 | — | 2 | To Do | | AZ-353 | C03 | Sanitize 5xx responses via IExceptionHandler | 1 | — | 3 | To Do | diff --git a/_docs/02_tasks/todo/AZ-351_refactor_fix_null_logger_migrator.md b/_docs/02_tasks/done/AZ-351_refactor_fix_null_logger_migrator.md similarity index 100% rename from _docs/02_tasks/todo/AZ-351_refactor_fix_null_logger_migrator.md rename to _docs/02_tasks/done/AZ-351_refactor_fix_null_logger_migrator.md diff --git a/_docs/02_tasks/todo/AZ-352_refactor_replace_empty_catch_extract_tile_coords.md b/_docs/02_tasks/done/AZ-352_refactor_replace_empty_catch_extract_tile_coords.md similarity index 100% rename from _docs/02_tasks/todo/AZ-352_refactor_replace_empty_catch_extract_tile_coords.md rename to _docs/02_tasks/done/AZ-352_refactor_replace_empty_catch_extract_tile_coords.md diff --git a/_docs/02_tasks/todo/AZ-363_refactor_delete_writeonly_counters.md b/_docs/02_tasks/done/AZ-363_refactor_delete_writeonly_counters.md similarity index 100% rename from _docs/02_tasks/todo/AZ-363_refactor_delete_writeonly_counters.md rename to _docs/02_tasks/done/AZ-363_refactor_delete_writeonly_counters.md diff --git a/_docs/03_implementation/batch_07_report.md b/_docs/03_implementation/batch_07_report.md new file mode 100644 index 0000000..12d0dd7 --- /dev/null +++ b/_docs/03_implementation/batch_07_report.md @@ -0,0 +1,78 @@ +# Batch Report + +**Batch**: 7 (refactor 03-code-quality-refactoring · Phase 1 — critical defensive fixes, part 1/2) +**Tasks**: AZ-351, AZ-352, AZ-363 +**Date**: 2026-05-10 +**Total Story Points**: 5 (2 + 2 + 1) + +## Task Results + +| Task | Status | Files Modified | Tests | AC Coverage | Issues | +|------|--------|---------------|-------|-------------|--------| +| AZ-351 Fix null logger to DatabaseMigrator | Done | 1 | 44/44 unit + 5/5 smoke | 2/2 ACs | None | +| AZ-352 Replace empty catch in ExtractTileCoordinatesFromFilename | Done | 3 (svc + csproj + test) | 4 new + 44/44 unit + 5/5 smoke | 3/3 ACs | None | +| AZ-363 Delete write-only counters in RegionRequestQueue | Done | 1 | 44/44 unit + 5/5 smoke | 2/2 ACs | None | + +## Files Changed + +- `SatelliteProvider.Api/Program.cs` (AZ-351) +- `SatelliteProvider.Services.RegionProcessing/RegionRequestQueue.cs` (AZ-363) +- `SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs` (AZ-352) +- `SatelliteProvider.Services.RouteManagement/SatelliteProvider.Services.RouteManagement.csproj` (AZ-352 — `InternalsVisibleTo` for test access) +- `SatelliteProvider.Tests/RouteProcessingServiceTests.cs` (AZ-352 — new test file, 4 tests) + +## AC Test Coverage: All covered + +| Task | AC | Test | +|------|----|------| +| AZ-351 | AC-1 (migrator receives real logger) | covered by smoke startup — any smoke test exercises `app.Services.GetRequiredService>()` | +| AZ-351 | AC-2 (tests stay green) | `scripts/run-tests.sh --smoke` passes (44 unit + 5 smoke) | +| AZ-352 | AC-1 (malformed filename → warning + sentinel) | `ExtractTileCoordinatesFromFilename_MalformedName_LogsWarningAndReturnsSentinel_AC1`, `..._TilePrefixWithNonNumericCoords_..._AC1`, `..._ValidName_ReturnsParsedCoordinates_AC1` | +| AZ-352 | AC-2 (unexpected exception propagates) | `ExtractTileCoordinatesFromFilename_NullPath_PropagatesArgumentNullException_AC2` | +| AZ-352 | AC-3 (tests stay green) | smoke green | +| AZ-363 | AC-1 (fields removed) | source verifies removal; `RegionRequestQueueTests` (4 tests) confirm queue behavior unchanged | +| AZ-363 | AC-2 (tests stay green) | smoke green | + +## Code Review Verdict: PASS + +### Phase 2 (Spec Compliance) +- All ACs satisfied with executed tests. +- No scope creep: each task touched only files declared in the change spec. + +### Phase 3 (Code Quality) +- AZ-352 conversion `private static` → `internal` instance method aligns with `coderule.mdc` (side-effecting code must not be `static`). +- No new bare/empty catch blocks introduced. +- `ArgumentNullException.ThrowIfNull` used (modern .NET pattern). + +### Phase 4 (Security) +- No new external input handling. Filename in log entry is local filesystem, not user-controlled secrets. + +### Phase 5 (Performance) +- AZ-363 removes two non-atomic `++` ops; marginal positive (eliminates contended writes). + +### Phase 6 (Cross-Task Consistency) +- Three tasks touch three disjoint components (Api, RouteManagement, RegionProcessing). Zero interaction. + +### Phase 7 (Architecture Compliance) +- All edits stay within the owning component's `Owns` glob. +- `InternalsVisibleTo("SatelliteProvider.Tests")` added to RouteManagement — test-discovery hatch, not a compile-time consumer relation; does not violate Layer 3 sibling rule. +- Module-layout note "Internal: (none)" for RouteManagement is now slightly out-of-date (one method became `internal`). Deferred to AZ-315 docs sync (already In Progress). + +## Auto-Fix Attempts: 0 +## Stuck Agents: None + +## Tracker Status (post-batch) + +| Task | Pre-batch | Post-batch (after commit) | +|------|-----------|---------------------------| +| AZ-351 | In Progress | → In Testing | +| AZ-352 | In Progress | → In Testing | +| AZ-363 | In Progress | → In Testing | + +## Next Batch + +**Batch 8** (Phase 1 part 2): AZ-356 (501 stubs), AZ-354 (strict CORS), AZ-353 (sanitize 5xx via IExceptionHandler) — 7 story points, 3 tasks, all WebApi component. + +## Commit + +`[AZ-351][AZ-352][AZ-363] Refactor 03 batch 1: critical defensive fixes` (commit hash recorded post-commit). diff --git a/_docs/_autodev_state.md b/_docs/_autodev_state.md index fca1ad1..76e7328 100644 --- a/_docs/_autodev_state.md +++ b/_docs/_autodev_state.md @@ -8,7 +8,7 @@ status: in_progress sub_step: phase: 4 name: execution - detail: "RUN_DIR=03-code-quality-refactoring; safety net green (40 unit + 5 smoke); epic AZ-350; 27 tickets (AZ-351..AZ-380, gaps at 355/358/361); /implement starting batch 1" + detail: "RUN_DIR=03-code-quality-refactoring; epic AZ-350; batch 7 done (AZ-351/352/363, 5 SP); 24 tickets remaining; next batch 8 = AZ-356/354/353 (Phase 1 part 2, 7 SP, WebApi)" retry_count: 0 cycle: 1 tracker: jira