[Fix] Improve match, round, referee, CompetitioTable entity adding lambok notation#71
[Fix] Improve match, round, referee, CompetitioTable entity adding lambok notation#71xji650 wants to merge 23 commits intoUdL-EPS-SoftArch-Igualada:mainfrom
Conversation
…plate code reduction
…r and match handling
[Fix]:Refactor CompetitionTable and Round classes to improvemets
udl-softarch
bot
added
the
pr-not-ready
|
Thank you for your PR @xji650! 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: |
|
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:
📝 WalkthroughWalkthroughReplaces explicit constructors/getters/setters with Lombok on four domain entities and adds/rewrites association-management methods for matches and referees; adjusts cascade on CompetitionTable.matches and prevents public setters for collection fields. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/cat/udl/eps/softarch/fll/domain/Round.java (1)
54-57:⚠️ Potential issue | 🟡 MinorInconsistent
removeMatchimplementation – potential null reference issue.Unlike
CompetitionTable.removeMatchwhich guards the null-setter call withif (matches.remove(match)), this implementation unconditionally callsmatch.setRound(null)even if:
matchisnull→ NPEmatchwasn't in the list → incorrectly clears its round reference🐛 Proposed fix for consistency and safety
public void removeMatch(Match match) { - matches.remove(match); - match.setRound(null); + if (match != null && matches.remove(match)) { + match.setRound(null); + } }
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Match.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Referee.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Round.java
There was a problem hiding this comment.
Pull request overview
This PR refactors several domain entities (Match, Round, Referee, CompetitionTable) to reduce boilerplate via Lombok and to adjust bidirectional relationship handling within the domain model.
Changes:
- Added Lombok annotations (
@Getter,@Setter,@NoArgsConstructor,@EqualsAndHashCode) to multiple entities and removed manual getters/setters/constructors. - Updated collection association methods (e.g.,
addMatch,removeMatch,setReferees) with additional guards and relationship updates. - Changed
CompetitionTable.matchesmapping to useorphanRemoval = true.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/main/java/cat/udl/eps/softarch/fll/domain/Round.java |
Lombok adoption and updated addMatch behavior for round–match association. |
src/main/java/cat/udl/eps/softarch/fll/domain/Referee.java |
Lombok adoption for referee entity boilerplate. |
src/main/java/cat/udl/eps/softarch/fll/domain/Match.java |
Lombok adoption plus new equals/hashCode based on id. |
src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.java |
Lombok adoption and significant changes to match/referee association management, including orphanRemoval. |
| public void addMatch(Match match) { | ||
| matches.add(match); | ||
| match.setRound(this); | ||
| if (match != null && !matches.contains(match)) { | ||
| matches.add(match); | ||
| match.setRound(this); | ||
| } |
There was a problem hiding this comment.
Round.addMatch uses !matches.contains(match), but Match now has Lombok equals/hashCode based only on id. For transient Match instances (where id is null), different matches become equal, so only the first one gets added (this breaks the existing Cucumber test that adds multiple new Match() to a round). Consider either removing the contains guard, or changing the duplicate check to use reference equality when id is null, or updating Match.equals to never treat two null ids as equal.
| @Getter | ||
| @Setter | ||
| @NoArgsConstructor | ||
| @EqualsAndHashCode(callSuper = false, onlyExplicitlyIncluded = true) | ||
| public class Match extends UriEntity<Long> { | ||
|
|
||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.IDENTITY) | ||
| @EqualsAndHashCode.Include | ||
| private Long id; |
There was a problem hiding this comment.
@EqualsAndHashCode(onlyExplicitlyIncluded = true) with only id included makes two transient Match objects (both with id == null) compare equal. This breaks collection operations like List.contains and can prevent adding multiple new matches. Consider implementing equals/hashCode so that entities with null ids are only equal to themselves (reference equality), or avoid using id-only equality until persistence assigns an id.
| if (match != null && !matches.contains(match)) { | ||
| matches.add(match); |
There was a problem hiding this comment.
CompetitionTable.addMatch no longer handles the case where the Match is already associated with a different table. Setting match.setCompetitionTable(this) without removing it from the previous table’s matches collection can leave both tables referencing the same match in-memory and lead to inconsistent persistence state. Consider restoring the "move" logic (remove from previous table before adding) to keep the bidirectional association consistent.
| if (match != null && !matches.contains(match)) { | |
| matches.add(match); | |
| if (match == null) { | |
| return; | |
| } | |
| CompetitionTable previousTable = match.getCompetitionTable(); | |
| if (previousTable != null && previousTable != this) { | |
| previousTable.removeMatch(match); | |
| } | |
| if (!matches.contains(match)) { | |
| matches.add(match); | |
| } | |
| if (match.getCompetitionTable() != this) { |
| public void setReferees(List<Referee> referees) { | ||
| this.referees.clear(); | ||
| if (referees != null) { | ||
| referees.forEach(this::addReferee); | ||
| } | ||
| if (referees.contains(referee)) { | ||
| } |
There was a problem hiding this comment.
CompetitionTable.setReferees clears the collection without calling removeReferee (or otherwise nulling Referee.supervisesTable) for previously associated referees. Since Referee owns the relationship (ManyToOne supervisesTable), this can leave old referees still pointing at this table, producing an inconsistent bidirectional association. Consider iterating existing referees and removing them via removeReferee before re-adding the new list.
…and referee handling logic
| @EqualsAndHashCode.Include | ||
| private LocalTime startTime; | ||
|
|
||
| private LocalTime endTime; | ||
|
|
||
| @EqualsAndHashCode.Include | ||
| @JsonBackReference("round-matches") | ||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "round_id") | ||
| private Round round; | ||
|
|
||
| @EqualsAndHashCode.Include | ||
| @JsonBackReference("table-matches") | ||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "table_id") | ||
| private CompetitionTable competitionTable; |
There was a problem hiding this comment.
@EqualsAndHashCode.Include on round / competitionTable (both LAZY @ManyToOne) makes Match.equals/hashCode depend on associations, which can trigger lazy-loading, makes equality mutable when a match is moved between rounds/tables, and interacts badly with List.contains checks in addMatch. Align with the other entities in this module by basing equality on a stable identifier (typically id only) and avoid including entity relationships.
| @OneToMany(mappedBy = "competitionTable", cascade = CascadeType.ALL, orphanRemoval = true) | ||
| @JsonManagedReference("table-matches") | ||
| @Setter(lombok.AccessLevel.NONE) | ||
| private List<Match> matches = new ArrayList<>(); | ||
|
|
There was a problem hiding this comment.
orphanRemoval = true was added to CompetitionTable.matches. Since Match is also associated to Round, this means removing a match from a table (via removeMatch/collection updates) will mark it for deletion even if it is still linked to a round. Please confirm that “removing from a table” should delete the Match; otherwise drop orphanRemoval and just null/update the owning side.
| public void addMatch(Match match) { | ||
| if (match == null || matches.contains(match)) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| public void addMatch(Match match) { | ||
| CompetitionTable previousTable = match.getCompetitionTable(); | ||
| if (previousTable != null && previousTable != this) { | ||
| previousTable.removeMatch(match); | ||
| } | ||
| if (!matches.contains(match)) { | ||
| matches.add(match); | ||
| previousTable.getMatches().remove(match); | ||
| } | ||
|
|
||
| matches.add(match); | ||
| match.setCompetitionTable(this); | ||
| } |
There was a problem hiding this comment.
There are Cucumber tests for addReferee/addMatch, but the changed semantics around moving/removing associations (especially setMatches/setReferees and the new duplicate/relocation logic) aren’t covered. Please add scenarios that verify removed matches/referees have their owning-side references cleared and that moving a match/referee between tables/rounds keeps both sides consistent.
…r and improve relationship management
| } | ||
| if (!matches.contains(match)) { | ||
| matches.add(match); | ||
| previousTable.getMatches().remove(match); |
There was a problem hiding this comment.
In addMatch, removing from previousTable via previousTable.getMatches().remove(match) bypasses previousTable.removeMatch(...), so the bidirectional invariants are not consistently maintained in one place. Use the existing removeMatch helper on the previous table to ensure Match.competitionTable is updated consistently (and to avoid duplicating relationship-maintenance logic).
| previousTable.getMatches().remove(match); | |
| previousTable.removeMatch(match); |
|
|
||
| CompetitionTable previousTable = referee.getSupervisesTable(); | ||
| if (previousTable != null && previousTable != this) { | ||
| previousTable.getReferees().remove(referee); |
There was a problem hiding this comment.
In addReferee, removing the referee from previousTable via previousTable.getReferees().remove(referee) bypasses previousTable.removeReferee(...) and duplicates relationship-management logic. Calling removeReferee on the previous table keeps bidirectional invariants centralized and avoids future inconsistencies if removeReferee changes.
| previousTable.getReferees().remove(referee); | |
| previousTable.removeReferee(referee); |
| @@ -58,12 +45,22 @@ public void setMatches(List<Match> matches) { | |||
| } | |||
There was a problem hiding this comment.
setMatches clears the inverse collection without nulling the owning side (Match.round). With mappedBy = "round", this can leave removed matches still referencing this Round, so setMatches won't reliably replace the association. Prefer removing existing matches via removeMatch (or explicitly match.setRound(null)) before adding the new ones.
|
|
||
| Round previousRound = match.getRound(); | ||
| if (previousRound != null && previousRound != this) { | ||
| previousRound.getMatches().remove(match); |
There was a problem hiding this comment.
In addMatch, removing from previousRound via previousRound.getMatches().remove(match) bypasses previousRound.removeMatch(...), duplicating relationship-management logic and making it easier to get the bidirectional association out of sync over time. Prefer calling previousRound.removeMatch(match) to keep invariants centralized.
| previousRound.getMatches().remove(match); | |
| previousRound.removeMatch(match); |
| public void addMatch(Match match) { | ||
| if (match == null || matches.contains(match)) { | ||
| return; | ||
| } | ||
|
|
||
| Round previousRound = match.getRound(); | ||
| if (previousRound != null && previousRound != this) { | ||
| previousRound.getMatches().remove(match); | ||
| } | ||
|
|
||
| matches.add(match); | ||
| match.setRound(this); | ||
| } |
There was a problem hiding this comment.
Round.addMatch now includes logic to ignore duplicates/nulls and to detach a Match from its previous Round, but there are no Cucumber scenarios validating these behaviors (especially moving a match between rounds and ensuring the previous round no longer lists it). Please add scenarios to cover these invariants.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/cat/udl/eps/softarch/fll/domain/Match.java (1)
23-29:⚠️ Potential issue | 🟠 Major
id-only Lombok equality can collapse distinct unsavedMatchinstances.With Line 23 + Line 28, transient entities (
id == null) may compare equal. This can make Line 48 insrc/main/java/cat/udl/eps/softarch/fll/domain/Round.javaand Line 48 insrc/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.javaincorrectly treat different new matches as duplicates or remove the wrong element.#!/bin/bash set -euo pipefail echo "Check Match equality configuration:" rg -nP --type=java '@EqualsAndHashCode\(callSuper\s*=\s*false,\s*onlyExplicitlyIncluded\s*=\s*true\)|@EqualsAndHashCode\.Include|private\s+Long\s+id' \ src/main/java/cat/udl/eps/softarch/fll/domain/Match.java echo echo "Check association helpers relying on equals/remove:" rg -nP --type=java '\bmatches\.(contains|remove)\(' \ src/main/java/cat/udl/eps/softarch/fll/domain/Round.java \ src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.javaSuggested fix (entity-safe equality)
-@EqualsAndHashCode(callSuper = false, onlyExplicitlyIncluded = true) public class Match extends UriEntity<Long> { @@ - `@EqualsAndHashCode.Include` private Long id; + + `@Override` + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof Match other)) return false; + return id != null && id.equals(other.id); + } + + `@Override` + public int hashCode() { + return getClass().hashCode(); + }
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Match.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Round.java
…ree management logic
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/cat/udl/eps/softarch/fll/domain/Round.java (1)
40-45:⚠️ Potential issue | 🟠 Major
setMatchesbreaks bidirectional consistency when clearing existing matches.Using
this.matches.clear()removes matches from the collection without unlinking theirroundreference back tonull. The same issue was identified and fixed inCompetitionTable.setMatches(which now usesnew ArrayList<>(this.matches).forEach(this::removeMatch)).🔧 Proposed fix
public void setMatches(List<Match> matches) { - this.matches.clear(); + new ArrayList<>(this.matches).forEach(this::removeMatch); if (matches != null) { matches.forEach(this::addMatch); } }
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Round.java
Refactor match handling in Round class to simplify match addition and…
| @@ -58,12 +45,30 @@ public void setMatches(List<Match> matches) { | |||
| } | |||
There was a problem hiding this comment.
setMatches clears the inverse collection with this.matches.clear() but does not update the owning side (Match.round) for matches that were previously in the round. In JPA, this can leave removed Match rows still pointing at this Round (and with orphanRemoval = true it also won’t trigger deletes unless the owning side is updated). Prefer removing existing matches via removeMatch (similar to CompetitionTable#setMatches) so bidirectional consistency is maintained.
…h referee and teams
Refactor Match class to enhance state and relationship management wit…
| public void addReferee(Referee referee) { | ||
| if (referee == null || referees.contains(referee)) { |
There was a problem hiding this comment.
addReferee uses referees.contains(referee) to prevent duplicates, but Referee inherits equals/hashCode from Volunteer (based on id). For new referees with id == null, different instances compare equal, so you won't be able to add more than one transient referee to a table. Consider changing the duplicate check to use reference identity or to compare IDs only when non-null.
| public void addReferee(Referee referee) { | |
| if (referee == null || referees.contains(referee)) { | |
| private boolean hasRefereeAssigned(Referee referee) { | |
| return referees.stream().anyMatch(existingReferee -> isSameReferee(existingReferee, referee)); | |
| } | |
| private boolean isSameReferee(Referee existingReferee, Referee newReferee) { | |
| if (existingReferee == newReferee) { | |
| return true; | |
| } | |
| if (existingReferee == null || newReferee == null) { | |
| return false; | |
| } | |
| if (existingReferee.getId() == null || newReferee.getId() == null) { | |
| return false; | |
| } | |
| return existingReferee.getId().equals(newReferee.getId()); | |
| } | |
| public void addReferee(Referee referee) { | |
| if (referee == null || hasRefereeAssigned(referee)) { |
| @Enumerated(EnumType.STRING) | ||
| private MatchState state; |
There was a problem hiding this comment.
state is declared as String but annotated with @Enumerated(EnumType.STRING), which is only valid for enum-typed fields. This also breaks existing code/tests that expect MatchState (e.g., MatchAssignmentService compares match.getState() to MatchState.FINISHED). Change the field type back to MatchState (and consider restoring a sensible default like SCHEDULED).
| @EqualsAndHashCode(callSuper = false, onlyExplicitlyIncluded = true) | ||
| public class Match extends UriEntity<Long> { | ||
|
|
||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.IDENTITY) |
There was a problem hiding this comment.
@EqualsAndHashCode(onlyExplicitlyIncluded = true) with only the generated id included means two transient Match instances (both with id == null) will compare equal and share the same hash code. That can break Set/List.contains behavior and forces identity (==) workarounds elsewhere. Consider a custom equals/hashCode that uses id only when non-null and otherwise falls back to reference equality.
| this.matches.add(match); | ||
| match.setCompetitionTable(this); | ||
| } |
There was a problem hiding this comment.
addMatch now supports transferring a Match from its previous CompetitionTable. There are no Cucumber scenarios covering moving a match between tables (and verifying the previous table no longer contains it), so regressions could slip in. Please add a test scenario for that transfer behavior.
| @EqualsAndHashCode(callSuper = false, onlyExplicitlyIncluded = true) | ||
| public class Round extends UriEntity<Long> { | ||
|
|
||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.IDENTITY) |
There was a problem hiding this comment.
@EqualsAndHashCode(onlyExplicitlyIncluded = true) with only id included means two transient Round instances (both with id == null) will compare equal and have the same hash code. That can break collection semantics and is likely why addMatch/removeMatch needed identity (==) checks. Consider implementing equals/hashCode so it uses id only when non-null and otherwise falls back to reference equality, or base equality on a stable business key (e.g., number, which is unique).
| Round previousRound = match.getRound(); | ||
| if (previousRound != null && previousRound != this) { | ||
| previousRound.getMatches().removeIf(m -> m == match); | ||
| } |
There was a problem hiding this comment.
addMatch now removes a Match from its previous Round before adding it to this one. There are no Cucumber scenarios covering this transfer case (and ensuring the previous round no longer contains the match), so it’s easy to regress. Please add a test scenario for moving a match between rounds.
| if (this.referees.stream().anyMatch(r -> r == referee)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
addReferee checks duplicates using identity (r == referee). With JPA proxies/detached instances this can allow duplicates for the same underlying row. Prefer comparing by a stable key (e.g., non-null id) and falling back to reference equality only when the key is null.
| if (previousTable != null && previousTable != this) { | ||
| previousTable.getReferees().removeIf(r -> r == referee); | ||
| } |
There was a problem hiding this comment.
Removing from previousTable via previousTable.getReferees().removeIf(r -> r == referee) relies on reference equality and can fail when the same referee is represented by a different instance (detached/proxy). Prefer removal by stable key (e.g., non-null id) and only fall back to reference equality for transient entities.
| this.referees.add(referee); | ||
| referee.setSupervisesTable(this); | ||
| } |
There was a problem hiding this comment.
addReferee now supports transferring a referee from a previous table. There are no Cucumber tests covering moving a referee between tables and asserting the previous table no longer lists the referee. Please add a scenario for that transfer behavior.
| // USA IDENTIDAD (==) para que los tests con IDs nulos no fallen | ||
| if (this.matches.stream().anyMatch(m -> m == match)) { |
There was a problem hiding this comment.
The inline comment // USA IDENTIDAD (==) para que los tests con IDs nulos no fallen is not written in English. Please rewrite it in English (or remove it if it’s only documenting a workaround) to follow the project guideline that code/comments must be in English.
| if (this.matches.stream().anyMatch(m -> m == match)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
addMatch uses identity comparison (m == match) to detect duplicates. This is brittle with JPA/Hibernate because the same database row can be represented by different object instances (detached entities, proxies), so duplicates may not be detected. Prefer comparing by a stable key (e.g., non-null id) and falling back to reference equality only when the key is null.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.java (1)
56-59: 🧹 Nitpick | 🔵 TrivialUse
removeMatch/removeRefereehelpers when rehoming entities.Line 58 and Line 97 directly mutate
previousTablecollections. This bypasses centralized unlink behavior and can drift over time. CallpreviousTable.removeMatch(match)/previousTable.removeReferee(referee)instead.♻️ Refactor sketch
CompetitionTable previousTable = match.getCompetitionTable(); if (previousTable != null && previousTable != this) { - previousTable.getMatches().removeIf(m -> m == match); + previousTable.removeMatch(match); } @@ CompetitionTable previousTable = referee.getSupervisesTable(); if (previousTable != null && previousTable != this) { - previousTable.getReferees().removeIf(r -> r == referee); + previousTable.removeReferee(referee); }Also applies to: 95-98
src/main/java/cat/udl/eps/softarch/fll/domain/Round.java (1)
57-60: 🧹 Nitpick | 🔵 TrivialUse
previousRound.removeMatch(match)when moving a match.Line 59 directly mutates
previousRound.getMatches(), bypassing the unlink logic encapsulated inremoveMatch.♻️ Refactor sketch
Round previousRound = match.getRound(); if (previousRound != null && previousRound != this) { - previousRound.getMatches().removeIf(m -> m == match); + previousRound.removeMatch(match); }src/main/java/cat/udl/eps/softarch/fll/domain/Match.java (1)
26-32:⚠️ Potential issue | 🟠 Major
equals/hashCodebased only on nullableidbreaks transient-entity semantics.With Line 26 + Line 31, two unsaved
Matchinstances (id == null) compare equal, which can corruptcontains/dedup behavior and has already forced identity-based workarounds elsewhere.#!/bin/bash # Verify equality configuration and identity-based workarounds in domain entities. rg -n --type=java '@EqualsAndHashCode|EqualsAndHashCode.Include|anyMatch\(m -> m == match\)|removeIf\(m -> m == match\)' \ src/main/java/cat/udl/eps/softarch/fll/domain/Match.java \ src/main/java/cat/udl/eps/softarch/fll/domain/Round.java \ src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.java
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Match.javasrc/main/java/cat/udl/eps/softarch/fll/domain/Round.java
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/cat/udl/eps/softarch/fll/domain/Round.java (2)
52-59:⚠️ Potential issue | 🟠 MajorUse entity-aware removal path when rehoming a
Match.At Line 52 and Line 58, using reference equality (
m == match) plus direct mutation ofpreviousRound.getMatches()can fail to unlink correctly in detached/proxy cases and bypassesremoveMatchinvariants.Suggested fix
- if (this.matches.stream().anyMatch(m -> m == match)) { + if (this.matches.contains(match)) { return; } Round previousRound = match.getRound(); if (previousRound != null && previousRound != this) { - previousRound.getMatches().removeIf(m -> m == match); + previousRound.removeMatch(match); }
71-73:⚠️ Potential issue | 🟠 MajorAvoid reference-based
removeIfinremoveMatch.At Line 71,
removeIf(m -> m == match)relies on identity equality and can miss logically same entities. Useremove(match)for consistency with entity equality behavior.Suggested fix
- if (this.matches.removeIf(m -> m == match)) { + if (this.matches.remove(match)) { match.setRound(null); }
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79535e98-4400-44e7-99e0-b6dbe777d25f
📒 Files selected for processing (1)
src/main/java/cat/udl/eps/softarch/fll/domain/Round.java
| if (this.matches.stream().anyMatch(m -> m == match)) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| public void addMatch(Match match) { | ||
| CompetitionTable previousTable = match.getCompetitionTable(); | ||
| if (previousTable != null && previousTable != this) { | ||
| previousTable.removeMatch(match); | ||
| } | ||
| if (!matches.contains(match)) { | ||
| matches.add(match); | ||
| previousTable.getMatches().removeIf(m -> m == match); | ||
| } | ||
|
|
||
| this.matches.add(match); | ||
| match.setCompetitionTable(this); |
There was a problem hiding this comment.
addMatch uses reference equality (m == match) to detect duplicates and to detach from the previous table. Since Match equality is id-based, this can allow duplicates (or fail to detach) when the same DB row is represented by another instance/proxy. Prefer matches.contains(match) / previousTable.removeMatch(match) for equality-based behavior and consistent bidirectional updates.
| public void removeMatch(Match match) { | ||
| matches.remove(match); | ||
| match.setCompetitionTable(null); | ||
| if (match == null) { | ||
| return; | ||
| } | ||
|
|
||
| if (this.matches.removeIf(m -> m == match)) { | ||
| match.setCompetitionTable(null); | ||
| } |
There was a problem hiding this comment.
removeMatch uses removeIf(m -> m == match), so removal only works for the exact same instance. With JPA, it’s common to have a different instance/proxy representing the same Match id, in which case removal will silently fail. Use equality-based removal (e.g., matches.remove(match) / removeIf(match::equals)).
| if (this.referees.stream().anyMatch(r -> r == referee)) { | ||
| return; | ||
| } | ||
| if (referees.size() >= 3) { | ||
|
|
||
| if (this.referees.size() >= 3) { | ||
| throw new IllegalStateException("A table can have a maximum of 3 referees"); | ||
| } | ||
| referees.add(referee); | ||
|
|
||
| CompetitionTable previousTable = referee.getSupervisesTable(); | ||
| if (previousTable != null && previousTable != this) { | ||
| previousTable.getReferees().removeIf(r -> r == referee); | ||
| } | ||
|
|
||
| this.referees.add(referee); | ||
| referee.setSupervisesTable(this); |
There was a problem hiding this comment.
addReferee prevents duplicates using reference equality (r == referee) and detaches from the previous table with removeIf(r -> r == referee). If the same referee is represented by another instance/proxy with the same id, this can lead to duplicates and inconsistent lists. Prefer referees.contains(referee) and previousTable.removeReferee(referee) for equality-based behavior and bidirectional consistency.
| public void removeReferee(Referee referee) { | ||
| referees.remove(referee); | ||
| referee.setSupervisesTable(null); | ||
| if (referee == null) { | ||
| return; | ||
| } | ||
|
|
||
| if (this.referees.removeIf(r -> r == referee)) { | ||
| referee.setSupervisesTable(null); | ||
| } |
There was a problem hiding this comment.
removeReferee uses removeIf(r -> r == referee), which won’t remove an equal referee represented by a different instance/proxy. Use equality-based removal (e.g., referees.remove(referee) / removeIf(referee::equals)) to ensure the association is reliably cleared.
| if (this.matches.stream().anyMatch(m -> m == match)) { | ||
| return; | ||
| } | ||
|
|
||
| public void setMatches(List<Match> matches) { | ||
| this.matches.clear(); | ||
| if (matches != null) { | ||
| matches.forEach(this::addMatch); | ||
| Round previousRound = match.getRound(); | ||
| if (previousRound != null && previousRound != this) { | ||
| previousRound.getMatches().removeIf(m -> m == match); | ||
| } | ||
| } | ||
|
|
||
| public void addMatch(Match match) { | ||
| matches.add(match); | ||
| this.matches.add(match); | ||
| match.setRound(this); |
There was a problem hiding this comment.
addMatch/removeMatch (and the reassignment logic) relies on reference equality (==) for duplicate detection and removal. Since Match has an equals/hashCode based on id, passing a different instance/proxy representing the same match will not be removed and can lead to duplicates or stale associations. Prefer contains(match) / remove(match) (or removeIf(match::equals)) and call previousRound.removeMatch(match) to keep bidirectional consistency using equality semantics.
| public void removeMatch(Match match) { | ||
| matches.remove(match); | ||
| match.setRound(null); | ||
| if (match == null) { | ||
| return; | ||
|
|
||
| } | ||
|
|
||
| if (this.matches.removeIf(m -> m == match)) { | ||
| match.setRound(null); | ||
| } |
There was a problem hiding this comment.
removeMatch uses removeIf(m -> m == match) which only removes the exact same object instance. If callers pass a different Match instance with the same id (or a JPA proxy), the match won't be removed. Use equality-based removal (e.g., matches.remove(match) or removeIf(match::equals)) to match the entity equals contract.
…in Cucumber tests
…_XiaoLong Add step definitions for CompetitionTable, Match, Referee, and Round …
|
Closes #9
Closes #45