Open vidhanio opened 1 year ago
Bump — I totally agree with this proposal!
We won't accept a blanket change to TryInto
for the reasons outlined in https://github.com/launchbadge/sqlx/pull/2649#issuecomment-1744094767, 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);
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!
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.
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?
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.
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:
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.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.
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. ❤️
@RobbieMcKinstry you'll have to query the column as "column_name: _"
to disable type checking on it.
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".
You were correct, by the way. Thank you very much for your insight. :)
Hi,
I'm new to Rust and i run into this problem while moving from query_as
to query_as!
.
I have an custom enum to handle a type
field in the db:
#[derive(Deserialize, Serialize)]
pub enum ConfigurationType {
Global,
SCOH
}
And i need to perform an insert using it like this:
pub struct Configuration {
pub id: sqlx::types::Uuid, // this field is auto-generated in db
pub enabled: bool,
pub r#type: ConfigurationType,
}
impl Configuration {
pub async fn generate(
tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
) -> Result<Vec<Configuration>, sqlx::Error> {
let res = sqlx::query_as!(Configuration,
r#"
INSERT INTO table (type, enable)
VALUES ($1, false), ($2, false)
RETURNING *"#,
ConfigurationType::Global as ConfigurationType,
ConfigurationType::SCOH as ConfigurationType,
).fetch_all(&mut **tx).await?;
Ok(res)
}
}
I try to use Decode
:
impl<'r, DB: Database> Decode<'r, DB> for ConfigurationType
where
String: sqlx::Decode<'r, DB>,
{
fn decode(
value: <DB as HasValueRef<'r>>::ValueRef,
) -> Result<Enum, Box<dyn Error + 'static + Send + Sync>> {
let value = <String as Decode<DB>>::decode(value)?;
match value.as_str() {
"global" => Ok(ConfigurationType::Global),
"scoh" => Ok(ConfigurationType::SCOH),
_ => Err("Invalid string as configuration type".into()),
}
}
}
but this is not working it still asking to implement the From
trait
the trait bound `ConfigurationType: std::convert::From<std::string::String>` is not satisfied required for `std::string::String` to implement `Into<ConfigurationType>
So i don't find any other solution than to implement the From with !panic
:
impl From<String> for ConfigurationType {
fn from(value: String) -> Self {
match value.as_str() {
"global" => ConfigurationType::Global,
"scoh" => ConfigurationType::SCOH,
_ => panic!("String {} cannot be used as ConfigurationType!", value),
}
}
}
I don't know if there is another way to solve this problem , but imho i understand why FromRow
is missing but without any guidance macro are a bit hard to understand and use.
The best thing to do would of course be to support FromRow
directly in query_as!
but it is clear from #514 that there are a lot of technical hurdles to make that happen.
The idea to use TryFrom
is nice, but I agree with @gyzerok that parsing should be granular, and data that is stored in the DB should not be treated the same as 'generic user data'. We can trust the DB to a higher extent as data coming from 'anywhere', under the assumption that it is very likely that the data was written only after passing checks in our apps before.
If we enable plain TryFrom
everywhere, we would lose out on a lot of type-safety, because for example i8::try_from(u64)
exists but will fail at runtime for any value larger than 63.
Instead, what about a specialized trait named e.g. TryFromType
:
From/Into
conversions (to make sure this is backwards-compatible with the current implementation of query_as!
that uses .into()
)The intention for this trait is to only be implemented for types for which parse failure is rare; i.e. parse failure means that something other than (bug-free code of) your app has been messing up the DB's contents.
For example:
i64
because there is no u64
in Sqlite or Postgres. TryFromType
would only fail iff someone inserts a negative i64 outside of the purview of Sqlx into the DB anyway.uuid
/json
/timestamp
s in the drivers for DBs that don't support them natively. Decoding fails iff someone were to insert an unparsable value in the backing TEXT column.
Is your feature request related to a problem? Please describe. currently, the
query_as!
macro performsInto::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 implementDecode
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 theimpl 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 ofinto()
. If it fails, it should take theTryInto::Error
and box it (much likeDecode::decode
) and bubble it up as ansqlx::Error
.From what I can tell, this would not break any previous code, due to the pre-existing blanket
impl
ofimpl<T, U> TryFrom<U> for T where U: From<T>
, and theErr
would just beInfallible
.Here is an example of when this would be useful:
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 implementingDecode
then still not have compile-time type safety for their query.Describe alternatives you've considered Manually implementing
Decode
.