Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate from docs.rs' builds API to status API #6900

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Jul 30, 2023

To simplify refactoring how our builds are processed I want to remove the /builds.json endpoint on docs.rs. As a replacement I added a new endpoint simply giving the status that sites like crates.io wants: are there docs for this version available to link to. This also fixes some small issues with the old endpoint (some edgecases meant the first build was successful but there were no docs, or there were docs but the first build was marked unsuccessful; if the version in the path doesn't match an exact version it is used as a semver query, but using a =version path like done here on the old API would hit CORS issues).

@Nemo157 Nemo157 force-pushed the docs.rs-status-api branch from 2ddc442 to f27dea7 Compare July 30, 2023 09:30
@Nemo157 Nemo157 force-pushed the docs.rs-status-api branch from f27dea7 to 23100b7 Compare July 30, 2023 09:55
@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-frontend 🐹 labels Jul 30, 2023
loadDocsBuildsTask = task(async () => {
return await ajax(`https://docs.rs/crate/${this.crateName}/${this.num}/builds.json`);
loadDocsStatusTask = task(async () => {
return await ajax(`https://docs.rs/crate/${this.crateName}/=${this.num}/status.json`);
Copy link
Member

@Turbo87 Turbo87 Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bildschirmfoto 2023-07-31 um 09 57 48

it looks like the URL with = just redirects to the same URL without the =. are you sure that this is necessary?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, I just had a quick look at the linked PR. looks like =1.2.3 will return 404 if the version does not exist and redirect to 1.2.3 if the version exists. then 1.2.3 will look for 1.2.3 and if that does not exist will try ^1.2.3, but since we know from the first call that it exists it won't interpret it as semver.

I'm not a huge fan of this API decision, but I guess we have to live with it now :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only redirects if that exact version is known about, without the = it does a semver match and will redirect to a different version if one is available. As an example https://docs.rs/crate/stylish-core/0.1.0/status.json redirects to https://docs.rs/crate/stylish-core/0.1.1/status.json (because of reasons around circular dependencies 0.1.0 was never added to the docs.rs database).

This is existing behavior for all the endpoints, including /builds.json, but that did not get CORS headers applied when it returned a redirect making it still get treated as an error, e.g. on https://crates.io/crates/stylish-core/0.1.0

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://docs.rs/crate/stylish-core/0.1.0/builds.json. (Reason: CORS request did not succeed). Status code: (null).

Since that is also fixed on this endpoint, it would link to the 0.1.1 docs without the =.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I understand that this is just legacy behavior we have to live with now. it's fine, was just a bit surprising to see that we need the extra ping-pong for exact version queries. maybe it would make sense for the JSON endpoints to return the content directly for =1.2.3 queries instead of redirecting?

@Turbo87 Turbo87 merged commit 5d7e104 into rust-lang:main Jul 31, 2023
@Nemo157 Nemo157 deleted the docs.rs-status-api branch July 31, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend 🐹 C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants