More aggressively validate ambiguous file paths#62
More aggressively validate ambiguous file paths#62jtratner wants to merge 1 commit intocounsyl:masterfrom
Conversation
jtratner
force-pushed
the
do-not-allow-writing-with-trailing-slash
branch
4 times, most recently
from
f6dfb91 to
32921e7
Compare
Closes counsyl#52 * Error on filepaths with trailing slash because they do not have a name and thus appear to be directories. * Add validation for bad paths to ``OBSFile``. * Warn if filesystem paths would not pass this validation (otherwise you have the possibility of something that works locally and not on OBS). This can be set to raise instead if you set ``stor.raise_on_ambiguous_filesystem_path`` to be True * Deprecate ``is_ambiguous()`` which didn't 100% serve the purpose we wanted anyways (since you can always tell when user means directory because of the user of ``copytree()`` and/or ``-r``) Sem-Ver: feature
jtratner
force-pushed
the
do-not-allow-writing-with-trailing-slash
branch
from
32921e7 to
4a498bf
Compare
| """ | ||
| kwargs.pop('swift_upload_kwargs', None) | ||
| if kwargs.pop('swift_upload_kwargs', None): | ||
| warnings.warn('swift_upload_kwargs will be removed in stor 2.0') |
There was a problem hiding this comment.
This doesn't seem discussed elsewhere in PR?
| if six.PY2 and encoding: # pragma: no cover | ||
| raise TypeError('encoding not supported in Python 2') | ||
| # TODO (jtratner): in stor 2.0 remove this explicit check and the warning | ||
| if pth.endswith('/'): |
There was a problem hiding this comment.
do you want the and not self.ext part from is_ambiguous ?
There was a problem hiding this comment.
isn't call to utils.validate_file_path right below sufficient?
| passed as keyword args to `SwiftPath.upload` if any writes | ||
| occur on the opened resource. | ||
|
|
||
| swift_upload_options (dict): DEPRECATED (use `stor.settings.use()` instead). |
There was a problem hiding this comment.
stor.settings.use() already added correct?
| # we use a try/except so that it's easier for the reader to tell why the exception was | ||
| # generated. | ||
| try: | ||
| if not dest.name: |
There was a problem hiding this comment.
is it slightly less explicit to rely on dest.name to verify no trailing slash rather than doing directly?
| return answer | ||
|
|
||
|
|
||
| def validate_file_path(dest, _warn=False): |
There was a problem hiding this comment.
nit: dest name implies this is always a check on destination path but it's called on both destination and source paths.
| if not dest.ext: | ||
| raise ValueError('file paths without extension are ambiguous') | ||
| except ValueError as e: | ||
| if ((_warn or is_filesystem_path(dest)) and |
There was a problem hiding this comment.
should is_filesystem_path be moved earlier on since ambiguous paths are fine in that setting so no need to do any validations?
| if not dest.name: | ||
| raise ValueError('file paths may not end with trailing slash') | ||
| if not dest.ext: | ||
| raise ValueError('file paths without extension are ambiguous') |
There was a problem hiding this comment.
requiring extensions for everything seems more restrictive that necessary and beyond what's required to resolve issue no?
krhaas
left a comment
krhaas
left a comment
There was a problem hiding this comment.
Made a pass, back to you
3 participants
Closes #52
Generally, validate ambiguous paths more robustly and almost always error when they end in trailing slash.
API Additions
stor.test.TestCasethat allows mocking out of both swift AND s3functionality in a test case.
Bug Fixes
open()will raise with an OBS path that ends in trailing slash, ratherthan generating an invalid/difficult OBS object.
Deprecations
open()orcopy()with OBS or filesystempaths that are ambiguous or end in a trailing slash. In the next version of
stor, this will be an error, to prevent differences between local and OBS
code. Set
stor:raise_on_ambiguous_filesystem_pathto get this behaviornow.
is_ambiguous()is now deprecated.cc @kristjaneerik