proposal: use `TryFrom` instead of `From` in `query_as!`

[vidhanio]

Is your feature request related to a problem? Please describe.
currently, the query_as! macro performs Into::into on returned items to convert them into the types of the struct fields. This solution only works, obviously, when the conversion is infallible. what if the conversion is fallible? sqlx requires one to implement Decode to fallibly convert from database types. The problem is that these have to be checked at run-time, as their type checking is done by the author in the impl Type, etc. However, what if your type can be converted (fallibly) from a built-in, already type-checked sqlx type (Uuid, String, etc.)?

Describe the solution you'd like
query_as! should use .try_into() in its generated code instead of into(). If it fails, it should take the TryInto::Error and box it (much like Decode::decode) and bubble it up as an sqlx::Error.

From what I can tell, this would not break any previous code, due to the pre-existing blanket impl of impl<T, U> TryFrom<U> for T where U: From<T>, and the Err would just be Infallible.

Here is an example of when this would be useful:

struct Username(String);

impl TryFrom<String> for Username {
	type Error = &'static str;

	fn try_from(s: String) -> Result<Self, Self::Error> {
		for c in c.chars() {
			if !('a'..='z').contains(c) {
				return "invalid character: (must be a-z)";
			}
		}

		return Self(s)
	}
}

struct User {
	username: Username,
	bio: String,
}

sqlx::query_as!(User, "SELECT username, bio FROM users"); // valid query, as `Username` implements `TryFrom` a built-in sqlx type.

Of course, this may seem like unneeded validation for a database retrieval, but one may want to avoid implementing From due to it exposing a hole in data validation, meaning they have to go through the hassle of implementing Decode then still not have compile-time type safety for their query.

Describe alternatives you've considered
Manually implementing Decode.

[bm-w]

Bump — I totally agree with this proposal!

[abonander]

We won't accept a blanket change to TryInto for the reasons outlined in #2649 (comment), but here's my alternative proposal:

We can extend #[derive(sqlx::Type)] to allow a custom Decode shim similar to #[serde(deserialize_with = "<path to function>)]":

#[derive(sqlx::Type)]
#[sqlx(transparent)] // Means encode to/decode from `String`
#[sqlx(decode_with = "decode_username")]
struct Username(String);

fn decode_username<DB: Database>(val: <DB as HasValueRef<'_>>::ValueRef) -> Result<Username, Box<dyn Error + Send + Sync + 'static>> {
    let username = String::decode(val)?;
    Ok(Username::try_from(username)?)
}

Or, since that function signature turns out to be rather gnarly, we could just add a special marker to use TryFrom, e.g.:

#[derive(sqlx::Type)]
#[sqlx(decode(try_from = "String"))]
struct Username(String);

[bm-w]

Agree with your reasons, and happy to see this alternative #[derive(sqlx::Type)] idea. Are you suggesting to implement both decode_with = … and decode(try_from = …)? I’d say both have their benefits: the former is most flexible, while the latter will be more ergonomic in most cases (but likely can‘t be used in some). I think decode(from_str) should be considered as well (i.e. using FromStr).

Are you open to @vidhanio or I taking a shot at implementing this? I’d be happy to!

[abonander]

I think decode(from_str) should be considered as well (i.e. using FromStr).

That is also one I'd like to see. It would be a great alternative solution to implementing direct support for every datatype from any third-party crate that someone wants to shove into or pull out of a database.

As for which one(s) to implement, that's dealer's choice really. They can be implemented independently or all at once.

[vidhanio]

sorry for the late reply, studying for a midterm 😅

just a question to confirm my intuition, this solution would not provide compile time type safety, correct? i assume this is a "type-erased" situation where at runtime the query would fail if the column was of the wrong type?

[abonander]

Type safety with custom types is limited right now either way, as the macros don't currently have a way of statically typechecking them. That's what #514 is meant to solve.

However, that is a breaking change whereas this is backwards compatible.

[gyzerok]

I've got sort of blocked by the same problem and wanted to bring my thoughts here.

First of all if I may comment on the proposed solutions the following looks appealing to me. It's almost the same as was proposed, but in my opinion decode(..) bit is unnecessary and without it API is in line with field attributes for FromRow which helps with the intuition of how to use the library.

#[derive(sqlx::Type)]
#[sqlx(try_from = "String")]
struct Username(String);

It looks like for the time being having an adjusted copy of the macro inside my own repository would be enough as a workaround. Unfortunately I am not as experienced in Rust so maybe @abonander you could provide a code sample here in the issue which could be copy-pasted directly? It would be highly appreciated.

Now thinking about the solution overall I see 2 different point of views here:

1. On one hand reusing TryFrom implementation is great, because it helps achieving additional safety with the newtype pattern. At least in my codebases I use "parse dont validate" idea extensively and find it really useful.
2. On the other hand it might make sense to treat parsing from the outside world (say request params) differently from obtaining value from the db. If we consider db as a source of truth then having "wrong" value there is not necessarily wrong by itself. Maybe we would like to adjust validation strictness on the API surface area, but have existing values as is. In that case I would still try to avoid having From<T> conversation, so user-space code can't violate the constants.

As an outcome from 2 it might make sense to design a different API, so granularity of parsing/decoding is possible.

[RobbieMcKinstry]

Hello! I wanted to ask about the stated work-around. I'm in the exact situation described by @vidhanio: I'm implementing an Email type...

#[derive(sqlx::Type)]
#[sqlx(transparent)]
pub struct Email(String);

My email is transparently a String, but I want to practice "Make illegal states unrepresentable" by only implementing TryFrom<String> for Email, and not implementing From<String> for Email.

The original issue indicates that I can implement Decode<'r, DB: Database> for Email as a workaround -- my Email type won't be compile-time typechecked, but I'll still be able to use query_file_as!. That's good enough for me for now; however, I can't seem to get this to work.

Here's my full code snippet:

#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, ToSchema)]
pub struct Email(String);

impl<'r, DB: Database> Decode<'r, DB> for Email
where
    &'r str: Decode<'r, DB>,
{
    fn decode(
        value: <DB as sqlx::database::HasValueRef<'r>>::ValueRef,
    ) -> Result<Self, sqlx::error::BoxDynError> {
        let string = <&str as Decode<DB>>::decode(value)?.to_owned();
        let result = Email::try_from(string).map_err(|err| Box::new(err))?;
        Ok(result)
    }
}

This yields the same error as before I implemented Decode:

the trait bound `email::Email: From<std::string::String>` is not satisfied
...
the trait `From<std::string::String>` is not implemented for `email::Email`
   = note: required for `std::string::String` to implement `Into<email::Email>`

Am I missing something? Any help would be appreciated. ❤️

[jplatte]

@RobbieMcKinstry you'll have to query the column as "column_name: _" to disable type checking on it.

[RobbieMcKinstry]

Thank you very much! I'll give that a try. :D I probably should have realized that, since I acknowledged "my type won't be compile-time typechecked".