rust-lang / crates.io

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

Crate listings duplicated on category subpages #524

Closed wesleywiser closed 7 years ago

wesleywiser commented 7 years ago

Some crates are being listed multiple times on the top-level category pages. For example, "Development tools":

image

It looks like this happens when the crate declares the top level category as well as one or more sub categories in its Cargo.toml file.

carols10cents commented 7 years ago

Ah interesting. I guess it wasn't clear that a crate would automatically be in the supercategory if listed in the subcategory.

carols10cents commented 7 years ago

The fix to this will involve changing the query that gets crates in a category to do a SELECT DISTINCT, and we should probably add a test for this... looks like I didn't add any tests for the crate index API route that use the category parameter, oops.

The test should be kind of like this keyword parameter test but will involve creating a category and a subcategory, adding a crate to that category and subcategory, and querying for the category.

Eh2406 commented 7 years ago

So I tried to write a test, I expect this version to pass: https://is.gd/1E9xIQ (this gose in tests/krate.rs) But it fails with bad response: (404, "Not Found").

When it passes the plan is to replace &["kw1".into()] with &["kw1".into(), "kw1::foo".into()] and have it fail. Then fix the bug and have it pass again.

What am I doing wrong? Am I on the right track?

carols10cents commented 7 years ago

@Eh2406 so the example test I cited was with keywords, while this bug is for categories. The test code you have is still calling the code to add a keyword to the crate: Keyword::update_crate(tx(&req), &krate, &["kw1".into()]).unwrap(); Instead, you'll need to add a category.

One difference between keywords and categories is that anyone can add a keyword by just starting to use it on their crate. Categories are set by crates.io, so you can only use a category that exists on crates.io already. So your test will need to create a category before putting a crate in the category. An example of a test that creates a category is this one. And in this particular case, you'll want to create two categories actually: the parent category and the child category.

Once the categories exist, you should be able to call Category::update_crate like this test does instead of the line in your code where Keyword::update_crate is.

Does that make sense?

Eh2406 commented 7 years ago

Yes. How long did I stare at the code without noting the Keyword right on the line I was worried about? It always seems to be something small and obvious, lol.

Eh2406 commented 7 years ago

So, added a test to update_crate. This test usefully tells us that the api/v1 summary statistics are correct. Unfortunately, that is not where the bug happens. Then I read your original description, add a comparable test to the end of index_queries and finally have a test for the bug! Tomorrow I can start work on fixing it and a pr. :-)

carols10cents commented 7 years ago

Merged in #558 so this is closed now!!