onelson / estuary

33 stars 8 forks source link

Add test to verify download endpoint. #7

Closed onelson closed 3 years ago

onelson commented 3 years ago

Interestingly, the test to check that downloading a crate that doesn't exist failed since I had accidentally marked the response as an ApiResponse.

I'm not actually sure if this is correct to do. I suspect the correct thing to do is to 404.

In thee case of responding with an ApiResult which will always ensure the status is 200 and encode the Display for the Err as a json payload, cargo will treat the json payload as the crate file. This will cause an error when the checksum in the index doesn't match the sha256 of the response body.

To me this is quite misleading.

tester on  master [?] is  v0.1.0 via  v1.48.0
❯ cargo check
  Downloaded my-crate v0.1.1 (registry `git://localhost:9418/`)
error: failed to verify the checksum of `my-crate v0.1.1 (registry `git://localhost:9418/`)`

With the endpoint updated to respond 404 when the crate file for the specified name/version combination, cargo shows the following:

❯ cargo check
error: failed to download from `http://localhost:7878/api/v1/crates/my-crate/0.1.1/download`

Caused by:
  failed to get 200 response from `http://localhost:7878/api/v1/crates/my-crate/0.1.1/download`, got 404

Interestingly, to see either of these errors you'd have to have index data that indicates the crate with that name/version exists. Cargo won't even try to fetch anything from the download URL if the index doesn't say it'll be there. With all this in mind, I'm going to respond 404 since the error message is more generally informative.

Additionally, if you are publishing crates to some location on disk that is served over http by some other location (think like your crate dir is set to some path nginx can serve), we can't expect to have as tight control over the response in this "missing crate file" case.