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

Jump to bottom

FileRangeReader Improved URI Handeling #3034

Merged
merged 4 commits into from
Jul 23, 2019
Merged

FileRangeReader Improved URI Handeling #3034

merged 4 commits into from
Jul 23, 2019

Conversation

jbouffard
Copy link
Contributor

@jbouffard jbouffard commented Jul 22, 2019 •
edited
Loading

Overview

This PR fixes a bug in FileRangeReaderProvider where it was unable to process certain URIs. Looking into the issue, I found the cause of the problem to be with how we were converting URIs to Files. It turns out that Paths.get has certain restrictions on how the URI has to be formatted. These differ from the URI specification, and can sometimes cause properly formatted URIs to be denied.

What I ended up deciding on was to simply pass in a String to Paths.get in order to get around the different URI requirements. To do that, I first check to see if the given URI had a scheme or not. If it doesn't then I just convert it to a String. Otherwise, I strip the scheme from the path and then pass in the scheme-less String.

Checklist

  • docs/CHANGELOG.rst updated, if necessary
  • Unit tests added for bug-fix or new feature

Closes #3031

@jbouffard jbouffard changed the title FileRangeReader Improved URi Handeling FileRangeReader Improved URI Handeling Jul 23, 2019
@jbouffard jbouffard requested review from pomadchin and jpolchlo July 23, 2019 12:54
@jbouffard
Cleaned up tests
965b6d3 
Signed-off-by: Jacob Bouffard <[email protected]>
@rossbernet rossbernet requested a review from moradology July 23, 2019 15:08
echeipesh
echeipesh approved these changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@echeipesh echeipesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@echeipesh echeipesh merged commit e7ff078 into locationtech:master Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@echeipesh echeipesh echeipesh approved these changes

@pomadchin pomadchin Awaiting requested review from pomadchin

@jpolchlo jpolchlo Awaiting requested review from jpolchlo

@moradology moradology Awaiting requested review from moradology

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RangeReader cannot handle file:/// URI
2 participants
@jbouffard @echeipesh