merge some modifications into a new composite#810
Open
Mathieu-Deharbe wants to merge 20 commits into
Open
Conversation
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
This comment was marked as low quality.
This comment was marked as low quality.
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
…te' into merge-modifications-into-composite
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
souissimai
reviewed
May 27, 2026
Contributor
souissimai
left a comment
souissimai
left a comment
There was a problem hiding this comment.
rename merge ==> Assemble
This comment was marked as low quality.
This comment was marked as low quality.
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
![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
🤖 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/repositories/ModificationRepository.java`:
- Around line 74-82: The method findCompositeIdByContainedModificationId
currently returns a single UUID but the query on
composite_modification_sub_modifications can yield multiple sm.id rows; change
the repository method signature to return List<UUID> (or List<String> if
casting) so all matching composite IDs are returned, and then update the
caller(s) to validate the result: if size==0 treat as "no owner", if size==1 use
that UUID, and if size>1 throw a domain error (MODIFICATION_ERROR) with a clear
message indicating ambiguous ownership; keep references to the repository method
name (findCompositeIdByContainedModificationId), the
composite_modification_sub_modifications table and sm.id in your validation
logic so it’s easy to trace.
In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`:
- Around line 915-929: The code currently assumes assembledModificationsUuids
has elements and silently drops missing IDs when mapping via
modificationRepository.findById; add input validation at the start of the method
to (1) check assembledModificationsUuids is non-empty and throw a clear
IllegalArgumentException (or domain-specific exception) if empty instead of
calling getFirst(), and (2) after mapping UUIDs with
modificationRepository.findById, verify that the count of found
ModificationEntity objects equals the input UUID list size and if not throw an
exception listing the missing UUIDs (compare assembledModificationsUuids to the
found IDs) rather than using filter(Optional::isPresent) to allow partial
assembly; update references to assembledModificationsUuids,
getModificationEntity, modificationRepository.findById and the
assembledModifications list to use this validation.
- Around line 917-974: The code captures targetIndex before removals so calling
modifications.add(targetIndex, newCompositeEntity) can throw IndexOutOfBounds or
insert at wrong place; before inserting into targetGroup.getModifications() or
targetComposite.getModifications() recompute a safe insertion index (e.g., int
insertionIndex = Math.min(targetIndex, modifications.size()); or recalc relative
position from the current list) and use modifications.add(insertionIndex,
newCompositeEntity); update both the targetGroup and targetComposite branches
where targetIndex and newCompositeEntity are used.
🪄 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: 0d9db6f8-769f-4da4-bfbf-0f1d8eb43c14
📒 Files selected for processing (6)
src/main/java/org/gridsuite/modification/server/CompositeController.javasrc/main/java/org/gridsuite/modification/server/entities/ModificationGroupEntity.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/test/java/org/gridsuite/modification/server/CompositeControllerTest.java
Comment on lines
+74
to
+82
| // return the uuid of the composite containing the modification sent as parameter | ||
| @Query(value = """ | ||
| SELECT CAST(sm.id AS VARCHAR) | ||
| FROM composite_modification_sub_modifications sm | ||
| INNER JOIN modification m ON sm.modification_id = m.id | ||
| WHERE sm.modification_id = :uuid | ||
| ORDER BY m.modifications_order | ||
| """, nativeQuery = true) | ||
| UUID findCompositeIdByContainedModificationId(UUID uuid); |
There was a problem hiding this comment.
Handle ambiguous ownership explicitly in findCompositeIdByContainedModificationId.
This query can return multiple sm.id values for one modification_id, but the method returns a single UUID. That creates a runtime failure path and hides data-integrity issues.
Suggested change
- UUID findCompositeIdByContainedModificationId(UUID uuid);
+ List<UUID> findCompositeIdsByContainedModificationId(UUID uuid);Then validate in caller:
0result: no owner1result: expected>1result: throw a domain error (MODIFICATION_ERROR) with clear message
🤖 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/ModificationRepository.java`
around lines 74 - 82, The method findCompositeIdByContainedModificationId
currently returns a single UUID but the query on
composite_modification_sub_modifications can yield multiple sm.id rows; change
the repository method signature to return List<UUID> (or List<String> if
casting) so all matching composite IDs are returned, and then update the
caller(s) to validate the result: if size==0 treat as "no owner", if size==1 use
that UUID, and if size>1 throw a domain error (MODIFICATION_ERROR) with a clear
message indicating ambiguous ownership; keep references to the repository method
name (findCompositeIdByContainedModificationId), the
composite_modification_sub_modifications table and sm.id in your validation
logic so it’s easy to trace.
Comment on lines
+915
to
+929
| final UUID firstModifUuid = assembledModificationsUuids.getFirst(); | ||
| final ModificationEntity firstModificationEntity = getModificationEntity(firstModifUuid); | ||
| final int targetIndex = firstModificationEntity.getModificationsOrder(); | ||
| ModificationGroupEntity targetGroup = firstModificationEntity.getGroup(); | ||
| CompositeModificationEntity targetComposite = null; | ||
| if (targetGroup == null) { | ||
| // the first modification is inside a composite | ||
| UUID targetCompositeUuid = modificationRepository.findCompositeIdByContainedModificationId(firstModifUuid); | ||
| targetComposite = compositeModificationRepository.findById(targetCompositeUuid).orElse(null); | ||
| } | ||
|
|
||
| // get all the modifications to be assembled, remove previous assignment | ||
| List<ModificationEntity> assembledModifications = assembledModificationsUuids.stream() | ||
| .map(modificationRepository::findById).filter(Optional::isPresent).map(Optional::get).toList(); | ||
| // 1. cleans and reorders the origin group if there is one : |
There was a problem hiding this comment.
Validate input list and fail on missing UUIDs instead of partial assembly.
Current flow throws on empty list (getFirst) and silently drops missing IDs (filter(Optional::isPresent)), which can produce partial, surprising results.
Suggested validation guard
+ if (assembledModificationsUuids == null || assembledModificationsUuids.isEmpty()) {
+ throw new NetworkModificationException(MODIFICATION_ERROR, "No modifications provided for assembly");
+ }
...
- List<ModificationEntity> assembledModifications = assembledModificationsUuids.stream()
- .map(modificationRepository::findById).filter(Optional::isPresent).map(Optional::get).toList();
+ List<ModificationEntity> assembledModifications = assembledModificationsUuids.stream()
+ .map(uuid -> modificationRepository.findById(uuid)
+ .orElseThrow(() -> new NetworkModificationException(MODIFICATION_NOT_FOUND,
+ String.format(MODIFICATION_NOT_FOUND_MESSAGE, uuid))))
+ .toList();📝 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
| final UUID firstModifUuid = assembledModificationsUuids.getFirst(); | |
| final ModificationEntity firstModificationEntity = getModificationEntity(firstModifUuid); | |
| final int targetIndex = firstModificationEntity.getModificationsOrder(); | |
| ModificationGroupEntity targetGroup = firstModificationEntity.getGroup(); | |
| CompositeModificationEntity targetComposite = null; | |
| if (targetGroup == null) { | |
| // the first modification is inside a composite | |
| UUID targetCompositeUuid = modificationRepository.findCompositeIdByContainedModificationId(firstModifUuid); | |
| targetComposite = compositeModificationRepository.findById(targetCompositeUuid).orElse(null); | |
| } | |
| // get all the modifications to be assembled, remove previous assignment | |
| List<ModificationEntity> assembledModifications = assembledModificationsUuids.stream() | |
| .map(modificationRepository::findById).filter(Optional::isPresent).map(Optional::get).toList(); | |
| // 1. cleans and reorders the origin group if there is one : | |
| if (assembledModificationsUuids == null || assembledModificationsUuids.isEmpty()) { | |
| throw new NetworkModificationException(MODIFICATION_ERROR, "No modifications provided for assembly"); | |
| } | |
| final UUID firstModifUuid = assembledModificationsUuids.getFirst(); | |
| final ModificationEntity firstModificationEntity = getModificationEntity(firstModifUuid); | |
| final int targetIndex = firstModificationEntity.getModificationsOrder(); | |
| ModificationGroupEntity targetGroup = firstModificationEntity.getGroup(); | |
| CompositeModificationEntity targetComposite = null; | |
| if (targetGroup == null) { | |
| // the first modification is inside a composite | |
| UUID targetCompositeUuid = modificationRepository.findCompositeIdByContainedModificationId(firstModifUuid); | |
| targetComposite = compositeModificationRepository.findById(targetCompositeUuid).orElse(null); | |
| } | |
| // get all the modifications to be assembled, remove previous assignment | |
| List<ModificationEntity> assembledModifications = assembledModificationsUuids.stream() | |
| .map(uuid -> modificationRepository.findById(uuid) | |
| .orElseThrow(() -> new NetworkModificationException(MODIFICATION_NOT_FOUND, | |
| String.format(MODIFICATION_NOT_FOUND_MESSAGE, uuid)))) | |
| .toList(); | |
| // 1. cleans and reorders the origin group if there is one : |
🤖 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 915 - 929, The code currently assumes assembledModificationsUuids
has elements and silently drops missing IDs when mapping via
modificationRepository.findById; add input validation at the start of the method
to (1) check assembledModificationsUuids is non-empty and throw a clear
IllegalArgumentException (or domain-specific exception) if empty instead of
calling getFirst(), and (2) after mapping UUIDs with
modificationRepository.findById, verify that the count of found
ModificationEntity objects equals the input UUID list size and if not throw an
exception listing the missing UUIDs (compare assembledModificationsUuids to the
found IDs) rather than using filter(Optional::isPresent) to allow partial
assembly; update references to assembledModificationsUuids,
getModificationEntity, modificationRepository.findById and the
assembledModifications list to use this validation.
Comment on lines
+917
to
+974
| final int targetIndex = firstModificationEntity.getModificationsOrder(); | ||
| ModificationGroupEntity targetGroup = firstModificationEntity.getGroup(); | ||
| CompositeModificationEntity targetComposite = null; | ||
| if (targetGroup == null) { | ||
| // the first modification is inside a composite | ||
| UUID targetCompositeUuid = modificationRepository.findCompositeIdByContainedModificationId(firstModifUuid); | ||
| targetComposite = compositeModificationRepository.findById(targetCompositeUuid).orElse(null); | ||
| } | ||
|
|
||
| // get all the modifications to be assembled, remove previous assignment | ||
| List<ModificationEntity> assembledModifications = assembledModificationsUuids.stream() | ||
| .map(modificationRepository::findById).filter(Optional::isPresent).map(Optional::get).toList(); | ||
| // 1. cleans and reorders the origin group if there is one : | ||
| ModificationGroupEntity originGroup = assembledModifications.stream() | ||
| .map(ModificationEntity::getGroup) | ||
| .filter(Objects::nonNull) | ||
| .findFirst() | ||
| .orElse(null); | ||
| if (originGroup != null) { | ||
| List<ModificationEntity> originGroupModifications = originGroup.getModifications(); | ||
| originGroupModifications.removeIf(mod -> assembledModificationsUuids.contains(mod.getId())); | ||
| originGroup.setModifications(originGroupModifications); | ||
| assembledModifications.forEach(modificationEntity -> modificationEntity.setGroup(null)); | ||
| } | ||
| // 2. cleans the composites whose submodifications are assembled into a new one | ||
| for (ModificationEntity assembledModification : assembledModifications.stream().filter(mod -> mod.getGroup() == null).toList()) { | ||
| UUID compositeUuid = modificationRepository.findCompositeIdByContainedModificationId(assembledModification.getId()); | ||
| if (compositeUuid != null) { | ||
| CompositeModificationEntity previousOwner = compositeModificationRepository.findById(compositeUuid).orElse(null); | ||
| if (previousOwner != null) { | ||
| List<ModificationEntity> modificationsLeft = previousOwner.getModifications() | ||
| .stream() | ||
| .filter(mod -> !assembledModificationsUuids.contains(mod.getId())) | ||
| .toList(); | ||
| previousOwner.setModifications(modificationsLeft); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // create the new composite | ||
| CompositeModificationInfos newCompositeInfos = CompositeModificationInfos.builder() | ||
| .modificationsInfos(List.of()) | ||
| .name("Composite modification") | ||
| .build(); | ||
| CompositeModificationEntity newCompositeEntity = (CompositeModificationEntity) ModificationEntity.fromDTO(newCompositeInfos); | ||
| newCompositeEntity.setModificationsOrder(targetIndex); | ||
|
|
||
| // assign modifications | ||
| newCompositeEntity.setModifications(assembledModifications); | ||
| // put the new composite in the target group or composite | ||
| if (targetGroup != null) { | ||
| List<ModificationEntity> modifications = targetGroup.getModifications(); | ||
| modifications.add(targetIndex, newCompositeEntity); | ||
| targetGroup.setModifications(modifications); | ||
| } else if (targetComposite != null) { | ||
| List<ModificationEntity> modifications = targetComposite.getModifications(); | ||
| modifications.add(targetIndex, newCompositeEntity); | ||
| for (int i = 0; i < targetComposite.getModifications().size(); i++) { |
There was a problem hiding this comment.
Recompute insertion index after removals to avoid IndexOutOfBoundsException.
targetIndex is captured before detaching/removing assembled items. If items before targetIndex are removed from the same target list, add(targetIndex, ...) can fail or place the composite at the wrong position.
Suggested fix pattern
- final int targetIndex = firstModificationEntity.getModificationsOrder();
+ final int originalTargetIndex = firstModificationEntity.getModificationsOrder();
...
- modifications.add(targetIndex, newCompositeEntity);
+ long removedBefore = assembledModifications.stream()
+ .filter(m -> m.getGroup() != null && m.getGroup().getId().equals(targetGroup.getId()))
+ .filter(m -> m.getModificationsOrder() < originalTargetIndex)
+ .count();
+ int insertionIndex = Math.max(0, originalTargetIndex - (int) removedBefore);
+ modifications.add(Math.min(insertionIndex, modifications.size()), newCompositeEntity);Apply equivalent logic in the targetComposite branch.
🤖 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 917 - 974, The code captures targetIndex before removals so calling
modifications.add(targetIndex, newCompositeEntity) can throw IndexOutOfBounds or
insert at wrong place; before inserting into targetGroup.getModifications() or
targetComposite.getModifications() recompute a safe insertion index (e.g., int
insertionIndex = Math.min(targetIndex, modifications.size()); or recalc relative
position from the current list) and use modifications.add(insertionIndex,
newCompositeEntity); update both the targetGroup and targetComposite branches
where targetIndex and newCompositeEntity are used.
souissimai
approved these changes
May 29, 2026
souissimai
reviewed
May 29, 2026
| // the first modification is inside a composite | ||
| UUID targetCompositeUuid = modificationRepository.findCompositeIdByContainedModificationId(firstModifUuid); | ||
| targetComposite = compositeModificationRepository.findById(targetCompositeUuid).orElse(null); | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion :
It can be refactored maybe , extract small methods to simplify
|
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.
assemble some modifications into a new composite
-> new endpoint which received the list of modifications to be assembled and return the uuid of the new composite.