Diesel: left_join makes it possible to try to stuff nullable values into a non-nullable Queryable

[icefoxen]

Setup

Pardon the vagueness, my database-fu is too weak to know how to explain this more concisely. This is also slightly simplified so that I can try to explain it at all.

We have two tables, TableA and TableB. We have two Queryable structs, QueryA and QueryB. TableA and TableB have no nullable fields, and QueryA and QueryB contain no Options, and life is good.

Now we add this:

#[derive(Queryable, ...)]
struct QueryC {
    a: QueryA,
    b: QueryB,
}

We can write a diesel query that does an inner join on A and B and stuffs the result into QueryC, and it works: table_a::dsl::table_a.inner_join(table_b::dsl::table_b).select(...).get_results<QueryC>().

However, when we do a left_join() instead, it compiles but panics at runtime with 'Could not get crate list?: DeserializationError(UnexpectedNullError)'. The left_join() produces, basically, (QueryA, Option<QueryB>) but that's not what QueryC expects, and trying to make it contain that fails to compile.

...except now that I test more, it does not-compile, as is correct, but when you do what I'm actually trying to do (which is wrap the whole bloody thing in yet another inner join) it fails. Heck, I'm just going to give you the raw code:

table! {
    crates (id) {
        id -> Int8,
        name -> Varchar,
    }
}

table! {
    crate_versions (id) {
        id -> Int8,
        crate_id -> Int8,
        version -> Varchar,
        checksum -> Varchar,
        yanked -> Bool,
    }
}

table! {
    analyze_cratefile (id) {
        id -> Int8,
        crate_versions_id -> Int8,
        size -> Int8,
        valid -> Bool,
        checksum_matches -> Bool,
    }
}

    // This is the bit that compiles and shouldn't, and panics at runtime
    let crate_versions = c::crates
        .inner_join(cv::crate_versions.left_join(ac::analyze_cratefile))
        .select((
            (cv::id, c::name, cv::version, cv::checksum, cv::yanked),
            (ac::size, ac::valid, ac::checksum_matches),
        ))
        .order_by(c::name)
        .get_results::<CrateWithAnalysis>(conn)
        .expect("Could not get crate list?");

I hope you can make heads or tails of this, 'cause I can't. Please let me know if you need any more information and I'll try to provide, once I've had more than six hours of sleep.

Versions

• Rust: 1.32
• Diesel: 1.4
• Database: PostgreSQL 11
• Operating System Linux

Feature Flags

• diesel: postgres, extras

Checklist

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

[weiznich]

Seems like you've found the same bug that we've investigated a few hours before that report 🙈
So let me summarize shortly what we've already know:

• The problem here is that a left join always returns a nullable field, independently from the schema definition
• Diesel enforces that for single left joins. There it's not possible to select such a non nullable field. (To select that field you need to use column.nullable() instead)
• There seems to be a bug as soon as more than one join is involved, then the not nullable field is also accepted.

So basically that code above should result in a compiler error but it doesen't. To workaround the runtime error: just call .nullable() on each column coming from the table of the left join. (In your case ac)

[icefoxen]

Well thank you for the confirmation that I'm not crazy at least! :D

Is there a way to, instead of having each individual column being nullable, have the entire sub-struct be nullable? So instead of having:

struct Analysis {
    size: Option<...>,
    valid: Option<...>,
    checksum_matches: Option<...>,
}
struct CrateWithAnalysis {
    a: QueryA,
    b: Analysis,
}

I could do:

struct Analysis {
    size: ...,
    valid: ...,
    checksum_matches: ...,
}
struct CrateWithAnalysis {
    a: QueryA,
    b: Option<Analysis>,
}

[icefoxen]

Oh nice, got it. Can do (ac::size, ac::valid, ac::checksum_matches).nullable() and it appears to work.

Thank you! You may close this as necessary.

[sgrif]

So this problem is beyond left_join. It also happens for inner_join. We can fix this relatively easily for nested left_joins, but for mixed joins I don't see a way we can fix this without overlapping marker traits or disjointness on associated types, neither of which look likely to be stable soon.

https://gist.github.com/sgrif/32710a5f01e1ba36e1ae3fcff2145765 is a compile test demonstrating all the problem cases. We can fix the foo.left_join(bar).left_join(baz) allowing non-nullable columns from bar by changing https://github.com/diesel-rs/diesel/blob/a0c1e297269d3c8c136d3f9d3691d220939f9a95/diesel/src/macros/mod.rs#L45 to be Self: SelectableExpression<Left>.

However, we have the same problem with foo.left_join(bar).inner_join(baz), and I don't see a clear path to solving that case, even with a license to make breaking changes.

[sgrif]

We actually might be able to fix this by replacing all of the SelectableExpression impls related to join to be concrete impls for every combination of tables in allow_tables_to_appear_in_same_query!, but I'm worried about the impact that will have on compile times. @aturon @nikomatsakis this is one of those cases I've talked to y'all about in the past, if either of you have some time I'd love to walk you through this issue for us in more detail

[Ten0]

I don't see a way we can fix this without overlapping marker traits or disjointness on associated types, neither of which look likely to be stable soon.

Note that there's this workaround that allows for disjointness based on associated types.
It's pretty verbose but eventually gets the job done (provided it can actually be applied in this case which I haven't been digging enough into to be sure of).

[sgrif]

It looks like something along those lines may actually work. Ultimately what we need is for impl SelectableExpression<Join<Left, Right, Inner>> for column to have where column: SelectableExpression<Left> or where column: SelectableExpression<Right>. Since that can only be true for one of those two types today, we can have another trait which selects either Left or Right based on which one the table appears in. E.g. something like this:

impl Select<Left, Right> for (Once, Never) {
    type Selection = Left;
}

impl Select<Left, Right> for (Never, Once) {
    type Selection = Right;
}

impl<Left, Right> SelectableExpression<Join<Left, Right, Inner>> for Column
where
    Left: AppearsInFromClause<table>,
    Right: AppearsInFromClause<table>,
    (Left::Count, Right::Count): Select<Left, Right>,
    Self: SelectableExpression<
         <(Left::Count, Right::Count) as Select<Left, Right>>::Selection,
    >,
{
}

That with the change in my earlier comment fixes the bug, but surfaces a new problem. Our impl SelectableExpression<Join<Left, Right, Kind>> for Nullable<T> actually relies on this bug, so I'll have to figure out a new way to make that work. But I think it's possible.

[sgrif]

PR coming shortly

[sgrif]

2234 correctly causes the problem code to fail to compile