Add an endpoint sharing a composite modification of a node - #1070
Add an endpoint sharing a composite modification of a node#1070flomillot wants to merge 4 commits into
Conversation
The composite modification is moved out of the study into a directory of GridExplore, where it becomes a shared modification, and is replaced in the node by a reference to it. Since the applied modifications are left unchanged, the node does not need to be rebuilt. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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. Comment |
GhilesA
left a comment
GhilesA
left a comment
There was a problem hiding this comment.
Just apply the sonar recommandation on QUERY_PARAM_GROUP_UUID
| notificationService.emitEndModificationEquipmentNotification(studyUuid, nodeUuid, childrenUuids); | ||
| } | ||
| notificationService.emitElementUpdated(studyUuid, userId); | ||
| } |
There was a problem hiding this comment.
missing catch in case of failures midway !
we have to handle the rollback in case of error
There was a problem hiding this comment.
In theory I agree with you but doesn't seem like it's what we do usually in the application. Can you show me where it's done actually in the service, as an exemple ? I mostly see logs.
| // does not need to be rebuilt | ||
| UUID sharedCompositeUuid = networkModificationService.extractCompositeModificationToShare(groupUuid, modificationUuid, name); | ||
| directoryService.createElement(parentDirectoryUuid, description, sharedCompositeUuid, name, DirectoryService.MODIFICATION, userId); | ||
| directoryService.createsReferencesToSharedComposites(List.of(sharedCompositeUuid), userId, nodeUuid); |
There was a problem hiding this comment.
if directory server fails to create element and the fails to createsReferencesToSharedComposites
=> orphaned directory element !
There was a problem hiding this comment.
Yes, and it's already the case everywhere in Study serverr, see insertCompositeNetworkModifications for instance., which is pretty close.
The rollback is actually not managed in Study server unlike in Explore server (cf executeWithRollback).
I think it need a dedicated technical story.
There was a problem hiding this comment.
No dedicated technical story needed here IMO so wrapping createsReferencesToSharedComposites + extractCompositeModificationToShare in a try/catch right after createElement with directoryService.deleteElement in the catch, is enough
There was a problem hiding this comment.
I'll ask a TL about it, because if we do it we should do something like in explore server which is not a quick fix.
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
| * by a reference to it, so that it can be stored as an element in the directory server. | ||
| * @return the uuid of the extracted composite modification | ||
| */ | ||
| public UUID extractCompositeModificationToShare(@NonNull UUID groupUuid, @NonNull UUID modificationUuid, @NonNull String name) { |
There was a problem hiding this comment.
why return UUID ?
the implementation of extractCompositeModificationToShare in NetworkModificationRepository return uuid which is always equal to modificationUuid (the composite entity keeps its own id only its container changes )
There was a problem hiding this comment.
It's an API good practice, when you use a POST you always return the ID or the created element.
I hesitated to return the reference modification ID but actually what we create and the most important is the shared modification.
There was a problem hiding this comment.
But indeed we don't use this a lot, so maybe I should not return anything.
| } | ||
|
|
||
| @PostMapping(value = "/studies/{studyUuid}/nodes/{nodeUuid}/network-modifications/{modificationUuid}/share") | ||
| @Operation(summary = "Move a composite modification of the node into a directory, replacing it by a reference to it") |
There was a problem hiding this comment.
it is about moving or extract or share ?
@operation(summary = "Extract the composite modification and replace it with a reference") it is better ? what do you think
There was a problem hiding this comment.
Well when you share a modification you extract it from the node and move it into the directory so all three.
I prefer move for API description however.
The network modification server does not return it anymore, the composite modification keeping its uuid when extracted. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
|
3 participants
Adds
POST /v1/studies/{studyUuid}/nodes/{nodeUuid}/network-modifications/{modificationUuid}/share?name=&description=&parentDirectoryUuid=, which moves a composite modification of a node out of the study into a directory of GridExplore, and replaces it in the node by a reference to this now shared modification.The write permission on the target directory and the uniqueness of the name are checked before anything is moved. The composite is then extracted by the network modification server — keeping its uuid — stored as a
MODIFICATIONelement of the directory server, and referenced from the node.Since the applied modifications are left unchanged, the node is not invalidated: only the modifications update notifications are emitted, so the front refetches the list.
Requires gridsuite/network-modification-server#874.