Merge #919

919: Fix keywords that weren't lowercased before inserting r=carols10cents

Background for anyone unfamiliar with crates.io's database schema:

- We've got crates
- We've got keywords
- Crates can have many keywords and keywords can have many crates
- There's a join table `crates_keywords` that contains records associating crate.ids and keyword.ids

This migration fixes invalid data that currently exists in the database. Between
commits d5c5635 and 4c2bd42, it was possible for keywords to be
created without lowercasing them first. Then, due to this code:

```
pub fn find_or_create_all(conn: &PgConnection, names: &[&str]) ->
QueryResult<Vec<Keyword>> {
    // ...elided...

    keywords::table
        .filter(::lower(keywords::keyword).eq(any(&lowercase_names)))
        .load(conn)
}
```

which selected any keywords that matched the given keywords *regardless*
of case and associated these keywords with crates, many crates now have
BOTH the uppercase and lowercase variants of a keyword associated with
them.

That is, the keywords table contains:

| id | keyword | |
|----|---------|-|
| 1  | foo     | |
| 2  | Foo     | this record should not exist |

And a crate with id 5 that was published with any of:

`keywords = ["foo"]`
`keywords = ["Foo"]`
`keywords = ["FOO"]` etc

in their Cargo.toml metadata would get the following records created in
the crates_keywords table:

| crate_id | keyword_id | |
|----------|------------|-|
| 5        | 1          | this record is correct, for all cases |
| 5        | 2          | this record is incorrect, for all cases |

So this migration removes all the crates_keywords rows associated with
keywords that aren't lowercased and then removes the keywords
themselves.

BUT!

There are some crates that only have these associations currently with
the uppercased keyword! That is, crate id 5 has this in crates_keywords:

| crate_id | keyword_id |
|----------|------------|
| 5        | 2          |

So BEFORE making the fix above, this migration inserts the corresponding
lowercased keyword association for these crates (so 5, 1 in this case)
so that these crates can be treated like all the rest that are
associated with both.

ALSO!

There are some keywords that have only ever been used in their uppercase
variant, so the corresponding lowercase keyword doesn't exist at all.
Such as:

| id | keyword |
|----|---------|
| 3  | BAR     |

In this case, the migration updates the keyword value, leaving the
associated crates_keywords alone.

WHEW!

Author: bors-voyager[bot]

migrations/20170728002039_fix_keywords/down.sql

@@ -0,0 +1 @@
+-- Not reversible

migrations/20170728002039_fix_keywords/up.sql

@@ -0,0 +1,68 @@
+-- For all keywords that have a corresponding lowercased keyword,
+-- (these keywords should not have been created but there was a bug
+-- that created them; that has been fixed and no more are being created)
+WITH messed_up_keywords AS (
+    select keywords.id as upper_id, k.id as lower_id
+    from keywords
+    inner join keywords as k on LOWER(keywords.keyword) = k.keyword
+    where LOWER(keywords.keyword) != keywords.keyword
+)
+-- Find all the crates that use the uppercased keyword BUT NOT the lowercased keyword
+-- (many crates are associated with both lower and upper cased because of a bug)
+, messed_up_crates AS (
+    select crate_id, upper_id, lower_id
+    from crates_keywords
+    inner join messed_up_keywords on crates_keywords.keyword_id = messed_up_keywords.upper_id
+    where messed_up_keywords.lower_id not in (
+        select keyword_id
+        from crates_keywords as ck
+        where ck.crate_id = crates_keywords.crate_id
+    )
+)
+-- Associate these crates with the lowercased keyword AS WELL AS the uppercased keyword
+INSERT INTO crates_keywords (crate_id, keyword_id)
+SELECT crate_id, lower_id as keyword_id
+FROM messed_up_crates
+;
+
+-- For all keywords that have a corresponding lowercased keyword,
+-- (this is repeated exactly from above)
+WITH messed_up_keywords AS (
+    select keywords.id as upper_id, k.id as lower_id
+    from keywords
+    inner join keywords as k on LOWER(keywords.keyword) = k.keyword
+    where LOWER(keywords.keyword) != keywords.keyword
+)
+-- Delete records associating crates to the uppercased keyword
+DELETE
+FROM crates_keywords
+WHERE crates_keywords.keyword_id IN (
+    SELECT upper_id FROM messed_up_keywords
+)
+;
+
+-- For all keywords that have a corresponding lowercased keyword,
+-- (this is repeated exactly from above)
+WITH messed_up_keywords AS (
+    select keywords.id as upper_id, k.id as lower_id
+    from keywords
+    inner join keywords as k on LOWER(keywords.keyword) = k.keyword
+    where LOWER(keywords.keyword) != keywords.keyword
+)
+-- Delete the uppercased keyword
+-- No more crates should be associated with these keywords because of
+-- the previous delete.
+DELETE
+FROM keywords
+WHERE keywords.id IN (
+    SELECT upper_id FROM messed_up_keywords
+)
+;
+
+-- For all keywords that are not properly lowercased but do not
+-- have a corresponding lowercased keyword, update them to be
+-- lower cased, preserving any crate associations using them.
+UPDATE keywords
+SET keyword = lower(keyword)
+WHERE keyword != lower(keyword)
+;

src/keyword.rs

@@ -71,7 +71,7 @@ impl Keyword {
                 .execute(conn)?;
         }
         keywords::table
-            .filter(::lower(keywords::keyword).eq(any(&lowercase_names)))
+            .filter(keywords::keyword.eq(any(&lowercase_names)))
             .load(conn)
     }
 
@@ -189,3 +189,45 @@ pub fn show(req: &mut Request) -> CargoResult<Response> {
     }
     Ok(req.json(&R { keyword: kw.encodable() }))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use dotenv::dotenv;
+    use std::env;
+    use diesel;
+    use diesel::connection::SimpleConnection;
+
+    #[derive(Insertable)]
+    #[table_name = "keywords"]
+    struct NewKeyword<'a> {
+        keyword: &'a str,
+    }
+
+    fn pg_connection() -> PgConnection {
+        let _ = dotenv();
+        let database_url =
+            env::var("TEST_DATABASE_URL").expect("TEST_DATABASE_URL must be set to run tests");
+        let conn = PgConnection::establish(&database_url).unwrap();
+        // These tests deadlock if run concurrently
+        conn.batch_execute("BEGIN;").unwrap();
+        conn
+    }
+
+    #[test]
+    fn dont_associate_with_non_lowercased_keywords() {
+        let conn = pg_connection();
+        // The code should be preventing lowercased keywords from existing,
+        // but if one happens to sneak in there, don't associate crates with it.
+        let bad_keyword = NewKeyword { keyword: "NO" };
+
+        diesel::insert(&bad_keyword)
+            .into(keywords::table)
+            .execute(&conn)
+            .unwrap();
+
+        let associated = Keyword::find_or_create_all(&conn, &["no"]).unwrap();
+        assert_eq!(associated.len(), 1);
+        assert_eq!(associated.first().unwrap().keyword, "no");
+    }
+}