[SVCS-294] Add rename validation #213
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Johnetordoff
changed the title
…erbutler into drop-box-slash-issue * 'develop' of https://github.com/CenterForOpenScience/waterbutler: (77 commits) final cleanups for onedrive make provider readonly start cleaning up and reorganizing onedrive implement Microsoft OneDrive provider using dropbox as base added onedrive provider -- copied from Dropbox don't assume file metadata has a modified/created date add mypy type annotations add modified/created dates to file metadata expand docstrings add ruby serialization workaround to download clean up commit_sha vs. branch_name handling add tests for revisions and uninit-ed repos add artificial test for missing folder remove unused and obsolete code rewrite test suite for provider changes and coverage update some more docstrings on the provider GL provider is read-only: folder creation is not allowed add workaround for non-existent directories not being reported in GL document workarounds in download & _fetch_file_contents remove unneeded error wrapping from metadata ... # Conflicts: # waterbutler/server/api/v1/provider/movecopy.py
cslzchen
requested changes
Jun 27, 2018
There was a problem hiding this comment.
For ⛺️ 🔥 :
I tried on OSFStorage (staging) and it failed with a different but misleading error message.
I think this is a general issue. The fix is not to allow those character but to catch exceptions and throw meaningful error messages. Fortunately, this PR fixes the issue at the core provider level 👍 . All we need to do is to check each provider and add the list FORBIDDEN_FILENAME_CHARS accordingly. Last but not least, we need unit tests for this.
NyanHelsing
reviewed
Jul 16, 2018
| @@ -83,6 +83,8 @@ class BaseProvider(metaclass=abc.ABCMeta): | |||
| """ | |||
|
|
|||
| BASE_URL = None | |||
| FORBIDDEN_FILENAME_CHARS = [] | |||
| FORBIDDEN_FOLDERNAME_CHARS = ['/'] | |||
There was a problem hiding this comment.
Make this an abc property on the BaseProvider
|
TODO: Double check this isn't supported, and clarify pr message a little bit. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Currently you can rename a file or folder whatever you like, this creates a variety of error behaviors for the different providers. For Dropbox filenames with slashes create bad folders and files, for other providers the rename will just fail with no explanation, this fix alerts the user they are using an illegal character.
Changes
Add two static variables for the base provider FORBIDDEN_FILENAME_CHARS and FORBIDDEN_FOLDERNAME_CHARS which are used by validate_rename to bar the use of invalid characters.
Also cleans up some superfluous code in movecopy.py
Side Effects
None that I know of.
Tickets
https://openscience.atlassian.net/browse/SVCS-294