Crates who have yanked all their versions should only return name, owners, and that it's yanked

[carols10cents]

In search results and on the crate's page, we should be displaying crates with all of their versions yanked (henceforth referred to as "yanked crates") differently (#145), but the backend needs to tell the frontend that a crate is in this state.

Instructions for fixing this:

• Add a field named all_versions_yanked to EncodableCrate and everywhere we create EncodableCrate instances, set that field to false if there exists any non-yanked versions for this crate. I'd recommend doing a select(exists()) query using diesel similar to this query
• If we're constructing an EncodableCrate instance for a yanked crate, these fields should be changed too:
  • badges, description, homepage, documentation, and repository should be None
  • max_version should be "0.0.0"

Some other fields, like keywords and categories, are used for search/crate lists, and this issue isn't going to change search results, so I think we should still display why a crate showed up in a particular list of crates.

[seanlinsley]

I'd like to work on this. It'll be my first Rust code contribution! Thanks for making it so accessible to new people ❤️ I actually got here from the RustConf keynote on YouTube.

[carols10cents]

Hi @seanlinsley!!! It's awesome to have you!! Please let me know if you have any questions ❤️

[seanlinsley]

When running diesel migration run I noticed this error:

NOTICE:  table "schema_migrations" does not exist, skipping

The backend server appears to boot, though some tables are missing when running the tests:

---- category::tests::category_toplevel_includes_subcategories_in_crate_cnt stdout ----
  thread 'category::tests::category_toplevel_includes_subcategories_in_crate_cnt' panicked at 'called `Result::unwrap()` on an `Err` value: DatabaseError(__Unknown, "relation \"categories\" does not exist")', src/libcore/result.rs:860:4

---- category::tests::category_toplevel_applies_limit_and_offset stdout ----
  thread 'category::tests::category_toplevel_applies_limit_and_offset' panicked at 'called `Result::unwrap()` on an `Err` value: DatabaseError(__Unknown, "relation \"categories\" does not exist")', src/libcore/result.rs:860:4

---- category::tests::category_toplevel_orders_by_crates_cnt_when_sort_given stdout ----
  thread 'category::tests::category_toplevel_orders_by_crates_cnt_when_sort_given' panicked at 'called `Result::unwrap()` on an `Err` value: DatabaseError(__Unknown, "relation \"categories\" does not exist")', src/libcore/result.rs:860:4

---- category::tests::category_toplevel_applies_limit_and_offset_after_grouping stdout ----
  thread 'category::tests::category_toplevel_applies_limit_and_offset_after_grouping' panicked at 'called `Result::unwrap()` on an `Err` value: DatabaseError(__Unknown, "relation \"categories\" does not exist")', src/libcore/result.rs:860:4

---- category::tests::category_toplevel_excludes_subcategories stdout ----
  thread 'category::tests::category_toplevel_excludes_subcategories' panicked at 'called `Result::unwrap()` on an `Err` value: DatabaseError(__Unknown, "relation \"categories\" does not exist")', src/libcore/result.rs:860:4
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- keyword::tests::dont_associate_with_non_lowercased_keywords stdout ----
  thread 'keyword::tests::dont_associate_with_non_lowercased_keywords' panicked at 'called `Result::unwrap()` on an `Err` value: DatabaseError(__Unknown, "relation \"keywords\" does not exist")', src/libcore/result.rs:860:4


failures:
    category::tests::category_toplevel_applies_limit_and_offset
    category::tests::category_toplevel_applies_limit_and_offset_after_grouping
    category::tests::category_toplevel_excludes_subcategories
    category::tests::category_toplevel_includes_subcategories_in_crate_cnt
    category::tests::category_toplevel_orders_by_crates_cnt_when_sort_given
    keyword::tests::dont_associate_with_non_lowercased_keywords

test result: FAILED. 12 passed; 6 failed; 0 ignored; 0 measured; 0 filtered out

The setup instructions say that cargo test should run the migrations automatically. Perhaps that's broken?

A caveat was that I ran diesel migration run (for the dev env) twice before it ran successfully (because of misconfiguration). Perhaps they modified some global state before exiting?

These were the exact errors:

# I hadn't set up `.env` yet

$ diesel migration run
The --database-url argument must be passed, or the DATABASE_URL environment variable must be set.

# Homebrew's install doesn't provide a postgres user

$ diesel migration run
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ConnectionError(BadConnection("FATAL:  role \"postgres\" does not exist\n"))', src/libcore/result.rs:860:4
note: Run with `RUST_BACKTRACE=1` for a backtrace.

[seanlinsley]

Indeed, the test database has no tables in it.

[seanlinsley]

Oddly diesel migration run --database-url cargo_registry_test doesn't do anything.

[sgrif]

It's a clap bug. clap-rs/clap#978 Unfortunately you need to do diesel migration --database-url=... run. Your database URL should probably be postgres://localhost/cargo_registry_test

[sgrif]

If you have TEST_DATABASE_URL set in a .env file, just running cargo test should migrate it automatically

[carols10cents]

If you have TEST_DATABASE_URL set in a .env file, just running cargo test should migrate it automatically

The trick is that cargo test migrates the database when you compile the tests. If you had the tests compiled previously, didn't change anything, and reran cargo test, that wouldn't migrate the test database :(

I'm not sure how to fix the underlying issue, ideas welcome over at #988.

In the meantime, @seanlinsley, if you change a test file and run cargo test, or alternatively run cargo clean and then cargo test, does that get the tests to pass? Sorry about the trouble :( :(

[seanlinsley]

Sorry for the delay on this; picking it back up today.

[seanlinsley]

cargo clean indeed worked. I'm not familiar enough with the problem space to suggest a fix.

I didn't delete the database myself like described in #988. I thought I'd seen an exception during the test setup when running cargo test for the first time, but that doesn't seem to have been the case...

[seanlinsley]

encodable should definitely send down this attribute, but what about minimal_encodable?

[seanlinsley]

It looks like the show action uses encodable and the index action uses minimal_encodable, so yes they both need to be updated.

Both the show & index actions already load all versions into memory:

crates.io/src/krate/mod.rs

Lines 880 to 882 in ef1ac58

	let mut versions = Version::belonging_to(&krate).load::<Version>(&*conn)?;
	versions.sort_by(|a, b| b.num.cmp(&a.num));
	let ids = versions.iter().map(|v| v.id).collect();

crates.io/src/krate/mod.rs

Lines 736 to 740 in ef1ac58

	let versions = Version::belonging_to(&crates)
	.load::<Version>(&*conn)?
	.grouped_by(&crates)
	.into_iter()
	.map(|versions| Version::max(versions.into_iter().map(|v| v.num)));

So it may be more efficient to construct this attribute in memory, instead of doing another database request.

This should work for the show page:

let all_versions_yanked = versions.iter().any(|v| !v.yanked).collect();

For the index page, I was thinking of updating the existing versions...map to also return all_versions_yanked, such that the zip...map ends up looking like this:

@@ -744,7 +748,7 @@ pub fn index(req: &mut Request) -> CargoResult<Response> {
         .zip(perfect_matches)
         .zip(recent_downloads)
         .map(
-            |(((max_version, krate), perfect_match), recent_downloads)| {
+            |((((max_version, all_versions_yanked), krate), perfect_match), recent_downloads)| {
                 // FIXME: If we add crate_id to the Badge enum we can eliminate
                 // this N+1
                 let badges = badges::table

I'm not totally happy with that though; it seems like that code has a tendency to become more and more nested. I'll also need to learn a bit more Rust to write it 😄

[seanlinsley]

Using select(exists()) was recommended, which would make sense for the index page. I'd usually expect to see that on the main select by joining the versions table, however the main select isn't currently joining many other tables:

crates.io/src/krate/mod.rs

Lines 602 to 615 in ef1ac58

	let mut query = crates::table
	.left_join(
	crate_downloads::table.on(
	crates::id
	.eq(crate_downloads::crate_id)
	.and(crate_downloads::date.gt(date(now - 90.days()))),
	),
	)
	.group_by(crates::id)
	.select((
	ALL_COLUMNS,
	AsExpression::<Bool>::as_expression(false),
	recent_downloads.clone(),
	))

---

The summary route has the same problem of having a lot of custom code to build max_version...

crates.io/src/krate/mod.rs

Lines 797 to 802 in ef1ac58

	.map(|versions| Version::max(versions.into_iter().map(|v| v.num)))
	.zip(krates)
	.map(|(max_version, krate)| {
	Ok(krate.minimal_encodable(max_version, None, false, None))
	})
	.collect()

I'm tempted to update encodable and minimal_encodable to accept a vector of versions, instead of vectors of version numbers and max_version. That's already the case for keywords, categories, and badges, but not recent downloads.

[seanlinsley]

Ah, the problem with that is that the versions already loaded are excluding yanked versions.

Another thing to note is that it's not as simple as select(exists()) or .any(|v| !v.yanked), since you wouldn't want all_versions_yanked to equal true if it's a brand new crate that hasn't released and versions. Or is that not possible?

[seanlinsley]

I'm having trouble figuring out how to get grouped_by to work with zip, so that it'll pass down all versions grouped by a crate.

[seanlinsley]

Unsure if the zip issue went away because I fixed some type errors, but at least it's no longer a compile error. The only remaining error I have is:

error[E0619]: the type of this value must be known in this context
   --> src/krate/mod.rs:360:40
    |
360 |         let max_version = Version::max(versions.into_iter().map(|v| v.num));
    |                                        ^^^^^^^^^^^^^^^^^^^^

From this code:

    pub fn encodable(
        self,
        versions: Vec<Version>,
        keywords: Option<&[Keyword]>,
        categories: Option<&[Category]>,
        badges: Option<Vec<Badge>>,
        exact_match: bool,
        recent_downloads: Option<i64>,
    ) -> EncodableCrate {
        let (versions, yanked_versions) = versions.into_iter().partition(|v| !v.yanked);

        let max_version = Version::max(versions.into_iter().map(|v| v.num));

        let all_versions_yanked = yanked_versions.len() > 0 && versions.len() == 0;

Which is odd, because the line right above that doesn't have an issue calling into_iter.

[seanlinsley]

Ah, I guess the extra call to into_iter should be unnecessary because partition returns iterators. So the problem might be that I'm reassigning the versions variable to a different type (from vector to iterator).

[seanlinsley]

Nope, it's still a problem when defining it to a new variable:

let (live_versions, yanked_versions) = versions.into_iter().partition(|v| !v.yanked);

let max_version = Version::max(live_versions.map(|v| v.num));

[seanlinsley]

This is the first version I've written that compiles 🎉

let (live_versions, yanked_versions): (Vec<Version>, Vec<Version>) = versions.into_iter().partition(|v| !v.yanked);

let live_versions = live_versions.into_iter();

let version_ids = live_versions.clone().map(|v| v.id);
let max_version = Version::max(live_versions.clone().map(|v| v.num));

let all_versions_yanked = yanked_versions.len() > 0 && live_versions.len() == 0;

It certainly seems far from optimal, having to call clone. I had to do that to get around E0382.

[hbina]

Hi, if no one is working on this issue I want to pick it up :3

[carols10cents]

Hi @hbina, sorry for the delay in responding, no one is currently working on this that I know of so please feel free if you're still interested!

[sorcerix]

Is anyone currently working on this? If not would I be able to take it on?

[carols10cents]

I don't know of any work being done on this right now, please go ahead! Let me know if you have any questions!

[willolisp]

Hey there! Anyone working on this one right now? Also, would this be a good first issue for someone completely new to the language :)? Cheers!

[carols10cents]

@willolisp I don't think there's anyone working on it, and yes, I think this would be a good first issue! Please ask if you have questions, thanks!

[willolisp]

@carols10cents cool! Then I'll give this one a try. Thanks so much!

[hayleyjames]

Hey 👋

Is anyone working on this at the moment? If not, I would like to tackle this issue.

@carols10cents Assuming I can work on this, should EncodableCrate::from_minimal also support all_versions_yanked?

[carols10cents]

@hayleyjames I would expect any progress on this to be shown on the issue or in a connected PR, so it looks like no one is working on this right now!

Assuming I can work on this, should EncodableCrate::from_minimal also support all_versions_yanked?

Yes, on a quick glance, I think so, because from_minimal is used on the search page and that's an important spot we want to use all_versions_yanked.

[hayleyjames]

@carols10cents Thanks! I'll start working on this.

[mario-areias]

Hi @carols10cents I put a PR together for this issue based on the comments from this issue: #5314

This is my first PR in Rust, so I probably did some silly mistakes 😅 . Let me know what needs to be changed and I will keep working on it :)

[Turbo87]

we talked about this situation in the crates.io team meeting last week and decided that we would like to change course on this issue. only displaying reduced information has quite a few downsides (e.g. who do you contact to maybe take over ownership of the crate) and we would prefer to display yanked crates/versions like their unyanked counterparts, but with some sort of banner that makes it very visible that the users is looking at a yanked crate/version.

since this is a significant change to the original issue description I'll close this issue and create a new one, based on the above.