fix(validators): scope BeUniqueSquadNumber to "Create" rule set (#424)#426
Merged
nanotaboada merged 4 commits intomasterfrom Mar 21, 2026
Merged
fix(validators): scope BeUniqueSquadNumber to "Create" rule set (#424)#426nanotaboada merged 4 commits intomasterfrom
nanotaboada merged 4 commits intomasterfrom
Conversation
Organizes all PlayerRequestModelValidator rules into explicit CRUD-named rule sets: \"Create\" (POST, includes BeUniqueSquadNumber) and \"Update\" (PUT, omits it). The controller passes IncludeRuleSets(\"Create\") or IncludeRuleSets(\"Update\") so only the appropriate rules run per operation. Adds ValidateAsync_SquadNumber_BelongsToPlayerBeingUpdated_ReturnsNoErrors as a regression test for the PUT 400 bug. Updates all controller test mocks to match on IValidationContext (the overload the rule-set extension method routes through). 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)
WalkthroughThis PR implements FluentValidation rule sets to fix a bug where the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Out-of-scope changes
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 PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Introduced alongside the first reported bug (#424) to standardize how future issues are filed. The template mirrors the structure used in #424: Description, Steps to Reproduce, Expected / Actual Behavior, Environment, and an optional Possible Solution section. Co-authored-by: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerValidatorTests.cs (1)
154-179: Consider adding coverage for "Update" rule set field validation.The new test verifies that
BeUniqueSquadNumberis not triggered for updates, but there's no test confirming that the "Update" rule set still validates structural fields (e.g., emptyFirstName). This would ensure the rule set isn't accidentally empty.🧪 Suggested additional test
[Fact] [Trait("Category", "Unit")] public async Task ValidateAsync_UpdateRuleSet_FirstNameEmpty_ReturnsValidationError() { // Arrange var request = PlayerFakes.MakeRequestModelForUpdate(10); request.FirstName = string.Empty; var validator = CreateValidator(); // Act var result = await validator.ValidateAsync( request, options => options.IncludeRuleSets("Update") ); // Assert result.IsValid.Should().BeFalse(); result.Errors.Should().Contain(error => error.PropertyName == "FirstName"); }🤖 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/PlayerValidatorTests.cs` around lines 154 - 179, Add a unit test to ensure the "Update" rule set still validates structural fields by creating a request via PlayerFakes.MakeRequestModelForUpdate(10), setting request.FirstName = string.Empty, calling validator.ValidateAsync(request, options => options.IncludeRuleSets("Update")) with a validator from CreateValidator(), and asserting the result is invalid and contains an error with PropertyName "FirstName"; place this alongside the existing ValidateAsync_SquadNumber_BelongsToPlayerBeingUpdated_ReturnsNoErrors test in PlayerValidatorTests to guard against the "Update" rule set becoming empty.src/Dotnet.Samples.AspNetCore.WebApi/Validators/PlayerRequestModelValidator.cs (2)
45-86: Consider extracting shared rules to reduce duplication.Both "Create" and "Update" rule sets define identical rules for
FirstName,LastName,SquadNumber(basic checks),AbbrPosition, andDateOfBirth. This duplication increases maintenance risk—if a rule changes, it must be updated in both places.FluentValidation supports
Include()to share common rules or you could define common rules outside the rule sets and useRuleSetonly for the uniqueBeUniqueSquadNumbercheck.♻️ Proposed refactor to reduce duplication
public PlayerRequestModelValidator( IPlayerRepository playerRepository, TimeProvider? timeProvider = null ) { _playerRepository = playerRepository; var clock = timeProvider ?? TimeProvider.System; + // Common structural rules (run for all rule sets) + RuleFor(player => player.FirstName) + .NotEmpty() + .WithMessage("FirstName is required."); + + RuleFor(player => player.LastName) + .NotEmpty() + .WithMessage("LastName is required."); + + RuleFor(player => player.SquadNumber) + .NotEmpty() + .WithMessage("SquadNumber is required.") + .GreaterThan(0) + .WithMessage("SquadNumber must be greater than 0."); + + RuleFor(player => player.AbbrPosition) + .NotEmpty() + .WithMessage("AbbrPosition is required.") + .Must(Position.IsValidAbbr) + .WithMessage("AbbrPosition is invalid."); + + When( + player => player.DateOfBirth.HasValue, + () => + { + RuleFor(player => player.DateOfBirth) + .Must(date => date!.Value.Date < clock.GetUtcNow().Date) + .WithMessage("DateOfBirth must be a date in the past.") + .Must(date => + date!.Value.Date >= new DateTime(1900, 1, 1, 0, 0, 0, DateTimeKind.Utc) + ) + .WithMessage("DateOfBirth must be on or after January 1, 1900."); + } + ); + // "Create" rule set — POST /players - // Includes BeUniqueSquadNumber to prevent duplicate squad numbers on insert. + // Adds BeUniqueSquadNumber to prevent duplicate squad numbers on insert. RuleSet( "Create", () => { - RuleFor(player => player.FirstName) - .NotEmpty() - .WithMessage("FirstName is required."); - // ... (remove all duplicated rules) RuleFor(player => player.SquadNumber) .MustAsync(BeUniqueSquadNumber) .WithMessage("SquadNumber must be unique."); } ); - // "Update" rule set — PUT /players/squadNumber/{n} - // BeUniqueSquadNumber is intentionally omitted... - RuleSet( - "Update", - () => - { - // ... (remove entire Update rule set - common rules apply automatically) - } - ); + // "Update" rule set — empty; common rules apply automatically. + // BeUniqueSquadNumber is intentionally omitted. + RuleSet("Update", () => { }); }Note: When using
IncludeRuleSets("Create")orIncludeRuleSets("Update"), you'll also need to include the default rule set:options.IncludeRuleSets("Create", "default")to run both the shared rules and the rule-set-specific rules.Also applies to: 88-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Dotnet.Samples.AspNetCore.WebApi/Validators/PlayerRequestModelValidator.cs` around lines 45 - 86, Extract the shared validation rules (RuleFor for FirstName, LastName, SquadNumber basic checks without MustAsync BeUniqueSquadNumber, AbbrPosition with Position.IsValidAbbr, and the When(DateOfBirth.HasValue) block using clock.GetUtcNow and the 1900 lower-bound) into a reusable rule set or private method (e.g., CreateSharedRules or a default ruleset) and then call Include(...) or IncludeRuleSets(...) inside both RuleSet("Create", ...) and RuleSet("Update", ...); keep the BeUniqueSquadNumber MustAsync only in the "Create" RuleSet and ensure the Update RuleSet references the shared rules so duplication in RuleFor and the DateOfBirth checks is removed.
62-63:BeUniqueSquadNumberperforms business validation via repository lookup.Per coding guidelines, validators should handle structural validation only, while business validation rules belong in services. The
BeUniqueSquadNumbercheck queries the repository to enforce a uniqueness constraint, which is business logic.Consider moving this check to
PlayerService.CreateAsync()where the duplicate check already exists (controller line 59), or document this as an intentional exception to the guideline for fail-fast validation.Based on learnings: "Implement business validation rules in services, not in validators; validators should handle structural validation only"
Also applies to: 131-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Dotnet.Samples.AspNetCore.WebApi/Validators/PlayerRequestModelValidator.cs` around lines 62 - 63, The validator PlayerRequestModelValidator currently calls the async repository check BeUniqueSquadNumber (structural vs business rule); remove the MustAsync(BeUniqueSquadNumber) calls (including the duplicate at lines ~131-134) from PlayerRequestModelValidator and instead enforce uniqueness inside the service layer by adding the duplicate-squad-number check in PlayerService.CreateAsync (or ensure the existing check there runs and returns a validation/error result). Update any controller flow that relied on the validator to handle the error path returned by PlayerService.CreateAsync and adjust tests to assert the duplicate-squad-number behavior through the service instead of the validator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/Dotnet.Samples.AspNetCore.WebApi/Validators/PlayerRequestModelValidator.cs`:
- Around line 45-86: Extract the shared validation rules (RuleFor for FirstName,
LastName, SquadNumber basic checks without MustAsync BeUniqueSquadNumber,
AbbrPosition with Position.IsValidAbbr, and the When(DateOfBirth.HasValue) block
using clock.GetUtcNow and the 1900 lower-bound) into a reusable rule set or
private method (e.g., CreateSharedRules or a default ruleset) and then call
Include(...) or IncludeRuleSets(...) inside both RuleSet("Create", ...) and
RuleSet("Update", ...); keep the BeUniqueSquadNumber MustAsync only in the
"Create" RuleSet and ensure the Update RuleSet references the shared rules so
duplication in RuleFor and the DateOfBirth checks is removed.
- Around line 62-63: The validator PlayerRequestModelValidator currently calls
the async repository check BeUniqueSquadNumber (structural vs business rule);
remove the MustAsync(BeUniqueSquadNumber) calls (including the duplicate at
lines ~131-134) from PlayerRequestModelValidator and instead enforce uniqueness
inside the service layer by adding the duplicate-squad-number check in
PlayerService.CreateAsync (or ensure the existing check there runs and returns a
validation/error result). Update any controller flow that relied on the
validator to handle the error path returned by PlayerService.CreateAsync and
adjust tests to assert the duplicate-squad-number behavior through the service
instead of the validator.
In `@test/Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerValidatorTests.cs`:
- Around line 154-179: Add a unit test to ensure the "Update" rule set still
validates structural fields by creating a request via
PlayerFakes.MakeRequestModelForUpdate(10), setting request.FirstName =
string.Empty, calling validator.ValidateAsync(request, options =>
options.IncludeRuleSets("Update")) with a validator from CreateValidator(), and
asserting the result is invalid and contains an error with PropertyName
"FirstName"; place this alongside the existing
ValidateAsync_SquadNumber_BelongsToPlayerBeingUpdated_ReturnsNoErrors test in
PlayerValidatorTests to guard against the "Update" rule set becoming empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dbda2b24-0cd3-4296-809c-3bbc301cd14a
📒 Files selected for processing (5)
.github/ISSUE_TEMPLATE/bug_report.mdsrc/Dotnet.Samples.AspNetCore.WebApi/Controllers/PlayerController.cssrc/Dotnet.Samples.AspNetCore.WebApi/Validators/PlayerRequestModelValidator.cstest/Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerControllerTests.cstest/Dotnet.Samples.AspNetCore.WebApi.Tests/Unit/PlayerValidatorTests.cs
Configures SonarCloud analysis for the project: - Scopes sources and tests to their respective directories - Excludes build artifacts, generated files, and EF Core migrations from analysis and coverage metrics - Excludes the test project from duplicate code (CPD) detection — Fakes, Mocks, and Stubs are intentionally repetitive by design Co-authored-by: Claude <noreply@anthropic.com>
…tion The "Create" and "Update" rule sets in PlayerRequestModelValidator share common rules by design; each operation's validation is intentionally kept self-contained for readability. // NOSONAR only suppresses rule-based issues and has no effect on CPD metrics, so the file is listed under sonar.cpd.exclusions instead. Co-authored-by: Claude <noreply@anthropic.com>
|
This was referenced Mar 21, 2026
This was referenced Mar 29, 2026
6 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
1 participant
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.
Summary
PlayerRequestModelValidatorrules into explicit CRUD-named rule sets:"Create"(POST) and"Update"(PUT)BeUniqueSquadNumberlives only in"Create"— on PUT the player already exists, so the uniqueness check would always failopts.IncludeRuleSets("Create")oropts.IncludeRuleSets("Update"), making the intent self-documentingValidateAsync_SquadNumber_BelongsToPlayerBeingUpdated_ReturnsNoErrorsto prevent the bug from silently returningIValidationContext(the overload the rule-set extension method routes through internally)Closes #424
Test plan
dotnet build --configuration Releasepassesdotnet test --settings .runsettings— all 39 tests pass (32 pre-existing + 1 new regression test + 6 controller tests unblocked)PUT /players/squadNumber/10with"squadNumber": 10returns204 No ContentPOST /playerswith a duplicate squad number still returns400 Bad Requestwith"SquadNumber must be unique."🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes
Tests