Repository: seung-lab/cloud-volume
Pull request #350 (2020-06-16 13:51:57)

address issue with windows basepath extraction
To address: [https://github.com/seung-lab/cloud-volume/issues/349](https://github.com/seung-lab/cloud-volume/issues/349), an issue where windows paths were not parsed properly. Specifically, the bucket was getting added to the cloudpath twice.   

I added a simple test to make sure the fix works. 

william-silversmith: In the extract paths code, there's a call to os.path.sep I don't fully
understand why the windows tests pass on linux when os.path.sep will return
/ instead of \

On Wed, Jun 17, 2020, 9:49 AM Austin Hoag <notifications@github.com> wrote:

> *@austinhoag* commented on this pull request.
> ------------------------------
>
> In test/test_paths.py
> <https://github.com/seung-lab/cloud-volume/pull/350#discussion_r441559605>
> :
>
> > @@ -140,3 +140,13 @@ def okgoogle(url):
>    assert path.bucket == '/tmp'
>    assert path.dataset == 'removeme'
>    assert path.layer == 'layer'
> +
> +
> +def test_windows_path_extraction():
> +  """ This test is added specifically to make sure
> +  that no_bucket_basepath does not contain the bucket,
> +  for windows paths. """
> +  extract = paths.extract(r'file://C:\wow\this\is\a\cool\path', windows=True, disable_toabs=True)
> +  assert extract.protocol == 'file'
> +  assert extract.bucket == 'C:\\wow\\'
>
> Hmm, I'm not sure I follow your point.
>
> That part of the test is actually from the other test that already existed
> in this file. I only added the line:
>
> assert 'wow' not in extract.no_bucket_basepath
>
> everything else was copy paste of the top of the previous test.
>
> In any case the tests pass on linux and windows platforms, so I'm not sure
> why it wouldn't pass automated testing. The split_char is set to '\\' in
> the test because windows=True in:
>
> extract = paths.extract(r'file://C:\wow\this\is\a\cool\path', windows=True, disable_toabs=True)
>
> Maybe there is something I don't understand about the automated testing.
> Can you explain what you mean?
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <https://github.com/seung-lab/cloud-volume/pull/350#discussion_r441559605>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AATGQSJRSK4DBHQGT4QXIPTRXDCWRANCNFSM4N7UPGVQ>
> .
>

william-silversmith: To be clear, this is a preexisting issue, not one you added.
austinhoag: Oh, I see what you mean. I mistook the code block you had written as the actual code, not a suggestion. You are right that the separator was set by `os.path.sep` which on linux is `/`. It was lucky that the test passed on linux. It was because `splittlies` was a list of length 1, where the single entry was the entire cloudpath, such that the following code block was getting executed:
```python
 elif len(splitties) == 1:
    dataset = bucket
    layer = splitties[0]
    basepath = split_char.join(splitties[:-1])
    no_bucket_basepath = split_char.join(splitties[1:-1])
```
so `no_bucket_basepath` was getting set to an empty string, so my assert statement:
`assert 'wow' not in no_bucket_basepath` was true by accident.

I added your suggested change, and I also had to change the windows regex slightly to get the bucket and basepath to come out correctly for windows paths. Tests should now pass for the right reason on linux. 



Patch:
diff --git a/cloudvolume/paths.py b/cloudvolume/paths.py
index 33afa4a..aca35d9 100644
--- a/cloudvolume/paths.py
+++ b/cloudvolume/paths.py
@@ -117,7 +117,7 @@ def extract(cloudpath, windows=None, disable_toabs=False):
   if len(cloudpath) == 0:
     return ExtractedPath('','','','','','','')
 
-  windows_file_re = re.compile(r'((?:\w:\\)[\d\w_\.\-]+(?:\\)?)') # for C:\what\a\great\path
+  windows_file_re = re.compile(r'((?:\w:\\)[\d\w_\.\-]+)')
   bucket_re = re.compile(r'^(/?[~\d\w_\.\-]+(?::\d+)?)/') # posix /what/a/great/path
   
   error = UnsupportedProtocolError(CLOUDPATH_ERROR.format(cloudpath))
@@ -131,26 +131,24 @@ def extract(cloudpath, windows=None, disable_toabs=False):
     abspath = toabs    
 
   fmt, protocol, cloudpath = extract_format_protocol(cloudpath)
-  
+
   split_char = '/'
   if protocol == 'file':
     cloudpath = abspath(cloudpath)
     if windows:
       bucket_re = windows_file_re
-    split_char = os.path.sep
+      split_char = '\\'
 
   match = re.match(bucket_re, cloudpath)
   if not match:
     raise error
 
   (bucket,) = match.groups()
-
   splitcloudpath = cloudpath 
   if splitcloudpath[0] == split_char:
     splitcloudpath = splitcloudpath[1:]
   if splitcloudpath[-1] == split_char:
     splitcloudpath = splitcloudpath[:-1]
-
   splitties = splitcloudpath.split(split_char)
   if len(splitties) == 0:
     return ExtractedPath(fmt, protocol, bucket, cloudpath, '', bucket, '')
@@ -161,8 +159,12 @@ def extract(cloudpath, windows=None, disable_toabs=False):
     no_bucket_basepath = split_char.join(splitties[1:-1])
   elif len(splitties) >= 2:
     dataset, layer = splitties[-2:]
-    no_bucket_basepath = split_char.join(splitties[1:-1])
-    basepath = split_char.join([bucket] + splitties[1:-1])
+    if windows:
+      no_bucket_basepath = split_char.join(splitties[2:-1])
+      basepath = split_char.join([bucket] + splitties[2:-1])
+    else:
+      no_bucket_basepath = split_char.join(splitties[1:-1])
+      basepath = split_char.join([bucket] + splitties[1:-1])
 
   return ExtractedPath(
     fmt, protocol, bucket, 


Test patch:
diff --git a/test/test_paths.py b/test/test_paths.py
index 531174a..96ede67 100644
--- a/test/test_paths.py
+++ b/test/test_paths.py
@@ -8,20 +8,6 @@ from cloudvolume.exceptions import UnsupportedProtocolError
 from cloudvolume import lib
 
 def test_path_extraction():
-  extract = paths.extract(r'file://C:\wow\this\is\a\cool\path', windows=True, disable_toabs=True)
-  print(extract)
-  assert extract.protocol == 'file'
-  assert extract.bucket == 'C:\\wow\\'
-
-  # on linux the toabs prepends the current path because it
-  # doesn't understand C:\... so we can't test that here.
-  # assert extract.path == 'this\\is\\a\\cool\\path' 
-
-  try:
-    extract = paths.strict_extract(r'file://C:\wow\this\is\a\cool\path', windows=False, disable_toabs=True)
-    assert False 
-  except UnsupportedProtocolError:
-    pass
 
   def shoulderror(url):
     try:
@@ -140,3 +126,23 @@ def test_path_extraction():
   assert path.bucket == '/tmp'
   assert path.dataset == 'removeme'
   assert path.layer == 'layer'
+
+
+def test_windows_path_extraction():
+  extract = paths.extract(r'file://C:\wow\this\is\a\cool\path', windows=True, disable_toabs=True)
+  assert extract.format == 'precomputed'
+  assert extract.protocol == 'file'
+  assert extract.bucket == 'C:\\wow'
+  assert extract.basepath == 'C:\\wow\\this\\is\\a\\cool'
+  assert extract.no_bucket_basepath == 'this\\is\\a\\cool'  
+  assert extract.dataset == 'cool'  
+  assert extract.layer == 'path'  
+
+  extract = paths.extract('file://C:\\wow\\this\\is\\a\\cool\\path\\', windows=True, disable_toabs=True)
+  assert extract.format == 'precomputed'
+  assert extract.protocol == 'file'
+  assert extract.bucket == 'C:\\wow'
+  assert extract.basepath == 'C:\\wow\\this\\is\\a\\cool'
+  assert extract.no_bucket_basepath == 'this\\is\\a\\cool'  
+  assert extract.dataset == 'cool'  
+  assert extract.layer == 'path'  
\ No newline at end of file