[PNE-203] Fix reading file links on Windows.#950
Conversation
If FileCollection.read is called with a FileLink which encodes a local path (i.e. starts with file:///), then it's a local file. Due to the differences in path handling on Windows and Unix, we need to explicitly convert the path portion of that URL into a filepath.
|
I tested this by spinning up a Windows VM in VirtualBox and running the snippet here on a variety of Windows-style paths (and a few Linux), and ensuring the output looked right. |
kroenlein
left a comment
kroenlein
left a comment
There was a problem hiding this comment.
How thoroughly was this tested? It sounds reasonable, but I'd like to maybe see additional tests added, since there were no additions to the test suite in this PR.
Also, I note that the documentation to url2pathname says
This function uses unquote() to decode path.
which sounds like we might have a double-decode bug in this workflow.
While I'm sure there's a way to test Windows filepaths in a Linux environment, no obvious way revealed itself on Googling, which means it will be a lot more effort than it's worth for this bug, given it was reported months ago but seemingly not caused many of our customers any problems. Also, we use I was just looking for a quick win in some downtime, but given this has a bit more to it, I'm gonna put it aside and come back to it later. |
| path = Path(unquote_plus(url2pathname(urlparse(file_link.url).path))) | ||
| parsed_url = urlparse(file_link.url) | ||
| if parsed_url.netloc not in {'', '.', 'localhost'}: | ||
| raise ValueError("Non-local UNCs (e.g., Windows network paths) are not supported.") |
There was a problem hiding this comment.
This is a new restriction, is it not?
There was a problem hiding this comment.
Formally it's a new restriction, but we would have broken on this case before because we were not considering the netloc value.
There was a problem hiding this comment.
As a side note, I would have fixed this rather than raising an exception, except without a Windows box to experiment on, I don't trust that my implementation will be reliable. Stackoverflow
kroenlein
left a comment
kroenlein
left a comment
There was a problem hiding this comment.
With changes, I this is ready.
2 participants
If FileCollection.read is called with a FileLink which encodes a local path (i.e. starts with file:///), then it's a local file. Due to the differences in path handling on Windows and Unix, we need to explicitly convert the path portion of that URL into a filepath.
PR Type:
Adherence to team decisions