From d3973453267b2fd0ed94e0f79c63980f09ff72c5 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Wed, 8 Mar 2017 08:35:00 -0500 Subject: [PATCH] Fix `reverse_dependencies` So in #592, I accidentally changed the meaning of the `reverse_dependencies` query from "select 10 crates where the max version of that crate depends on this one" to "select 10 crates where that crate had a version with the same number as this one's max version, which depended on this crate" which is nonsense and wrong. There's actually no way that we can truly replicate the old behavior without breaking pagination or loading all the versions of every crate which has ever depended on another into memory and doing the limiting locally... which would defeat the purpose of pagination. The only real alternative here is to revert #592 and go back to updating the `max_version` column. I still think this approach is the right one, even with the added complexity to this column, as updating `max_version` will always be bug-prone unless we can do it in SQL itself. It's too bad we can't enable arbitrary extensions on heroku PG, as we could actually create a true semver type that links to the rust crate if we could. The main difference in behavior between this and the `max_version` column is that if a crate had *only* prerelease versions, and only some of those prerelease versions depended on another crate, it's effectively random whether it would appear in this list or not. It's a very niche edge case and one that I'm not terribly concerned about. I'd need to play around with indexes to see if this query could be optimized further, but the performance should be reasonable, as the window function will only require a single table scan. Fixes #602. --- .../down.sql | 3 + .../up.sql | 17 ++++ src/krate.rs | 18 ++-- src/tests/krate.rs | 96 +++++++++++++++++++ 4 files changed, 127 insertions(+), 7 deletions(-) create mode 100644 migrations/20170308140537_create_to_semver_no_prerelease/down.sql create mode 100644 migrations/20170308140537_create_to_semver_no_prerelease/up.sql diff --git a/migrations/20170308140537_create_to_semver_no_prerelease/down.sql b/migrations/20170308140537_create_to_semver_no_prerelease/down.sql new file mode 100644 index 00000000000..f6fed16eca1 --- /dev/null +++ b/migrations/20170308140537_create_to_semver_no_prerelease/down.sql @@ -0,0 +1,3 @@ +DROP INDEX versions_to_semver_no_prerelease_idx; +DROP FUNCTION to_semver_no_prerelease(text); +DROP TYPE semver_triple; diff --git a/migrations/20170308140537_create_to_semver_no_prerelease/up.sql b/migrations/20170308140537_create_to_semver_no_prerelease/up.sql new file mode 100644 index 00000000000..378e86371b4 --- /dev/null +++ b/migrations/20170308140537_create_to_semver_no_prerelease/up.sql @@ -0,0 +1,17 @@ +CREATE TYPE semver_triple AS ( + major int4, + minor int4, + teeny int4 +); + +CREATE FUNCTION to_semver_no_prerelease(text) RETURNS semver_triple IMMUTABLE AS $$ + SELECT ( + split_part($1, '.', 1)::int4, + split_part($1, '.', 2)::int4, + split_part(split_part($1, '+', 1), '.', 3)::int4 + )::semver_triple + WHERE strpos($1, '-') = 0 + $$ LANGUAGE SQL +; + +CREATE INDEX versions_to_semver_no_prerelease_idx ON versions (crate_id, to_semver_no_prerelease(num) DESC NULLS LAST) WHERE NOT yanked; diff --git a/src/krate.rs b/src/krate.rs index d4f4afb389a..2f02a398ac2 100644 --- a/src/krate.rs +++ b/src/krate.rs @@ -432,12 +432,17 @@ impl Crate { -> CargoResult<(Vec<(Dependency, String, i32)>, i64)> { let select_sql = " FROM dependencies - INNER JOIN versions + INNER JOIN ( + SELECT versions.*, + row_number() OVER (PARTITION BY crate_id ORDER BY to_semver_no_prerelease(num) DESC NULLS LAST) rn + FROM versions + WHERE NOT yanked + ) versions ON versions.id = dependencies.version_id INNER JOIN crates ON crates.id = versions.crate_id WHERE dependencies.crate_id = $1 - AND versions.num = $2 + AND rn = 1 "; let fetch_sql = format!("SELECT DISTINCT ON (crate_downloads, crate_name) dependencies.*, @@ -445,19 +450,18 @@ impl Crate { crates.name AS crate_name {} ORDER BY crate_downloads DESC - OFFSET $3 - LIMIT $4", + OFFSET $2 + LIMIT $3", select_sql); let count_sql = format!("SELECT COUNT(DISTINCT(crates.id)) {}", select_sql); let stmt = conn.prepare(&fetch_sql)?; - let max_version = self.max_version(conn)?.to_string(); - let vec: Vec<_> = stmt.query(&[&self.id, &max_version, &offset, &limit])? + let vec: Vec<_> = stmt.query(&[&self.id, &offset, &limit])? .iter() .map(|r| (Model::from_row(&r), r.get("crate_name"), r.get("crate_downloads"))) .collect(); let stmt = conn.prepare(&count_sql)?; - let cnt: i64 = stmt.query(&[&self.id, &max_version])?.iter().next().unwrap().get(0); + let cnt: i64 = stmt.query(&[&self.id])?.iter().next().unwrap().get(0); Ok((vec, cnt)) } diff --git a/src/tests/krate.rs b/src/tests/krate.rs index 4ada73d6eb7..f2f73239bbd 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -1183,6 +1183,102 @@ fn reverse_dependencies() { assert_eq!(deps.meta.total, 0); } +#[test] +fn reverse_dependencies_when_old_version_doesnt_depend_but_new_does() { + let (_b, app, middle) = ::app(); + + let v100 = semver::Version::parse("1.0.0").unwrap(); + let v110 = semver::Version::parse("1.1.0").unwrap(); + let v200 = semver::Version::parse("2.0.0").unwrap(); + let mut req = ::req(app, Method::Get, + "/api/v1/crates/c1/reverse_dependencies"); + ::mock_user(&mut req, ::user("foo")); + let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), &v110); + let _ = ::mock_crate_vers(&mut req, ::krate("c2"), &v100); + let (_, c2v2) = ::mock_crate_vers(&mut req, ::krate("c2"), &v200); + + ::mock_dep(&mut req, &c2v2, &c1, None); + + let mut response = ok_resp!(middle.call(&mut req)); + let deps = ::json::(&mut response); + assert_eq!(deps.dependencies.len(), 1); + assert_eq!(deps.meta.total, 1); + assert_eq!(deps.dependencies[0].crate_id, "c2"); +} + +#[test] +fn reverse_dependencies_when_old_version_depended_but_new_doesnt() { + let (_b, app, middle) = ::app(); + + let v100 = semver::Version::parse("1.0.0").unwrap(); + let v200 = semver::Version::parse("2.0.0").unwrap(); + let mut req = ::req(app, Method::Get, + "/api/v1/crates/c1/reverse_dependencies"); + ::mock_user(&mut req, ::user("foo")); + let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), &v100); + let (_, c2v1) = ::mock_crate_vers(&mut req, ::krate("c2"), &v100); + let _ = ::mock_crate_vers(&mut req, ::krate("c2"), &v200); + + ::mock_dep(&mut req, &c2v1, &c1, None); + + let mut response = ok_resp!(middle.call(&mut req)); + let deps = ::json::(&mut response); + assert_eq!(deps.dependencies.len(), 0); + assert_eq!(deps.meta.total, 0); +} + +#[test] +fn prerelease_versions_not_included_in_reverse_dependencies() { + let (_b, app, middle) = ::app(); + + let v100 = semver::Version::parse("1.0.0").unwrap(); + let v110_pre = semver::Version::parse("1.1.0-pre").unwrap(); + let mut req = ::req(app, Method::Get, + "/api/v1/crates/c1/reverse_dependencies"); + ::mock_user(&mut req, ::user("foo")); + let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), &v100); + let _ = ::mock_crate_vers(&mut req, ::krate("c2"), &v110_pre); + let (_, c3v1) = ::mock_crate_vers(&mut req, ::krate("c3"), &v100); + let _ = ::mock_crate_vers(&mut req, ::krate("c3"), &v110_pre); + + ::mock_dep(&mut req, &c3v1, &c1, None); + + let mut response = ok_resp!(middle.call(&mut req)); + let deps = ::json::(&mut response); + assert_eq!(deps.dependencies.len(), 1); + assert_eq!(deps.meta.total, 1); + assert_eq!(deps.dependencies[0].crate_id, "c3"); +} + +#[test] +fn yanked_versions_not_included_in_reverse_dependencies() { + let (_b, app, middle) = ::app(); + + let v100 = semver::Version::parse("1.0.0").unwrap(); + let v200 = semver::Version::parse("2.0.0").unwrap(); + let mut req = ::req(app, Method::Get, + "/api/v1/crates/c1/reverse_dependencies"); + ::mock_user(&mut req, ::user("foo")); + let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), &v100); + let _ = ::mock_crate_vers(&mut req, ::krate("c2"), &v100); + let (_, c2v2) = ::mock_crate_vers(&mut req, ::krate("c2"), &v200); + + ::mock_dep(&mut req, &c2v2, &c1, None); + + let mut response = ok_resp!(middle.call(&mut req)); + let deps = ::json::(&mut response); + assert_eq!(deps.dependencies.len(), 1); + assert_eq!(deps.meta.total, 1); + assert_eq!(deps.dependencies[0].crate_id, "c2"); + + t!(c2v2.yank(req.tx().unwrap(), true)); + + let mut response = ok_resp!(middle.call(&mut req)); + let deps = ::json::(&mut response); + assert_eq!(deps.dependencies.len(), 0); + assert_eq!(deps.meta.total, 0); +} + #[test] fn author_license_and_description_required() { let (_b, app, middle) = ::app();