Skip to content

Commit 6cb62fc

Browse files
fix: address valid review comments from PR 1074 (#1075)
## Summary - evaluated unresolved review comments from #1074 and applied fixes for the valid ones - updated sitemap route filtering to exclude only the `/sitemap.xml` route template - stopped using per-request `UtcNow` as sitemap `lastmod` fallback when content has no timestamp - moved `UseOutputCache()` to run after authentication/authorization/rate-limiting middleware - renamed sitemap startup validation log methods to match their behavior - tightened sitemap exclusion assertion and added null-`LastModified` coverage ## Validation - `dotnet restore /p:AccessToNugetFeed=false` - `dotnet build --configuration Release --no-restore /p:AccessToNugetFeed=false` ✅ - `dotnet test --project EssentialCSharp.Web.Tests/EssentialCSharp.Web.Tests.csproj --no-build --configuration Release` ⚠️ fails in existing integration tests (`CaptchaService_Verify_Success`, `KnownBreachedPassword_IsDetected`) unrelated to this change - `parallel_validation` code review ✅ - `parallel_validation` CodeQL timed out after one successful run earlier in session (0 alerts), then timed out on rerun after final tiny polish --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: BenjaminMichaelis <22186029+BenjaminMichaelis@users.noreply.github.com>
1 parent 1f8c403 commit 6cb62fc

3 files changed

Lines changed: 58 additions & 25 deletions

File tree

EssentialCSharp.Web.Tests/SitemapXmlHelpersTests.cs

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ public async Task GenerateSitemapXml_DoesNotIncludeSitemapRoute()
205205
var allUrls = nodes.Select(n => n.Url).ToList();
206206

207207
// /sitemap.xml should not list itself
208-
await Assert.That(allUrls).DoesNotContain(url => url.Contains("sitemap", StringComparison.OrdinalIgnoreCase));
208+
await Assert.That(allUrls).DoesNotContain(url => url.EndsWith("/sitemap.xml", StringComparison.OrdinalIgnoreCase));
209209
}
210210

211211
[Test]
@@ -233,6 +233,29 @@ public async Task GenerateSitemapXml_UsesLastModifiedDateFromSiteMapping()
233233
await Assert.That(siteMappingNode.LastModificationDate).IsEqualTo(specificLastModified);
234234
}
235235

236+
[Test]
237+
public async Task GenerateSitemapXml_DoesNotSetLastModifiedDateWhenSiteMappingDateIsMissing()
238+
{
239+
// Arrange
240+
var baseUrl = "https://test.example.com/";
241+
var siteMappings = new List<SiteMapping>
242+
{
243+
CreateSiteMapping(1, 1, true, "test-page-1")
244+
};
245+
246+
// Act
247+
var routeConfigurationService = _Factory.Services.GetRequiredService<IRouteConfigurationService>();
248+
SitemapXmlHelpers.GenerateSitemapXml(
249+
siteMappings,
250+
routeConfigurationService,
251+
baseUrl,
252+
out var nodes);
253+
254+
// Assert
255+
var siteMappingNode = nodes.First(node => node.Url.Contains("test-page-1"));
256+
await Assert.That(siteMappingNode.LastModificationDate).IsNull();
257+
}
258+
236259
private static SiteMapping CreateSiteMapping(
237260
int chapterNumber,
238261
int pageNumber,

EssentialCSharp.Web/Helpers/SitemapXmlHelpers.cs

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,6 @@ public static void EnsureSitemapHealthy(List<SiteMapping> siteMappings)
2020

2121
public static void GenerateSitemapXml(IEnumerable<SiteMapping> siteMappings, IRouteConfigurationService routeConfigurationService, string baseUrl, out List<SitemapNode> nodes)
2222
{
23-
DateTime newDateTime = DateTime.UtcNow;
24-
2523
// Routes should end up with leading slash
2624
baseUrl = baseUrl.TrimEnd('/');
2725

@@ -40,7 +38,7 @@ public static void GenerateSitemapXml(IEnumerable<SiteMapping> siteMappings, IRo
4038
.Where(route => !route.Contains("error", StringComparison.OrdinalIgnoreCase))
4139
.Where(route => !route.Contains("index", StringComparison.OrdinalIgnoreCase))
4240
.Where(route => !route.Contains("identity", StringComparison.OrdinalIgnoreCase))
43-
.Where(route => !route.Contains("sitemap", StringComparison.OrdinalIgnoreCase))
41+
.Where(route => !IsSitemapRoute(route))
4442
.Select(route => $"/{route}")
4543
.ToList();
4644

@@ -54,14 +52,26 @@ public static void GenerateSitemapXml(IEnumerable<SiteMapping> siteMappings, IRo
5452
}
5553

5654
// Add site mappings from content
57-
nodes.AddRange(siteMappings.Where(item => item.IncludeInSitemapXml).Select<SiteMapping, SitemapNode>(siteMapping => new($"{baseUrl.TrimEnd('/')}/{siteMapping.Keys.First()}")
55+
nodes.AddRange(siteMappings.Where(item => item.IncludeInSitemapXml).Select(siteMapping =>
5856
{
59-
LastModificationDate = siteMapping.LastModified ?? newDateTime,
60-
ChangeFrequency = ChangeFrequency.Daily,
61-
Priority = 0.8M
57+
SitemapNode node = new($"{baseUrl.TrimEnd('/')}/{siteMapping.Keys.First()}")
58+
{
59+
ChangeFrequency = ChangeFrequency.Daily,
60+
Priority = 0.8M
61+
};
62+
63+
if (siteMapping.LastModified is DateTime lastModified)
64+
{
65+
node.LastModificationDate = lastModified;
66+
}
67+
68+
return node;
6269
}));
6370
}
6471

72+
private static bool IsSitemapRoute(string route) =>
73+
route.TrimStart('/').Equals("sitemap.xml", StringComparison.OrdinalIgnoreCase);
74+
6575
private static ChangeFrequency GetChangeFrequencyForRoute(string route)
6676
{
6777
return route.ToLowerInvariant() switch

EssentialCSharp.Web/Program.cs

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -505,8 +505,7 @@ await McpJsonRpcResponseWriter.WriteErrorAsync(
505505
}
506506
app.UseStaticFiles();
507507

508-
app.UseRouting();
509-
app.UseOutputCache();
508+
app.UseRouting();
510509

511510
app.UseWhen(
512511
context => context.Request.Path.StartsWithSegments("/mcp"),
@@ -537,9 +536,10 @@ await McpJsonRpcResponseWriter.WriteErrorAsync(
537536
await next(context);
538537
}));
539538

540-
app.UseRateLimiter();
541-
542-
app.UseAuthorization();
539+
app.UseRateLimiter();
540+
541+
app.UseAuthorization();
542+
app.UseOutputCache();
543543

544544
app.UseMiddleware<ReferralMiddleware>();
545545

@@ -578,13 +578,13 @@ await McpJsonRpcResponseWriter.WriteErrorAsync(
578578
try
579579
{
580580
SitemapXmlHelpers.EnsureSitemapHealthy(siteMappingService.SiteMappings.ToList());
581-
LogSitemapGenerationSucceeded(logger);
582-
}
583-
catch (Exception ex)
584-
{
585-
LogSitemapGenerationFailed(logger, ex);
586-
// Continue startup even if sitemap validation fails
587-
}
581+
LogSitemapValidationSucceeded(logger);
582+
}
583+
catch (Exception ex)
584+
{
585+
LogSitemapValidationFailed(logger, ex);
586+
// Continue startup even if sitemap validation fails
587+
}
588588

589589
app.Run();
590590
}
@@ -598,11 +598,11 @@ private static bool IsMcpTransportRequest(HttpRequest request) =>
598598
[LoggerMessage(Level = LogLevel.Error, Message = "Unhandled exception on {Path}")]
599599
private static partial void LogUnhandledException(ILogger<Program> logger, Exception? exception, PathString path);
600600

601-
[LoggerMessage(Level = LogLevel.Information, Message = "Sitemap validation completed successfully during application startup")]
602-
private static partial void LogSitemapGenerationSucceeded(ILogger<Program> logger);
603-
604-
[LoggerMessage(Level = LogLevel.Error, Message = "Failed to validate sitemap during application startup")]
605-
private static partial void LogSitemapGenerationFailed(ILogger<Program> logger, Exception exception);
601+
[LoggerMessage(Level = LogLevel.Information, Message = "Sitemap validation completed successfully during application startup")]
602+
private static partial void LogSitemapValidationSucceeded(ILogger<Program> logger);
603+
604+
[LoggerMessage(Level = LogLevel.Error, Message = "Failed to validate sitemap during application startup")]
605+
private static partial void LogSitemapValidationFailed(ILogger<Program> logger, Exception exception);
606606

607607
[LoggerMessage(Level = LogLevel.Warning, Message = "Ignoring invalid TryDotNet origin in CSP: {Origin}")]
608608
private static partial void LogIgnoringInvalidTryDotNetOrigin(ILogger logger, string origin);

0 commit comments

Comments
 (0)