GDCC/Update storageIO processing to match RemoteOverlay #8923
Conversation
| //False will prevent direct uploads | ||
| protected static boolean isValidIdentifier(String driverId, String storageId) { | ||
| return true; | ||
| return false; |
There was a problem hiding this comment.
Is this going to cause a problem for SwiftAccessIO because it doesn't implement the method so now the DataAccess line 367 will return false instead of true?
There was a problem hiding this comment.
Swift doesn't do direct uploads so this nominally doesn't affect the way Swift has been used. This will prevent someone turning direct-upload on/using the /add file endpoints as a workaround for Swift until there's a check added for the Swift store. It will also prevent any new store types from allowing direct upload unless they implement a type-specific isValidIdentifier method. (Note that true was only introduced in the Remote Store PR and the intent was to flip this to false once file and S3 also had stricter validation tests in place.)
| logger.warning("Non-normalized path in submitted identifier " + storageId); | ||
| return false; | ||
| } | ||
| System.out.println(normalized.getFileName().toString()); |
There was a problem hiding this comment.
Is this println line here on purpose, or were you planning to remove it before this is merged? (just checking; there's a couple more in the PR).
There was a problem hiding this comment.
Good catch - changed the 2 in the main code to logger.fine (left the one in the test).
|
(As is the custom with your PRs, it is merged, but it's left up to you to delete the branch, or not) |
4 participants
What this PR does / why we need it: PR #7325 added a mechanism to validate inputs for direct-upload-related datafile submissions that occurs earlier in the add pipeline than other checks we have. This PR adds those checks for other store subclasses.
Which issue(s) this PR closes:
Closes #
Special notes for your reviewer:
Suggestions on how to test this: Should verify that flyway script works if the constraint already exists (i.e. it should just warn and not fail.)
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: