Add a modification reference as a new modification type#807
Open
SlimaneAmar wants to merge 5 commits intomainfrom
Open
Add a modification reference as a new modification type#807SlimaneAmar wants to merge 5 commits intomainfrom
SlimaneAmar wants to merge 5 commits intomainfrom
Conversation
added 2 commits
April 28, 2026 17:09
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new ModificationReference feature: a JPA entity for modification references, registry & modification-type mappings, repository conversion logic to resolve referenced modifications, DB changelog to persist references, and integration tests including COPY behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as Controller
participant Repo as NetworkModificationRepository
participant RefRepo as ModificationRepository
participant DB as Database
Client->>Controller: request (e.g., GET/COPY involving a reference)
Controller->>Repo: load modification(s)
Repo->>DB: query modification_reference (if entity is a reference)
DB-->>Repo: modification_reference row
Repo->>RefRepo: getReferenceById(referenceId)
RefRepo->>DB: query referenced modification
DB-->>RefRepo: referenced modification row
RefRepo-->>Repo: referenced ModificationEntity / DTO (optimized conversion)
Repo-->>Controller: assembled ModificationReferenceInfos (includes referenceInfos)
Controller-->>Client: response with resolved reference payload
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 47 minutes and 15 seconds.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: 5
🤖 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/main/java/org/gridsuite/modification/server/entities/ModificationReferenceEntity.java`:
- Around line 59-64: Validate and guard against nulls for referenceType,
referenceId and referenceInfos before dereferencing in the
conversion/constructor that sets this.referenceType and this.referenceId and
calls modificationReferenceInfos.getReferenceInfos(); specifically check
modificationReferenceInfos, modificationReferenceInfos.getReferenceType(),
modificationReferenceInfos.getReferenceId(), and
modificationReferenceInfos.getReferenceInfos() (and its
getMessageType()/getMapMessageValues()) and either throw a clear
IllegalArgumentException or handle missing values gracefully; only call
setMessageType(...) and new ObjectMapper().writeValueAsString(...) when
referenceInfos is non-null, and include a descriptive error message mentioning
ModificationReferenceEntity/ modificationReferenceInfos to aid debugging.
In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`:
- Around line 489-499: The loadModificationReference method calls
modificationRepository.getReferenceById(...) and passes its result directly to
toModificationsInfosOptimized, which can NPE if the repository returns null;
update loadModificationReference to guard the reference result (from
modificationRepository.getReferenceById) before conversion: retrieve the
reference into a local variable, check for null (or empty) and either pass an
empty/nullable-safe value to toModificationsInfosOptimized or set referenceInfos
to an appropriate default, keeping the existing fields (uuid, activated,
description, date, stashed, referenceId, referenceType) intact; references to
modify: loadModificationReference, modificationRepository.getReferenceById, and
toModificationsInfosOptimized.
In `@src/main/resources/db/changelog/changesets/changelog_20260424T113143Z.xml`:
- Around line 4-13: Make the schema enforce that references are present by
adding NOT NULL constraints on modification_reference.reference_id and
modification_reference.reference_type and a check constraint that both are set
together; update the Liquibase changeset that defines table
modification_reference (and/or add a new changeSet) to run addNotNullConstraint
for columns reference_id and reference_type and add a logical checkConstraint
(e.g., "reference_id IS NOT NULL AND reference_type IS NOT NULL") on
modification_reference; optionally, if there is a single concrete target type,
add the corresponding addForeignKeyConstraint from
modification_reference.reference_id to that target table, otherwise leave
polymorphic FK logic to application code.
In
`@src/test/java/org/gridsuite/modification/server/modifications/ModificationReferenceTest.java`:
- Around line 49-51: buildModificationUpdate() currently just returns
buildModification(), so the update tests reuse the creation payload; change
buildModificationUpdate() to produce a distinct ModificationInfos instance
(e.g., clone the object returned by buildModification() then mutate one or more
fields such as name, version, description, or attributes) so the update path
exercises a changed payload, and update any assertions in the test to verify the
modified fields differ (use the ModificationInfos constructor/setters or a
builder to make the change).
- Around line 64-70: The tests testCreationModificationMessage and
testUpdateModificationMessage currently misuse assertNotNull by passing the
constant ModificationType.COMPOSITE_MODIFICATION.name() as the first parameter
and modificationInfos.getMessageType() as the message, so they always pass;
replace those assertNotNull calls with
assertEquals(ModificationType.COMPOSITE_MODIFICATION.name(),
modificationInfos.getMessageType()) in both methods to assert the actual message
type equals the expected value.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 459859bb-1f68-43e6-8758-552f6600df57
📒 Files selected for processing (8)
src/main/java/org/gridsuite/modification/server/entities/EntityRegistry.javasrc/main/java/org/gridsuite/modification/server/entities/ModificationReferenceEntity.javasrc/main/java/org/gridsuite/modification/server/modifications/ModificationTypeWithPreloadingStrategy.javasrc/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.javasrc/main/resources/db/changelog/changesets/changelog_20260424T113143Z.xmlsrc/main/resources/db/changelog/db.changelog-master.yamlsrc/test/java/org/gridsuite/modification/server/ModificationControllerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/ModificationReferenceTest.java
added 2 commits
April 29, 2026 14:51
…ion-server into shared-modification
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/main/java/org/gridsuite/modification/server/entities/ModificationReferenceEntity.java`:
- Around line 60-63: The update method in ModificationReferenceEntity currently
casts modificationInfos to ModificationReferenceInfos without checking its
runtime type, which can throw ClassCastException; modify
ModificationReferenceEntity.update to first verify modificationInfos is an
instance of ModificationReferenceInfos (using instanceof or equivalent), then
call assignAttributes((ModificationReferenceInfos) modificationInfos) only when
the check passes, and if not, throw a controlled domain error (e.g.,
IllegalArgumentException or your project's DomainException) with a clear message
indicating the expected DTO type; keep the call to
super.update(modificationInfos) as appropriate and reference the update and
assignAttributes methods and the ModificationInfos/ModificationReferenceInfos
types when making the change.
In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`:
- Around line 490-506: loadModificationReference(...) calls
toModificationsInfosOptimized(referencedEntity) with no cycle detection which
can cause stack overflow on self/cyclic references; modify
toModificationsInfosOptimized to accept a recursion guard (e.g., a Set of
visited modification IDs or a max-depth param) and check/mark
referencedEntity.getId() before recursing, returning a safe placeholder or
skipping recursive expansion when a cycle is detected; update
loadModificationReference (and the other call sites at the other reference
handling locations) to create/pass the visited set (add the current modification
id before the call) so cycles are detected and prevented.
- Line 30: Remove the unused import of DirectoryService from the top of
NetworkModificationRepository: delete the line importing
org.gridsuite.modification.server.service.DirectoryService (or alternatively
reference DirectoryService where needed); ensure no other code in class
NetworkModificationRepository references DirectoryService so Checkstyle
UnusedImports no longer fails.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd6ef7cf-1a81-42c7-bb29-11488c9bb807
📒 Files selected for processing (4)
src/main/java/org/gridsuite/modification/server/entities/ModificationReferenceEntity.javasrc/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.javasrc/main/resources/db/changelog/changesets/changelog_20260424T113143Z.xmlsrc/test/java/org/gridsuite/modification/server/modifications/ModificationReferenceTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/db/changelog/changesets/changelog_20260424T113143Z.xml
Contributor
Mathieu-Deharbe
left a comment
Mathieu-Deharbe
left a comment
There was a problem hiding this comment.
Read but not executed yet. I don't want to mess with my database until this is completely finished.
Comment on lines
+27
to
+31
| @Column(name = "referenceId") | ||
| UUID referenceId; | ||
|
|
||
| @Column(name = "referenceType") | ||
| String referenceType; |
Contributor
There was a problem hiding this comment.
Those will be turned into reference_id and reference_type in the database won't they ? If yes, why not directly :
Suggested change
| @Column(name = "referenceId") | |
| UUID referenceId; | |
| @Column(name = "referenceType") | |
| String referenceType; | |
| @Column(name = "reference_id") | |
| UUID referenceId; | |
| @Column(name = "reference_type") | |
| String referenceType; |
The reference would be more precise, easier to track.
Comment on lines
+496
to
+497
| .messageType(modificationEntity.getMessageType()) | ||
| .messageValues(modificationEntity.getMessageValues()) |
Contributor
There was a problem hiding this comment.
I am not sure of how the update will happen when the referenced modification is changed so I may be wrong. But just in case shouldn't we use the message type and message values of the referencedEntity here ?
Isn't it how we want this modification reference to be displayed in the front ?
| .referenceType(ModificationReferenceInfos.Type.SAMPLE) | ||
| .referenceId(loadModificationInfo.getUuid()) | ||
| .referenceInfos(loadModificationInfo) | ||
| .stashed(false) |
Contributor
There was a problem hiding this comment.
? :
Suggested change
| .stashed(false) |
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.
PR Summary