rust-lang / crates.io

The Rust package registry
https://crates.io
Apache License 2.0
3.01k stars 603 forks source link

Latest Version of Crate Does Not Show For Alpha Versions #1144

Closed sunjay closed 5 years ago

sunjay commented 7 years ago

Example: https://crates.io/crates/turtle (example will work until I publish a non-alpha version)

Test case:

Problem: crates.io does not display the 1.0.0-alpha.0 version. If your 0.x.x versions are yanked, it will actually display:

CrateName 0.x.x

This crate has been yanked, but it is still available for download for other crates that may be depending on it.

You may wish to view all versions to find one that has not been yanked.

Viewing the other versions shows that there is a 1.0.0-alpha.0.

That means that it isn't finding that the latest version of the crate is 1.0.0-alpha.0 (possibly due to the -alpha.0 at the end). It still believes the 0.x.x versions are the latest.

Both cargo itself and docs.rs seem to work. If you use a "*" as your version, cargo will download the alpha correctly. If you just go to the crate URL on docs.rs, it'll redirect you correctly.


Instructions to fix added by carol:

carols10cents commented 7 years ago

Have some 0.x.x versions (not sure if they need to be yanked or not to reproduce the problem, but in my case they were)

I think they do need to be yanked.

The reason is that we don't want to recommend prerelease versions if there are non-prerelease versions; we made this change in https://github.com/rust-lang/crates.io/pull/577. What I suspect is happening is that when we look to see if there are non-prerelease versions, we're not checking to make sure there are non-prerelease, non-yanked versions.

carols10cents commented 7 years ago

Another example: https://crates.io/crates/apint

This crate currently has only prerelease versions without any yanked non-prerelease versions, and the prerelease versions are recommended: https://crates.io/crates/shoop

behos commented 7 years ago

I'll pick this up. Very well-defined issue!

behos commented 7 years ago

Another test case which will require some more logic (expands the "Has only prerelease versions, the largest prerelease version should be recommended" test case)

Edit: This doesn't really apply, see message below

behos commented 7 years ago

Unfortunately half of the logic here lives on the server-side which allows for issues to re-appear undetected.

https://github.com/rust-lang/crates.io/blob/master/src/krate/mod.rs#L421-L431

fn version_and_crate(req: &mut Request) -> CargoResult<(Version, Crate)> {
    let crate_name = &req.params()["crate_id"];
    let semver = &req.params()["version"];
    if semver::Version::parse(semver).is_err() {
        return Err(human(&format_args!("invalid semver: {}", semver)));
    };
    let conn = req.db_conn()?;
    let krate = Crate::by_name(crate_name).first::<Crate>(&*conn)?;
    let version = Version::belonging_to(&krate)
        .filter(versions::num.eq(semver))
        .first(&*conn)
        .map_err(|_| {
            human(&format_args!(
                "crate `{}` does not have a version `{}`",
                crate_name,
                semver
            ))
        })?;
    Ok((version, krate))
}

This currently sets the latest non-yanked version as the max-version of the crate which helps us in the happy case, meaning we can skip checking if the latest version in yanked in general. However, if this filter goes away, the front-end will happily render the latest version even if it's yanked.

It seems like the right approach here would be either to remove the yanked check on the backend and always return the latest version (or even no version at all), letting the front-end decide on what to recommend, or include the pre-release checks in the backend, basically removing the need to override the max_version on the front-end since whatever the backend marks as max_version would be the version to use here as well.

Any recommendations around this?

carols10cents commented 7 years ago

It seems like the right approach here would be either to remove the yanked check on the backend and always return the latest version (or even no version at all), letting the front-end decide on what to recommend, or include the pre-release checks in the backend, basically removing the need to override the max_version on the front-end since whatever the backend marks as max_version would be the version to use here as well.

Any recommendations around this?

Hi! Sorry for the delay on this!

I'm not sure what to do either! I'm not sure if there are other places that either the frontend or backend use the max version logic that might be impacted if we move all the logic to one or the other. Maybe try one and see if any tests fail? Our test coverage isn't that great though... My other concern is if anyone is making use of this value from the backend JSON API, in which case we shouldn't remove it, but if it's giving wrong/inconsistent information we should fix it.

I think your pull request is relevant regardless and would fix this issue at least, so I'm going to work on merging that now!

behos commented 6 years ago

From the discussion in #1158:

Max version will be calculated as the latest non-yanked, non-prerelease version, followed by the latest non-yanked, prerelease version followed by 0.0.0. This should be true for every crate object returned by the API

"Just updated" will need a separate solution to be able to not follow this convention. (Maybe we can introduce another field in the API which corresponds to any latest version)

There's no longer any JavaScript magic done on the individual crate page front-end to get the latest version since the one provided from the backend is the correct one. (Which means discarding this PR)

Shields.io will work out of the box for lists since it uses max_version as well. (Ideally we would change the front-end to always request a specific version from shields.io to avoid this kind of issues in the future)

hbina commented 5 years ago

Is this issue still being worked on?

smarnach commented 5 years ago

Since there hasn't been any activity in almost two years, I don't think this is still actively being worked on. Feel free to work on this if you are interested. You can build on top of @behos' work, but it's probably somewhat out of date for the current codebase.

hbina commented 5 years ago

Hi, I have came back after relearning JavaScript >.< . If my understanding of JavaScript is correct, the previous solution finds any version that is stable and not yanked. If this fails, then it falls back to maxVersion. The issue is, if it had search everything and found nothing, that means it also cannot find maxVersion!! Thus, my solution is to simply find anything that is not yet yanked even if it is unstable. If even this fails, then we truly have nothing to offer. I have the commit ready, let me know if this is good enough. Cheers.

hbina commented 5 years ago

I believe this can be closed now per https://github.com/rust-lang/crates.io/pull/1857 ,@carols10cents ?

sunjay commented 5 years ago

My original reproduction steps no longer lead the bug. Closing now.