Skip to content

feat: use try_into in sqlx::query_as! conversions #2649

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

Closed
wants to merge 1 commit into from

Conversation

vidhanio
Copy link

closes #2648

@abonander
Copy link
Collaborator

This is a breaking change.

@vidhanio
Copy link
Author

vidhanio commented Aug 2, 2023

should the blanket TryFrom where From impl not prevent this from being breaking?

@bm-w
Copy link

bm-w commented Sep 1, 2023

IMO this would be a great improvement — @abonander, could you perhaps expand a little on your previous comment? If this is indeed a breaking change, are you outright rejecting this PR, or do you see a path forward to getting this merged, perhaps in an upcoming release that would permit breaking changes?

@abonander
Copy link
Collaborator

If I'm honest, it's mostly a gut feeling that a sweeping change like this should not sneak into a minor release. Even if it seems like it should be backward-compatible, there may be some subtle detail that we're not seeing that comes back to bite us later.

We should also be considerate of forward compatibility. If this change goes out in a minor release and someone updates their code to rely on it, then has to downgrade for whatever reason (e.g. to work around a bug), they're going to have a huge headache.

In all, my gut is telling me this is something that should be reserved for 0.8.0.

@jplatte
Copy link
Contributor

jplatte commented Sep 26, 2023

Separately from the question of whether it's a breaking change, is this really the right thing to do? It would allow querying a table with an INTEGER column into a struct that has a field of type u8. While I guess it is convenient that it allows you to use unsigned types with postgres without extensions if you don't exceed i64::MAX and such, I really worry about this masking bugs where the types are unintentionally different. IMHO try_into decoding should be opt-in per query, not the default, if supported at all.

@abonander
Copy link
Collaborator

That is a fair concern as well. It has a lot of potential to turn compile-time errors into runtime errors, which is exactly the opposite of what we want.

@vidhanio
Copy link
Author

vidhanio commented Sep 29, 2023

Thank you for your comments, I definitely understand the concerns and agree about avoiding turning compile time errors into runtime errors. I also agree this is too large of a change to be a small patch.

Would a try_query_*! set of macros make more sense to solve this issue?

@abonander
Copy link
Collaborator

I think it would be better to be able to opt into this on a per-field basis, and require the user to explicitly specify the type to decode to ensure type-safety.

Ultimately, we want query_as!() to be able to work with #[derive(FromRow)], and I think that would be the best place to do this, e.g. #[sqlx(try_from = "<type to decode from>")]. That feature is being discussed in #514. It theoretically doesn't have any blockers but as I explain in my latest comments there, the support from the language is not quite where I want it to be yet.

I have an alternative, more immediately actionable idea which I'll elaborate on in the motivating issue, #2648. I'm going to close this PR as I'm not prepared to merge it as written because of the problems we've discussed.

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.

proposal: use TryFrom instead of From in query_as!
4 participants