Modifications applications: split long running transaction before/after the computations (#690)

This requires to reorganize the code and the transactions the fully load the data,
hence the changes in NetworkModificationRepository with the new intermediate methods.

Add testInsertCompositeModifications for symmetry with other code paths duplicate/move/
in the switch/case of ModificationController ?action=...

Co-authored-by: Slimane AMAR <[email protected]>

Author: jonenst

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

@@ -106,17 +106,20 @@ public void deleteAll() {
     }
 
     @Transactional // To have all create in the same transaction (atomic)
-    // TODO Remove transaction when errors will no longer be sent to the front
     // This method should be package-private and not used as API of the service as it uses ModificationEntity and
     // we want to encapsulate the use of Entity related objects to this service.
     // Nevertheless We have to keep it public for transactional annotation.
     public List<ModificationEntity> saveModifications(UUID groupUuid, List<ModificationEntity> modifications) {
         return saveModificationsNonTransactional(groupUuid, modifications);
     }
 
-    @Transactional // To have all create in the same transaction (atomic)
-    // TODO Remove transaction when errors will no longer be sent to the front
+    @Transactional
     public List<ModificationEntity> saveModificationInfos(UUID groupUuid, List<ModificationInfos> modifications) {
+        return saveModificationInfosNonTransactional(groupUuid, modifications);
+    }
+
+    private List<ModificationEntity> saveModificationInfosNonTransactional(UUID groupUuid,
+            List<ModificationInfos> modifications) {
         List<ModificationEntity> entities = modifications.stream().map(ModificationEntity::fromDTO).toList();
 
         return saveModificationsNonTransactional(groupUuid, entities);
@@ -165,8 +168,13 @@ private List<ModificationEntity> saveModificationsNonTransactional(UUID groupUui
     }
 
     @Transactional
-    // TODO Remove transaction when errors will no longer be sent to the front
     public List<ModificationEntity> moveModifications(UUID destinationGroupUuid, UUID originGroupUuid, List<UUID> modificationsToMoveUUID, UUID referenceModificationUuid) {
+        List<ModificationEntity> movedModifications = moveModificationsNonTransactional(destinationGroupUuid, originGroupUuid, modificationsToMoveUUID, referenceModificationUuid);
+        loadFullModificationsEntities(movedModifications);
+        return movedModifications;
+    }
+
+    private List<ModificationEntity> moveModificationsNonTransactional(UUID destinationGroupUuid, UUID originGroupUuid, List<UUID> modificationsToMoveUUID, UUID referenceModificationUuid) {
         // read origin group and modifications
         ModificationGroupEntity originModificationGroupEntity = getModificationGroup(originGroupUuid);
         List<ModificationEntity> originModificationEntities = originModificationGroupEntity.getModifications()
@@ -273,7 +281,7 @@ public List<ModificationInfos> getModifications(UUID groupUuid, boolean onlyMeta
 
     public List<ModificationInfos> getModifications(UUID groupUuid, boolean onlyMetadata, boolean errorOnGroupNotFound, boolean onlyStashed) {
         try {
-            return onlyMetadata ? getModificationsMetadata(groupUuid, onlyStashed) : getModificationsEntities(List.of(groupUuid), onlyStashed).stream().map(this::getModificationInfos).toList();
+            return onlyMetadata ? getModificationsMetadata(groupUuid, onlyStashed) : getModificationsInfos(List.of(groupUuid), onlyStashed);
         } catch (NetworkModificationException e) {
             if (e.getType() == MODIFICATION_GROUP_NOT_FOUND && !errorOnGroupNotFound) {
                 return List.of();
@@ -432,7 +440,7 @@ private TabularCreationInfos loadTabularCreation(ModificationEntity modification
             .build();
     }
 
-    public ModificationInfos getModificationInfos(ModificationEntity modificationEntity) {
+    private ModificationInfos getModificationInfos(ModificationEntity modificationEntity) {
         if (modificationEntity instanceof TabularModificationEntity) {
             return loadTabularModification(modificationEntity);
         } else if (modificationEntity instanceof TabularCreationEntity) {
@@ -441,7 +449,7 @@ public ModificationInfos getModificationInfos(ModificationEntity modificationEnt
         return modificationEntity.toModificationInfos();
     }
 
-    public List<ModificationEntity> getModificationsEntities(List<UUID> groupUuids, boolean onlyStashed) {
+    private List<ModificationEntity> getModificationsEntitiesNonTransactional(List<UUID> groupUuids, boolean onlyStashed) {
         Stream<ModificationEntity> entityStream = groupUuids.stream().flatMap(this::getModificationEntityStream);
         if (onlyStashed) {
             return entityStream.filter(m -> m.getStashed() == onlyStashed).toList();
@@ -450,6 +458,26 @@ public List<ModificationEntity> getModificationsEntities(List<UUID> groupUuids,
         }
     }
 
+    @Transactional(readOnly = true)
+    public List<ModificationEntity> getModificationsEntities(List<UUID> groupUuids, boolean onlyStashed) {
+        List<ModificationEntity> modificationsEntities = getModificationsEntitiesNonTransactional(groupUuids, onlyStashed);
+        loadFullModificationsEntities(modificationsEntities);
+        return modificationsEntities;
+    }
+
+    private void loadFullModificationsEntities(List<ModificationEntity> modificationsEntities) {
+        // Force load subentities/collections, needed later when the transaction is closed
+        // Necessary for applying network modifications
+        // to avoid LazyInitialisationException. Maybe better to refactor to return the dto ?
+        // And refactor to more efficiently load the data (avoid 1+N) ?
+        modificationsEntities.forEach(this::getModificationInfos);
+    }
+
+    private List<ModificationInfos> getModificationsInfos(List<UUID> groupUuids, boolean onlyStashed) {
+        return getModificationsEntitiesNonTransactional(groupUuids, onlyStashed).stream()
+            .map(this::getModificationInfos).toList();
+    }
+
     @Transactional(readOnly = true)
     public ModificationInfos getModificationInfo(UUID modificationUuid) {
         return getModificationInfos(getModificationEntity(modificationUuid));
@@ -523,6 +551,10 @@ public Integer getModificationsCount(@NonNull UUID groupUuid, boolean stashed) {
 
     @Transactional(readOnly = true)
     public List<ModificationInfos> getModificationsInfos(@NonNull List<UUID> uuids) {
+        return getModificationsInfosNonTransactional(uuids);
+    }
+
+    private List<ModificationInfos> getModificationsInfosNonTransactional(List<UUID> uuids) {
         // Spring-data findAllById doc says: the order of elements in the result is not guaranteed
         Map<UUID, ModificationEntity> entities = modificationRepository.findAllById(uuids)
             .stream()
@@ -551,6 +583,10 @@ public List<ModificationInfos> getBasicNetworkModificationsFromComposite(@NonNul
 
     @Transactional(readOnly = true)
     public List<ModificationInfos> getCompositeModificationsInfos(@NonNull List<UUID> uuids) {
+        return getCompositeModificationsInfosNonTransactional(uuids);
+    }
+
+    private List<ModificationInfos> getCompositeModificationsInfosNonTransactional(@NonNull List<UUID> uuids) {
         List<ModificationInfos> entities = new ArrayList<>();
         uuids.forEach(uuid -> {
             List<UUID> foundEntities = modificationRepository.findModificationIdsByCompositeModificationId(uuid);
@@ -566,6 +602,10 @@ public List<ModificationInfos> getCompositeModificationsInfos(@NonNull List<UUID
 
     @Transactional(readOnly = true)
     public List<ModificationInfos> getActiveModificationsInfos(@NonNull UUID groupUuid) {
+        return getActiveModificationsInfosNonTransactional(groupUuid);
+    }
+
+    private List<ModificationInfos> getActiveModificationsInfosNonTransactional(UUID groupUuid) {
         return getModificationEntityStream(groupUuid).filter(m -> !m.getStashed()).map(this::getModificationInfos).toList();
     }
 
@@ -787,4 +827,16 @@ private void deleteTabularModificationSubModifications(TabularModificationEntity
         // line function works for any modification case
         lineModificationRepository.deleteTabularModificationModifications(modificationId, subModificationsIds);
     }
+
+    @Transactional
+    public List<ModificationEntity> saveDuplicateModifications(@NonNull UUID targetGroupUuid, UUID originGroupUuid, @NonNull List<UUID> modificationsUuids) {
+        List<ModificationInfos> modificationInfos = originGroupUuid != null ? getActiveModificationsInfosNonTransactional(originGroupUuid) : getModificationsInfosNonTransactional(modificationsUuids);
+        return saveModificationInfosNonTransactional(targetGroupUuid, modificationInfos);
+    }
+
+    @Transactional
+    public List<ModificationEntity> saveCompositeModifications(@NonNull UUID targetGroupUuid, @NonNull List<UUID> modificationsUuids) {
+        List<ModificationInfos> modificationInfos = getCompositeModificationsInfosNonTransactional(modificationsUuids);
+        return saveModificationInfosNonTransactional(targetGroupUuid, modificationInfos);
+    }
 }

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

@@ -191,9 +191,6 @@ public void restoreNetworkModifications(UUID groupUuid, @NonNull List<UUID> modi
             networkModificationRepository.getModificationsCount(groupUuid, false));
     }
 
-    // No transactional because we need to save modification in DB also in case of error
-    // Transaction made in 'saveModifications' method
-    // TODO Add transaction when errors will no longer be sent to the front
     public NetworkModificationsResult createNetworkModification(@NonNull UUID groupUuid, @NonNull ModificationInfos modificationInfo, @NonNull List<ModificationApplicationContext> applicationContexts) {
         List<ModificationEntity> modificationEntities = networkModificationRepository.saveModificationInfos(groupUuid, List.of(modificationInfo));
 
@@ -237,7 +234,6 @@ public Network cloneNetworkVariant(UUID networkUuid,
         return network;
     }
 
-    @Transactional
     public NetworkModificationResult buildVariant(@NonNull UUID networkUuid, @NonNull BuildInfos buildInfos) {
         // Apply all modifications belonging to the modification groups uuids in buildInfos
         List<ModificationApplicationGroup> modificationGroupsInfos = new ArrayList<>();
@@ -246,6 +242,7 @@ public NetworkModificationResult buildVariant(@NonNull UUID networkUuid, @NonNul
                 Set<UUID> modificationsToExclude = buildInfos.getModificationUuidsToExclude().get(groupUuid);
                 List<ModificationEntity> modifications = List.of();
                 try {
+                    // FullDto needed for toModificationInfos() after the modifications have been applied
                     modifications = networkModificationRepository.getModificationsEntities(List.of(groupUuid), false)
                         .stream()
                         .filter(m -> modificationsToExclude == null || !modificationsToExclude.contains(m.getId()))
@@ -287,11 +284,11 @@ public void deleteNetworkModifications(UUID groupUuid, List<UUID> modificationsU
         }
     }
 
-    @Transactional
     public NetworkModificationsResult moveModifications(@NonNull UUID destinationGroupUuid, @NonNull UUID originGroupUuid, UUID beforeModificationUuid,
                                                                        @NonNull List<UUID> modificationsToMoveUuids, @NonNull List<ModificationApplicationContext> applicationContexts,
                                                                        boolean applyModifications) {
         // update origin/destinations groups to cut and paste all modificationsToMove
+        // FullDto needed for toModificationInfos() after the modifications have been applied
         List<ModificationEntity> modificationEntities = networkModificationRepository.moveModifications(destinationGroupUuid, originGroupUuid, modificationsToMoveUuids, beforeModificationUuid);
 
         List<Optional<NetworkModificationResult>> result = applyModifications && !modificationEntities.isEmpty() ? applyModifications(destinationGroupUuid, modificationEntities, applicationContexts) : List.of();
@@ -333,23 +330,19 @@ private Optional<NetworkModificationResult> applyModifications(UUID networkUuid,
         return Optional.empty();
     }
 
-    @Transactional
     public NetworkModificationsResult duplicateModifications(@NonNull UUID targetGroupUuid, UUID originGroupUuid, @NonNull List<UUID> modificationsUuids, @NonNull List<ModificationApplicationContext> applicationContexts) {
         if (originGroupUuid != null && !modificationsUuids.isEmpty()) { // Duplicate modifications from a group or from a list only
             throw new NetworkModificationServerException(DUPLICATION_ARGUMENT_INVALID);
         }
-        List<ModificationInfos> modificationInfos = originGroupUuid != null ? networkModificationRepository.getActiveModificationsInfos(originGroupUuid) : networkModificationRepository.getModificationsInfos(modificationsUuids);
-        List<ModificationEntity> duplicateModifications = networkModificationRepository.saveModificationInfos(targetGroupUuid, modificationInfos);
+        List<ModificationEntity> duplicateModifications = networkModificationRepository.saveDuplicateModifications(targetGroupUuid, originGroupUuid, modificationsUuids);
         return new NetworkModificationsResult(
             duplicateModifications.stream().map(ModificationEntity::getId).toList(),
             applyModifications(targetGroupUuid, duplicateModifications, applicationContexts)
         );
     }
 
-    @Transactional
     public NetworkModificationsResult insertCompositeModifications(@NonNull UUID targetGroupUuid, @NonNull List<UUID> modificationsUuids, @NonNull List<ModificationApplicationContext> applicationContexts) {
-        List<ModificationInfos> modificationInfos = networkModificationRepository.getCompositeModificationsInfos(modificationsUuids);
-        List<ModificationEntity> modificationEntities = networkModificationRepository.saveModificationInfos(targetGroupUuid, modificationInfos);
+        List<ModificationEntity> modificationEntities = networkModificationRepository.saveCompositeModifications(targetGroupUuid, modificationsUuids);
         return new NetworkModificationsResult(modificationEntities.stream().map(ModificationEntity::getId).toList(), applyModifications(targetGroupUuid, modificationEntities, applicationContexts));
     }
 

src/test/java/org/gridsuite/modification/server/service/BuildTest.java

@@ -362,6 +362,11 @@ void testIndexationAfterBuild(final MockWebServer server) {
                 .couplingDevices(Arrays.asList(CouplingDeviceInfos.builder().busbarSectionId1("vl9_1_1").busbarSectionId2("vl9_2_1").build()))
                 .build()));
         // add new Load
+        // Need an entity with a lazyloaded subentity/collection to test that all required
+        // data has been loaded during the initial completed transaction before applying modifications:
+        // LoadCreationInfos and other modifications in this test do the job because it has the free properties Collection.
+        // Is there a good way to add in this test that we have 2 short transactions
+        // instead of one long idle transaction ?
         equipmentsToAdd.add(ModificationEntity.fromDTO(LoadCreationInfos.builder()
                 .equipmentId("newLoad")
                 .equipmentName("newLoad")