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

Mention calling of SQL functions in Selectable docs #3751

Merged
merged 9 commits into from
Apr 5, 2024

Conversation

ISibboI
Copy link
Contributor

@ISibboI ISibboI commented Aug 16, 2023

In the custom select expression example for the Selectable trait, mention that this can be used to call arbitrary SQL functions as well. Mention the CURRENT_TIMESTAMP function explicitly.

Relates to #3739

In the custom select expression example for the `Selectable` trait, mention that this can be used to call arbitrary SQL functions as well. Mention the `CURRENT_TIMESTAMP` function explicitly.

Relates to diesel-rs#3739
@weiznich weiznich requested a review from a team August 16, 2023 14:45
@ISibboI
Copy link
Contributor Author

ISibboI commented Aug 16, 2023

Seems like some test fails, which is strange, because I didn't change any code. Must be some random failure, because it talks about a deadlock...

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR. I have some minor wording request otherwise this looks good 👍

diesel/src/expression/mod.rs Outdated Show resolved Hide resolved
///
/// * avoid nesting types, or to
/// * populate fields with values other than table columns, such as
/// the result of an SQL function like `CURRENT_TIMESTAMP()`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until we generate the appropriate type from sql_function I don't think that currently works, does it? (#3745)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! I have now added a field to the doctest below this comment that gets initialised with diesel::dsl::now. It seems to complain about some trait not being implemented:

error[E0277]: the trait bound `expression::select_by::SelectBy<UserPost, _>: load_dsl::private::CompatibleType<_, _>` is not satisfied
    --> src/expression/mod.rs:596:12
     |
37   |     .first(connection)?;
     |      ----- ^^^^^^^^^^ the trait `load_dsl::private::CompatibleType<_, _>` is not implemented for `expression::select_by::SelectBy<UserPost, _>`
     |      |
     |      required by a bound introduced by this call
     |
     = help: the trait `load_dsl::private::CompatibleType<U, DB>` is implemented for `expression::select_by::SelectBy<U, DB>`
     = note: required for `SelectStatement<FromClause<JoinOn<Join<table, table, Inner>, Grouped<Eq<Nullable<user_id>, Nullable<id>>>>>, ..., ..., ..., ..., ...>` to implement `LoadQuery<'_, _, _>`
     = note: the full type name has been written to '/tmp/rustdoctestBQRQaQ/rust_out.long-type-2905737123615719141.txt'
note: required by a bound in `first`
    --> /home/sibbo/git/diesel/diesel/src/query_dsl/mod.rs:1780:22
     |
1780 |         Limit<Self>: LoadQuery<'query, Conn, U>,
     |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `RunQueryDsl::first`

Is this what you mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually for the particular case of dsl::now given how it's defined it would work, however if a user were to define a custom function that does CURRENT_TIMESTAMP() using sql_function! that wouldn't work.

I suspect your error comes from the type you are selecting to, and you may get a more explicit error message using check_for_backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, thanks. Then I should update the documentation.

diesel/src/expression/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Thomas B <9094255+Ten0@users.noreply.github.com>
@weiznich weiznich enabled auto-merge April 5, 2024 11:56
@weiznich weiznich added this pull request to the merge queue Apr 5, 2024
Merged via the queue into diesel-rs:master with commit cabd7df Apr 5, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants