Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get Composite Modification endpoint #541

Merged
merged 15 commits into from
Nov 13, 2024
Merged

get Composite Modification endpoint #541

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
190e807
draft getCompositeModification endpoint
Mathieu-Deharbe Oct 8, 2024
94c049d
metadata instead of complete ModificationInfos
Mathieu-Deharbe Oct 9, 2024
0dcc187
optimize database requests
Mathieu-Deharbe Oct 10, 2024
b4fe982
Merge branch 'main' into get-composite-modification-content
Mathieu-Deharbe Oct 21, 2024
04d95b4
Merge branch 'main' into get-composite-modification-content
Mathieu-Deharbe Oct 21, 2024
959bfd4
TUs
Mathieu-Deharbe Oct 22, 2024
659dfb2
pleasing sonar
Mathieu-Deharbe Oct 22, 2024
b001a7f
cleaning + order by
Mathieu-Deharbe Oct 24, 2024
6886fb4
Merge branch 'main' into get-composite-modification-content
Mathieu-Deharbe Oct 24, 2024
d890755
clean post merge conflicts
Mathieu-Deharbe Oct 24, 2024
002e663
post reviews
Mathieu-Deharbe Oct 31, 2024
6277098
removed unhandled 404
Mathieu-Deharbe Oct 31, 2024
8dbf573
Merge branch 'main' into get-composite-modification-content
Mathieu-Deharbe Oct 31, 2024
5443632
removed empty lines
Mathieu-Deharbe Nov 6, 2024
4d88cac
Merge branch 'main' into get-composite-modification-content
Mathieu-Deharbe Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/main/java/org/gridsuite/modification/server/NetworkModificationController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,18 @@ public ResponseEntity<UUID> createNetworkCompositeModification(@RequestBody List
return ResponseEntity.ok().body(networkModificationService.createNetworkCompositeModification(modificationUuids));
}

@GetMapping(value = "/network-composite-modification/{id}", produces = MediaType.APPLICATION_JSON_VALUE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@GetMapping(value = "/network-composite-modification/{id}", produces = MediaType.APPLICATION_JSON_VALUE)
@GetMapping(value = "/network-composite-modification/{uuid}", produces = MediaType.APPLICATION_JSON_VALUE)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK done and updated with a /network-modifications. I think this is clearer.

@Operation(summary = "Get the content of a composite modification via its id")
@ApiResponses(value = {@ApiResponse(responseCode = "200", description = "Content of the composite modification"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Content is not clear enough and it is generic description
maybe "The modifications list of the composite"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of updated all of this to be closer to @GetMapping(value = "/groups/{groupUuid}/network-modifications", produces = MediaType.APPLICATION_JSON_VALUE)

I think it was clearer that my previous endpoint.

@ApiResponse(responseCode = "404", description = "This composite modification does not exist")}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

404 code is for not found response
for readability reasons I suggest
@ApiResponses(value = { @ApiResponse(responseCode = "200", description = "Content of the composite modification"), @ApiResponse(responseCode = "404", description = "Composite modification not found") })

If the id is not found in the database, an exception should be thrown by networkModificationService
Currently, it returns 200 OK even if the id doesn’t exist !...
fix it (with try-catch or another solution)

Copy link
Contributor Author

@Mathieu-Deharbe Mathieu-Deharbe Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Message changed. Remobved that 404.

)
public ResponseEntity<List<ModificationInfos>> getCompositeModificationContent(@PathVariable("id") UUID compositeModificationId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PathVariable(value = "id", required = true) UUID compositeModificationId

to ensure that the id is mandatory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For PathVariable boolean required() default true

If I add a true, intellij calls it a redundant parameter.

return ResponseEntity.ok()
.contentType(MediaType.APPLICATION_JSON)
.body(networkModificationService.getCompositeModificationContentMetadata(compositeModificationId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metadata ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed getNetworkModificationsFromComposite

I am still not very satisfied of it, but still better than metadata indeed.

);
}

@PostMapping(value = "/network-modifications", consumes = MediaType.APPLICATION_JSON_VALUE)
@Operation(summary = "Duplicate some modifications without group ownership")
@ApiResponses(value = {@ApiResponse(responseCode = "200", description = "The duplicated modifications uuids mapped with their source uuid")})
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/org/gridsuite/modification/server/repositories/ModificationRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ public interface ModificationRepository extends JpaRepository<ModificationEntity
@Query(value = "SELECT new ModificationEntity(m.id, m.type) FROM ModificationEntity m WHERE m.id IN (?1)")
List<ModificationEntity> findMetadataIn(List<UUID> uuids);

/**
* @return base data of the network modifications (the data from the main common table, not those specific to each modification)
*/
@Query(value = "SELECT new ModificationEntity(m.id, m.type, m.date, m.stashed, m.activated, m.messageType, m.messageValues) FROM ModificationEntity m WHERE m.id IN (?1) order by m.modificationsOrder")
List<ModificationEntity> findBaseDataByIdIn(List<UUID> uuids);

@Query(value = "SELECT m FROM ModificationEntity m WHERE m.id IN (?1) ORDER BY m.modificationsOrder")
List<ModificationEntity> findAllByIdIn(List<UUID> uuids);

Expand Down
25 changes: 19 additions & 6 deletions ...in/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,15 +394,28 @@ public List<ModificationInfos> getModificationsInfos(@NonNull List<UUID> uuids)
return uuids.stream().map(entities::get).filter(Objects::nonNull).map(this::getModificationInfos).toList();
}

/**
* @param onlyCommonData if true, only returns the basic data common to all the modifications. If false, returns complete modifications
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain the basic of the data and what do you mean by onlyCommonData?
It doesn't make sense to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the network modifications use the ModificationInfos. The data inside this class is the common data to all the network modifications.

But they also have specific data extending this data like BatteryModificationInfos or LineCreationInfos.

* @return the data from all the network modification contained in the composite modification sent as parameters
*/
@Transactional(readOnly = true)
public List<ModificationInfos> getCompositeModificationsInfos(@NonNull List<UUID> uuids) {
public List<ModificationInfos> getCompositeModificationsContentInfos(@NonNull List<UUID> uuids, boolean onlyCommonData) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need to rename this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but I don't think this is a good name : it sounds like we are getting the infos of the composite modifications but we are not. We get the infos of the network modifications that are inside the composite modifications and don't get those of the composite modifications themselves.

It sent me in the wrong direction when I was looking at functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this method should return wether a list of modification base infos or a modification full infos. You could rather implement 2 two methods

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it gets too confusing. I separated them.

List<ModificationInfos> entities = new ArrayList<>();
uuids.forEach(uuid -> {
List<UUID> foundEntities = modificationRepository.findModificationIdsByCompositeModificationId(uuid);
List<ModificationInfos> orderedModifications = foundEntities
.stream()
.map(this::getModificationInfo)
.toList();
List<UUID> networkModificationsUuids = modificationRepository.findModificationIdsByCompositeModificationId(uuid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

foundEntities

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean renaming networkModificationsUuids ? But those are not entities.

List<ModificationInfos> orderedModifications;
if (onlyCommonData) {
List<ModificationEntity> networkModifications = modificationRepository.findBaseDataByIdIn(networkModificationsUuids);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

networkModificationsEntities

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

orderedModifications = networkModifications
.stream()
.map(this::getModificationInfos)
.toList();
} else {
orderedModifications = networkModificationsUuids
.stream()
.map(this::getModificationInfo)
.toList();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

 if (uuids.isEmpty()) {
            return Collections.emptyList(); 
        }

and we can do better by fetching all modification uuids in one go like

 .collect(Collectors.toMap(
                        uuid -> uuid,
                        modificationRepository::findModificationIdsByCompositeModificationId));
            }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, but I completely changed this funciton anyway so I don't think this is still needed.

entities.addAll(orderedModifications);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

for (UUID uuid : uuids) {
            List<UUID> networkModificationsUuids = networkModificationsMap.get(uuid);
            if (onlyMetadata) {
                List<ModificationEntity> networkModifications = modificationRepository.findBaseDataByIdIn(networkModificationsUuids);
                entities.addAll(networkModifications.stream()
                        .map(this::getModificationInfos)
                        .toList());
            } else {
                entities.addAll(networkModificationsUuids.stream()
                        .map(this::getModificationInfo)
                        .toList());
            }
        }

        return Collections.unmodifiableList(entities); 
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same answer than the previous comment.

);
Expand Down
10 changes: 8 additions & 2 deletions src/main/java/org/gridsuite/modification/server/service/NetworkModificationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ public List<ModificationInfos> getNetworkModifications(UUID groupUuid, boolean o
return getNetworkModifications(groupUuid, onlyMetadata, errorOnGroupNotFound, false);
}

@Transactional(readOnly = true)
// Need a transaction for collections lazy loading
Mathieu-Deharbe marked this conversation as resolved.
Show resolved Hide resolved
public List<ModificationInfos> getCompositeModificationContentMetadata(UUID compositeModificationUuid) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContentMetadata ? can be implied, you might shorten it to getCompositeModificationMetadata

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed, removed all references to Metadata.

return networkModificationRepository.getCompositeModificationsContentInfos(List.of(compositeModificationUuid), true);
}

@Transactional(readOnly = true)
public ModificationInfos getNetworkModification(UUID networkModificationUuid) {
return networkModificationRepository.getModificationInfo(networkModificationUuid);
Expand Down Expand Up @@ -289,7 +295,7 @@ public Optional<NetworkModificationResult> duplicateModifications(UUID targetGro
public Optional<NetworkModificationResult> insertCompositeModifications(UUID targetGroupUuid,
UUID networkUuid, String variantId,
ReportInfos reportInfos, List<UUID> modificationsUuids) {
List<ModificationInfos> modificationInfos = networkModificationRepository.getCompositeModificationsInfos(modificationsUuids);
List<ModificationInfos> modificationInfos = networkModificationRepository.getCompositeModificationsContentInfos(modificationsUuids, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContentInfos ? (content or infos have the same meaning) you could keep only Infos

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed.

networkModificationRepository.saveModificationInfos(targetGroupUuid, modificationInfos);
return applyModifications(networkUuid, variantId, reportInfos, modificationInfos);
}
Expand Down Expand Up @@ -332,7 +338,7 @@ public Optional<NetworkModificationResult> applyModificationsFromUuids(UUID netw
String variantId,
ReportInfos reportInfos,
List<UUID> modificationsUuids) {
List<ModificationInfos> modificationInfos = networkModificationRepository.getCompositeModificationsInfos(modificationsUuids);
List<ModificationInfos> modificationInfos = networkModificationRepository.getCompositeModificationsContentInfos(modificationsUuids, false);
return applyModifications(networkUuid, variantId, reportInfos, modificationInfos);
}
}
20 changes: 14 additions & 6 deletions src/test/java/org/gridsuite/modification/server/ModificationControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ class ModificationControllerTest {

private static final String URI_NETWORK_MODIF_BASE = "/v1/network-modifications";
private static final String URI_COMPOSITE_NETWORK_MODIF_BASE = "/v1/network-composite-modifications";
private static final String URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT = "/v1/network-composite-modification/";
private static final String URI_NETWORK_MODIF_PARAMS = "&groupUuid=" + TEST_GROUP_ID + "&reportUuid=" + TEST_REPORT_ID + "&reporterId=" + UUID.randomUUID();
private static final String URI_NETWORK_MODIF = URI_NETWORK_MODIF_BASE + "?networkUuid=" + TEST_NETWORK_ID + URI_NETWORK_MODIF_PARAMS;
private static final String URI_NETWORK_MODIF_BUS_BREAKER = URI_NETWORK_MODIF_BASE + "?networkUuid=" + TEST_NETWORK_BUS_BREAKER_ID + URI_NETWORK_MODIF_PARAMS;
Expand Down Expand Up @@ -695,9 +696,10 @@ void testMoveModificationInSameGroup() throws Exception {

@Test
void testNetworkCompositeModification() throws Exception {
// Insert a switch modification in the group
List<ModificationInfos> modificationList = createSomeSwitchModifications(TEST_GROUP_ID, 1);
assertEquals(1, modificationRepository.getModifications(TEST_GROUP_ID, true, true).size());
// Insert some switch modifications in the group
int modificationsNumber = 2;
List<ModificationInfos> modificationList = createSomeSwitchModifications(TEST_GROUP_ID, modificationsNumber);
assertEquals(modificationsNumber, modificationRepository.getModifications(TEST_GROUP_ID, true, true).size());

// Create a composite modification with the switch modification
List<UUID> modificationUuids = modificationList.stream().map(ModificationInfos::getUuid).toList();
Expand All @@ -710,7 +712,13 @@ void testNetworkCompositeModification() throws Exception {
.build();
UUID compositeModificationUuid = mapper.readValue(mvcResult.getResponse().getContentAsString(), new TypeReference<>() { });
assertThat(modificationRepository.getModificationInfo(compositeModificationUuid)).recursivelyEquals(compositeModificationInfos);
assertEquals(1, modificationRepository.getModifications(TEST_GROUP_ID, true, true).size());
assertEquals(modificationsNumber, modificationRepository.getModifications(TEST_GROUP_ID, true, true).size());

// get the composite modification metadata
mvcResult = mockMvc.perform(get(URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT + compositeModificationUuid))
.andExpect(status().isOk()).andReturn();
List<ModificationInfos> compositeModificationContent = mapper.readValue(mvcResult.getResponse().getContentAsString(), new TypeReference<>() { });
assertEquals(modificationsNumber, compositeModificationContent.size());

// Insert the composite modification in the group
mvcResult = mockMvc.perform(
Expand All @@ -725,10 +733,10 @@ void testNetworkCompositeModification() throws Exception {
assertApplicationStatusOK(mvcResult);

List<ModificationInfos> newModificationList = modificationRepository.getModifications(TEST_GROUP_ID, false, true);
assertEquals(2, newModificationList.size());
assertEquals(modificationsNumber * 2, newModificationList.size());
List<UUID> newModificationUuidList = newModificationList.stream().map(ModificationInfos::getUuid).toList();
assertEquals(modificationUuids.get(0), newModificationUuidList.get(0));
assertThat(modificationList.get(0)).recursivelyEquals(newModificationList.get(1));
assertThat(modificationList.get(0)).recursivelyEquals(newModificationList.get(modificationsNumber));
}

@Test
Expand Down
Loading