Refactor paginating queries to a trait #751
Conversation
sgrif
force-pushed
the
sg-paginate-trait
branch
2 times, most recently
from
a0a168a to
f2ab630
Compare
This pattern gets duplicated quite a bit (and it should be duplicated even more since we still have places where we're doing more than one query to paginate). Additionally, using the `sql` function will cause a query to be removed from the prepared statement cache (since Diesel doesn't know if the string was generated dynamically or not). This moves it out into a trait, and represents a paginated query in a bit more of an abstract form from the SQL point of view. This will slightly change the SQL in the places that it's being used, so that it looks more like what we have in `src/krate_reverse_dependencies.sql`, applying the limit/offset as late as possible. This will not affect the query plan unless they are using `distinct`, or if `limit`/`offset` are called prior to `paginate`. Even if those cases do happen, this query is probably what we meant anyway.
|
|
||
| if sort == "crates" { | ||
| query = query.order(keywords::crates_cnt.desc()); | ||
| } else { | ||
| query = query.order(keywords::keyword.asc()); | ||
| } | ||
|
|
||
| let data = query.load::<(Keyword, i64)>(&*conn)?; | ||
| let data = query.paginate(limit, offset).load::<(Keyword, i64)>(&*conn)?; |
There was a problem hiding this comment.
It's a little unfortunate that selecting the total number of results is now gone from this location, but we have to still remember to include the i64 type in the load type here. Not sure what to do differently though, maybe if we made a paginated_load that only took the types this code knows about and inserts the i64?
Not a huge deal, this PR is still a huge improvement for factoring out duplication and raw SQL everywhere, just a thought for a possible next step and/or if pagination becomes a more general diesel thing.
There was a problem hiding this comment.
I doubt I'd add it to Diesel, but this could definitely be extracted to a crate. I have considered adding a method that lets you specify the type you're going to deserialize to outside of load. (The primary motivation is that Version::belonging_to(&crate).load::<Version> bugs me) If that were present, we could definitely have this be .paginate::<Keyword> and have the paginate method call that with (T, i64). I haven't explored how much work it'd be to add that method though
This pattern gets duplicated quite a bit (and it should be duplicated
even more since we still have places where we're doing more than one
query to paginate). Additionally, using the
sqlfunction will cause aquery to be removed from the prepared statement cache (since Diesel
doesn't know if the string was generated dynamically or not).
This moves it out into a trait, and represents a paginated query in a
bit more of an abstract form from the SQL point of view. This will
slightly change the SQL in the places that it's being used, so that it
looks more like what we have in
src/krate_reverse_dependencies.sql,applying the limit/offset as late as possible. This will not affect the
query plan unless they are using
distinct, or iflimit/offsetarecalled prior to
paginate. Even if those cases do happen, this query isprobably what we meant anyway.