feat: partial snapshot download requestes turned into POST #5633
Conversation
Signed-off-by: SimoneFiorani <[email protected]>
Signed-off-by: SimoneFiorani <[email protected]>
Signed-off-by: SimoneFiorani <[email protected]>
|
|
||
| this.downloadJson.addClickHandler(e -> { | ||
| this.downloadModal.hide(); | ||
| this.wiregraphDownloadConsumer.accept("JSON"); |
There was a problem hiding this comment.
Would it make sense to externalise the JSON and XML in constants?
It should help in keeping the alignment in the code in case of changes/refactorings
| && !checkBox.getText().equals(MSGS.removeAllAnchorText())) { | ||
| selectedPids.add(checkBox.getText()); | ||
| if (checkBox.getValue().booleanValue()) { | ||
| selectedPidsBuilder.append(checkBox.getText() + ","); |
There was a problem hiding this comment.
is the trailing comma an issue for the last entry of this builder?
There was a problem hiding this comment.
The servlet that finally downloads the snapshot splits the string using the comma as separator, so the last comma just disappear without any effect. If you prefare I can add a final check that remove the last comma, let me know
There was a problem hiding this comment.
up to you. just wanted to make sure we are aware and is covered for any potential corner case
There was a problem hiding this comment.
I added a line that removes the last comma. The payload is more clear and it's more maintanable
| private void downloadEntireSnapshot(Long snapshotId, String format) { | ||
| RequestQueue.submit(context -> this.gwtXSRFService.generateSecurityToken(context.callback(token -> { | ||
| xsrfTokenField.setValue(token.getToken()); | ||
| pidsListField.setValue("EntireSnapshot"); |
There was a problem hiding this comment.
Would it make more sense to download the entire snapshot if no pidsList is specified?
I am kind of scared of those magic values
There was a problem hiding this comment.
I agree with you. Indeed, I started with the idea "EmptyValue --> Download All", but then I changed my mind to avoid empty values...but thinking about it, it's probably more secure and robust. I'll change it
Signed-off-by: SimoneFiorani <[email protected]>
Signed-off-by: SimoneFiorani <[email protected]>
This PR follows #5627 improving the snapshot download mechanism.
The Snapshot download request is now turned into a more robus POST request, removing the link to the native javascript code from the DownloadHelper class used before.
This PR also avoids also the problem that occurs when the session expires during the selection of the PIDs to be downloaded: indeed, if the session expired after the snapshot download popup spawned, clicking on one of the download button led to a sort of undefined state of the webUI that would stop working. With this PR, if the session expires the token generation fails and the correct error popup is shown correctly.