Fix reverse_dependencies

So in rust-lang#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 rust-lang#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 rust-lang#602.

Author: sgrif

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;

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;

src/krate.rs

@@ -432,32 +432,36 @@ 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.*,
                                         crates.downloads AS crate_downloads,
                                         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))
     }

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::<RevDeps>(&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::<RevDeps>(&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::<RevDeps>(&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::<RevDeps>(&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::<RevDeps>(&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();