Feat: Add basic field validations to domain entities#61
Feat: Add basic field validations to domain entities#61pol-rivero merged 42 commits intoUdL-EPS-SoftArch-Igualada:mainfrom
Conversation
…lidation Domain entities basic field validation
|
Thank you for your PR @xertrec! Now, you should wait for the automated code analysis by CodeRabbit, Copilot and SonarQube. Once you are confident that you have fixed all the detected issues and this PR is ready to be merged, add a comment with exactly one word: |
udl-softarch
bot
added
the
pr-not-ready
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a DomainValidation utility and DomainValidationException; introduces validated public static create(...) factory methods and protected no‑arg constructors across many domain entities; restructures Team and TeamMember (relations and factories); updates services, tests, and step definitions to use the new factories. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/cat/udl/eps/softarch/fll/steps/TeamSteps.java (1)
43-47: 🧹 Nitpick | 🔵 TrivialPrefer full
Team.create(...)to avoid transient partially-initialized teams.Using the single-arg factory and then setters weakens the “validate at creation time” goal. Use the 4-arg factory directly in these flows.
♻️ Proposed refactor
- currentTeam = Team.create(name); - currentTeam.setCity(city); - currentTeam.setFoundationYear(2000); - currentTeam.setCategory("Challenge"); + currentTeam = Team.create(name, city, 2000, "Challenge");- Team team = Team.create(name); - team.setCity(city); - team.setFoundationYear(2000); - team.setCategory("Challenge"); + Team team = Team.create(name, city, 2000, "Challenge");- currentTeam = Team.create(name); - currentTeam.setCity("Igualada"); - currentTeam.setFoundationYear(2000); - currentTeam.setCategory("Challenge"); + currentTeam = Team.create(name, "Igualada", 2000, "Challenge");Also applies to: 78-82, 95-99
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (34)
src/main/java/cat/udl/eps/softarch/fll/domain/Award.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Coach.javasrc/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.javasrc/main/java/cat/udl/eps/softarch/fll/domain/DomainValidation.javasrc/main/java/cat/udl/eps/softarch/fll/domain/DomainValidationException.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Edition.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Floater.javasrc/main/java/cat/udl/eps/softarch/fll/domain/MatchResult.javasrc/main/java/cat/udl/eps/softarch/fll/domain/MediaContent.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Record.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Referee.javasrc/main/java/cat/udl/eps/softarch/fll/domain/ScientificProject.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Team.javasrc/main/java/cat/udl/eps/softarch/fll/domain/TeamMember.javasrc/main/java/cat/udl/eps/softarch/fll/domain/User.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Venue.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Volunteer.javasrc/test/java/cat/udl/eps/softarch/fll/domain/AwardValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/CoachValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/CompetitionTableValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/DomainValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/EditionValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/FloaterValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/MatchResultValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/MediaContentValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/RecordValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/RefereeValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/ScientificProjectValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/TeamMemberValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/TeamValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/UserValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/VenueValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/steps/TeamSteps.javasrc/test/java/cat/udl/eps/softarch/fll/steps/VolunteerStepDefs.java
src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.java
Outdated
Show resolved
Hide resolved
src/test/java/cat/udl/eps/softarch/fll/domain/MediaContentValidationTest.java
Show resolved
Hide resolved
src/test/java/cat/udl/eps/softarch/fll/domain/MediaContentValidationTest.java
Show resolved
Hide resolved
src/test/java/cat/udl/eps/softarch/fll/domain/ScientificProjectValidationTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR implements creation-time domain validations to prevent constructing invalid domain entities, aligning with issue #35’s goal of enforcing domain invariants in the domain layer.
Changes:
- Introduces
DomainValidation+DomainValidationExceptionand uses them from newcreate(...)factory methods across multiple domain entities. - Updates Cucumber step definitions to use the new factories (e.g.,
Team.create(...)) instead of constructors. - Adds unit tests covering valid construction and expected validation failures for multiple domain entities and the validation helpers.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/cat/udl/eps/softarch/fll/steps/VolunteerStepDefs.java | Updates team creation to use Team.create(...) in step defs. |
| src/test/java/cat/udl/eps/softarch/fll/steps/TeamSteps.java | Updates team creation to use Team.create(...) in step defs. |
| src/test/java/cat/udl/eps/softarch/fll/domain/VenueValidationTest.java | Adds validation tests for Venue.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/UserValidationTest.java | Adds validation tests for User.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/TeamValidationTest.java | Adds validation tests for Team.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/TeamMemberValidationTest.java | Adds validation tests for TeamMember.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/ScientificProjectValidationTest.java | Adds validation tests for ScientificProject.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/RefereeValidationTest.java | Adds validation tests for Referee.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/RecordValidationTest.java | Adds validation tests for Record.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/MediaContentValidationTest.java | Adds validation tests for MediaContent.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/MatchResultValidationTest.java | Adds validation tests for MatchResult.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/FloaterValidationTest.java | Adds validation tests for Floater.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/EditionValidationTest.java | Adds validation tests for Edition.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/DomainValidationTest.java | Adds tests for the DomainValidation helper methods. |
| src/test/java/cat/udl/eps/softarch/fll/domain/CompetitionTableValidationTest.java | Adds validation tests for CompetitionTable.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/CoachValidationTest.java | Adds validation tests for Coach.create(...). |
| src/test/java/cat/udl/eps/softarch/fll/domain/AwardValidationTest.java | Adds validation tests for Award.create(...). |
| src/main/java/cat/udl/eps/softarch/fll/domain/Volunteer.java | Centralizes volunteer field validation/initialization for subclasses. |
| src/main/java/cat/udl/eps/softarch/fll/domain/Venue.java | Adds Venue.create(...) with domain validations. |
| src/main/java/cat/udl/eps/softarch/fll/domain/User.java | Adds User.create(...) with domain validations. |
| src/main/java/cat/udl/eps/softarch/fll/domain/TeamMember.java | Adds TeamMember.create(...) with domain validations. |
| src/main/java/cat/udl/eps/softarch/fll/domain/Team.java | Adds Team.create(...) factory overloads with domain validations and refactors mappings placement. |
| src/main/java/cat/udl/eps/softarch/fll/domain/ScientificProject.java | Adds ScientificProject.create(...) with domain validations. |
| src/main/java/cat/udl/eps/softarch/fll/domain/Referee.java | Adds Referee.create(...) leveraging shared volunteer initialization. |
| src/main/java/cat/udl/eps/softarch/fll/domain/Record.java | Adds Record.create(...) with domain validations. |
| src/main/java/cat/udl/eps/softarch/fll/domain/MediaContent.java | Adds MediaContent.create(...) with domain validations. |
| src/main/java/cat/udl/eps/softarch/fll/domain/MatchResult.java | Adds MatchResult.create(...) with domain validations. |
| src/main/java/cat/udl/eps/softarch/fll/domain/Floater.java | Adds Floater.create(...) leveraging shared volunteer initialization. |
| src/main/java/cat/udl/eps/softarch/fll/domain/Edition.java | Adds Edition.create(...) with domain validations. |
| src/main/java/cat/udl/eps/softarch/fll/domain/DomainValidationException.java | Introduces a dedicated exception type for domain validation failures. |
| src/main/java/cat/udl/eps/softarch/fll/domain/DomainValidation.java | Introduces reusable validation helpers (non-blank, email, non-negative, non-null). |
| src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.java | Adds CompetitionTable.create(...) with domain validations. |
| src/main/java/cat/udl/eps/softarch/fll/domain/Coach.java | Adds Coach.create(...) with domain validations. |
| src/main/java/cat/udl/eps/softarch/fll/domain/Award.java | Adds Award.create(...) with domain validations. |
src/test/java/cat/udl/eps/softarch/fll/domain/TeamValidationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/cat/udl/eps/softarch/fll/domain/TeamMemberValidationTest.java
Outdated
Show resolved
Hide resolved
src/main/java/cat/udl/eps/softarch/fll/domain/ScientificProject.java
Outdated
Show resolved
Hide resolved
src/main/java/cat/udl/eps/softarch/fll/domain/DomainValidation.java
Outdated
Show resolved
Hide resolved
src/test/java/cat/udl/eps/softarch/fll/domain/TeamMemberValidationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/cat/udl/eps/softarch/fll/domain/TeamValidationTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
src/main/java/cat/udl/eps/softarch/fll/domain/Venue.java (1)
34-42:⚠️ Potential issue | 🟠 MajorFactory validation is bypassable while implicit public construction remains.
Venue.create(...)validates correctly, but invalid instances can still be created directly via implicitnew Venue()(no explicit protected constructor). This still violates strict creation-time invariants.🔧 Proposed fix
public class Venue { + protected Venue() { + } `@Id` `@GeneratedValue`(strategy = GenerationType.IDENTITY)
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
src/main/java/cat/udl/eps/softarch/fll/domain/Award.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Coach.javasrc/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.javasrc/main/java/cat/udl/eps/softarch/fll/domain/DomainValidation.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Floater.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Record.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Referee.javasrc/main/java/cat/udl/eps/softarch/fll/domain/ScientificProject.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Team.javasrc/main/java/cat/udl/eps/softarch/fll/domain/TeamMember.javasrc/main/java/cat/udl/eps/softarch/fll/domain/User.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Venue.javasrc/test/java/cat/udl/eps/softarch/fll/domain/AwardValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/MatchResultValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/MediaContentValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/TeamMemberValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/TeamValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/domain/VenueValidationTest.javasrc/test/java/cat/udl/eps/softarch/fll/steps/TeamSteps.javasrc/test/java/cat/udl/eps/softarch/fll/steps/VolunteerStepDefs.java
src/main/java/cat/udl/eps/softarch/fll/domain/DomainValidation.java
Outdated
Show resolved
Hide resolved
src/test/java/cat/udl/eps/softarch/fll/domain/TeamMemberValidationTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
src/main/java/cat/udl/eps/softarch/fll/domain/ScientificProject.java (1)
27-33:⚠️ Potential issue | 🔴 CriticalFactory validation can still be bypassed by direct instantiation.
create(...)validates correctly, but the class still has an implicit public no-arg constructor, so invalid objects can be created withnew ScientificProject().🔒 Proposed fix
public class ScientificProject extends UriEntity<Long> { + protected ScientificProject() { + } `@Id`src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.java (1)
27-32:⚠️ Potential issue | 🔴 CriticalCreation invariants are bypassable due implicit public constructor.
create(id)enforces non-blank id, but callers can still bypass it usingnew CompetitionTable().🔒 Proposed fix
public class CompetitionTable extends UriEntity<String> { + protected CompetitionTable() { + } `@Id`src/main/java/cat/udl/eps/softarch/fll/domain/Coach.java (1)
41-49:⚠️ Potential issue | 🔴 CriticalFactory guarantees are bypassable because constructor remains public by default.
Coach.create(...)validates correctly, butnew Coach()still allows invalid instances before persistence-time validation.🔒 Proposed fix
public class Coach extends UriEntity<Integer> { + protected Coach() { + } `@Id`src/main/java/cat/udl/eps/softarch/fll/domain/Floater.java (1)
30-37:⚠️ Potential issue | 🔴 CriticalFactory validations can still be bypassed through direct construction.
Floater.create(...)is validated, but an implicit public no-arg constructor still permits unvalidatednew Floater()instances.🔒 Proposed fix
public class Floater extends Volunteer { + protected Floater() { + } `@NotBlank`(message = "Student code is mandatory")src/main/java/cat/udl/eps/softarch/fll/domain/User.java (1)
41-51:⚠️ Potential issue | 🔴 CriticalValidated factory remains bypassable due implicit public constructor.
Even with
create(...), callers can still instantiateUserdirectly withnew User(), violating creation-time invariant enforcement.🔒 Proposed fix
public class User extends UriEntity<String> implements UserDetails { + protected User() { + } public static final PasswordEncoder passwordEncoder = new BCryptPasswordEncoder();src/main/java/cat/udl/eps/softarch/fll/domain/Record.java (1)
39-45:⚠️ Potential issue | 🔴 CriticalProtected constructor required to enforce validated object creation.
Record.create(...)validatesname, but Lombok's@Dataannotation generates a public no-arg constructor that bypasses validation. Direct instantiation likenew Record()is currently possible (as evidenced inDBInitialization.java), allowing entities to violate the@NotBlankconstraint on thenamefield.🔒 Proposed fix
public class Record extends UriEntity<Long> { + protected Record() { + } `@Id`
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
src/main/java/cat/udl/eps/softarch/fll/domain/Coach.javasrc/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.javasrc/main/java/cat/udl/eps/softarch/fll/domain/DomainValidation.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Floater.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Record.javasrc/main/java/cat/udl/eps/softarch/fll/domain/ScientificProject.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Team.javasrc/main/java/cat/udl/eps/softarch/fll/domain/TeamMember.javasrc/main/java/cat/udl/eps/softarch/fll/domain/User.javasrc/test/java/cat/udl/eps/softarch/fll/domain/TeamMemberValidationTest.java
| package cat.udl.eps.softarch.fll.domain; | ||
|
|
||
| public class DomainValidationException extends RuntimeException { | ||
|
|
||
| public DomainValidationException(String message) { | ||
| super(message); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is already a cat.udl.eps.softarch.fll.exception.DomainValidationException (with an error code) and a DomainValidationExceptionHandler that catches that type. Introducing a second DomainValidationException in the domain package means exceptions thrown by DomainValidation won’t be handled consistently (and may surface as 500s) and creates ambiguous imports. Prefer using a single exception type (reuse the existing one, or update the handler + callers to use this one) and keep a structured error code as required by #35.
| private DomainValidation() { | ||
| } | ||
|
|
||
| public static void requireNonBlank(String value, String fieldName) { | ||
| if (value == null) { | ||
| throw new DomainValidationException(fieldName + " must not be null"); | ||
| } | ||
| if (value.isBlank()) { | ||
| throw new DomainValidationException(fieldName + " must not be blank"); | ||
| } | ||
| } | ||
|
|
||
| public static void requireValidEmail(String email, String fieldName) { | ||
| requireNonBlank(email, fieldName); | ||
| if (!EMAIL_PATTERN.matcher(email).matches()) { | ||
| throw new DomainValidationException(fieldName + " must be a valid email address"); | ||
| } | ||
| } | ||
|
|
||
| public static void requireNonNegative(Integer value, String fieldName) { | ||
| if (value == null) { | ||
| throw new DomainValidationException(fieldName + " must not be null"); | ||
| } | ||
| if (value < 0) { | ||
| throw new DomainValidationException(fieldName + " must not be negative"); | ||
| } | ||
| } | ||
|
|
||
| public static void requireMin(Integer value, Integer minValue, String fieldName) { | ||
| if (value == null) { | ||
| throw new DomainValidationException(fieldName + " must not be null"); | ||
| } | ||
| if (value < minValue) { | ||
| throw new DomainValidationException(fieldName + " must not be less than " + minValue); | ||
| } | ||
| } | ||
|
|
||
| public static void requirePast(LocalDate value, String fieldName) { | ||
| if (value == null) { | ||
| throw new DomainValidationException(fieldName + " must not be null"); | ||
| } | ||
| if (!value.isBefore(LocalDate.now())) { | ||
| throw new DomainValidationException(fieldName + " must be in the past"); | ||
| } | ||
| } | ||
|
|
||
| public static void requireLengthBetween(String value, Integer minLength, Integer maxLength, String fieldName) { | ||
| if (value == null) { | ||
| throw new DomainValidationException(fieldName + " must not be null"); | ||
| } | ||
| if (value.length() < minLength) { | ||
| throw new DomainValidationException(fieldName + " length must not be less than " + minLength); | ||
| } | ||
| if (value.length() > maxLength) { | ||
| throw new DomainValidationException(fieldName + " length must not be more than " + maxLength); | ||
| } | ||
| } | ||
|
|
||
| public static void requireMaxLength(String value, Integer maxLength, String fieldName) { | ||
| if (value == null) { | ||
| throw new DomainValidationException(fieldName + " must not be null"); | ||
| } | ||
| if (value.length() > maxLength) { | ||
| throw new DomainValidationException(fieldName + " length must not be more than " + maxLength); | ||
| } | ||
| } | ||
|
|
||
| public static void requireNonNull(Object value, String fieldName) { | ||
| if (value == null) { | ||
| throw new DomainValidationException(fieldName + " must not be null"); |
There was a problem hiding this comment.
DomainValidation currently throws a message-only DomainValidationException (e.g., "name must not be blank"). Issue #35 asks for specific validation error categories (e.g., EMPTY_NAME, NULL_ID, NEGATIVE_SCORE), and the codebase already has a coded cat.udl.eps.softarch.fll.exception.DomainValidationException. Consider throwing a coded exception (or adding a code field here) so callers/controllers can reliably distinguish validation failures without parsing messages.
| private DomainValidation() { | |
| } | |
| public static void requireNonBlank(String value, String fieldName) { | |
| if (value == null) { | |
| throw new DomainValidationException(fieldName + " must not be null"); | |
| } | |
| if (value.isBlank()) { | |
| throw new DomainValidationException(fieldName + " must not be blank"); | |
| } | |
| } | |
| public static void requireValidEmail(String email, String fieldName) { | |
| requireNonBlank(email, fieldName); | |
| if (!EMAIL_PATTERN.matcher(email).matches()) { | |
| throw new DomainValidationException(fieldName + " must be a valid email address"); | |
| } | |
| } | |
| public static void requireNonNegative(Integer value, String fieldName) { | |
| if (value == null) { | |
| throw new DomainValidationException(fieldName + " must not be null"); | |
| } | |
| if (value < 0) { | |
| throw new DomainValidationException(fieldName + " must not be negative"); | |
| } | |
| } | |
| public static void requireMin(Integer value, Integer minValue, String fieldName) { | |
| if (value == null) { | |
| throw new DomainValidationException(fieldName + " must not be null"); | |
| } | |
| if (value < minValue) { | |
| throw new DomainValidationException(fieldName + " must not be less than " + minValue); | |
| } | |
| } | |
| public static void requirePast(LocalDate value, String fieldName) { | |
| if (value == null) { | |
| throw new DomainValidationException(fieldName + " must not be null"); | |
| } | |
| if (!value.isBefore(LocalDate.now())) { | |
| throw new DomainValidationException(fieldName + " must be in the past"); | |
| } | |
| } | |
| public static void requireLengthBetween(String value, Integer minLength, Integer maxLength, String fieldName) { | |
| if (value == null) { | |
| throw new DomainValidationException(fieldName + " must not be null"); | |
| } | |
| if (value.length() < minLength) { | |
| throw new DomainValidationException(fieldName + " length must not be less than " + minLength); | |
| } | |
| if (value.length() > maxLength) { | |
| throw new DomainValidationException(fieldName + " length must not be more than " + maxLength); | |
| } | |
| } | |
| public static void requireMaxLength(String value, Integer maxLength, String fieldName) { | |
| if (value == null) { | |
| throw new DomainValidationException(fieldName + " must not be null"); | |
| } | |
| if (value.length() > maxLength) { | |
| throw new DomainValidationException(fieldName + " length must not be more than " + maxLength); | |
| } | |
| } | |
| public static void requireNonNull(Object value, String fieldName) { | |
| if (value == null) { | |
| throw new DomainValidationException(fieldName + " must not be null"); | |
| private static final String ERROR_CODE_NULL_VALUE = "NULL_VALUE"; | |
| private static final String ERROR_CODE_INVALID_EMAIL = "INVALID_EMAIL"; | |
| private static final String ERROR_CODE_NEGATIVE_VALUE = "NEGATIVE_VALUE"; | |
| private static final String ERROR_CODE_BELOW_MIN_VALUE = "BELOW_MIN_VALUE"; | |
| private static final String ERROR_CODE_VALUE_NOT_IN_PAST = "VALUE_NOT_IN_PAST"; | |
| private static final String ERROR_CODE_STRING_TOO_SHORT = "STRING_TOO_SHORT"; | |
| private static final String ERROR_CODE_STRING_TOO_LONG = "STRING_TOO_LONG"; | |
| private static final String ERROR_CODE_EXCEEDS_MAX_LENGTH = "EXCEEDS_MAX_LENGTH"; | |
| private static final String ERROR_CODE_NULL_OBJECT = "NULL_OBJECT"; | |
| private DomainValidation() { | |
| } | |
| private static void throwValidationException(String errorCode, String message) { | |
| throw new DomainValidationException("[" + errorCode + "] " + message); | |
| } | |
| public static void requireNonBlank(String value, String fieldName) { | |
| if (value == null) { | |
| throwValidationException(ERROR_CODE_NULL_VALUE, fieldName + " must not be null"); | |
| } | |
| if (value.isBlank()) { | |
| throwValidationException(ERROR_CODE_STRING_TOO_SHORT, fieldName + " must not be blank"); | |
| } | |
| } | |
| public static void requireValidEmail(String email, String fieldName) { | |
| requireNonBlank(email, fieldName); | |
| if (!EMAIL_PATTERN.matcher(email).matches()) { | |
| throwValidationException(ERROR_CODE_INVALID_EMAIL, fieldName + " must be a valid email address"); | |
| } | |
| } | |
| public static void requireNonNegative(Integer value, String fieldName) { | |
| if (value == null) { | |
| throwValidationException(ERROR_CODE_NULL_VALUE, fieldName + " must not be null"); | |
| } | |
| if (value < 0) { | |
| throwValidationException(ERROR_CODE_NEGATIVE_VALUE, fieldName + " must not be negative"); | |
| } | |
| } | |
| public static void requireMin(Integer value, Integer minValue, String fieldName) { | |
| if (value == null) { | |
| throwValidationException(ERROR_CODE_NULL_VALUE, fieldName + " must not be null"); | |
| } | |
| if (value < minValue) { | |
| throwValidationException(ERROR_CODE_BELOW_MIN_VALUE, fieldName + " must not be less than " + minValue); | |
| } | |
| } | |
| public static void requirePast(LocalDate value, String fieldName) { | |
| if (value == null) { | |
| throwValidationException(ERROR_CODE_NULL_VALUE, fieldName + " must not be null"); | |
| } | |
| if (!value.isBefore(LocalDate.now())) { | |
| throwValidationException(ERROR_CODE_VALUE_NOT_IN_PAST, fieldName + " must be in the past"); | |
| } | |
| } | |
| public static void requireLengthBetween(String value, Integer minLength, Integer maxLength, String fieldName) { | |
| if (value == null) { | |
| throwValidationException(ERROR_CODE_NULL_VALUE, fieldName + " must not be null"); | |
| } | |
| if (value.length() < minLength) { | |
| throwValidationException(ERROR_CODE_STRING_TOO_SHORT, fieldName + " length must not be less than " + minLength); | |
| } | |
| if (value.length() > maxLength) { | |
| throwValidationException(ERROR_CODE_STRING_TOO_LONG, fieldName + " length must not be more than " + maxLength); | |
| } | |
| } | |
| public static void requireMaxLength(String value, Integer maxLength, String fieldName) { | |
| if (value == null) { | |
| throwValidationException(ERROR_CODE_NULL_VALUE, fieldName + " must not be null"); | |
| } | |
| if (value.length() > maxLength) { | |
| throwValidationException(ERROR_CODE_EXCEEDS_MAX_LENGTH, fieldName + " length must not be more than " + maxLength); | |
| } | |
| } | |
| public static void requireNonNull(Object value, String fieldName) { | |
| if (value == null) { | |
| throwValidationException(ERROR_CODE_NULL_OBJECT, fieldName + " must not be null"); |
| @ManyToMany | ||
| @JoinTable( | ||
| name = "team_coach", | ||
| joinColumns = @JoinColumn(name = "team_name"), | ||
| inverseJoinColumns = @JoinColumn(name = "coach_id")) | ||
| @ToString.Exclude | ||
| @Setter(AccessLevel.NONE) | ||
| private Set<Coach> trainedBy = new HashSet<>(); |
There was a problem hiding this comment.
trainedBy is declared twice in Team (also appears again later in the file), which will cause a compilation error and duplicate JPA mappings. Remove one of the declarations and keep a single @ManyToMany mapping for coaches (including the intended @Setter(AccessLevel.NONE) if you want to prevent external mutation).
| @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}) | ||
| @JoinTable( | ||
| name = "team_floaters", | ||
| joinColumns = @JoinColumn(name = "team_name"), | ||
| inverseJoinColumns = @JoinColumn(name = "floater_id")) | ||
| @ToString.Exclude | ||
| @Setter(AccessLevel.NONE) | ||
| private Set<Floater> floaters = new HashSet<>(); |
There was a problem hiding this comment.
floaters is also declared twice in Team (it’s redefined again later with the same @JoinTable), which will not compile and creates ambiguous ORM mappings. Keep a single floaters field/mapping and delete the duplicate definition.
| public static ScientificProject create(Integer score) { | ||
| DomainValidation.requireNonNegative(score, "score"); | ||
|
|
||
| ScientificProject project = new ScientificProject(); | ||
| project.score = score; | ||
| return project; | ||
| } |
There was a problem hiding this comment.
ScientificProject.create(Integer score) returns an instance with team and edition left null, even though both associations are annotated @NotNull/nullable=false. This allows creation of an invalid domain entity and contradicts the invariant-at-creation goal; consider requiring Team team and Edition edition in the factory (or otherwise ensuring these required references are set/validated before returning).
| public static Coach create(String name, String emailAddress) { | ||
| DomainValidation.requireNonBlank(name, "name"); | ||
| DomainValidation.requireValidEmail(emailAddress, "emailAddress"); | ||
|
|
||
| Coach coach = new Coach(); | ||
| coach.name = name; | ||
| coach.emailAddress = emailAddress; | ||
| return coach; | ||
| } |
There was a problem hiding this comment.
Coach.create(...) adds validation, but the class still has an implicit public no-arg constructor plus Lombok-generated setters (@Data), so callers can still instantiate new Coach() with null/blank required fields and bypass domain invariants. If the goal is to prevent invalid entities from being created, consider making the no-arg constructor protected (for JPA only) and validating required setters (or removing the setters for required fields).
| public static Award create(String name, Edition edition, Team winner) { | ||
| DomainValidation.requireNonBlank(name, "name"); | ||
| DomainValidation.requireNonNull(edition, "edition"); | ||
| DomainValidation.requireNonNull(winner, "winner"); | ||
|
|
||
| Award award = new Award(); | ||
| award.name = name; | ||
| award.edition = edition; | ||
| award.winner = winner; | ||
| return award; |
There was a problem hiding this comment.
Award.create(...) validates inputs, but Award still has a public no-arg constructor and Lombok-generated setters via @Data, so invalid awards (blank name, null edition/winner) can still be instantiated and mutated directly, bypassing the new validation. To fully enforce the invariant at creation time, consider making the no-arg constructor protected and validating/removing setters for mandatory fields.
| public static CompetitionTable create(String id) { | ||
| DomainValidation.requireNonBlank(id, "id"); | ||
| CompetitionTable table = new CompetitionTable(); | ||
| table.id = id; | ||
| return table; |
There was a problem hiding this comment.
Adding CompetitionTable.create(...) introduces validation, but CompetitionTable still has a public no-arg constructor and an unvalidated setId(...) method, so new CompetitionTable() / setId(null|blank) can still create an invalid entity. To align with the domain-invariant goal, consider making the constructor protected and validating/removing setId (or routing it through DomainValidation.requireNonBlank).
| Judge judge = Judge.create("Test Judge " + judgeAlias, "judge_" + Random.from(RandomGenerator.getDefault()).nextFloat() + "@test.com", "123456789"); | ||
| judge = judgeRepository.save(judge); |
There was a problem hiding this comment.
The email generation using Random.from(RandomGenerator.getDefault()).nextFloat() is relatively low-entropy and can collide across runs (and makes failures hard to reproduce), which can lead to flaky tests when emailAddress has a uniqueness constraint. Prefer using a UUID (as before) or System.nanoTime() to guarantee uniqueness.
|
ready |
udl-softarch
bot
removed
the
pr-not-ready
|
This PR is now marked as ready to be merged. |
|
@xertrec Please fix the merge conflict |
# Conflicts: # src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.java # src/main/java/cat/udl/eps/softarch/fll/domain/Edition.java # src/main/java/cat/udl/eps/softarch/fll/domain/Match.java # src/main/java/cat/udl/eps/softarch/fll/domain/Referee.java # src/main/java/cat/udl/eps/softarch/fll/domain/Round.java # src/main/java/cat/udl/eps/softarch/fll/service/MatchScoreRegistrationService.java # src/test/java/cat/udl/eps/softarch/fll/controller/MatchAssignmentControllerTest.java # src/test/java/cat/udl/eps/softarch/fll/service/EditionVolunteerServiceTest.java # src/test/java/cat/udl/eps/softarch/fll/steps/ManageVenueStepDefs.java # src/test/java/cat/udl/eps/softarch/fll/steps/MatchScoreRegistrationStepDefs.java # src/test/java/cat/udl/eps/softarch/fll/steps/ProjectRoomSteps.java # src/test/java/cat/udl/eps/softarch/fll/steps/TestCompetitionTable.java # src/test/java/cat/udl/eps/softarch/fll/steps/TestReferee.java
|
|
LGTM |
Closes #35