Port `crates#index` to use Diesel #609

sgrif · 2017-03-09T17:44:10Z

I decided to tackle this one next since we have an N+1 queries bug on
the max versions, it's a relatively high traffic endpoint, and the code
benefitted the most from Diesel.

There was probably more code added to support migrating the tests over
than to support the actual index function, but all those pieces will
start to get used as more endpoints are ported over. There were a few
tests that were hitting the followings and owners endpoints which I have
temporarily changed to hit the database directly instead.

Porting those endpoints would mean porting over more tests, which might
mean porting over more endpoints, etc. We didn't lose any coverage
overall, but we should still go back to hitting the endpoints directly
in tests when we can.

sgrif · 2017-03-09T17:48:34Z

src/krate.rs

+}
+
+impl<'a> NewCrate<'a> {
+    pub fn create_or_update(


This is the same functionality as find_or_insert, but with a more clear name, and using Diesel. It's only used in tests currently.

sgrif · 2017-03-09T17:50:22Z

src/krate.rs

    let (offset, limit) = req.pagination(10, 100)?;
-    let query = req.query();


I recommend using split diff here.

sgrif · 2017-03-09T18:41:28Z

One thing that I didn't do here, but would be pretty easy to add if we want it is the ability to compose the branches in index. e.g. being able to write ?category=foo&keyword=bar&q=baz and have it do a full text search for baz on crates that have the category foo and the keyword bar.

And by pretty easy I mean that it's literally just removing the else from all the else ifs

I decided to tackle this one next since we have an N+1 queries bug on the max versions, it's a relatively high traffic endpoint, and the code benefitted the most from Diesel. There was probably more code added to support migrating the tests over than to support the actual `index` function, but all those pieces will start to get used as more endpoints are ported over. There were a few tests that were hitting the followings and owners endpoints which I have temporarily changed to hit the database directly instead. Porting those endpoints would mean porting over more tests, which might mean porting over more endpoints, etc. We didn't lose any coverage overall, but we should still go back to hitting the endpoints directly in tests when we can.

All the methods which do >1 query which modifies data should be wrapped in a transaction

We should do this all over the application, as there's tons of endpoints which are doing a separate query for the count when a window function would achieve the same thing more efficiently.

carols10cents

I see the tests that are hitting the database instead of the endpoints, but it looks to me like they're not really testing the same things? I could be convinced that this is ok if it's going to be temporary, just wanted to check that it's what you were intending.

carols10cents · 2017-03-11T19:37:26Z

migrations/20170307211844_versions_yanked_is_not_nullalbe/up.sql

@@ -0,0 +1 @@
+ALTER TABLE versions ALTER COLUMN yanked SET NOT NULL;


Just checked to be sure: 0 records in production versions have yanked IS NULL

carols10cents · 2017-03-11T20:16:00Z

src/krate.rs

-        let max_version = krate.max_version(conn)?;
-        crates.push(krate.minimal_encodable(max_version, Some(badges)));
+    let mut query = crates_query(&params, req.user())?;
+    let sort = params.get("sort").map(|s| &**s).unwrap_or("alpha");


Love how the sorting is all in one place now 😻

carols10cents · 2017-03-11T20:30:24Z

src/tests/krate.rs

-                                               .with_query(&query)));
-    assert_eq!(::json::<CrateList>(&mut response).crates.len(), 1);
+    // FIXME: Once owner endpoints use diesel, go back to hitting the real endpoint
+    assert_eq!(1, req.tx().unwrap().query("SELECT COUNT(*) FROM crate_owners


So this FIXME test is still doing an assertion...

carols10cents · 2017-03-11T20:32:15Z

src/tests/krate.rs

-    req.with_path("/api/v1/crates/foo_following/following")
-       .with_method(Method::Get);
-    let mut response = ok_resp!(middle.call(&mut req));
-    assert!(::json::<F>(&mut response).following);


... but this assertion looks commented out....

So basically this endpoint is tested elsewhere, and I'm assuming that ?following=1 is enough to generally verify this behavior, especially since I'm planning on hitting the following endpoints next after this is merged.

carols10cents · 2017-03-11T20:33:23Z

src/tests/krate.rs

+    // FIXME: Go back to hitting the actual endpoint once it's using Diesel
+    req.db_conn().unwrap()
+        .execute("TRUNCATE TABLE follows")
+        .unwrap();


This test that's hitting the database instead of the endpoint looks like it's doing a lot more than the endpoint does...

Yes, I'm assuming that the table was empty when the test started running.

carols10cents · 2017-03-11T20:33:38Z

src/tests/krate.rs

-    req.with_path("/api/v1/crates/foo_following/following")
-       .with_method(Method::Get);
-    let mut response = ok_resp!(middle.call(&mut req));
-    assert!(!::json::<F>(&mut response).following);


...and this assertion looks commented out...

carols10cents · 2017-03-11T20:42:22Z

src/krate.rs

+
+        let perfect_match = crates::name.eq(q_string).desc();
+        if sort == "downloads" {
+            query = query.order((perfect_match, crates::downloads.desc()));


Oh I guess sorting isn't in one place anymore, but I guess that's ok. This is better than it was!

I'm guessing later calls to order override earlier ones, like in activerecord?

Yes, calls to order override the earlier ones. It's actually unlike Active Record (.reorder replaces earlier ones, .order appends). The only method in Diesel which doesn't override earlier calls is .filter.

carols10cents · 2017-03-11T20:49:54Z

One thing that I didn't do here, but would be pretty easy to add if we want it is the ability to compose the branches in index. e.g. being able to write ?category=foo&keyword=bar&q=baz and have it do a full text search for baz on crates that have the category foo and the keyword bar.

And by pretty easy I mean that it's literally just removing the else from all the else ifs

Yeah I do want to do that eventually, but I think we also need to put some thought into which ordering takes precedence when, and how we convey what's going on in the UI as well. So it's not quite literally just that, but I'm glad to know the backend is closer at least :)

carols10cents · 2017-03-11T20:59:08Z

Ok, fair enough :)

sgrif · 2017-03-11T21:04:21Z

Hopefully this will be the largest one. ❤️

sgrif commented Mar 9, 2017

View reviewed changes

sgrif force-pushed the sg-port-crates-index2 branch 2 times, most recently from f635d52 to d9bfcdd Compare March 11, 2017 01:24

sgrif added 5 commits March 11, 2017 13:03

Field shorthands aren't stable in released Rust yet

0d7e68b

Rename a few poorly named methods, add some transactions

b35f76a

All the methods which do >1 query which modifies data should be wrapped in a transaction

Refactor crates#index to use a single query for crates and count

129713e

We should do this all over the application, as there's tons of endpoints which are doing a separate query for the count when a window function would achieve the same thing more efficiently.

Fix merge conflicts

8d5b143

sgrif force-pushed the sg-port-crates-index2 branch from d9bfcdd to 8d5b143 Compare March 11, 2017 18:06

carols10cents reviewed Mar 11, 2017

View reviewed changes

carols10cents merged commit 113ad2d into rust-lang:master Mar 11, 2017

sgrif deleted the sg-port-crates-index2 branch March 11, 2017 21:18

carols10cents mentioned this pull request Mar 14, 2017

Fix category query to look up on slug #627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port `crates#index` to use Diesel #609

Port `crates#index` to use Diesel #609

sgrif commented Mar 9, 2017

sgrif Mar 9, 2017

sgrif Mar 9, 2017

sgrif commented Mar 9, 2017 •

edited

Loading

carols10cents left a comment

carols10cents Mar 11, 2017

carols10cents Mar 11, 2017

carols10cents Mar 11, 2017

carols10cents Mar 11, 2017

sgrif Mar 11, 2017

carols10cents Mar 11, 2017

sgrif Mar 11, 2017

carols10cents Mar 11, 2017

carols10cents Mar 11, 2017

sgrif Mar 11, 2017

carols10cents commented Mar 11, 2017

carols10cents commented Mar 11, 2017

sgrif commented Mar 11, 2017

		let (offset, limit) = req.pagination(10, 100)?;
		let query = req.query();

		@@ -0,0 +1 @@
		ALTER TABLE versions ALTER COLUMN yanked SET NOT NULL;

Port crates#index to use Diesel #609

Port crates#index to use Diesel #609

Conversation

sgrif commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgrif commented Mar 9, 2017 • edited Loading

carols10cents left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carols10cents commented Mar 11, 2017

carols10cents commented Mar 11, 2017

sgrif commented Mar 11, 2017

Port `crates#index` to use Diesel #609

Port `crates#index` to use Diesel #609

sgrif commented Mar 9, 2017 •

edited

Loading