Skip to content

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

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

Closed
7 tasks
sunjay opened this issue Oct 23, 2017 · 12 comments
Closed
7 tasks

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

sunjay opened this issue Oct 23, 2017 · 12 comments
Labels
C-bug 🐞 Category: unintended, undesired behavior E-has-mentor E-help-wanted

Comments

@sunjay
Copy link
Member

sunjay commented Oct 23, 2017

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

Test case:

  • 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)
  • Release a 1.0.0-alpha.0 version (doesn't matter what number you put after the alpha)

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:

  • This check that tries to find the latest stable version should find the latest stable NON-YANKED version instead. The version has a yanked boolean property accessible via version.get('yanked').
  • Create more tests similar to this test that set up a bunch of different situations:
    • All versions yanked should get the "all versions yanked" page
    • Has larger prerelease versions and non prerelease versions, all NOT yanked, the version recommended on the page should be the largest non prerelease version
    • Has only prerelease versions, the largest prerelease version should be recommended
    • (This issue) Has larger prerelease versions and non prerelease versions, all non prerelease versions yanked, the version recommended on the page should be the largest prerelease version
    • Has larger prerelease versions (say 1.0.0-alpha), a smaller non-yanked non-prerelease version (say 0.8.0), and a yanked non-prerelease version in the middle (say 0.9.0), the recommended version should be the non-yanked non-prerelease version
    • Any other test cases you think of that I haven't!
@carols10cents
Copy link
Member

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 #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 carols10cents added A-versions C-bug 🐞 Category: unintended, undesired behavior labels Oct 24, 2017
@carols10cents
Copy link
Member

carols10cents commented Oct 24, 2017

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
Copy link

behos commented Nov 3, 2017

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

@behos
Copy link

behos commented Nov 5, 2017

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

  • All versions yanked except a pre-release version in the middle (with newer and older prerelease and stable versions) should recommend the non-yanked pre-release

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

@behos
Copy link

behos commented Nov 5, 2017

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
Copy link
Member

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
Copy link

behos commented Dec 23, 2017

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
Copy link
Contributor

hbina commented Sep 28, 2019

Is this issue still being worked on?

@smarnach
Copy link
Contributor

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
Copy link
Contributor

hbina commented Sep 29, 2019

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 added a commit to hbina/rustomword-rs that referenced this issue Sep 29, 2019
@hbina
Copy link
Contributor

hbina commented Oct 7, 2019

I believe this can be closed now per #1857 ,@carols10cents ?

@sunjay
Copy link
Member Author

sunjay commented Oct 7, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug 🐞 Category: unintended, undesired behavior E-has-mentor E-help-wanted
Projects
None yet
Development

No branches or pull requests

5 participants