get_content_url(follow_redirects=True) should stop when reaching a "folder"

[yarikoptic]

Discovered by @satra .
ATM for a zarr it would try to follow through with the HEAD request for the "folder" and that pukes. I think it should be ok to catch that exception and if URL already ends with / then just stop at that URL. Or may be @jwodder you see a better/more standardized way?

A sample case:

In [17]: import dandi.dandiapi as da

In [18]: dandi = da.DandiAPIClient.for_dandi_instance("dandi")

In [19]: ds = dandi.get_dandiset('000108')

In [20]: asset = ds.get_asset_by_path('sub-MITU01/ses-20210521h17m17s06/micr/sub-MITU01_ses-20210521h17m17s06_sample-178_stain-LEC_run-1_chunk-1_SPIM.ngff')

In [21]: asset.get_content_url('s3')
Out[21]: 'https://dandiarchive.s3.amazonaws.com/zarr/56509720-870c-4f43-ae41-7b75f9590722/'

In [22]: asset.get_content_url('s3', follow_redirects=True)
Error 404 while sending HEAD request to https://dandiarchive.s3.amazonaws.com/zarr/56509720-870c-4f43-ae41-7b75f9590722/: 
---------------------------------------------------------------------------
HTTP404Error                              Traceback (most recent call last)
<ipython-input-22-7673eb38d60d> in <module>
----> 1 asset.get_content_url('s3', follow_redirects=True)

~/proj/dandi/dandi-cli-master/dandi/dandiapi.py in get_content_url(self, regex, follow_redirects, strip_query)
   1251             )
   1252         if follow_redirects is True:
-> 1253             url = self.client.request(
   1254                 "HEAD", url, json_resp=False, allow_redirects=True
   1255             ).url

~/proj/dandi/dandi-cli-master/dandi/dandiapi.py in request(self, method, path, params, data, files, json, headers, json_resp, retry_statuses, **kwargs)
    241                 lgr.error("%s: %s", msg, result.text)
    242             if result.status_code == 404:
--> 243                 raise HTTP404Error(msg, response=result)
    244             else:
    245                 raise requests.HTTPError(msg, response=result)

HTTP404Error: Error 404 while sending HEAD request to https://dandiarchive.s3.amazonaws.com/zarr/56509720-870c-4f43-ae41-7b75f9590722/

In [23]: asset.get_content_url()
Out[23]: 'https://api.dandiarchive.org/api/assets/1838aeed-c773-400e-84b1-be0b8fa63dd9/download/'

In [24]: asset.get_content_url(follow_redirects=True)
Error 404 while sending HEAD request to https://api.dandiarchive.org/api/assets/1838aeed-c773-400e-84b1-be0b8fa63dd9/download/: 
---------------------------------------------------------------------------
HTTP404Error                              Traceback (most recent call last)
<ipython-input-24-b6f368972ab4> in <module>
----> 1 asset.get_content_url(follow_redirects=True)

~/proj/dandi/dandi-cli-master/dandi/dandiapi.py in get_content_url(self, regex, follow_redirects, strip_query)
   1251             )
   1252         if follow_redirects is True:
-> 1253             url = self.client.request(
   1254                 "HEAD", url, json_resp=False, allow_redirects=True
   1255             ).url

~/proj/dandi/dandi-cli-master/dandi/dandiapi.py in request(self, method, path, params, data, files, json, headers, json_resp, retry_statuses, **kwargs)
    241                 lgr.error("%s: %s", msg, result.text)
    242             if result.status_code == 404:
--> 243                 raise HTTP404Error(msg, response=result)
    244             else:
    245                 raise requests.HTTPError(msg, response=result)

HTTP404Error: Error 404 while sending HEAD request to https://api.dandiarchive.org/api/assets/1838aeed-c773-400e-84b1-be0b8fa63dd9/download/

the question is either we should not bother redirecting for any / or only for the urls from the s3.amazonaws.com subdomains since we know that there is no real "folder/" on S3 (although "folder" key could exist)... for now I think we could stop following for any / url

[jwodder]

@yarikoptic I don't think it makes much sense to call get_content_url() on a Zarr in the first place. The purpose of the method is to get a URL for downloading the asset, but there is no such URL for a Zarr.

[yarikoptic]

Well, I guess it is just a matter of generic programming to be able to request target content URL regardless of its type

[jwodder]

@yarikoptic I'm not clear on what you're saying. Are you still asking for the change described in the original comment or something else?

[yarikoptic]

Yes, I think it would be a good idea for function to not crash - let's just not try following redirects on s3 folders

[jwodder]

@yarikoptic So what exactly do you want the new behavior to be? Should it be that, if an HTTP error response is returned for a URL that ends in a slash, the URL is returned instead of erroring?

[jwodder]

@yarikoptic Ping; please respond to last comment.

[yarikoptic]

I think "the URL is returned instead of erroring" would provide us the desired effect here.

[jwodder]

@yarikoptic Note that the code currently just takes a URL from the asset metadata and lets requests follow all the redirects. If this URL does not end in a slash but one of the URLs it is redirected to does (or vice versa), and the final result is an HTTP error, what should happen?

[jwodder]

@yarikoptic I still need an answer to my last comment.

[yarikoptic]

just return the last URL for which HEAD request failed. In effect "we follow redirects until we cannot"

[github-actions]

🚀 Issue was released in 0.46.2 🚀