counsyl / stor

A cross-compatible CLI and Python API for accessing block and object storage
https://counsyl.github.io/stor/
Other
35 stars 12 forks source link

Add trailing slash to file_proxy_url if absent. #136

Closed pkaleta closed 4 years ago

pkaleta commented 4 years ago

@jtratner

Right now, this example from the docs doesn't work, because proxy URL doesn't have a trailing slash and therefore the gateway part is stripped off of the generated URL:

In [3]: with stor.settings.use({'dx': {'file_proxy_url': 'https://my-dnax-proxy.example.com/gateway'}}): 
   ...:     print(stor.Path('dx://proj:/folder/mypath.csv').temp_url()) 
   ...:                          
https://my-dnax-proxy.example.com/proj/folder/mypath.csv

It does work however, if you add a trailing slash to the proxy URL:

In [2]: with stor.settings.use({'dx': {'file_proxy_url': 'https://my-dnax-proxy.example.com/gateway/'}}): 
   ...:     print(stor.Path('dx://proj:/folder/mypath.csv').temp_url()) 
   ...:                                                                                                                                                                                                                                
https://my-dnax-proxy.example.com/gateway/proj/folder/mypath.csv

This PR adds a trailing slash to the file_proxy_url if it's missing. I've also added test that replicates the issue and proves that this fix works.

jtratner commented 4 years ago

I wonder if we should throw exception instead? Is there case where this would be wrong?

pkaleta commented 4 years ago

Is there case where this would be wrong?

Hm, I can't imagine a case where you'd specify https://my-gateway.foo.com/gateway, but didn't want the gateway part to be included in the generated URLs.

If we decide to fail loudly, we probably should do this when configuring stor (via stor.settings.update) and when reading from DX_FILE_PROXY_URL env var, rather than when temp_url() is actually called.

WDYT?