Just Updated column lists greatest, not the latest version

[bluss]

Right now the Just Updated column lists ndarray (0.5.0-alpha.1) even though the new upload that triggered it was 0.4.9. ndarray @ crates.io

[Eh2406]

I think this is fix/changed?

[sgrif]

No, the summary page still shows the max version next to crates in all positions.

[carols10cents]

Yep, this is where the version is coming from for the /summary JSON that's used on the home page.

[carols10cents]

Ok, so currently the way we get the "just updated" list is:

• Query for crate records where created_at != updated_at (because if they were equal, that would mean the first version of the crate was just published), order by updated_at descending, and take the first 10.
• Using the same encode_crates closure as the other queries do, load all the version records for the crates in this list and pick the largest semver.

There are a number of possible solutions here; I'm not sure which is best.

• A solution that doesn't change the database queries at all would be that for the "just updated" list only, use a different function than the encode_crates closure. Instead of picking the Version::max, pick the version that has created_at equal to the crate's updated_at.
• Along the same lines but implemented by changing the queries, instead of getting ALL versions for the 10 "just updated" crates, query for only the versions of the crate whose created_at is equal to the crate's updated_at. Probably a tiny bit more efficient, but adds complexity to the queries, and I don't think we're worried about the performance of the existing queries.
• Change the query to sort the versions table by created_at descending instead of the crates table, and exclude versions that have their created_at equal to their crate's created_at to exclude new crates. This query seems to get me the same crates as the original:

select crates.name, versions.id, versions.num, crates.updated_at, versions.created_at 
from versions 
join crates on crates.id = versions.crate_id 
and crates.created_at != versions.created_at 
order by versions.created_at desc 
limit 10;

I kind of like this solution because it does one query instead of 2, but it diverges the "just updated" query the most from the other queries, which adds maintenance complexity. I'm also not sure off the top of my head how to translate this to diesel.

Implementing this solution should definitely add tests that capture the current behavior in the common case and the desired behavior in the case described in this issue, around here:

crates.io/src/tests/krate.rs

Lines 1224 to 1288 in 21e04f3

	#[test]
	fn summary_new_crates() {
	let (app, anon, user) = TestApp::init().with_user();
	let user = user.as_model();
	app.db(|conn| {
	let krate = CrateBuilder::new("some_downloads", user.id)
	.version(VersionBuilder::new("0.1.0"))
	.description("description")
	.keyword("popular")
	.downloads(20)
	.recent_downloads(10)
	.expect_build(conn);
	let krate2 = CrateBuilder::new("most_recent_downloads", user.id)
	.version(VersionBuilder::new("0.2.0"))
	.keyword("popular")
	.downloads(5000)
	.recent_downloads(50)
	.expect_build(conn);
	let krate3 = CrateBuilder::new("just_updated", user.id)
	.version(VersionBuilder::new("0.1.0"))
	.expect_build(conn);
	CrateBuilder::new("with_downloads", user.id)
	.version(VersionBuilder::new("0.3.0"))
	.keyword("popular")
	.downloads(1000)
	.expect_build(conn);
	new_category("Category 1", "cat1", "Category 1 crates")
	.create_or_update(conn)
	.unwrap();
	Category::update_crate(conn, &krate, &["cat1"]).unwrap();
	Category::update_crate(conn, &krate2, &["cat1"]).unwrap();
	// set total_downloads global value for `num_downloads` prop
	update(metadata::table)
	.set(metadata::total_downloads.eq(6000))
	.execute(&*conn)
	.unwrap();
	// update 'just_updated' krate. Others won't appear because updated_at == created_at.
	let updated = Utc::now().naive_utc();
	update(&krate3)
	.set(crates::updated_at.eq(updated))
	.execute(&*conn)
	.unwrap();
	});
	let json: SummaryResponse = anon.get("/api/v1/summary").good();
	assert_eq!(json.num_crates, 4);
	assert_eq!(json.num_downloads, 6000);
	assert_eq!(json.most_downloaded[0].name, "most_recent_downloads");
	assert_eq!(
	json.most_recently_downloaded[0].name,
	"most_recent_downloads"
	);
	assert_eq!(json.popular_keywords[0].keyword, "popular");
	assert_eq!(json.popular_categories[0].category, "Category 1");
	assert_eq!(json.just_updated.len(), 1);
	assert_eq!(json.just_updated[0].name, "just_updated");
	assert_eq!(json.new_crates.len(), 4);
	}

[smarnach]

What is the purpose of the "Just updated" section on the homepage? I haven't personally found it useful so far.

[carols10cents]

What is the purpose of the "Just updated" section on the homepage? I haven't personally found it useful so far.

That's a great question! At least originally, I think the idea was to quickly show the activity going on, because it's mildly interesting.

[bruceadams]

I've been stumbling toward what I hope is a fix for this issue. I started with the tests, which led me to decide that adding newest_version along side of max_version made some sense.

I have an incomplete PR #1931 which has a commented out test that fails. I think that is just a test setup problem. Also, the PR code figures out which version is the most recently updated, but doesn't bubble that information all the way up to the user interface.

My question here is about the approach. Does the general shape of the PR code seem good? I didn't take any of the three paths that @carols10cents suggested. Oops!

I am unsettled to realize there there are two other places in the code that gets all the versions and the picks the highest version: src/controllers/krate/metadata.rs and src/controllers/krate/search.rs. I don't see a low impact way to fold those queries into my new top_versions function.

[bruceadams]

Ah. As usual, once I start talking about some code, I find other ways to look at it. The stuff I changed doesn't include the query used the GET /summary route. My (commented out) test failure is a real failure. (I'm remain unsure if the test setup is correct, but no way that test can pass as the code stands.)

[bruceadams]

I'm happy with my PR at this point. The path I took adds a field, newest_version, into EncodableCrate and makes sure that field is correctly populated. (I don't know if adding a field to EncodableCrate is a bad idea.) Two of the database queries are unchanged. The remaining, relevant database query (in models/krate.rs) now selects two fields instead of just one field. There is a little bit more processing of the query results, but I believe it is a small amount of additional work for the server.

What remains to do is to use newest_version in the user interface.

[bruceadams]

Hmm. Looking at https://crates.io home page… I find the crate versions in several columns confusing. It's pretty clear that page always shows the highest version number for each crate mentioned. For example, under Most Downloaded right now is syn (1.0.11) and yet, that is misleading. syn version 1.0.11 is less than a day old. The listed, new version isn't most downloaded. Older versions of syn have been downloaded far more times.

[carols10cents]

Hi Bruce! Thank you so much for working on this!!

My question here is about the approach. Does the general shape of the PR code seem good? I didn't take any of the three paths that @carols10cents suggested. Oops!

That's totally fine!!! I'm extremely open to ideas that I wasn't able to think of! I see now that I was assuming the change to the logic should be pushed down into the query and have the SQL be responsible, but your approach separates the queries (which are actually fine as-is) from the display in the UI (which is rightfully a separate concern). Neat, and I like it!

Hmm. Looking at crates.io home page… I find the crate versions in several columns confusing. It's pretty clear that page always shows the highest version number for each crate mentioned. For example, under Most Downloaded right now is syn (1.0.11) and yet, that is misleading. syn version 1.0.11 is less than a day old. The listed, new version isn't most downloaded. Older versions of syn have been downloaded far more times.

I... hadn't really thought about how the version number doesn't really make sense for the most downloaded list! That list uses the total number of downloads for all versions of that crate, so perhaps we shouldn't display a version number for that column at all. That also goes for the "Most Recently Downloaded" column.

Review of the PR incoming, with some pointers to the frontend code that'll need to be updated with this approach.

[bruceadams]

I think this was fixed by #1931, meaning this issue can be closed.