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

Link to specific version for yanked crate during search #2379

Merged
merged 4 commits into from
Feb 14, 2024

Conversation

Tyrubias
Copy link
Contributor

@Tyrubias Tyrubias commented Jan 5, 2024

Queries the database during search to check if a release is yanked. If it is, link to the specific version rather than simply latest to avoid seeing the "The requested version does not exist" message.

Closes #2373

@Tyrubias Tyrubias requested a review from a team as a code owner January 5, 2024 04:53
@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Jan 5, 2024
@Tyrubias
Copy link
Contributor Author

Tyrubias commented Jan 5, 2024

I haven't been able to test this locally because I'm still in the process of manually seeding my local database with data. If anyone has advice on how to do this, I would appreciate the help 😄

Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

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

Thank you for the help!

From the code I would say this works, but we have some edge cases to think about.

Looking at #1691 and #2239, we want /latest/ in the URL for all cases where possible.

In #2373 we say that we only want the exact version here, when all versions for a crate are yanked. Or to be more specific: we want an exact version here for all the cases where /latest/ would lead to a 404, while versions exist. Looking at our match_version logic the only case where we would get a 404 here is when all versions are yanked.

This is why initially I was thinking about actually checking all releases in the search-handler SQL, so see if all of them are yanked, and giving this information to the template to render the specific version then.

Now back to your approach:
releases.latest_version_id is set after each build for a crate, so the latest non-pre and non-yanked version if these exist, or the newest version. So while in many cases the latest_version_id is only yanked, when all releases are yanked, there might be cases around pre-releases where this isn't true. Also, looking through our logic here I see an edge case when a release is directly yanked, where we don't update latest_version_id accordingly.

In the end, about test-data:
the easiest way to get in valid test-data is to manually run some builds on releases, if that's easily possible.

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jan 6, 2024
@Tyrubias Tyrubias force-pushed the linked-yanked-on-search branch 2 times, most recently from e57cd4e to 70e5534 Compare January 24, 2024 07:38
@Tyrubias Tyrubias requested a review from syphar January 24, 2024 07:39
@github-actions github-actions bot added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Jan 24, 2024
@Tyrubias
Copy link
Contributor Author

I changed the search handler to retrieve all versions, then set a flag if all the versions are yanked. To be honest, I'm not entirely happy with this approach, so if you have any tips for how to improve this I would greatly appreciate it.

I think this addresses the edge cases because it appears that when everything but pre-releases are yanked, the pre-release is set as the latest version (and that's the one we default to pulling with this code).

@Tyrubias
Copy link
Contributor Author

Also are there any tests I should be adding for this change?

Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

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

Thank for for your continued work on this! and sorry for the delayed response from my side.

Also are there any tests I should be adding for this change?

We have a generic search test which could be extended. You would add a new fake crate to the mocked crates.io response, create a yanked fake release in the db, and check if the link does include the explicit version.

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jan 31, 2024
@Tyrubias Tyrubias force-pushed the linked-yanked-on-search branch from 70e5534 to 76e65dd Compare February 13, 2024 06:55
@Tyrubias Tyrubias force-pushed the linked-yanked-on-search branch from 76e65dd to a7154f1 Compare February 13, 2024 07:02
@Tyrubias Tyrubias requested a review from syphar February 13, 2024 07:16
@github-actions github-actions bot added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Feb 13, 2024
@syphar syphar merged commit a0c5cca into rust-lang:master Feb 14, 2024
11 checks passed
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Feb 14, 2024
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clicking on a search result for a fully yanked crate takes you to a version not found page
2 participants