refactor(api)!: add /squadNumber/ path segment to routes#420
Merged
nanotaboada merged 6 commits intomasterfrom Mar 18, 2026
Merged
refactor(api)!: add /squadNumber/ path segment to routes#420nanotaboada merged 6 commits intomasterfrom
nanotaboada merged 6 commits intomasterfrom
Conversation
BREAKING CHANGE: old /players/{int} routes return 404;
use /players/squadNumber/{int} instead.
Co-authored-by: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRoutes for player-by-squad-number endpoints now include an explicit "squadNumber" segment; README updated accordingly. Controller error/validation responses were migrated to ProblemDetails/ValidationProblem, PUT adds a route/body squadNumber consistency check and sanitised logging, and unit tests were updated to match new routes and response shapes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Out-of-scope changes
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs (2)
172-185:⚠️ Potential issue | 🟠 MajorReturn RFC 7807 Problem Details on controller error paths.
These branches return plain
BadRequest/NotFoundresults instead of RFC 7807 payloads, which breaks the controller error contract guideline.Example pattern to apply in these branches
- return TypedResults.BadRequest(errors); + return TypedResults.ValidationProblem( + validation.Errors + .GroupBy(e => e.PropertyName) + .ToDictionary(g => g.Key, g => g.Select(e => e.ErrorMessage).ToArray()) + ); - return TypedResults.NotFound(); + return TypedResults.Problem( + statusCode: StatusCodes.Status404NotFound, + title: "Player not found", + detail: $"No player found with squad number {squadNumber}." + );As per coding guidelines:
src/**/Controllers/*.cs: "Return RFC 7807 Problem Details for all error responses in controllers".Also applies to: 188-190, 218-220
167-191:⚠️ Potential issue | 🟠 MajorEnforce route/body squad number consistency in
PUT.
PutAsyncvalidates existence by routesquadNumber, but performs update using onlyplayerfrom body. If they differ, the wrong resource can be updated.Proposed guard
public async Task<IResult> PutAsync( [FromRoute] int squadNumber, [FromBody] PlayerRequestModel player ) { + if (player.SquadNumber != squadNumber) + { + logger.LogWarning( + "PUT /players/squadNumber/{SquadNumber} failed: route/body squad number mismatch", + squadNumber + ); + return TypedResults.Problem( + statusCode: StatusCodes.Status400BadRequest, + title: "Route/body mismatch", + detail: "Route squadNumber must match payload squadNumber." + ); + } + var validation = await validator.ValidateAsync(player);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs` around lines 167 - 191, PutAsync currently validates input and existence by the route squadNumber but calls playerService.UpdateAsync(player) using the body-only model, which can update the wrong resource if player.SquadNumber differs; add a guard after validation and existence check that verifies player.SquadNumber == squadNumber (or normalize by assigning player.SquadNumber = squadNumber) and if they mismatch log a warning referencing PutAsync and the squadNumber/player.SquadNumber, then return TypedResults.BadRequest with an explanatory error; ensure the check is placed before calling playerService.UpdateAsync so UpdateAsync receives the correct squad number.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 223-224: Update the README quick-reference routes for the two
player endpoints to match the controller's route templates by changing the path
parameter from {squadNumber} to {squadNumber:int} so the PUT and DELETE entries
(`PUT /players/squadNumber/{squadNumber}` and `DELETE
/players/squadNumber/{squadNumber}`) consistently reflect the integer constraint
used by the controller.
---
Outside diff comments:
In `@src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs`:
- Around line 167-191: PutAsync currently validates input and existence by the
route squadNumber but calls playerService.UpdateAsync(player) using the
body-only model, which can update the wrong resource if player.SquadNumber
differs; add a guard after validation and existence check that verifies
player.SquadNumber == squadNumber (or normalize by assigning player.SquadNumber
= squadNumber) and if they mismatch log a warning referencing PutAsync and the
squadNumber/player.SquadNumber, then return TypedResults.BadRequest with an
explanatory error; ensure the check is placed before calling
playerService.UpdateAsync so UpdateAsync receives the correct squad number.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eaa1f56f-9f94-4366-b645-093d6726ca52
📒 Files selected for processing (2)
README.mdsrc/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs
…ging - Replace Serilog @-destructuring with explicit newline stripping on the player's string representation to address CodeQL alert #384 (CWE-117). Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs (1)
127-146:⚠️ Potential issue | 🟠 MajorUse RFC 7807 Problem Details for error responses on these endpoints.
The touched GET/PUT/DELETE squad-number actions still return bare
NotFound()/BadRequest(...)responses. This violates the controller error-response contract.As per coding guidelines "src/**/Controllers/*.cs: Return RFC 7807 Problem Details for all error responses in controllers".
Also applies to: 172-189, 216-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs` around lines 127 - 146, The GetBySquadNumberAsync action in PlayerController (and the sibling PUT/DELETE squad-number actions) returns bare NotFound()/BadRequest(...) responses; update these to return RFC 7807 Problem Details instead: construct a ProblemDetails (or one of the provided ProblemDetails-derived types) with status, title, detail (include contextual info such as the squadNumber and reason), and instance, log the same contextual information via logger, and return it via TypedResults.Problem (or the controller's Problem helper) so the endpoint complies with the "Return RFC 7807 Problem Details for all error responses in controllers" guideline; apply the same pattern to the other squad-number handlers referenced (lines 172-189, 216-225).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs`:
- Line 127: Add integration tests that exercise the route declared by
[HttpGet("squadNumber/{squadNumber:int}", Name = "RetrieveBySquadNumber")] on
PlayerController and cover GET, PUT and DELETE for
/players/squadNumber/{squadNumber:int} (start a
TestServer/WebApplicationFactory, call the real HTTP endpoints, assert status
codes and response bodies). Replace all usages of TypedResults.BadRequest() and
TypedResults.NotFound() in PlayerController (the methods handling squadNumber
and the other endpoints referenced) with RFC 7807-compliant Problem Details
responses: construct a ProblemDetails (or create an instance with properties
type, title, status, detail, instance) and return it with the appropriate status
code so every 400/404 response includes type, title, status, detail and instance
fields. Ensure your integration tests assert the Problem Details schema is
returned for error cases.
---
Outside diff comments:
In `@src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs`:
- Around line 127-146: The GetBySquadNumberAsync action in PlayerController (and
the sibling PUT/DELETE squad-number actions) returns bare
NotFound()/BadRequest(...) responses; update these to return RFC 7807 Problem
Details instead: construct a ProblemDetails (or one of the provided
ProblemDetails-derived types) with status, title, detail (include contextual
info such as the squadNumber and reason), and instance, log the same contextual
information via logger, and return it via TypedResults.Problem (or the
controller's Problem helper) so the endpoint complies with the "Return RFC 7807
Problem Details for all error responses in controllers" guideline; apply the
same pattern to the other squad-number handlers referenced (lines 172-189,
216-225).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cecdd760-dd2e-4ced-8efe-ff7b5b8987e9
📒 Files selected for processing (1)
src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
9 tasks
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs (1)
402-437:⚠️ Potential issue | 🟠 MajorAdd HTTP-level integration coverage for new and legacy routes.
These unit tests validate controller logic but not endpoint routing. Acceptance criteria requires verifying
/players/squadNumber/{squadNumber:int}behavior and that old/players/{int}returns 404, which needs integration tests (e.g.,WebApplicationFactory+HttpClient).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs` around lines 402 - 437, Add HTTP-level integration tests alongside the existing unit test: create an integration test using WebApplicationFactory to host the app and HttpClient to call the new route "/players/squadNumber/{squadNumber:int}" and assert it returns the expected status and payload when calling the PutAsync endpoint on PlayerController, and also call the legacy "/players/{id}" route and assert it returns 404; update or add tests in PlayerControllerTests (or a new integration test class) that spin up the factory, perform PUT requests to both routes, and assert the new route is handled while the legacy route yields 404, ensuring the test covers routing behavior rather than just invoking PlayerController.PutAsync directly.src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs (1)
34-35:⚠️ Potential issue | 🟠 MajorReturn RFC 7807 for 409 Conflict in
PostAsync.
PostAsyncstill returnsTypedResults.Conflict()without Problem Details payload. This breaks the controller error-response contract used elsewhere in this file.Suggested fix
- [ProducesResponseType(StatusCodes.Status409Conflict)] + [ProducesResponseType<ProblemDetails>(StatusCodes.Status409Conflict)] @@ - return TypedResults.Conflict(); + return TypedResults.Problem( + statusCode: StatusCodes.Status409Conflict, + title: "Conflict", + detail: $"Player with Squad Number '{player.SquadNumber}' already exists.", + instance: HttpContext?.Request?.Path.ToString() + );As per coding guidelines "src/**/Controllers/*.cs: Return RFC 7807 Problem Details for all error responses in controllers".
Also applies to: 53-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs` around lines 34 - 35, PostAsync currently returns TypedResults.Conflict() without an RFC 7807 Problem Details body; update PostAsync to return a ProblemDetails instance (Microsoft.AspNetCore.Mvc.ProblemDetails) populated with Title/Detail/Status (409) and optionally Instance or type, and return it with TypedResults.Conflict(problemDetails) so the controller follows the RFC7807 error-response contract; apply the same change to the other error-returning branches in this file (the block around lines 53-60) where TypedResults.Conflict() or other error results are returned without a ProblemDetails payload.
🧹 Nitpick comments (1)
src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs (1)
209-232: Check route/body squad-number mismatch before existence lookup.In
PutAsync, the mismatch guard runs afterRetrieveBySquadNumberAsync. Moving mismatch validation earlier avoids an unnecessary service call and gives deterministic 400 semantics for malformed requests.Suggested refactor
- if (await playerService.RetrieveBySquadNumberAsync(squadNumber) == null) - { - logger.LogWarning("PUT /players/squadNumber/{SquadNumber} not found", squadNumber); - return TypedResults.Problem( - statusCode: StatusCodes.Status404NotFound, - title: "Not Found", - detail: $"Player with Squad Number '{squadNumber}' was not found.", - instance: HttpContext?.Request?.Path.ToString() - ); - } if (player.SquadNumber != squadNumber) { logger.LogWarning( "PutAsync squad number mismatch: route {SquadNumber} != body {PlayerSquadNumber}", squadNumber, player.SquadNumber ); return TypedResults.Problem( statusCode: StatusCodes.Status400BadRequest, title: "Bad Request", detail: "Squad number in the route does not match squad number in the request body.", instance: HttpContext?.Request?.Path.ToString() ); } + if (await playerService.RetrieveBySquadNumberAsync(squadNumber) == null) + { + logger.LogWarning("PUT /players/squadNumber/{SquadNumber} not found", squadNumber); + return TypedResults.Problem( + statusCode: StatusCodes.Status404NotFound, + title: "Not Found", + detail: $"Player with Squad Number '{squadNumber}' was not found.", + instance: HttpContext?.Request?.Path.ToString() + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs` around lines 209 - 232, The PutAsync method currently calls playerService.RetrieveBySquadNumberAsync(squadNumber) before validating that the route squadNumber matches the body (player.SquadNumber); move the squad-number mismatch guard (the if that compares player.SquadNumber != squadNumber and the logger.LogWarning/TypedResults.Problem 400 return) to run before calling playerService.RetrieveBySquadNumberAsync to avoid the unnecessary service lookup and produce a deterministic 400 response for bad requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs`:
- Around line 59-63: The test uses a conditional check "if (result is
ValidationProblem httpResult)" which can skip assertions when the type is wrong;
replace this pattern by asserting the type directly (e.g., call
result.Should().BeOfType<ValidationProblem>() or
Assert.IsType<ValidationProblem>(result)), then cast to ValidationProblem (var
httpResult = (ValidationProblem)result) and assert httpResult.StatusCode ==
StatusCodes.Status400BadRequest; apply the same change to the other occurrences
flagged (around the blocks that currently use "if (result is ...)" so each test
fails if the result type is incorrect.
---
Duplicate comments:
In `@src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs`:
- Around line 34-35: PostAsync currently returns TypedResults.Conflict() without
an RFC 7807 Problem Details body; update PostAsync to return a ProblemDetails
instance (Microsoft.AspNetCore.Mvc.ProblemDetails) populated with
Title/Detail/Status (409) and optionally Instance or type, and return it with
TypedResults.Conflict(problemDetails) so the controller follows the RFC7807
error-response contract; apply the same change to the other error-returning
branches in this file (the block around lines 53-60) where
TypedResults.Conflict() or other error results are returned without a
ProblemDetails payload.
In `@test/Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs`:
- Around line 402-437: Add HTTP-level integration tests alongside the existing
unit test: create an integration test using WebApplicationFactory to host the
app and HttpClient to call the new route
"/players/squadNumber/{squadNumber:int}" and assert it returns the expected
status and payload when calling the PutAsync endpoint on PlayerController, and
also call the legacy "/players/{id}" route and assert it returns 404; update or
add tests in PlayerControllerTests (or a new integration test class) that spin
up the factory, perform PUT requests to both routes, and assert the new route is
handled while the legacy route yields 404, ensuring the test covers routing
behavior rather than just invoking PlayerController.PutAsync directly.
---
Nitpick comments:
In `@src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs`:
- Around line 209-232: The PutAsync method currently calls
playerService.RetrieveBySquadNumberAsync(squadNumber) before validating that the
route squadNumber matches the body (player.SquadNumber); move the squad-number
mismatch guard (the if that compares player.SquadNumber != squadNumber and the
logger.LogWarning/TypedResults.Problem 400 return) to run before calling
playerService.RetrieveBySquadNumberAsync to avoid the unnecessary service lookup
and produce a deterministic 400 response for bad requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dfd8123d-fce8-4f5a-9cb1-be16eaa13137
📒 Files selected for processing (3)
README.mdsrc/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cstest/Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
…#418) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs (1)
105-111: Consider extracting "Not Found" to a constant.Static analysis flagged that the literal
"Not Found"is used 5 times across the file. While not critical, extracting this to a constant would reduce duplication and make future changes easier.♻️ Optional refactor
+private const string NotFoundTitle = "Not Found"; + // Then use in Problem() calls: - title: "Not Found", + title: NotFoundTitle,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs` around lines 105 - 111, The literal "Not Found" is repeated in PlayerController (used in the TypedResults.Problem calls); extract it to a single constant (e.g., private const string NotFoundTitle = "Not Found") at the top of the PlayerController class and replace all occurrences of the literal in methods like the TypedResults.Problem call that returns 404 so they reference NotFoundTitle instead; ensure the constant is suitably scoped (private or private static) so all methods in the controller can use it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs`:
- Around line 243-253: The null-conditional operator on player is redundant
because player is non-null after validation and dereference; update the
sanitization to call player.ToString() directly (replace the "?.ToString()" and
first "?" usage) when building sanitizedPlayerString in PlayerController so the
string replacements always execute, then pass sanitizedPlayerString into
logger.LogInformation as before.
---
Nitpick comments:
In `@src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cs`:
- Around line 105-111: The literal "Not Found" is repeated in PlayerController
(used in the TypedResults.Problem calls); extract it to a single constant (e.g.,
private const string NotFoundTitle = "Not Found") at the top of the
PlayerController class and replace all occurrences of the literal in methods
like the TypedResults.Problem call that returns 404 so they reference
NotFoundTitle instead; ensure the constant is suitably scoped (private or
private static) so all methods in the controller can use it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4425e43-a8fd-4df8-82fe-ceb32140aefc
📒 Files selected for processing (2)
src/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cstest/Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cs
…#418) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
|
This was referenced Mar 21, 2026
This was referenced Mar 29, 2026
6 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #418
Summary by CodeRabbit
Refactor
Bug Fixes
Documentation
Tests
Chores