Parse Github Asset Based on Accessibility

[jacderida]

For supporting private releases, the ReleaseAsset parsing function was changed to use a different field in the JSON document, namely from browser_download_url -> url. This change breaks the download of assets for public Releases, as those still use the previous field.

You can see this in the documentation for the API: https://docs.github.com/en/rest/reference/repos#releases

On a public release, the 'url' field on the asset doesn't return the download location for the asset, so the download_url for the ReleaseAsset struct isn't set correctly.

This change introduces an is_private parameter on the from_asset function set the field based on whether the release is private or not. It's determined to be private based on whether an auth token has been specified.

[jaemk]

@jacderida Thanks for pointing this out. I see what you're saying, and I see that running the github example says it's going to download from the "asset" url instead of from the actual "download" url e.g.

 cargo run --example github --features "archive-tar archive-zip compression-flate2 compression-zip-deflate"
 
 ...
 Checking target-arch... x86_64-apple-darwin
Checking current version... v0.27.0
Checking latest released version... v9.9.10
New release found! v0.27.0 --> v9.9.10
New release is *NOT* compatible

github release status:
  * Current exe: "/Users/james/dev/self_update/target/debug/examples/github"
  * New exe release: "self_update-v9.9.10-x86_64-apple-darwin.tar.gz"
  * New exe download url: "https://api.github.com/repos/jaemk/self_update/releases/assets/9719319"

And then curling that url directly just returns json

curl -L https://api.github.com/repos/jaemk/self_update/releases/assets/9719319 
{"url":"https://api.github.com/repos/jaemk/self_update/releases/assets/9719319","id":9719319,"node_id":"MDEyOlJlbGVhc2VBc3NldDk3MTkzMTk=","name":"self_update-v9.9.10-x86_64-apple-darwin.tar.gz","label":"","uploader":{"login":"jaemk","id":13563490,"node_id":"MDQ6VXNlcjEzNTYzNDkw","avatar_url":"https://avatars.githubusercontent.com/u/13563490?v=4","gravatar_id":"","url":"https://api.github.com/users/jaemk","html_url":"https://github.com/jaemk","followers_url":"https://api.github.com/users/jaemk/followers","following_url":"https://api.github.com/users/jaemk/following{/other_user}","gists_url":"https://api.github.com/users/jaemk/gists{/gist_id}","starred_url":"https://api.github.com/users/jaemk/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jaemk/subscriptions","organizations_url":"https://api.github.com/users/jaemk/orgs","repos_url":"https://api.github.com/users/jaemk/repos","events_url":"https://api.github.com/users/jaemk/events{/privacy}","received_events_url":"https://api.github.com/users/jaemk/received_events","type":"User","site_admin":false},"content_type":"application/gzip","state":"uploaded","size":1508298,"download_count":91,"created_at":"2018-11-16T05:09:22Z","updated_at":"2018-11-16T05:09:22Z","browser_download_url":"https://github.com/jaemk/self_update/releases/download/v9.9.10/self_update-v9.9.10-x86_64-apple-darwin.tar.gz"}

Now.. what's baffling me is that the github example still "works"... when I run it, it does download the actual release, unpackages it, and overwrites my ./target/debug/examples/github with the executable that was uploaded back in 2018... Any ideas?

[jacderida]

Hmm, that's strange indeed. I'm honestly not sure. What we were seeing was the JSON document being downloaded rather than the actual asset.

To be honest, I did want to do some more testing before I submitted the PR to you. The commit where the field was changed from browser_download_url was to support private repositories. I was just working under the assumption that since the field was changed when implementing functionality related to private releases, then for a private release, the API set the URL for the asset to the url field rather than browser_download_url. Otherwise I couldn't see a reason for the change.

I didn't get time to validate that assumption, though. I guess it's possible that GIthub maybe changed the field at some point, but they changed it back again?

We are trying to publish one of our crates that use this one and right now it's using a git reference for this dependency which contains my fix. So I just didn't quite have time to do any more elaborate testing, sorry.

[jaemk]

Ah, I see why it's working - when you send an accept: application/octet-stream in the request github will return the asset's content instead of the info as json:

curl -Lv \
    -H 'accept: application/octet-stream' \
    https://api.github.com/repos/jaemk/self_update/releases/assets/9719319 \
    -o download.tar.gz \
    && tar -xf download.tar.gz \
    && ./github

And the request here just happens to automatically set an accept header: https://github.com/jaemk/self_update/blob/af699de622fc5570490158fb04c79a4b63d580c7/src/lib.rs?_pjax=%23js-repo-pjax-container%2C%20div%5Bitemtype%3D%22http%3A%2F%2Fschema.org%2FSoftwareSourceCode%22%5D%20main%2C%20%5Bdata-pjax-container%5D#L678 (I added a couple printlns to check if there were any other headers being sent):

github release status:
  * Current exe: "/Users/james/dev/self_update/target/debug/examples/github"
  * New exe release: "self_update-v9.9.10-x86_64-apple-darwin.tar.gz"
  * New exe download url: "https://api.github.com/repos/jaemk/self_update/releases/assets/9719319"

The new release will be downloaded/extracted and the existing binary will be replaced.
Do you want to continue? [Y/n]
Downloading...
REQ: https://api.github.com/repos/jaemk/self_update/releases/assets/9719319 -> {"accept": "application/octet-stream", "user-agent": "rust-reqwest/self-update"}

So I think the simpler move would be to explicitly add an accept: application/octet-stream in the request instead of relying on reqwest to set it implicitly (not sure why it even does) and instead of making the download-url-key dynamic

[jacderida]

OK, thanks. However, I'm not quite sure what the way forward is here. Would you like me to close this PR and open another one? Or were you going to make that change?

[jaemk]

Either or - You could do it here or in a new PR, up to you

[jacderida]

Sorry for taking so long to get back to you. Lots of work got in the way.

I was just about to address this. If I understand you correctly, you're asking me to change the code to add the application/octet-stream header?

However, this is already being done here: https://github.com/jaemk/self_update/blob/master/src/update.rs#L217

It also shows that header as set in the output on your post. I think I've possibly misunderstood something. Could you advise please?

[jaemk]

hm, yeah sorry I missed that - guess everything's all set already

[jacderida]

Given we still experience the problem with the incorrect URL, where does that leave us?

[jaemk]

Interesting. Can you confirm (or share a link to your release) whether curling your release with an -H 'accept: application/octet-stream' and the asset["url"] still results in a json document being downloaded?

[jacderida]

Sorry for taking some time to get back to you.

So I found the problem. We had been using your crate not just for updating the current crate, but also for downloading assets from releases for other crates. Here's the code we were using:

self_update::Download::from_url(&asset.download_url)
         .show_progress(true)
         .download_to(&tmp_tarball)
         .wrap_err_with(|| format!("Error downloading release asset '{}'", asset.download_url))?;

So the problem was, the octet-stream header wasn't being included at this point. I've since replaced this code with something else, but if we were still using it, we could modify it on our end to set the header. I notice you've done that in an example.

I personally think it would be a better API if this header was set in download_to, as then the caller doesn't need to be aware of it, but I'm not sure you would agree. I can submit a modification for that, or if you don't agree, you can feel free to just close the PR.

[jaemk]

Ah, I see. Yes, I agree that it would be better if the header was set automatically. Could you make that change here or in a new PR?