Introduce modification container abstraction#823
Open
Meklo wants to merge 14 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactors modification ownership from group-only to container-aware (GROUP/COMPOSITE): new container abstractions, entity JPA changes, container-scoped repository queries and CTEs, container-aware move/reorder logic, controller/service signature changes, DB migration, and test updates. ChangesPolymorphic Container Model
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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. 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: 14
🧹 Nitpick comments (1)
src/main/java/org/gridsuite/modification/server/repositories/ModificationApplicationRepository.java (1)
21-23: ⚡ Quick winAlign parameter names with the container model.
groupUuidis now misleading in both signatures since the method contract is container-based. Renaming tocontainerIds(ormodificationContainerIds) will make call sites clearer and reduce migration confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/server/repositories/ModificationApplicationRepository.java` around lines 21 - 23, Rename the misleading parameter name groupUuid to a container-based name in the two repository method signatures: change the parameter in deleteAllByNetworkUuidAndModificationContainerIdIn(UUID networkUuid, List<UUID> groupUuid) and deleteAllByModificationContainerIdIn(List<UUID> groupUuid) to something like containerIds or modificationContainerIds to reflect the container model; update all call sites and tests that reference these method signatures to use the new parameter name so compilation and code clarity are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/gridsuite/modification/server/entities/CompositeModificationEntity.java`:
- Around line 80-90: setModifications currently clears the `@OneToMany` collection
(this.modifications.clear()) which causes Hibernate to mark existing children as
orphans and schedule deletes before you re-add them; instead, modify the
association incrementally in setModifications(List<ModificationEntity>
newChildren): validate newChildren non-null, iterate newChildren to update each
child's setModificationsOrder(i) and attachToContainer(this) for new or existing
children, add only genuinely new entities to this.modifications, and remove only
those children present in this.modifications but absent from newChildren so
orphanRemoval behaves correctly; this preserves managed instances and avoids
delete/insert churn and constraint/order issues.
In
`@src/main/java/org/gridsuite/modification/server/entities/ModificationEntity.java`:
- Around line 142-149: The attachToContainer/detachFromContainer helpers in
ModificationEntity currently only update containerType but must also keep the
mapped scalar FK containerId in sync; update
attachToContainer(ModificationContainer) to assign this.containerId =
container.getId() (or the container's identifier accessor) and update
detachFromContainer() to set this.containerId = null so the in-memory entity
matches the collection-managed FK (this is required because
ModificationApplicationEntity.toModificationApplicationInfos() reads
getModification().getContainerId()).
In
`@src/main/java/org/gridsuite/modification/server/entities/ModificationGroupEntity.java`:
- Around line 68-78: setModifications currently clears the modifications list
then re-attaches only survivors, leaving omitted children with stale container
metadata; modify setModifications in ModificationGroupEntity to first compute
the set of removed children (elements in the existing modifications collection
that are not present in the incoming newChildren list) and for each removed
child call the appropriate detach logic on ModificationEntity (e.g. clear
container id/type or invoke a detachFromContainer method) before
clearing/replacing the collection, then attach and setModificationsOrder on the
surviving newChildren as today; ensure you update the modifications collection
only after detaching removed children so container ownership remains consistent.
In
`@src/main/java/org/gridsuite/modification/server/NetworkModificationController.java`:
- Line 18: Remove the unused import of ModificationContainerType from
NetworkModificationController: locate the import line "import
org.gridsuite.modification.server.entities.ModificationContainerType;" in the
NetworkModificationController class and delete it so Checkstyle no longer fails
for an unused import.
- Around line 104-116: The MOVE branch passes sourceContainerId
(originGroupUuid) directly to networkModificationService.moveModifications even
when it's null; restore the previous fallback by defaulting sourceContainerId to
targetContainerId when null for MOVE. Update the switch MOVE case to use
(sourceContainerId == null ? targetContainerId : sourceContainerId) before
calling networkModificationService.moveModifications (and keep passing
targetContainerId, beforeModificationUuid, modificationContextInfos.getFirst(),
modificationContextInfos.getSecond()) so same-container moves work when
originGroupUuid is omitted.
In
`@src/main/java/org/gridsuite/modification/server/repositories/ModificationRepository.java`:
- Around line 110-175: Fix the Checkstyle failures in the multi-line
`@NativeQuery` text blocks by normalizing indentation and blank-line placement:
ensure each triple-quoted SQL block aligns with the annotation (no leading
spaces inside the text block), remove or add blank lines so there are no extra
blank lines inside the text blocks, and keep consistent indentation for all
three methods findOnlyCompositeChildrenUuids, findAllChildrenUuids, and
getCompositesMaxDepth (and the preceding CompositeDepth interface section) so
the opening and closing triple quotes are directly under `@NativeQuery` and the
SQL lines have consistent left alignment; after fixing the text-block formatting
re-run the formatter/checkstyle to confirm the errors on the affected blocks are
resolved.
In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`:
- Line 227: The code in NetworkModificationRepository creates
destinationModificationEntities with improper spacing and missing braces; split
the combined statements into two properly spaced lines (one for creating the
ArrayList from destinationModificationGroupEntity.getModifications() and a
separate line calling removeIf(Objects::isNull)), normalize spaces so there is a
single space around operators and after commas, and add braces for the
single-line if statements referenced (the two ifs around lines noted) so they
become full-blocks with { ... } — apply the same formatting and brace fixes to
the other occurrences mentioned (the similar hunk at the other lines 335 and
384-385) to satisfy SingleSpaceSeparator, EmptyLineSeparator, and NeedBraces
checkstyle rules.
- Around line 188-191: The loop over modifications is double-incrementing the
order: modificationGroupEntity.addModification(m, order++) consumes one
post-increment and then m.setModificationsOrder(order); order++ increments
again, producing gaps. Fix by using a single increment operation: pass the
current order into addModification and set the same value on m, then increment
order once (or use a single post-increment consistently). Update the loop around
modificationGroupEntity.addModification(...), ModificationEntity m,
setModificationsOrder(...) and the order variable so only one increment occurs
per iteration.
- Around line 1064-1071: The getContainerType method in
NetworkModificationRepository should not return null for unknown container
UUIDs; replace the final return null with throwing a
NetworkModificationException (include a clear message like "Unknown container
UUID: <uuid>") so callers receive a domain error. Locate getContainerType and
after checking modificationGroupRepository.findById(containerUuid) and
compositeModificationRepository.findById(containerUuid), throw new
NetworkModificationException including the containerUuid and a descriptive
message instead of returning null; ensure the method signature and imports
reference NetworkModificationException and that any callers (e.g., move flow)
will handle or propagate this domain exception.
- Around line 266-287: The same-container branch drops stashed children because
sourceChildren is created by filtering out stashed items before reordering; when
sameContainer is true you must preserve stashed modifications. Change the logic
so you build a fullChildren list from source.getModifications() that includes
stashed items and use that full list for publishChildren (and for re-inserting
order via insertModifications), while still computing moved via
removeModifications(fullChildren, modificationUuids) or by deriving moved
separately; ensure functions referenced (source.getModifications(),
removeModifications, insertModifications, publishChildren, sameContainer)
operate on the full list so stashed children are not detached during
same-container reorders.
In
`@src/main/java/org/gridsuite/modification/server/service/NetworkModificationService.java`:
- Line 36: The import of ModificationGroupRepository in
NetworkModificationService is unused and causing a Checkstyle failure; open
NetworkModificationService.java and remove the line importing
org.gridsuite.modification.server.repositories.ModificationGroupRepository
(ensure there are no references to ModificationGroupRepository in methods of
NetworkModificationService or replace them with the correct repository type if
mistakenly used), then recompile/run checks to confirm the Checkstyle error is
resolved.
In `@src/main/resources/config/application.yaml`:
- Line 18: The default config enables Hibernate SQL logging via the show_sql
property (hibernate.show_sql), which should not be on in the default runtime
profile; remove or set show_sql: false from the base application.yaml and
instead place show_sql: true in a non-production profile (e.g.,
application-dev.yaml or under a spring.profiles: dev section) so only the
development/local profile (referencing the show_sql / hibernate.show_sql
property) emits SQL logs.
In `@src/main/resources/db/changelog/changesets/changelog_20260522T151740Z.xml`:
- Around line 4-40: The migration leaves modification.container_id and
modification.container_type nullable; add a step to make them NOT NULL after the
backfill and ensure there are no leftover NULLs: first run the existing backfill
UPDATEs (they reference group_id, composite_modification_sub_modifications, and
set container_id/container_type), then add a precondition or validation that
SELECT COUNT(*) FROM modification WHERE container_id IS NULL OR container_type
IS NULL returns zero, and finally run ALTER TABLE modification ALTER COLUMN
container_id SET NOT NULL and ALTER COLUMN container_type SET NOT NULL (or the
equivalent <modifyDataType>/<addNotNullConstraint> in your changelog), keeping
the index modification_container_idx and drop steps unchanged; if any rows
remain, fail the migration so you can handle or backfill them explicitly.
In
`@src/test/java/org/gridsuite/modification/server/service/SupervisionTest.java`:
- Line 90: Restore the Mockito stubbing for modificationMock.getContainerId() in
testReindexElements so groupUuid is set and asserted: re-add a line stubbing
modificationMock.getContainerId() to return groupMock (or the UUID/value used
elsewhere), then update the test to assert that
ModificationApplicationEntity.toModificationApplicationInfos() propagates that
container id (groupUuid) into the resulting infos; target the modificationMock
and groupMock stubs and the testReindexElements method where
toModificationApplicationInfos() is exercised.
---
Nitpick comments:
In
`@src/main/java/org/gridsuite/modification/server/repositories/ModificationApplicationRepository.java`:
- Around line 21-23: Rename the misleading parameter name groupUuid to a
container-based name in the two repository method signatures: change the
parameter in deleteAllByNetworkUuidAndModificationContainerIdIn(UUID
networkUuid, List<UUID> groupUuid) and
deleteAllByModificationContainerIdIn(List<UUID> groupUuid) to something like
containerIds or modificationContainerIds to reflect the container model; update
all call sites and tests that reference these method signatures to use the new
parameter name so compilation and code clarity are preserved.
🪄 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: 8e51e903-6868-420a-bb45-cff1571d66ed
📒 Files selected for processing (19)
src/main/java/org/gridsuite/modification/server/CompositeController.javasrc/main/java/org/gridsuite/modification/server/NetworkModificationController.javasrc/main/java/org/gridsuite/modification/server/elasticsearch/ModificationApplicationInfosService.javasrc/main/java/org/gridsuite/modification/server/entities/CompositeModificationEntity.javasrc/main/java/org/gridsuite/modification/server/entities/ContainerOps.javasrc/main/java/org/gridsuite/modification/server/entities/ModificationApplicationEntity.javasrc/main/java/org/gridsuite/modification/server/entities/ModificationContainer.javasrc/main/java/org/gridsuite/modification/server/entities/ModificationContainerType.javasrc/main/java/org/gridsuite/modification/server/entities/ModificationEntity.javasrc/main/java/org/gridsuite/modification/server/entities/ModificationGroupEntity.javasrc/main/java/org/gridsuite/modification/server/repositories/ModificationApplicationRepository.javasrc/main/java/org/gridsuite/modification/server/repositories/ModificationRepository.javasrc/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.javasrc/main/java/org/gridsuite/modification/server/service/NetworkModificationService.javasrc/main/resources/config/application.yamlsrc/main/resources/db/changelog/changesets/changelog_20260522T151740Z.xmlsrc/main/resources/db/changelog/db.changelog-master.yamlsrc/test/java/org/gridsuite/modification/server/CompositeControllerTest.javasrc/test/java/org/gridsuite/modification/server/service/SupervisionTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/gridsuite/modification/server/CompositeController.java
Comment on lines
+68
to
+78
| public void setModifications(List<ModificationEntity> newChildren) { | ||
| this.modifications.clear(); | ||
| if (newChildren == null || newChildren.isEmpty()) { | ||
| return; | ||
| } | ||
| for (int i = 0; i < newChildren.size(); i++) { | ||
| ModificationEntity child = newChildren.get(i); | ||
| child.attachToContainer(this); | ||
| child.setModificationsOrder(i); | ||
| this.modifications.add(child); | ||
| } |
There was a problem hiding this comment.
Detach children that drop out of setModifications().
clear() unlinks every current child, but only the surviving ones are re-attached. On ModificationGroupEntity there is no orphan removal, so anything omitted from newChildren can keep stale container metadata while the collection update clears container_id. That leaves the new (container_id, container_type) ownership model inconsistent.
Suggested direction
public void setModifications(List<ModificationEntity> newChildren) {
+ this.modifications.forEach(ModificationEntity::detachFromContainer);
this.modifications.clear();
if (newChildren == null || newChildren.isEmpty()) {
return;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/org/gridsuite/modification/server/entities/ModificationGroupEntity.java`
around lines 68 - 78, setModifications currently clears the modifications list
then re-attaches only survivors, leaving omitted children with stale container
metadata; modify setModifications in ModificationGroupEntity to first compute
the set of removed children (elements in the existing modifications collection
that are not present in the incoming newChildren list) and for each removed
child call the appropriate detach logic on ModificationEntity (e.g. clear
container id/type or invoke a detachFromContainer method) before
clearing/replacing the collection, then attach and setModificationsOrder on the
surviving newChildren as today; ensure you update the modifications collection
only after detaching removed children so container ownership remains consistent.
| order_updates: true | ||
| jdbc: | ||
| batch_size: 512 | ||
| show_sql: true |
There was a problem hiding this comment.
Avoid enabling hibernate.show_sql in the default runtime profile.
Turning this on globally can leak sensitive values to logs and increases overhead/noise in production.
Suggested change
- show_sql: true
+ # keep disabled in default profile; enable only in local/debug profile
+ show_sql: falseMove show_sql: true to a dedicated local/dev profile configuration if needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| show_sql: true | |
| # keep disabled in default profile; enable only in local/debug profile | |
| show_sql: false |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/resources/config/application.yaml` at line 18, The default config
enables Hibernate SQL logging via the show_sql property (hibernate.show_sql),
which should not be on in the default runtime profile; remove or set show_sql:
false from the base application.yaml and instead place show_sql: true in a
non-production profile (e.g., application-dev.yaml or under a spring.profiles:
dev section) so only the development/local profile (referencing the show_sql /
hibernate.show_sql property) emits SQL logs.
Comment on lines
+4
to
+40
| <addColumn tableName="modification"> | ||
| <column name="container_id" type="UUID"/> | ||
| <column name="container_type" type="VARCHAR(32)"/> | ||
| </addColumn> | ||
| <sql> | ||
| UPDATE modification | ||
| SET container_id = group_id, | ||
| container_type = 'GROUP' | ||
| WHERE group_id IS NOT NULL | ||
| AND container_id IS NULL; | ||
| </sql> | ||
|
|
||
|
|
||
| <sql> | ||
| UPDATE modification m | ||
| SET container_id = ( | ||
| SELECT j.id | ||
| FROM composite_modification_sub_modifications j | ||
| WHERE j.modification_id = m.id | ||
| ), | ||
| container_type = 'COMPOSITE' | ||
| WHERE m.container_id IS NULL | ||
| AND EXISTS ( | ||
| SELECT 1 | ||
| FROM composite_modification_sub_modifications j | ||
| WHERE j.modification_id = m.id | ||
| ); | ||
| </sql> | ||
|
|
||
| <createIndex tableName="modification" indexName="modification_container_idx"> | ||
| <column name="container_type"/> | ||
| <column name="container_id"/> | ||
| </createIndex> | ||
| <dropForeignKeyConstraint baseTableName="modification" | ||
| constraintName="group_id_fk_constraint"/> | ||
| <dropColumn tableName="modification" columnName="group_id"/> | ||
| <dropTable tableName="composite_modification_sub_modifications"/> |
There was a problem hiding this comment.
Enforce container columns as mandatory after backfill.
This migration leaves container_id / container_type nullable even though the new model addresses modifications through these fields. Null leftovers would create orphaned rows invisible to container-based queries.
Suggested hardening
<sql>
UPDATE modification m
SET container_id = (
SELECT j.id
FROM composite_modification_sub_modifications j
WHERE j.modification_id = m.id
),
container_type = 'COMPOSITE'
WHERE m.container_id IS NULL
AND EXISTS (
SELECT 1
FROM composite_modification_sub_modifications j
WHERE j.modification_id = m.id
);
</sql>
+ <preConditions onFail="HALT">
+ <sqlCheck expectedResult="0">
+ SELECT COUNT(*) FROM modification
+ WHERE container_id IS NULL OR container_type IS NULL
+ </sqlCheck>
+ </preConditions>
+
+ <addNotNullConstraint tableName="modification" columnName="container_id" columnDataType="UUID"/>
+ <addNotNullConstraint tableName="modification" columnName="container_type" columnDataType="VARCHAR(32)"/>
+
<createIndex tableName="modification" indexName="modification_container_idx">
<column name="container_type"/>
<column name="container_id"/>
</createIndex>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/resources/db/changelog/changesets/changelog_20260522T151740Z.xml`
around lines 4 - 40, The migration leaves modification.container_id and
modification.container_type nullable; add a step to make them NOT NULL after the
backfill and ensure there are no leftover NULLs: first run the existing backfill
UPDATEs (they reference group_id, composite_modification_sub_modifications, and
set container_id/container_type), then add a precondition or validation that
SELECT COUNT(*) FROM modification WHERE container_id IS NULL OR container_type
IS NULL returns zero, and finally run ALTER TABLE modification ALTER COLUMN
container_id SET NOT NULL and ALTER COLUMN container_type SET NOT NULL (or the
equivalent <modifyDataType>/<addNotNullConstraint> in your changelog), keeping
the index modification_container_idx and drop steps unchanged; if any rows
remain, fail the migration so you can handle or backfill them explicitly.
| Mockito.when(modificationMock.getId()).thenReturn(UUID.randomUUID()); | ||
| Mockito.when(groupMock.getId()).thenReturn(groupUuid); | ||
| Mockito.when(modificationMock.getGroup()).thenReturn(groupMock); | ||
| //Mockito.when(modificationMock.getContainerId()).thenReturn(groupMock); |
There was a problem hiding this comment.
Restore getContainerId() stubbing to keep this test meaningful.
At Line 90, the removed stub leaves groupUuid effectively unverified in testReindexElements. Since ModificationApplicationEntity.toModificationApplicationInfos() now reads modification.getContainerId(), this test should stub that value explicitly and assert it is propagated.
Proposed fix
- Mockito.when(groupMock.getId()).thenReturn(groupUuid);
- //Mockito.when(modificationMock.getContainerId()).thenReturn(groupMock);
+ Mockito.when(modificationMock.getContainerId()).thenReturn(groupUuid); verify(modificationApplicationInfosRepository, times(1)).saveAll(modificationListCaptor.capture());
assertThat(modificationListCaptor.getValue()).usingRecursiveComparison().isEqualTo(allModifications.stream().map(ModificationApplicationEntity::toModificationApplicationInfos).toList());
+ assertThat(modificationListCaptor.getValue()).allSatisfy(infos -> assertEquals(groupUuid, infos.getGroupUuid()));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/java/org/gridsuite/modification/server/service/SupervisionTest.java`
at line 90, Restore the Mockito stubbing for modificationMock.getContainerId()
in testReindexElements so groupUuid is set and asserted: re-add a line stubbing
modificationMock.getContainerId() to return groupMock (or the UUID/value used
elsewhere), then update the test to assert that
ModificationApplicationEntity.toModificationApplicationInfos() propagates that
container id (groupUuid) into the resulting infos; target the modificationMock
and groupMock stubs and the testReindexElements method where
toModificationApplicationInfos() is exercised.
There was a problem hiding this comment.
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/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java (1)
227-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelay destination creation until the move set is known to be non-empty.
Line 229 loads or creates the target container before Line 237 knows whether any requested UUID actually belongs to the source. A move request containing only unknown UUIDs will return an empty result but still persist a brand-new empty destination group.
Suggested change
ModificationContainer source = loadContainer(sourceType, sourceContainerId); boolean sameContainer = sourceType == targetType && sourceContainerId.equals(targetContainerId); - ModificationContainer target = sameContainer ? source : loadOrCreateContainer(targetType, targetContainerId); List<ModificationEntity> sourceChildren = source.getModifications().stream() .filter(Objects::nonNull) .filter(m -> !Boolean.TRUE.equals(m.getStashed())) @@ if (moved.isEmpty()) { return List.of(); } + + ModificationContainer target = sameContainer ? source : loadOrCreateContainer(targetType, targetContainerId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java` around lines 227 - 239, The code currently calls loadOrCreateContainer(targetType, targetContainerId) before knowing if any modifications will be moved, causing empty target groups to be persisted; change the flow so you first load the source via loadContainer(sourceType, sourceContainerId), build sourceChildren, compute moved = removeModifications(sourceChildren, modificationUuids) and if moved is empty return early, and only then (when moved is non-empty and sameContainer is false) call loadOrCreateContainer(targetType, targetContainerId) to obtain the target ModificationContainer; update any logic that assumed target was already available to run after this deferred creation.
♻️ Duplicate comments (3)
src/test/java/org/gridsuite/modification/server/service/SupervisionTest.java (1)
90-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore
getContainerId()stubbing to keep this test meaningful.The commented-out stub leaves
containerIdunverified intestReindexElements. SinceModificationApplicationEntity.toModificationApplicationInfos()now readsmodification.getContainerId(), this test should stub that value explicitly and assert it is propagated.Proposed fix
- //Mockito.when(modificationMock.getContainerId()).thenReturn(groupMock); + Mockito.when(modificationMock.getContainerId()).thenReturn(groupUuid);Add assertion after line 102:
verify(modificationApplicationInfosRepository, times(1)).saveAll(modificationListCaptor.capture()); assertThat(modificationListCaptor.getValue()).usingRecursiveComparison().isEqualTo(allModifications.stream().map(ModificationApplicationEntity::toModificationApplicationInfos).toList()); + assertThat(modificationListCaptor.getValue()).allSatisfy(infos -> assertEquals(groupUuid, infos.getGroupUuid()));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/modification/server/service/SupervisionTest.java` at line 90, Restore the Mockito stub for modificationMock.getContainerId() in testReindexElements so the test supplies a deterministic containerId (e.g., return groupMock) because ModificationApplicationEntity.toModificationApplicationInfos() now reads modification.getContainerId(); add a Mockito.when(modificationMock.getContainerId()).thenReturn(groupMock) near the other stubs for modificationMock and then add an assertion after the existing assertions that the produced ModificationApplicationInfo(s) contain the expected containerId value to verify propagation.src/main/java/org/gridsuite/modification/server/entities/ModificationGroupEntity.java (1)
55-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDetach children that drop out of
setModifications().
clear()unlinks every current child, but only the surviving ones are re-attached. Children omitted fromnewChildrenkeep stalecontainer_idmetadata while the collection update clears the reference. Withinsertable = false, updatable = falseon the@JoinColumn(line 32), this entity cannot updatecontainer_id, leaving the orphaned children with inconsistent ownership.Suggested direction
public void setModifications(List<ModificationEntity> newChildren) { + // Detach children being removed + if (newChildren != null && !newChildren.isEmpty()) { + Set<UUID> survivorIds = newChildren.stream() + .map(ModificationEntity::getId) + .collect(Collectors.toSet()); + this.modifications.stream() + .filter(child -> !survivorIds.contains(child.getId())) + .forEach(child -> child.detachFromContainer()); + } else { + this.modifications.forEach(ModificationEntity::detachFromContainer); + } this.modifications.clear(); if (newChildren == null || newChildren.isEmpty()) { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/server/entities/ModificationGroupEntity.java` around lines 55 - 66, setModifications currently clears the collection then re-attaches survivors, leaving omitted children with stale container metadata; before calling this.modifications.clear() compute which existing children are not present in newChildren and explicitly detach them (e.g., call a detach method or clear their container reference) so their container_id is nulled, then attach survivors with child.attachToContainer(this) and setModificationsOrder(i) as you already do.src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java (1)
231-247:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPreserve stashed children when reordering inside the same container.
sourceChildrenstrips out every stashed modification, and Line 247 writes that filtered list back to the container. A same-container move therefore detaches all stashed children from the container instead of just reordering the unstashed ones.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java` around lines 231 - 247, sourceChildren currently filters out stashed modifications from source.getModifications(), then on same-container reorders writes the filtered list back with source.setModifications(), which inadvertently removes stashed children; change the same-container path to preserve stashed items by performing removeModifications/insertModifications only on the unstashed subset without overwriting the container's full modification list—i.e., use source.getModifications() to reconstruct the final list by replacing only the moved unstashed entries (using removeModifications, insertModifications and beforeModificationUuid) and then call source.setModifications(...) with the merged list so stashed ModificationEntity (getStashed()) elements remain in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`:
- Around line 227-239: The code currently calls
loadOrCreateContainer(targetType, targetContainerId) before knowing if any
modifications will be moved, causing empty target groups to be persisted; change
the flow so you first load the source via loadContainer(sourceType,
sourceContainerId), build sourceChildren, compute moved =
removeModifications(sourceChildren, modificationUuids) and if moved is empty
return early, and only then (when moved is non-empty and sameContainer is false)
call loadOrCreateContainer(targetType, targetContainerId) to obtain the target
ModificationContainer; update any logic that assumed target was already
available to run after this deferred creation.
---
Duplicate comments:
In
`@src/main/java/org/gridsuite/modification/server/entities/ModificationGroupEntity.java`:
- Around line 55-66: setModifications currently clears the collection then
re-attaches survivors, leaving omitted children with stale container metadata;
before calling this.modifications.clear() compute which existing children are
not present in newChildren and explicitly detach them (e.g., call a detach
method or clear their container reference) so their container_id is nulled, then
attach survivors with child.attachToContainer(this) and setModificationsOrder(i)
as you already do.
In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`:
- Around line 231-247: sourceChildren currently filters out stashed
modifications from source.getModifications(), then on same-container reorders
writes the filtered list back with source.setModifications(), which
inadvertently removes stashed children; change the same-container path to
preserve stashed items by performing removeModifications/insertModifications
only on the unstashed subset without overwriting the container's full
modification list—i.e., use source.getModifications() to reconstruct the final
list by replacing only the moved unstashed entries (using removeModifications,
insertModifications and beforeModificationUuid) and then call
source.setModifications(...) with the merged list so stashed ModificationEntity
(getStashed()) elements remain in place.
In
`@src/test/java/org/gridsuite/modification/server/service/SupervisionTest.java`:
- Line 90: Restore the Mockito stub for modificationMock.getContainerId() in
testReindexElements so the test supplies a deterministic containerId (e.g.,
return groupMock) because
ModificationApplicationEntity.toModificationApplicationInfos() now reads
modification.getContainerId(); add a
Mockito.when(modificationMock.getContainerId()).thenReturn(groupMock) near the
other stubs for modificationMock and then add an assertion after the existing
assertions that the produced ModificationApplicationInfo(s) contain the expected
containerId value to verify propagation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b53da69-abd1-49da-a313-c80d53b3e03e
📒 Files selected for processing (15)
src/main/java/org/gridsuite/modification/server/NetworkModificationController.javasrc/main/java/org/gridsuite/modification/server/entities/CompositeModificationEntity.javasrc/main/java/org/gridsuite/modification/server/entities/ContainerOps.javasrc/main/java/org/gridsuite/modification/server/entities/ModificationContainer.javasrc/main/java/org/gridsuite/modification/server/entities/ModificationEntity.javasrc/main/java/org/gridsuite/modification/server/entities/ModificationGroupEntity.javasrc/main/java/org/gridsuite/modification/server/repositories/ModificationApplicationRepository.javasrc/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.javasrc/main/java/org/gridsuite/modification/server/service/SupervisionService.javasrc/test/java/org/gridsuite/modification/server/CompositeControllerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/CompositeModificationsTest.javasrc/test/java/org/gridsuite/modification/server/modifications/tabularmodifications/TabularGeneratorModificationsTest.javasrc/test/java/org/gridsuite/modification/server/service/ModificationIndexationTest.javasrc/test/java/org/gridsuite/modification/server/service/ModificationRepositoryTest.javasrc/test/java/org/gridsuite/modification/server/service/SupervisionTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/gridsuite/modification/server/entities/ContainerOps.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/gridsuite/modification/server/entities/ModificationContainer.java
- src/main/java/org/gridsuite/modification/server/entities/CompositeModificationEntity.java
- src/main/java/org/gridsuite/modification/server/entities/ModificationEntity.java
|
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.
PR Summary