-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
[DC-1117] Fix dataset name validation for createSnapshot #1703
[DC-1117] Fix dataset name validation for createSnapshot #1703
Conversation
@@ -172,8 +172,7 @@ public SnapshotService( | |||
*/ | |||
public String createSnapshot( | |||
SnapshotRequestModel snapshotRequestModel, AuthenticatedUserRequest userReq) { | |||
String sourceDatasetName = snapshotRequestModel.getContents().get(0).getDatasetName(); | |||
Dataset dataset = datasetService.retrieveByName(sourceDatasetName); | |||
Dataset dataset = getSourceDatasetsFromSnapshotRequest(snapshotRequestModel).get(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this is done both here and in SnapshotCreateFlight
. If we need it in both places (and this is the only place SnapshotCreateFlight
is called), then maybe Dataset
should be a flight parameter.
Also it looks like this work is repeated in the code that calls this, in SnapshotApiController.createSnapshot()
:
List<UUID> snapshotSourceDatasetIds =
snapshotService.getSourceDatasetIdsFromSnapshotRequest(snapshotRequestModel);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, it looks like there's even a todo to move the auth check to the flight? The Dataset
here is only used to set the default billing profile if there isn't one. I think this code could use some serious refactoring. Happy to get into it, but I think that might actually be pretty high scope creep because I would want to push a lot of this in there. Maybe tomorrow we can figure out what changes actually make sense to do? Or what work we are going to push if we want to prioritize the refactor. (Which might mean reprioritizing v2 createSnapshot)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I poked around with v2, definitely I would say is out of scope - that's a pretty serious refactor of how we do things. I am going to try to push some of the steps that are done in the controller and service into the job though, which might help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you take a look at #1705 when you have the chance? I think that's the real fix here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline - I think we have more design to happen here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good although I have a few more minor changes to remove Lists
src/main/java/bio/terra/app/controller/SnapshotsApiController.java
Outdated
Show resolved
Hide resolved
src/main/java/bio/terra/app/controller/SnapshotsApiController.java
Outdated
Show resolved
Hide resolved
src/main/java/bio/terra/service/snapshot/flight/create/SnapshotCreateFlight.java
Outdated
Show resolved
Hide resolved
"dataset-names", datasetNames, | ||
"dataset-ids", datasetIds, | ||
"dataset-names", sourceDataset.getName(), | ||
"dataset-ids", sourceDataset.getId().toString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be dataset-name and dataset-id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, cause that would make it different than existing labels. And I don't think I have confidence that users don't have workflows that fetch their google resources and check these labels. If we wanted to do that we would have to roll it out as a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this is something that could happen as part of the V2 endpoint change
src/test/java/bio/terra/service/snapshot/flight/create/SnapshotCreateFlightTest.java
Outdated
Show resolved
Hide resolved
@@ -195,9 +195,7 @@ public String createSnapshot( | |||
return jobService | |||
.newJob(description, SnapshotCreateFlight.class, snapshotRequestModel, userReq) | |||
.addParameter(CommonMapKeys.CREATED_AT, Instant.now().toEpochMilli()) | |||
.addParameter(JobMapKeys.IAM_RESOURCE_TYPE.getKeyName(), IamResourceType.DATASET) | |||
.addParameter(JobMapKeys.IAM_RESOURCE_ID.getKeyName(), dataset.getId()) | |||
.addParameter(JobMapKeys.IAM_ACTION.getKeyName(), IamAction.LINK_SNAPSHOT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you removing these keys because they aren't used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
|
No description provided.