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

Add a Selectable trait #2709

Closed
wants to merge 5 commits into from
Closed

Conversation

weiznich
Copy link
Member

@weiznich weiznich commented Apr 8, 2021

This commit adds a Selectable trait, a corresponding derive and some
methods that allow to construct the select / returning clause based on a
type implementing this trait before sending the corresponding query to
the database.

This trait is designed to be used in combination with Queryable which
represents a result of a query. Ultimately my hope here is that this
combination drastically reduces the number of type mismatches that
occur because of missmatches between query and struct implementing Queryable.

I feel that this is a much clearer implementation than #2367.

For now this is WIP, but I open this PR to gather some feedback on the design:

Points for discussion:

  • Do we even want something like this?
  • Are there better ways to implement this (especially in terms of integration into RunQueryDsl)?

Things I want to fix before merging:

  • Changelog
  • Compile tests
  • Updating some/all of the examples
  • Maybe add some notes to load/get_result that users should prefer to use this combination?

@weiznich weiznich requested a review from a team April 8, 2021 17:42
This commit adds a `Selectable` trait, a corresponding derive and some
methods that allow to construct the select / returning clause based on a
type implementing this trait before sending the corresponding query to
the database.

This trait is designed to be used in combination with `Queryable` which
represents a result of a query. Ultimativly my hope here is that this
combination drastically reduces the number of type missmatches that
occure because of missmatches between query and struct implementing `Queryable`.
Copy link
Contributor

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

I knew it was possible!

I leaved some comments to the docs which was unclear for me when I first looked at this. I think it should help to make them obvious as much as possible

/// type SelectExpression = (users::id, users::name);
///
/// fn selection() -> Self::SelectExpression {
/// (users::id, users::name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will even better to add comment there, something like:

// `id` and `name` there are names of you struct fields,
// they are listed in the same order as in your struct.
// You can override them by using `#[column_name = "..."]` attribute
// `users` are name from the `#[table_name = "..."]` attribute

Comment on lines +1309 to +1312
/// This method is similar to [load](self::RunQueryDsl::load), but sets
/// a corresponding select/returning clause based on the
/// [Selectable](crate::query_builder::Selectable) impl for `U`.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should also explain what the difference between Queryable and Selectable and when you may want to use each of them.

Also you can mention what bind method will be used: by name or by index

This allows to provide different implementations for different backends.
This matches the behaviour of the `Queryable` trait more closely.
This simply marks the selection as nullable.
@weiznich
Copy link
Member Author

weiznich commented Apr 9, 2021

Currently this is only a PR testing out some designs, that does not mean that this will end up being merged. After looking at the error messages generated by load_into and load_into_single when misused I have the opinion we should not merge this at is it yet. Just stating that LoadIntoDsl is not implemented is not very helpful for anyone. For me that's a no go.

There are a few possibilities here to move this forward:

  • Someone has a better way to write the trait bounds on LoadIntoDsl/RunQueryDsl in such a way that the compiler emits a more helpful error message a bit more in line with that what is currently emitted for a manual use of .select()
  • Someone teaches rustc to emit clearer error messages here

So if you @Mingun are interested in this feature, feel free to start working on of the proposed solutions.

Copy link
Member

@Ten0 Ten0 left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me. In particular I love the #[embed] behavior and consistency.

However I'm not super fond of the name load_into: I'm not sure how "into" conveys the idea that this is going to add a select clause.
.load<T>() already loads "into" T.
Maybe something like .select_load()/select_get_result()/select_first()? I feel like that would make for a more consistent naming, and make it more obvious that this does select (or the like) in addition to the other thing.

After looking at the error messages generated by load_into and load_into_single when misused I have the opinion we should not merge this at is it yet.

These seem pretty reasonable to me provided that LoadIntoDsl has some documentation that explains what to do when you encounter a such error, but I'm very familiar with Diesel at this point so I could understand not everyone feels this way.
Maybe rustc_on_unimplemented could help there though?

@weiznich
Copy link
Member Author

However I'm not super fond of the name load_into: I'm not sure how "into" conveys the idea that this is going to add a select clause.
.load() already loads "into" T.
Maybe something like .select_load()/select_get_result()/select_first()? I feel like that would make for a more consistent naming, and make it more obvious that this does select (or the like) in addition to the other thing.

Names for the methods are definitively up to discussion. I've just used something to show how this work.

These seem pretty reasonable to me provided that LoadIntoDsl has some documentation that explains what to do when you encounter a such error, but I'm very familiar with Diesel at this point so I could understand not everyone feels this way.

My main pain point here are some error messages like this. I would like to have some indication this does not work because it tries to select fields form a table that is not part of the query source. Similarly here I would expect some indication that this is caused by a missing group by clause.

Maybe rustc_on_unimplemented could help there though?

rustc_on_unimplemented is nightly only, so that does not really help here. rust-lang/rust#51992 would probably help, but there seems to be on progress there for the last few years.

Copy link
Member

@henryboisdequin henryboisdequin left a comment

Choose a reason for hiding this comment

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

Small nit otherwise LGTM

Comment on lines +1363 to +1365
/// This method behaves similar to [get_result](self::RunQueryDsl::get_result)
/// than [load_into](self::RunQueryDsl::load_into) behaves to
/// [load](self::RunQueryDsl::load)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// This method behaves similar to [get_result](self::RunQueryDsl::get_result)
/// than [load_into](self::RunQueryDsl::load_into) behaves to
/// [load](self::RunQueryDsl::load)
/// This method's behavior is more similar to [get_result](self::RunQueryDsl::get_result)
/// than [load_into](self::RunQueryDsl::load_into) behaves to
/// [load](self::RunQueryDsl::load)

@weiznich
Copy link
Member Author

As already written above I'm not really sold on this implementation. To first have a bit history on this feature here: The original request for something similar is #1435 and maybe #2037. Both propose something similar to this feature. Additionally there is with #2367 an implementation that takes a different stab on this problem.
I think my main problem here is that I'm not really sold on the exposed API of this implementation and also of #2367, but it's not obvious how a better API would look like.
Just for the record let's list my pain points for each implementation:
Concerting the implementation in #2367 I do not like the fact that this introduces a separate select method that requires a type parameter instead of value. Additionally it does not integrate with returning clauses at all. On the other hand I do like the fact that we do not duplicate any entry point on the RunQueryDsl side and the fact that this implementation ensures that you cannot use Selectable from type a while Queryable is comming from type b.
On the other hand this implementation skips the problem of having a separate select clause for this, but requires duplicated methods in RunQueryDsl instead. Additionally the error messages are a clear show stopper here.
The question is how could we solve this in a better way. For me it seems like changing #2367 to reuse somehow the existing select method may be a better solution here.

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