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

Selectable derive breaks embedded optional fields #3173

Closed
3 tasks done
toxeus opened this issue May 6, 2022 · 1 comment
Closed
3 tasks done

Selectable derive breaks embedded optional fields #3173

toxeus opened this issue May 6, 2022 · 1 comment
Labels

Comments

@toxeus
Copy link
Contributor

toxeus commented May 6, 2022

Setup

Versions

  • Rust: 1.60.0
  • Diesel: latest master
  • Database: sqlite
  • Operating System: Linux

Feature Flags

  • diesel: sqlite

Problem Description

For a struct with an embedded optional struct the Selectable derive doesn't work correctly and, therefore, the compilation fails.
There is an unit test that tests exactly this scenario but the underlying diesel schema definition mistakenly is lacking the Nullable wrapper for the optional field.

What are you trying to accomplish?

Compile the code without errors.

What is the expected output?

Successful compilation.

What is the actual output?

Compiling diesel_derives v2.0.0-rc.0 (/home/user/src/diesel/diesel_derives)
error[E0277]: the trait bound expression::select_by::SelectBy<selectable::embedded_option::A, Sqlite>: load_dsl::private::CompatibleType<_, Sqlite> is not satisfied
--> diesel_derives/tests/selectable.rs:95:10
|
95 | .get_result(conn);
| ^^^^^^^^^^ the trait load_dsl::private::CompatibleType<_, Sqlite> is not implemented for expression::select_by::SelectBy<selectable::embedded_option::A, Sqlite>
|
= help: the following implementations were found:
<expression::select_by::SelectBy<U, DB> as load_dsl::private::CompatibleType<U, DB>>
= note: required because of the requirements on the impl of LoadQuery<'_, _, _> for SelectStatement<FromClause<my_structs_nullable::table>, diesel::query_builder::select_clause::SelectClause<expression::select_by::SelectBy<selectable::embedded_option::A, Sqlite>>>
note: required by a bound in get_result
--> /home/user/src/diesel/diesel/src/query_dsl/mod.rs:1652:15
|
1652 | Self: LoadQuery<'query, Conn, U>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in get_result

For more information about this error, try rustc --explain E0277.
error: could not compile diesel_derives due to previous error

Are you seeing any additional errors?

No.

Steps to reproduce

Add the following schema definition

table! {
    my_structs_nullable (foo) {
        foo -> Integer,
        bar -> Nullable<Integer>,
    }
}

and patch this test to use my_structs_nullable instead.

Checklist

  • I have already looked over the issue tracker and the disussion forum for similar possible closed issues.
  • This issue can be reproduced on Rust's stable channel. (Your issue will be
    closed if this is not the case)
  • This issue can be reproduced without requiring a third party crate
@toxeus toxeus added the bug label May 6, 2022
weiznich added a commit to weiznich/diesel that referenced this issue May 20, 2022
This is a tricky one. It seems like the behaviour described in that
issue should work out of the box, but it doesn't. I've spend some time
to investigate various solutions to make this work, but I came to the
conclusion that the current behaviour is the correct one.

The underlying issue here is that such an query promotes the inner
`Nullable<_>` of the field onto the outer `Queryable` wrapper. Without
`Selectable` that would require a select clause like
`.select((table::column.assume_not_null(),).nullable())`. This is
technically a safe pattern, but requires the usage of the "advanced"
`assume_not_null()` method to forcibly convert types to their not null representation.

Possible solutions tried to make the enable constructs shown in that
issue:

* I've tried to make the `impl Selectable for Option<T>` return the
following `SelectExpression`:
`dsl::Nullable<dsl::AssumeNotNull<T::SelectExpression>>`
where `AssumeNotNull` converts each tuple element to the corresponding
not nullable expression, while `Nullable` wraps the tuple itself into a
nullable type wrapper.
* I've tried to apply a similar approach like that one above, but only
for derived impls by manipulating the generated code for a optional
field with `#[diesel(embed)]`

Both solutions require changes to our sql type system, as for example
allowing to load a non nullable value into a `Option<T>` to enable their
usage in a more general scope as the presented example case.
(See the added test cases for this). That by itself would be fine in my
opinion, as this is always a safe operation. Unfortunately the
`AssumeNotNull` transformation would be applied recursively for all
sub-tuples, which in turn would cause trouble with nested joins (again
see the examples). We would be able to workaround this issue by allowing
the `FromSql<ST, DB> for Option<T>` impl for non-nullable types to catch
null values, which in turn really feels like a bad hack. (You would like
to get an error there instead, but nested nullable information are
gone.)
All in all this lead me to the conclusion that the current behaviour is
correct.

This PR adds a few additional tests (an adjusted version of the test
from the bug report + more tests around nested joins) and does move
around some code bits that I noticed while working on this.

I think the official answer for the bug report would be: Either wrap the
inner type also in an `Option` or provide a manual `Selectable` impl
that does the "right" thing there.
@weiznich
Copy link
Member

Thanks for this bug report. I spend some time to investigate this.
I came to the conclusion that this is expected behaviour. Your inner field is not nullable in this case, while the corresponding field in your schema is nullable. This is a basic type mismatch, which always is considered as incompatible type. I would suggest to either wrap the inner type also in an Option or provide a manual Selectable impl
that does the "right" thing there.

See #3181 for more details.

Closed as not a bug.

weiznich added a commit that referenced this issue May 22, 2022
weiznich added a commit to weiznich/diesel that referenced this issue Jun 16, 2022
This is a tricky one. It seems like the behaviour described in that
issue should work out of the box, but it doesn't. I've spend some time
to investigate various solutions to make this work, but I came to the
conclusion that the current behaviour is the correct one.

The underlying issue here is that such an query promotes the inner
`Nullable<_>` of the field onto the outer `Queryable` wrapper. Without
`Selectable` that would require a select clause like
`.select((table::column.assume_not_null(),).nullable())`. This is
technically a safe pattern, but requires the usage of the "advanced"
`assume_not_null()` method to forcibly convert types to their not null representation.

Possible solutions tried to make the enable constructs shown in that
issue:

* I've tried to make the `impl Selectable for Option<T>` return the
following `SelectExpression`:
`dsl::Nullable<dsl::AssumeNotNull<T::SelectExpression>>`
where `AssumeNotNull` converts each tuple element to the corresponding
not nullable expression, while `Nullable` wraps the tuple itself into a
nullable type wrapper.
* I've tried to apply a similar approach like that one above, but only
for derived impls by manipulating the generated code for a optional
field with `#[diesel(embed)]`

Both solutions require changes to our sql type system, as for example
allowing to load a non nullable value into a `Option<T>` to enable their
usage in a more general scope as the presented example case.
(See the added test cases for this). That by itself would be fine in my
opinion, as this is always a safe operation. Unfortunately the
`AssumeNotNull` transformation would be applied recursively for all
sub-tuples, which in turn would cause trouble with nested joins (again
see the examples). We would be able to workaround this issue by allowing
the `FromSql<ST, DB> for Option<T>` impl for non-nullable types to catch
null values, which in turn really feels like a bad hack. (You would like
to get an error there instead, but nested nullable information are
gone.)
All in all this lead me to the conclusion that the current behaviour is
correct.

This PR adds a few additional tests (an adjusted version of the test
from the bug report + more tests around nested joins) and does move
around some code bits that I noticed while working on this.

I think the official answer for the bug report would be: Either wrap the
inner type also in an `Option` or provide a manual `Selectable` impl
that does the "right" thing there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants