Delete modifications by their ids, wherever they sit by flomillot · Pull Request #943 · gridsuite/network-modification-server

flomillot · 2026-10-19T15:42:00Z

DELETE /network-modifications now takes only the ids of the modifications to delete: the groupUuid parameter is removed, along with the deletion of a whole group content, which had no client. A modification is found by id whether it sits in a group, in a composite or is shared. Unknown ids are ignored instead of answering 404 when none is found, like deleting an unknown group.

Deleting modifications now removes them from the loaded container holding them, instead of each caller doing it, and reads the content of the deleted composites in one query instead of two per level.

Deleting modifications, groups or stashed modifications, and replacing a composite content, now unlink the deleted modification references from directory-server, nested ones included. This fixes the references left in directory-server when they were deleted inside a composite, e.g. when deleting or replacing a shared composite. The references are unlinked once the deletion is committed, not to hold the transaction during the calls to directory-server.

Drop the group parameter of DELETE /network-modifications, and the unused deletion of a whole group content: the modifications are found by id, in a group, nested in a composite or shared. Deleting a modification removes it from the loaded container holding it, and reads the content of the deleted composites in one query instead of one per level. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>

coderabbitai · 2026-10-19T15:42:51Z

Important

🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 4498ffb3-fcc5-4356-b004-97b9b003e06f

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Return early on an empty list instead of guarding each caller, read the composite ids straight from the deleted modifications, and check from outside whether a container content is loaded, so that an uninitialized proxy is not loaded by the check. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

Explain the PERSIST cascade that would cancel the deletion, split the container filter, and drop the replace composite test. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

…st it with the existing deletion test Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

Copilot

🟡 Changes recommended

Deletion ordering can violate foreign keys, and late user validation can leave Elasticsearch inconsistent after rejected requests.

2 open issues

What changed in this PR

This PR makes modification deletion ID-based across groups, composites, and shared modifications, and extends directory-reference cleanup to nested content.

Changes:

Removes group-scoped deletion parameters and ignores unknown IDs.
Centralizes container cleanup and batches composite-descendant reads.
Unlinks deleted references and updates API tests and query counts.

File	Description
src/test/java/org/gridsuite/modification/server/utils/ApiUtils.java	Removes group-scoped deletion helpers.
src/test/java/org/gridsuite/modification/server/service/ModificationRepositoryTest.java	Updates deletion calls and query counts.
src/test/java/org/gridsuite/modification/server/service/ModificationIndexationTest.java	Updates service calls.
src/test/java/org/gridsuite/modification/server/modifications/tabularmodifications/TabularGeneratorModificationsTest.java	Adapts tabular deletion tests.
src/test/java/org/gridsuite/modification/server/modifications/tabularcreations/TabularGeneratorCreationsTest.java	Updates repository deletion call.
src/test/java/org/gridsuite/modification/server/ModificationControllerTest.java	Tests ID-based deletion and reference cleanup.
src/main/java/org/gridsuite/modification/server/service/NetworkModificationService.java	Adds transactional reference cleanup.
src/main/java/org/gridsuite/modification/server/service/DirectoryService.java	Tolerates missing elements when unlinking.
src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java	Centralizes subtree deletion and container cleanup.
src/main/java/org/gridsuite/modification/server/repositories/ModificationRepository.java	Adds descendant-entity query.
src/main/java/org/gridsuite/modification/server/NetworkModificationController.java	Updates deletion parameters and user forwarding.
src/main/java/org/gridsuite/modification/server/error/NetworkModificationExceptionHandler.java	Removes obsolete error mappings.
src/main/java/org/gridsuite/modification/server/error/ModificationBusinessErrorCode.java	Removes obsolete deletion errors.
src/main/java/org/gridsuite/modification/server/entities/AbstractModificationContainerEntity.java	Adds loaded-content detection.

_{Review effort: Balanced}

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

ghazwarhili · 2026-10-22T14:04:53Z

+        if (deletedReferences.isEmpty()) {
+            return;
        }
+        if (userId == null) {


Delete shared composite from gridexplore now fails with 400 error
(userId is missing in the endpoint header of explore-server)

It works for me, userId is supposed to be propagated.
https://github.com/gridsuite/explore-server/blob/cf7b15ce9b16ef504ae572514dacd31fd45672f8/src/main/java/org/gridsuite/explore/server/config/RestClientConfig.java#L74
Maybe update your service ?

ghazwarhili · 2026-10-22T14:13:51Z

        deleteIndexedModificationGroups(groupUuids);
-        networkModificationRepository.deleteModificationGroups(groupUuids);
+        List<ModificationReferenceData> deletedReferences = networkModificationRepository.deleteModificationGroups(groupUuids);
+        removeDeletedElementReferences(deletedReferences, userId);


references deleted with one directory-server HTTP call inside Transactional method !?

Yeah now that it's Transactional I should refactor this too, thanks

ghazwarhili · 2026-10-22T14:16:21Z

+    @Transactional
+    public void deleteNetworkModifications(@NonNull List<UUID> modificationsUuids, String userId) {
+        List<ModificationReferenceData> deletedReferences = networkModificationRepository.deleteModifications(modificationsUuids);
+        removeDeletedElementReferences(deletedReferences, userId);


the same remark here (HTTP call inside DB transaction)
another remark : in this case if directory server returns 404 error for exemple ?

You mean this ?
https://github.com/gridsuite/network-modification-server/pull/943/changes/BASE..1be880589753791905eccb1b81f628b63bd89aa1#diff-b53a4d49d0775fd8983fe217f5e97cb3012086d77e790463643f80e6f1668acbR113

…on is committed Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

ghazwarhili · 2026-10-23T08:50:31Z

#943 (review) Could you fix Copilot’s feedback please @flomillot ?

basseche · 2026-10-23T09:26:02Z

+    @Query("""
+        WITH descendants AS (
+            SELECT m.id AS id FROM ModificationEntity m WHERE m.container.id IN (:containerIds)
+            UNION ALL


with Union All we can have duplicate ids. Shouldn't we use Union instead ?

Based on findAllDescendantModificationIdsByContainerIds, that was already with UNION ALL, but your're right.
I change then both.
Also add a .distinct().

flomillot · 2026-10-23T09:45:55Z

#943 (review) Could you fix Copilot’s feedback please @flomillot ?

I answered, no fix needed

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

…ications-by-list Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com> # Conflicts: # src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java # src/test/java/org/gridsuite/modification/server/ModificationControllerTest.java

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

basseche · 2026-10-23T12:52:51Z

+                    .retrieve()
+                    .toBodilessEntity();
+        } catch (HttpClientErrorException.NotFound _) {
+            // the shared element is gone, and its references with it


What about here ? Log or throw ?

Can add a log but I dont think its necessary

ghazwarhili · 2026-10-23T14:30:56Z


    @Transactional
-    public void deleteModificationGroups(List<UUID> groupUuids) {
+    public void deleteModificationGroups(List<UUID> groupUuids, String userId) {


here we remove the group in elasticsearch before the userId header check => the group could stay in the database with no index

=> too risky (I think we have a design issue here)

Same problem everywhere in the repos

ghazwarhili · 2026-10-23T14:36:37Z

+    /**
+     * After the commit, not to hold the deletion transaction during the calls to directory-server
+     */
+    @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)


@TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)

We haven’t used it anywhere else in the code, so we might need TL approval before introducing it here

@etiennehomer @SlimaneAmar

ghazwarhili · 2026-10-23T14:46:11Z

+        if (deletedReferences.isEmpty()) {
+            return;
        }
+        if (userId == null) {


i'm not sure about this check here it seems too late it should be done up front maybe or by making the header required before any work starts

=> I don't know it seems a design prb with this userId check

It's OK because I kept the Transactional, so everything is canceled (except ElasticSearch as you said). If I do it before, I need to do a recursive query in the DB just for that check.

ghazwarhili · 2026-10-23T15:07:27Z

    public ResponseEntity<Void> deleteStashedModificationFromGroups(@Parameter(description = "Group UUIDs") @RequestBody List<UUID> groupUuids,
                                                                   @RequestHeader(HEADER_USER_ID) String userId) {
-        networkModificationService.deleteStashedModificationFromGroups(groupUuids);
+        networkModificationService.deleteStashedModificationFromGroups(groupUuids, userId);


deleteStashedModificationFromGroups +
removeElementReferences(groupUuid, userId) for each groupuuid ?

=> delete stashed and active modifications ?

The links are deleted and it's a bug. It's cleaned and removed in #944.

ghazwarhili · 2026-10-23T15:15:49Z

-                String.format("Some of these modifications %s (to be deleted) were not found", modificationsUuids),
-                Map.of("ids", modificationsUuids));
+    @Transactional
+    public void deleteNetworkModifications(@NonNull List<UUID> modificationsUuids, String userId) {


MODIFICATIONS_NOT_FOUND and
MODIFICATION_WITH_GROUP_DELETION_FORBIDDEN are removed

=> deleting non existent modifications now returns 200 instead of 404, the group-deletion safe guard removed
=> The refacto changes the API behavior in my opinion
=> should be explicitly documented in the PR and validated with PO and TL maybe

As said above I asked the TL before removing the group guard, it's validated.
Will be read by code owner anyway.
Its already in the PR description : "Unknown ids are ignored instead of answering 404 when none is found, like deleting an unknown group." So now both endpoint are coherent.

ghazwarhili · 2026-10-23T15:17:28Z

        WITH descendants AS (
            SELECT m.id AS id FROM ModificationEntity m WHERE m.container.id IN (:containerIds)
-            UNION ALL
+            UNION


UNION ALL changes to UNION why ?

#943 (comment)

ghazwarhili · 2026-10-23T15:19:11Z

+        )
+        SELECT m FROM ModificationEntity m JOIN descendants d ON m.id = d.id
+        """)
+    List<ModificationEntity> findAllDescendantModificationsByContainerIds(@Param("containerIds") Collection<UUID> containerIds);


it is seems the same or a duplicated query like findAllDescendantModificationIdsByContainerIds

One return the entities, the other one only the IDs. It avoid a JOIN and a stream later on.

…ications-by-list Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com> # Conflicts: # src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java

sonarqubecloud · 2026-10-23T16:15:46Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
97.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

flomillot requested review from EtienneLt, SlimaneAmar and etiennehomer as code owners October 19, 2026 15:42

flomillot added 6 commits October 20, 2026 13:21

Clarify why deleted modifications leave their loaded containers

802c070

Explain the PERSIST cascade that would cancel the deletion, split the container filter, and drop the replace composite test. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

Unlink deleted modification references from directory-server on deletion

26fd7b9

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

Ignore unknown modifications on deletion and extract the references data

62f2e3d

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

Clarify the names of the deleted references and modifications

ae5975b

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

Roll back the deletion when the references cannot be unlinked, and te…

44e1cb4

…st it with the existing deletion test Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

etiennehomer requested a balanced review from Copilot October 21, 2026 10:50

Copilot started reviewing on behalf of etiennehomer October 21, 2026 10:50

View session

Copilot AI reviewed Oct 21, 2026

View reviewed changes

Comment thread src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java

Comment thread src/main/java/org/gridsuite/modification/server/service/NetworkModificationService.java

Remove or shorten comments restating the code

1be8805

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

ghazwarhili requested changes Oct 22, 2026

View reviewed changes

Unlink the deleted references from directory-server once their deleti…

45ec9c8

…on is committed Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

flomillot requested a review from ghazwarhili October 23, 2026 07:58

basseche self-requested a review October 23, 2026 08:51

basseche reviewed Oct 23, 2026

View reviewed changes

Comment thread src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java

flomillot added 3 commits October 23, 2026 11:54

Avoid duplicates among the modifications to delete

c083163

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

Make explicit that the modifications to delete are never null

8617a81

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

flomillot requested a review from basseche October 23, 2026 10:38

Pass the user when deleting a modification in the common test

ab32e32

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

basseche reviewed Oct 23, 2026

View reviewed changes

ghazwarhili requested changes Oct 23, 2026

View reviewed changes

ghazwarhili reviewed Oct 23, 2026

View reviewed changes

flomillot requested review from basseche and ghazwarhili October 23, 2026 15:59

Merge remote-tracking branch 'origin/main' into refactor/delete-modif…

5187a7f

…ications-by-list Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com> # Conflicts: # src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java

Conversation

flomillot commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flomillot Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghazwarhili commented Oct 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flomillot commented Oct 23, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flomillot Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flomillot Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flomillot Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flomillot Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Oct 23, 2026

Quality Gate passed

Uh oh!

This branch has not been deployed

flomillot commented Oct 19, 2026 •

edited

Loading

coderabbitai Bot commented Oct 19, 2026 •

edited

Loading

flomillot Oct 22, 2026 •

edited

Loading

flomillot Oct 23, 2026 •

edited

Loading

flomillot Oct 23, 2026 •

edited

Loading

flomillot Oct 23, 2026 •

edited

Loading

flomillot Oct 23, 2026 •

edited

Loading