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

[DC-1117] Fix dataset name validation for createSnapshot #1703

Merged
merged 15 commits into from
Jun 11, 2024
Merged

[DC-1117] Fix dataset name validation for createSnapshot #1703

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
6aedfa6
Fix dataset name validation for createSnapshot
s-rubenstein Jun 6, 2024
eca193a
Fetch datasets only once
s-rubenstein Jun 7, 2024
fed6d6f
Make source dataset a parameter for createSnapshot flight
s-rubenstein Jun 7, 2024
2187ad2
Only pass around ID
s-rubenstein Jun 7, 2024
a73de1a
Store ID
s-rubenstein Jun 10, 2024
f88070d
Pull lists
s-rubenstein Jun 10, 2024
e6af73c
Fix compilation
s-rubenstein Jun 10, 2024
19e2cd4
Remove old comment
s-rubenstein Jun 10, 2024
defe7b5
Fix spotless
s-rubenstein Jun 10, 2024
6175cbc
Fix tests
s-rubenstein Jun 10, 2024
a294a32
Merge branch 'develop' into sr/dc-1117-update-create-snapshot-endpoint
s-rubenstein Jun 10, 2024
a6b9413
Add bad request exception and content validation
s-rubenstein Jun 10, 2024
41eae96
Fix merge
s-rubenstein Jun 10, 2024
967ac74
Fix merge again and remove unused code
s-rubenstein Jun 10, 2024
f129d18
Fix grammar
s-rubenstein Jun 10, 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
3 changes: 1 addition & 2 deletions src/main/java/bio/terra/service/snapshot/SnapshotService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

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);

Copy link
Contributor Author

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)

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 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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

if (snapshotRequestModel.getProfileId() == null) {
snapshotRequestModel.setProfileId(dataset.getDefaultProfileId());
logger.warn(
Expand Down
Loading