import shared composites#830
Conversation
…ion-server into shared-modification
…ion-server into shared-modification
…ion-server into shared-modification
…ion-server into shared-modification
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>
…ion-server into shared-modification
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>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java (1)
717-723: ⚡ Quick winBatch-load references data instead of issuing one query per UUID.
Line 719–723 performs one
findByIdcall per UUID, which creates an N+1 query pattern for large requests. This endpoint is list-based; loading once withfindAllByIdIn(...)and then filtering in-memory will reduce DB round-trips.🤖 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 717 - 723, The getReferencesData method is experiencing an N+1 query problem where each iteration of the loop calls findById on the modificationRepository for individual UUIDs. Instead of looping through modificationUuids and making separate findById calls, refactor this to use a single batch query by calling findAllByIdIn with the entire modificationUuids list. After retrieving all records at once, filter the results in-memory to identify which ones are not found (throw the exception if expected records are missing), and then apply the existing filtering logic checking for stashed status and ModificationReferenceEntity type. This approach reduces database round-trips from N queries to a single query.src/test/java/org/gridsuite/modification/server/service/ModificationIndexationTest.java (1)
311-318: ⚡ Quick winAdd coverage for the new shared-reference path.
This test only exercises
isShared=false(split behavior). The new shared insertion branch and reference lookup flow remain untested, so regressions in the new feature can pass unnoticed.🤖 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/ModificationIndexationTest.java` around lines 311 - 318, The test in ModificationIndexationTest currently only covers the case where isShared is set to false in the CompositesToBeInserted constructor, leaving the new shared insertion branch and reference lookup flow untested. Add an additional test case or extend the existing test to also exercise the isShared=true path by creating another CompositesToBeInserted instance with isShared set to true, and verify that the splitCompositeModifications method correctly handles the shared reference behavior. This ensures that both the split behavior (isShared=false) and the shared insertion/reference lookup behavior (isShared=true) are properly tested to catch potential regressions.
🤖 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/NetworkModificationController.java`:
- Around line 266-269: The issue is that the getNetworkModifications call on
line 266 only retrieves root-level modifications, missing references inside
composite children. To fix this, you need to modify the logic to include nested
modifications from composites in the netModUuids list before passing it to
getReferencesData. Either adjust the parameters passed to
getNetworkModifications to include composite children modifications, or add
logic to extract and collect all nested modification UUIDs from composite
modifications in addition to the root ones, ensuring that all modification UUIDs
(root and nested) are included in the referencesData mapping.
In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`:
- Around line 967-994: The code casts to CompositeModificationInfos without type
validation on lines 972 and 986, which can cause ClassCastException if the UUID
refers to a non-composite modification. Additionally, when composite
modifications are not found, the shared branch silently skips them while the
non-shared branch only logs, both allowing partial inserts to succeed. Add
instanceof checks before both cast operations to validate the type, and throw an
exception (instead of returning null or continuing silently) when a required
composite modification cannot be found to ensure the operation fails completely
rather than succeeding partially.
---
Nitpick comments:
In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`:
- Around line 717-723: The getReferencesData method is experiencing an N+1 query
problem where each iteration of the loop calls findById on the
modificationRepository for individual UUIDs. Instead of looping through
modificationUuids and making separate findById calls, refactor this to use a
single batch query by calling findAllByIdIn with the entire modificationUuids
list. After retrieving all records at once, filter the results in-memory to
identify which ones are not found (throw the exception if expected records are
missing), and then apply the existing filtering logic checking for stashed
status and ModificationReferenceEntity type. This approach reduces database
round-trips from N queries to a single query.
In
`@src/test/java/org/gridsuite/modification/server/service/ModificationIndexationTest.java`:
- Around line 311-318: The test in ModificationIndexationTest currently only
covers the case where isShared is set to false in the CompositesToBeInserted
constructor, leaving the new shared insertion branch and reference lookup flow
untested. Add an additional test case or extend the existing test to also
exercise the isShared=true path by creating another CompositesToBeInserted
instance with isShared set to true, and verify that the
splitCompositeModifications method correctly handles the shared reference
behavior. This ensures that both the split behavior (isShared=false) and the
shared insertion/reference lookup behavior (isShared=true) are properly tested
to catch potential regressions.
🪄 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: 3f451ef4-e13e-4818-895a-466c1eb11f5d
📒 Files selected for processing (6)
src/main/java/org/gridsuite/modification/server/CompositeController.javasrc/main/java/org/gridsuite/modification/server/NetworkModificationController.javasrc/main/java/org/gridsuite/modification/server/dto/CompositesToBeInserted.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/service/ModificationIndexationTest.java
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>
| /** | ||
| * filters out the netmods which are not references and returns the references data as : | ||
| * referenced element uuid -> container of the reference (uuid of the composite if there is one, null if it is at the root level) | ||
| */ |
There was a problem hiding this comment.
Why using the word data as we just return a map ?
What is a reference ?
For me the names and comments are not really clear. It's hard to understand what does the endpoints by reading them.
There was a problem hiding this comment.
Data is simply a generic name. A Map is data. But I can write map instead of data if you want.
The comment says what is returned. Do you want me to precise what an element is ? To say that this is an element from directory server ? Or something else ?
There was a problem hiding this comment.
For this one, I would write, for exemple :
From a list of network modification UUIDs, apply a filter and return the ones that are of type reference, mapped to the UUID of the container that owns them.
But there is probably better.
The thing is everything is data, so it does make it easier to undertstand and is a bit useless for me.
I would prefer nothing or map indeed, but lets wait what think the other reviewer.
There was a problem hiding this comment.
I removed data.
But I didn't change the comment because the important part is the one about composites and null if the references doesn't target a composite.
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
…port-shared-composite
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>
|



Uh oh!
There was an error while loading. Please reload this page.