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

enhancement: (#2934) Allows flatten attribute to work for Optional values when deriving FromRow #3667

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jonnyso
Copy link

@jonnyso jonnyso commented Jan 5, 2025

fixes #2934

@abonander
Copy link
Collaborator

This change appears innocuous enough, but I'm worried that people won't understand what this impl is for and will assume it does something different, i.e. make .fetch_one() return None instead of erroring on an empty resultset. That's not really possible with our current API design, which makes this impl a potential footgun since it'd compile but behave differently than expected at runtime.

I think I'd prefer if this was something much more specific to #[sqlx(flatten)], and I think we can accomplish that without adding a new public blanket impl, using a trick called autoderef specialization: http://lukaskalbertodt.github.io/2019/12/05/generalized-autoref-based-specialization.html

I used the same trick in #3356 although the solution I'm imagining isn't going to look exactly like that.

@abonander
Copy link
Collaborator

(As for why to use autoderef specialization instead of just checking if the type is wrapped in Option: that can't see through type aliases.)

@jonnyso
Copy link
Author

jonnyso commented Jan 6, 2025

I agree it would be inconsistent with the way .fetch_one() works right now, I'll look into the suggestions you made, thanks !

@jonnyso

This comment was marked as resolved.

@jonnyso jonnyso changed the title enhancement: (#2934) implement FromRow for Option<T> where T: FromRow enhancement: (#2934) Allows flatten attribute to work for Optional values when deriving FromRow Jan 8, 2025
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.

flatten for optionals
2 participants