Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AllColumns pattern isn't compatible with BoxedQuery pattern #1979

Open
2 tasks done
carols10cents opened this issue Feb 6, 2019 · 3 comments
Open
2 tasks done

AllColumns pattern isn't compatible with BoxedQuery pattern #1979

carols10cents opened this issue Feb 6, 2019 · 3 comments

Comments

@carols10cents
Copy link

Possibly related: #1435

Setup

Versions

  • Rust: rustc 1.32.0 (9fda7c223 2019-01-16), edition = "2018"
  • Diesel: 1.4.1
  • Database: postgres 10.5
  • Operating System: macOS Mojave

Feature Flags

  • diesel: ["postgres"]

Problem Description

What are you trying to accomplish?

In crates.io's code, I'm trying to add a column to our versions table that exists in the database but is never selected by the Queryable model, using the AllColumns pattern described in the composing applications diesel guide that we've successfully used for the crates table.

The problem is there's a set of functions that return versions::BoxedQuery, using the BoxedQuery pattern described in the same guide, and those functions stop compiling when I add the AllColumn types.

What is the expected output?

Successful compilation

What is the actual output?

error[E0277]: the trait bound `(i32, i32, std::string::String): diesel::Queryable<(diesel::sql_types::Integer, diesel::sql_types::Integer, diesel::sql_types::Text, diesel::sql_types::Nullable<diesel::sql_types::Text>), _>` is not satisfied
   --> src/main.rs:123:47
    |
123 |     let versions = first_crate.all_versions().load::<Version>(&conn).unwrap();
    |                                               ^^^^ the trait `diesel::Queryable<(diesel::sql_types::Integer, diesel::sql_types::Integer, diesel::sql_types::Text, diesel::sql_types::Nullable<diesel::sql_types::Text>), _>` is not implemented for `(i32, i32, std::string::String)`
    |
    = help: the following implementations were found:
              <(A, B, C) as diesel::Queryable<(SA, SB, SC), __DB>>
              <(A, B, C) as diesel::Queryable<diesel::sql_types::Record<(SA, SB, SC)>, diesel::pg::Pg>>
    = note: required because of the requirements on the impl of `diesel::Queryable<(diesel::sql_types::Integer, diesel::sql_types::Integer, diesel::sql_types::Text, diesel::sql_types::Nullable<diesel::sql_types::Text>), _>` for `version::Version`
    = note: required because of the requirements on the impl of `diesel::query_dsl::LoadQuery<_, version::Version>` for `diesel::query_builder::BoxedSelectStatement<'_, (diesel::sql_types::Integer, diesel::sql_types::Integer, diesel::sql_types::Text, diesel::sql_types::Nullable<diesel::sql_types::Text>), schema::versions::table, diesel::pg::Pg>`

Are you seeing any additional errors?

No

Steps to reproduce

I've extracted a minimal example into this repo: https://github.com/carols10cents/diesel-mvce

At this commit, with the call to the all_versions function commented out, the code compiles and works as I expect.

At this commit, which uncomments out the call to the all_versions function, I get the compilation error shown above.

Checklist

  • I have already looked over the issue tracker for similar issues.
  • This issue can be reproduced on Rust's stable channel. (Your issue will be
    closed if this is not the case)
@weiznich
Copy link
Member

weiznich commented Feb 6, 2019

As a workaround you need to define your own BoxedQuery type alias with the right sql type.

This change "fixes" the minimal example:

diff --git a/src/main.rs b/src/main.rs
index ff7d64d..19129e6 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -14,6 +14,8 @@ mod krate {
     use crate::version::Version;
     use diesel::pg::Pg;
     use diesel::prelude::*;
+    use diesel::dsl::SqlTypeOf;
+    use diesel::query_builder::BoxedSelectStatement;
 
     #[derive(Debug, Clone, Queryable, Identifiable, Associations, AsChangeset, QueryableByName)]
     #[table_name = "crates"]
@@ -34,25 +36,32 @@ mod krate {
         }
     }
 
+    type BoxedVersionQuery<'a, Backend> =
+        BoxedSelectStatement<'a, SqlTypeOf<super::version::AllColumns>, versions::table, Backend>;
+
     pub trait CrateVersions {
-        fn all_versions(&self) -> versions::BoxedQuery<'_, Pg>;
+        fn all_versions(&self) -> BoxedVersionQuery<'_, Pg>;
     }
 
     impl CrateVersions for Crate {
-        fn all_versions(&self) -> versions::BoxedQuery<'_, Pg> {
-            Version::belonging_to(self).into_boxed()
+        fn all_versions(&self) -> BoxedVersionQuery<'_, Pg> {
+            Version::belonging_to(self)
+                .select(super::version::ALL_COLUMNS)
+                .into_boxed()
         }
     }
 
     impl CrateVersions for Vec<Crate> {
-        fn all_versions(&self) -> versions::BoxedQuery<'_, Pg> {
+        fn all_versions(&self) -> BoxedVersionQuery<'_, Pg> {
             self.as_slice().all_versions()
         }
     }
 
     impl CrateVersions for [Crate] {
-        fn all_versions(&self) -> versions::BoxedQuery<'_, Pg> {
-            Version::belonging_to(self).into_boxed()
+        fn all_versions(&self) -> BoxedVersionQuery<'_, Pg> {
+            Version::belonging_to(self)
+                .select(super::version::ALL_COLUMNS)
+                .into_boxed()
         }
     }
 }
@@ -70,7 +79,7 @@ mod version {
         pub num: String,
     }
 
-    type AllColumns = (versions::id, versions::crate_id, versions::num);
+    pub type AllColumns = (versions::id, versions::crate_id, versions::num);
 
     pub const ALL_COLUMNS: AllColumns = (versions::id, versions::crate_id, versions::num);

@carols10cents
Copy link
Author

Thank you for the workaround!

carols10cents added a commit to integer32llc/crates.io that referenced this issue Feb 9, 2019
A workaround is needed to get the all columns pattern to work with the
boxed query pattern, see diesel-rs/diesel#1979

I don't like how many places needed to be changed, going to see if I can
refactor a bit.
@weiznich
Copy link
Member

Another update here: The BoxedQuery type generated by the table! macro has a ST parameter that is set by default to the corresponding SQL type of all columns. We probably want to mention that one can easily change the corresponding SQL type to the correct one using SqlTypeOf<super::version::AllColumns> in the corresponding guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants