mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-22 19:01:15 +00:00
[AZ-351][AZ-352][AZ-363] Refactor 03 batch 1: critical defensive fixes
AZ-351: Resolve ILogger<DatabaseMigrator> directly from DI in Program.cs instead of casting ILogger<Program> (which always returned null). Migrator now logs through Serilog at startup. AZ-352: Drop empty catch in RouteProcessingService.ExtractTileCoordinatesFromFilename. Convert the method from private static to internal instance so it can use the existing _logger (per coderule: side-effecting code must not be static). Add typed null-guard via ArgumentNullException.ThrowIfNull so unexpected exceptions propagate. Adds InternalsVisibleTo on the RouteManagement csproj for SatelliteProvider.Tests, plus 4 unit tests in RouteProcessingServiceTests.cs covering AC-1 (valid / malformed / non-numeric) and AC-2 (null path propagation). AZ-363: Delete _totalEnqueued / _totalDequeued fields and the two non-atomic ++ writes in RegionRequestQueue. Fields were write-only dead code and a thread-safety hazard. Tests: 44/44 unit + 5/5 smoke (scripts/run-tests.sh --smoke). Code review verdict: PASS, 0 findings, 0 auto-fix attempts. Batch report: _docs/03_implementation/batch_07_report.md. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -79,8 +79,8 @@ builder.Services.AddSwaggerGen(c =>
|
|||||||
|
|
||||||
var app = builder.Build();
|
var app = builder.Build();
|
||||||
|
|
||||||
var logger = app.Services.GetRequiredService<ILogger<Program>>();
|
var migratorLogger = app.Services.GetRequiredService<ILogger<DatabaseMigrator>>();
|
||||||
var migrator = new DatabaseMigrator(connectionString, logger as ILogger<DatabaseMigrator>);
|
var migrator = new DatabaseMigrator(connectionString, migratorLogger);
|
||||||
if (!migrator.RunMigrations())
|
if (!migrator.RunMigrations())
|
||||||
{
|
{
|
||||||
throw new Exception("Database migration failed. Application cannot start.");
|
throw new Exception("Database migration failed. Application cannot start.");
|
||||||
|
|||||||
@@ -9,8 +9,6 @@ public class RegionRequestQueue : IRegionRequestQueue
|
|||||||
{
|
{
|
||||||
private readonly Channel<RegionRequest> _queue;
|
private readonly Channel<RegionRequest> _queue;
|
||||||
private readonly ILogger<RegionRequestQueue>? _logger;
|
private readonly ILogger<RegionRequestQueue>? _logger;
|
||||||
private int _totalEnqueued = 0;
|
|
||||||
private int _totalDequeued = 0;
|
|
||||||
|
|
||||||
public RegionRequestQueue(int capacity, ILogger<RegionRequestQueue>? logger = null)
|
public RegionRequestQueue(int capacity, ILogger<RegionRequestQueue>? logger = null)
|
||||||
{
|
{
|
||||||
@@ -25,7 +23,6 @@ public class RegionRequestQueue : IRegionRequestQueue
|
|||||||
|
|
||||||
public async ValueTask EnqueueAsync(RegionRequest request, CancellationToken cancellationToken = default)
|
public async ValueTask EnqueueAsync(RegionRequest request, CancellationToken cancellationToken = default)
|
||||||
{
|
{
|
||||||
_totalEnqueued++;
|
|
||||||
await _queue.Writer.WriteAsync(request, cancellationToken);
|
await _queue.Writer.WriteAsync(request, cancellationToken);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -35,7 +32,6 @@ public class RegionRequestQueue : IRegionRequestQueue
|
|||||||
{
|
{
|
||||||
if (_queue.Reader.TryRead(out var request))
|
if (_queue.Reader.TryRead(out var request))
|
||||||
{
|
{
|
||||||
_totalDequeued++;
|
|
||||||
return request;
|
return request;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -607,25 +607,23 @@ public class RouteProcessingService : BackgroundService
|
|||||||
return earthRadiusMeters * c;
|
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 filename = Path.GetFileNameWithoutExtension(filePath);
|
||||||
var parts = filename.Split('_');
|
var parts = filename.Split('_');
|
||||||
|
|
||||||
if (parts.Length >= 4 && parts[0] == "tile")
|
if (parts.Length >= 4 && parts[0] == "tile" &&
|
||||||
{
|
int.TryParse(parts[2], out var tileX) &&
|
||||||
if (int.TryParse(parts[2], out var tileX) && int.TryParse(parts[3], out var tileY))
|
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_<timestamp>_<x>_<y>",
|
||||||
|
filePath);
|
||||||
return (-1, -1);
|
return (-1, -1);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+4
@@ -19,4 +19,8 @@
|
|||||||
<ProjectReference Include="..\SatelliteProvider.DataAccess\SatelliteProvider.DataAccess.csproj" />
|
<ProjectReference Include="..\SatelliteProvider.DataAccess\SatelliteProvider.DataAccess.csproj" />
|
||||||
</ItemGroup>
|
</ItemGroup>
|
||||||
|
|
||||||
|
<ItemGroup>
|
||||||
|
<InternalsVisibleTo Include="SatelliteProvider.Tests" />
|
||||||
|
</ItemGroup>
|
||||||
|
|
||||||
</Project>
|
</Project>
|
||||||
|
|||||||
@@ -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<ILogger<RouteProcessingService>> loggerMock)
|
||||||
|
{
|
||||||
|
loggerMock = new Mock<ILogger<RouteProcessingService>>();
|
||||||
|
var routeRepo = new Mock<IRouteRepository>();
|
||||||
|
var regionRepo = new Mock<IRegionRepository>();
|
||||||
|
var serviceProvider = new Mock<IServiceProvider>();
|
||||||
|
var storageOptions = Options.Create(new StorageConfig());
|
||||||
|
|
||||||
|
return new RouteProcessingService(
|
||||||
|
routeRepo.Object,
|
||||||
|
regionRepo.Object,
|
||||||
|
serviceProvider.Object,
|
||||||
|
storageOptions,
|
||||||
|
loggerMock.Object);
|
||||||
|
}
|
||||||
|
|
||||||
|
private static void VerifyWarningLogged(Mock<ILogger<RouteProcessingService>> loggerMock, string substringInState)
|
||||||
|
{
|
||||||
|
loggerMock.Verify(
|
||||||
|
l => l.Log(
|
||||||
|
LogLevel.Warning,
|
||||||
|
It.IsAny<EventId>(),
|
||||||
|
It.Is<It.IsAnyType>((state, _) => state.ToString()!.Contains(substringInState)),
|
||||||
|
It.IsAny<Exception?>(),
|
||||||
|
It.IsAny<Func<It.IsAnyType, Exception?, string>>()),
|
||||||
|
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<ArgumentNullException>();
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -30,9 +30,9 @@ Roadmap: `_docs/04_refactoring/03-code-quality-refactoring/analysis/refactoring_
|
|||||||
|
|
||||||
| Task | C-ID | Title | Phase | Depends On | Points | Status |
|
| Task | C-ID | Title | Phase | Depends On | Points | Status |
|
||||||
|------|------|-------|-------|-----------|--------|--------|
|
|------|------|-------|-------|-----------|--------|--------|
|
||||||
| AZ-351 | C01 | Fix null logger to DatabaseMigrator | 1 | — | 2 | 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 | To Do |
|
| AZ-352 | C02 | Replace empty catch in ExtractTileCoordinatesFromFilename | 1 | — | 2 | Done (In Testing) |
|
||||||
| AZ-363 | C10 | Delete write-only counters in RegionRequestQueue | 1 | — | 1 | To Do |
|
| 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-356 | C05 | Stub endpoints return 501 | 1 | — | 2 | To Do |
|
||||||
| AZ-354 | C04 | Strict CORS by default | 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 |
|
| AZ-353 | C03 | Sanitize 5xx responses via IExceptionHandler | 1 | — | 3 | To Do |
|
||||||
|
|||||||
@@ -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<ILogger<DatabaseMigrator>>()` |
|
||||||
|
| 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).
|
||||||
@@ -8,7 +8,7 @@ status: in_progress
|
|||||||
sub_step:
|
sub_step:
|
||||||
phase: 4
|
phase: 4
|
||||||
name: execution
|
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
|
retry_count: 0
|
||||||
cycle: 1
|
cycle: 1
|
||||||
tracker: jira
|
tracker: jira
|
||||||
|
|||||||
Reference in New Issue
Block a user