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.

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

Fix S3 safe_join() to allow colons #322

Merged
merged 1 commit into from
Jun 5, 2017
Merged

Fix S3 safe_join() to allow colons #322

merged 1 commit into from
Jun 5, 2017

Conversation

jdufresne
Copy link
Contributor

Combine the identical s3boto3 and s3boto implementations of safe_join()
and its tests to reduce code duplication.

Fixes #248

@codecov-io
Copy link

codecov-io commented May 19, 2017

Codecov Report

Merging #322 into master will decrease coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #322      +/-   ##
==========================================
- Coverage    75.7%   75.51%   -0.19%     
==========================================
  Files          11       11              
  Lines        1552     1540      -12     
==========================================
- Hits         1175     1163      -12     
  Misses        377      377
Impacted Files Coverage Δ
storages/backends/s3boto3.py 84.63% <100%> (-0.5%) ⬇️
storages/utils.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 810b9ba...76da0eb. Read the comment docs.

@jleclanche
Copy link
Contributor

Why is this using urljoin at all instead of os.path.join? Clearly the input isn't URL path components, otherwise behaviour would be broken (as it is).

@jdufresne
Copy link
Contributor Author

Good question. I think using posixpath.join() does make more sense and actually simplifies the implementation. I have updated the PR with this change.

@jleclanche
Copy link
Contributor

Looks good. Can you think of any other scenarios not covered with the current tests? Changing this method feels a little stressful all in all.

Combine the identical s3boto3 and s3boto implementations of safe_join()
and its tests to reduce code duplication.

Fixes #248
@jdufresne
Copy link
Contributor Author

Can't think of any at the moment. I added a new test for utils.safe_join("base", "/etc/passwd") as well as fix a typo in test test_trailing_slash_multi(). If you can think of another test to write, I'll happily add it. Let me know.

@jleclanche
Copy link
Contributor

Happy with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants