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

Not working: parent.join(child.join(grandchild)) [in some cases] #1068

Closed
valeriansaliou opened this Issue Aug 4, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@valeriansaliou

valeriansaliou commented Aug 4, 2017

Setup

Versions

  • Rust: rustc 1.21.0-nightly (b75d1f0ce 2017-08-02)
  • Diesel: 0.15.2
  • Database: PG
  • Operating System macOS 10.12.6

Feature Flags

  • diesel: ["postgres"]
  • diesel_codegen: ["postgres"]

Problem Description

In certain relational schemas, performing a parent.join(child.join(grandchild)) doesn't work as expected (join can be an inner_join or left_join, same error).

The same code, using the simpler parent.join(child.join) call works.

If I change the schema and reverse the belongs_to relation between parent and child, the nested join works just fine. However that's the schema I want to rely on, plus my schema is SQL-valid.

What are you trying to accomplish?

I'm trying to perform a join w/ the following structure: parent.join(child.join(grandchild))

Given the following schema:

table! {
    parent (id) {
        id -> Integer,
        child_id -> Integer,
    }
}

table! {
    child (id) {
        id -> Integer,
        grand_child_id -> Integer,
    }
}

table! {
    grandchild (id) {
        id -> Integer,
    }
}

And the following model:

#[derive(Identifiable, Queryable, Associations)]
#[belongs_to(Child)]
#[table_name="parent"]
pub struct Parent {
    pub id: i32,
    pub child_id: i32,
}

#[derive(Identifiable, Queryable, Associations)]
#[belongs_to(GrandChild)]
#[table_name="child"]
pub struct Child {
    pub id: i32,
    pub grand_child_id: i32,
}

#[derive(Identifiable, Queryable, Associations)]
#[table_name="grandchild"]
pub struct GrandChild {
    pub id: i32,
}

What is the expected output?

The expected output is the equivalent of the full-SQL request:

SELECT "person_employment"."title", "company"."legal_name", "contact"."emails" FROM (
	"person_employment"
		INNER JOIN "company" ON "person_employment"."company_id" = "company"."id"
		INNER JOIN "contact" ON "company"."contact_id" = "contact"."id"
);

Which works just fine in a PG prompt, based on the bare SQL schema defined above.

What is the actual output?

Getting the following error when compiling, although all relevant belongs_to and enable_multi_table_joins! are set.

error[E0277]: the trait bound `diesel::query_source::joins::PleaseGenerateInverseJoinImpls<parent::table>: diesel::JoinTo<child::table>` is not satisfied
   --> src/lib.rs:141:10
    |
141 |         .inner_join(child::table.inner_join(grandchild::table))
    |          ^^^^^^^^^^ the trait `diesel::JoinTo<child::table>` is not implemented for `diesel::query_source::joins::PleaseGenerateInverseJoinImpls<parent::table>`

Are you seeing any additional errors?

No, only this one.

Steps to reproduce

  1. Gist: valeriansaliou/48f441bbcdcef29f5395b87b09dc9893
  2. Compile the code w/o changing it. It works. It's using the only relational schema I could get it to work with, however I don't want it this way (in this schema, a child references both its parent and grandchild)
  3. Uncomment the 2 commented sections regarding schema + models and comment out the other ones.
  4. Build it, you'll get the compilation error I'm talking about. This is using the schema that's more appropriate in that case, but which will never build w/ Diesel.
@sgrif

This comment has been minimized.

Member

sgrif commented Aug 4, 2017

Your names are backwards FYI. You're trying to go grandchild -> child -> parent

@valeriansaliou

This comment has been minimized.

valeriansaliou commented Aug 4, 2017

Yeah it's backwards, because otherwise I get:

error[E0275]: overflow evaluating the requirement `<child::table as diesel::JoinTo<grandchild::table>>::OnClause`
   --> src/lib.rs:141:34
    |
141 |         .inner_join(child::table.inner_join(grandchild::table))
@sgrif

This comment has been minimized.

Member

sgrif commented Aug 4, 2017

The code and error should be identical. Just your naming is wrong. What you're calling Parent should be called GrandChild. Changing the names wouldn't change the error.

sgrif added a commit that referenced this issue Aug 4, 2017

Don't generate join code from `#[belongs_to]`
The root of this problem is that `<User as HasTable>::Table` is not
considered by Rust to be a local type. We do not have the the
information required in `belongs_to` to generate the exact equivalent
code as `joinable!`

I had attempted to work around this with a
`PleaseGenerateInverseJoinImpls`, which worked for getting the `users:
JoinTo<posts>`. However, we *also* rely on that impl for the inverse of
all the composite impls in Diesel. So `posts:
JoinTo<SelectStatement>` wants `SelectStatement:
JoinTo<PleaseGenerateInverseJoinImpls<posts>`, which ultimately
simplifies down to `users:
JoinTo<PleaseGenerateInverseJoinImpls<posts>>`. Even if we could
generate that impl (we can't), that impl would be recursive, and thus
overlapping.

Ultimately we either need the language to change, or we need to generate
this code somewhere that we can directly reference both the parent and
child tables. I think the long term solution here is for us to follow
this PR up with one which infers the `joinable!` impls from foreign key
constraints in the database.

Fixes #1068.

sgrif added a commit that referenced this issue Aug 4, 2017

Don't generate join code from `#[belongs_to]`
The root of this problem is that `<User as HasTable>::Table` is not
considered by Rust to be a local type. We do not have the the
information required in `belongs_to` to generate the exact equivalent
code as `joinable!`

I had attempted to work around this with a
`PleaseGenerateInverseJoinImpls`, which worked for getting the `users:
JoinTo<posts>`. However, we *also* rely on that impl for the inverse of
all the composite impls in Diesel. So `posts:
JoinTo<SelectStatement>` wants `SelectStatement:
JoinTo<PleaseGenerateInverseJoinImpls<posts>`, which ultimately
simplifies down to `users:
JoinTo<PleaseGenerateInverseJoinImpls<posts>>`. Even if we could
generate that impl (we can't), that impl would be recursive, and thus
overlapping.

Ultimately we either need the language to change, or we need to generate
this code somewhere that we can directly reference both the parent and
child tables. I think the long term solution here is for us to follow
this PR up with one which infers the `joinable!` impls from foreign key
constraints in the database.

I don't like how much this generated code is coupled to the specific
impls we provide in Diesel. However, if I tried to accomplish the same
with a marker trait version of `PleaseGenerateInverseJoinImpls`, the
result is that a missing `joinable!` will result in "overflow attempting
to evaluate...", instead of complaining about a trait not being
implemented.

Fixes #1068.

sgrif added a commit that referenced this issue Aug 4, 2017

Don't generate join code from `#[belongs_to]`
The root of this problem is that `<User as HasTable>::Table` is not
considered by Rust to be a local type. We do not have the the
information required in `belongs_to` to generate the exact equivalent
code as `joinable!`

I had attempted to work around this with a
`PleaseGenerateInverseJoinImpls`, which worked for getting the `users:
JoinTo<posts>`. However, we *also* rely on that impl for the inverse of
all the composite impls in Diesel. So `posts:
JoinTo<SelectStatement>` wants `SelectStatement:
JoinTo<PleaseGenerateInverseJoinImpls<posts>`, which ultimately
simplifies down to `users:
JoinTo<PleaseGenerateInverseJoinImpls<posts>>`. Even if we could
generate that impl (we can't), that impl would be recursive, and thus
overlapping.

Ultimately we either need the language to change, or we need to generate
this code somewhere that we can directly reference both the parent and
child tables. I think the long term solution here is for us to follow
this PR up with one which infers the `joinable!` impls from foreign key
constraints in the database.

I don't like how much this generated code is coupled to the specific
impls we provide in Diesel. However, if I tried to accomplish the same
with a marker trait version of `PleaseGenerateInverseJoinImpls`, the
result is that a missing `joinable!` will result in "overflow attempting
to evaluate...", instead of complaining about a trait not being
implemented.

Fixes #1068.

@sgrif sgrif closed this in #1069 Aug 4, 2017

sgrif added a commit that referenced this issue Aug 4, 2017

Don't generate join code from `#[belongs_to]` (#1069)
The root of this problem is that `<User as HasTable>::Table` is not
considered by Rust to be a local type. We do not have the the
information required in `belongs_to` to generate the exact equivalent
code as `joinable!`

I had attempted to work around this with a
`PleaseGenerateInverseJoinImpls`, which worked for getting the `users:
JoinTo<posts>`. However, we *also* rely on that impl for the inverse of
all the composite impls in Diesel. So `posts:
JoinTo<SelectStatement>` wants `SelectStatement:
JoinTo<PleaseGenerateInverseJoinImpls<posts>`, which ultimately
simplifies down to `users:
JoinTo<PleaseGenerateInverseJoinImpls<posts>>`. Even if we could
generate that impl (we can't), that impl would be recursive, and thus
overlapping.

Ultimately we either need the language to change, or we need to generate
this code somewhere that we can directly reference both the parent and
child tables. I think the long term solution here is for us to follow
this PR up with one which infers the `joinable!` impls from foreign key
constraints in the database.

I don't like how much this generated code is coupled to the specific
impls we provide in Diesel. However, if I tried to accomplish the same
with a marker trait version of `PleaseGenerateInverseJoinImpls`, the
result is that a missing `joinable!` will result in "overflow attempting
to evaluate...", instead of complaining about a trait not being
implemented.

Fixes #1068.
@valeriansaliou

This comment has been minimized.

valeriansaliou commented Aug 4, 2017

Thanks a ton @sgrif 👍

@sgrif

This comment has been minimized.

Member

sgrif commented Aug 4, 2017

Don't thank me yet. I have a lot more work to do to get rid of the fallout from that change.

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