-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Proof of concept for query_tuple!
, similar to query!
but returns a tuple instead of an anonymous record
#3780
Conversation
assert_eq!(account.0, Some(1)); | ||
assert_eq!(account.1.as_deref(), Some("Herp Derpinson")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly why I don't think we should support this. The context is immediately and entirely lost. What if there's 50+ lines of code between the query and where a value is used? It won't be obvious at all what account.0
or account.1
is supposed to be.
If the idea was to immediately destructure the tuple into separate bindings, like an augmented query_scalar!()
, I could maybe understand that:
let (account_id, account_name) = sqlx::query_tuple!(...).fetch_one(&mut conn).await?
However, even that loses the context that they're all connected to a single (logical if not physical) database record--unless you prefix the names like I did here, but even then, account_id
is the same number of characters as account.id
.
I don't see the value here. Honestly, I think this would make for less maintainable code than the status quo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand where you're coming from, but I do think this could have its place. It sounds like a general argument against tuples, not so much an argument about tuples specifically in sqlx.
Don't get me wrong, I agree that tuples are not as precise as structs with named fields. That's why we don't just use tuples everywhere. But tuples do have their place; there is a reason that tuples are in the language, and I think it could be reasonably used within sqlx as well. Certainly not always, but sometimes, much the same way as tuples are used generally in Rust 🙂.
I think the example you give with destructuring is a very nice usage for instance. I mean, I could definitely imagine situations where this...
let (account_id, account_name) = sqlx::query_tuple!(...).fetch_one(&mut conn).await?;
println!("Account ID: {}", account_id);
println!("Account name: {}", account_name);
... would be preferable in comparison to this:
let row = sqlx::query!(...).fetch_one(&mut conn).await?;
println!("Account ID: {}", row.account_id);
println!("Account name: {}", row.account_name);
I mean, when you look at it in that light, there really is not much of a difference, but the tuple version is sometimes preferable I would say, just the same way as tuples are sometimes preferable in other situations with Rust types.
Another way to think of it: Think of query_tuple!
as a kind of compromise between query_scalar!
and query!
. Tbh, if we had query_tuple!
, maybe query_scalar!
wouldn't even be necessary as you could just fetch a tuple of a single element and it would be effectively equivalent.
I'd also like to point out that with your line of reasoning, you could argue that FromRow
should not be implemented for tuples. But it is, and it leads to a kind of weird asymmetry in the API, where I can use query_as
(the function) to get a tuple, but there is no way to use the checked macros to get a tuple.
// If this is okay...
let (account_id, account_name): (i32, String) = sqlx::query_as(...).fetch_one(&mut conn).await?;
// ... then why is this bad?
let (account_id, account_name) = sqlx::query_tuple!(...).fetch_one(&mut conn).await?;
// Or in other words: If query_as for tuples is provided,
// why isn't there an equivalent way to get the same thing with macros?
// Also note that query_tuple! is much better than query_as in this situation
// as it checks the query at compile-time and you don't even need to specify
// the type of the tuple, as you need to do in the query_as case.
With your reasoning, it would at least be consistent if FromRow
was not implemented for tuples and you had to use a named type. But that is not the case and I do feel that is the right choice, because sometimes tuples is a more convenient option. In that vein, I think the right choice would be to allow the same usage for the macro case, hence providing query_tuple
:)
I hope that makes sense and I hope you can see where I'm coming from :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abonander it's been a week so hope you don't mind the ping 🙂. Did you have a chance to read my comment above? Do you have any thoughts on the asymmetry in the API between query_as
(the function) and query!
(the macro) with regards to tuples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pinging me doesn't get me to respond faster. I already get notifications for discussions across the whole repository. I have a full-time job that isn't SQLx and it's kept me very busy lately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally fair, take your time 🙏
It's kind of a different situation. Implementing You also still need to either explicitly specify the tuple types or give the compiler enough information to infer it, requiring the code to at least be somewhat self-documenting. Conversely, by making a distinct macro, this is presenting that option front-and-center as something users should consider, which I don't agree with. Additionally, it inherently hides the actual structure of the tuple in a way that wouldn't always be easy to deduce from the SQL alone. Enabling useful patterns is one thing, enabling bad code is another. I'd be inclined to discuss supporting this functionality as an operating mode of Given all that, and to reduce the backlog, I'm going to close this PR. |
Currently I don't know of any way to get a tuple from a checked query, so I adapted some of the code for the anonymous record case (i.e.
query!
) and it seemed to work when I tried using it in my own project. See also the discussion I started here.Is there some caveats to this implementation that I'm not aware of? I'd love to see this functionality in sqlx and I'm a bit surprised that it doesn't exist yet seeing how easy it was to get this proof of concept, which is why I am suspecting there might be a problem with this? 😅 Please enlighten me!
Is this a breaking change?
No, it only adds a feature.