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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add JSON support to FromRow derive #2121

Merged
merged 2 commits into from Jul 31, 2023
Merged

Conversation

95ulisse
Copy link
Contributor

Hello 馃憢

This PR is a proposal to discuss JSON support for the FromRow derive. The proposal is to add a new json attribute to indicate that a field should be decoded using sqlx::types::Json instead of the field type.

Motivation

The currently existing derive for FromRow allows quick and easy type conversions using TryFrom. This saves us a lot of boilerplate code just for the sake of converting types. Unfortunately TryFrom-based conversions are not enough for json values:

#[derive(FromRow)]
struct Record {
  #[sqlx(try_from = "i64")]
  counter: u64,
  #[sqlx(try_from = "Json<Metadata>")] // <- Does not work
  metadata: Metadata
}

#[derive(serde::Deserialize)]
struct Metadata {
  some: String,
  fields: String.
}

Due to Rust's rules, we can't just add an impl<T> From<Json<T>> for T as that would violate the orphan rules, so at the moment, we are stuck with one of two solutions:

  1. Change the type of the metadata field from Metadata to Json<Metadata>.
  2. Manually implement FromRow.

Neither of these two solutions are ideal IMHO, hence this proposal of adding a new json attribute that will take care of the JSON deserialization and all the necessary type conversions. Using this new attribute, the previous example would become:

#[derive(FromRow)]
struct Record {
    #[sqlx(try_from = "i64")]
    counter: u64,
    #[sqlx(json)] // <- New attribute
    metadata: Metadata
}

#[derive(serde::Deserialize)]
struct Metadata {
    some: String,
    fields: String.
}

Example generated code

The example above would (roughly) expand to:

#[derive(serde::Deserialize)]
struct Metadata {
    some: String,
    fields: String.
}

struct Record {
    counter: u64,
    metadata: Metadata
}

impl<'a, R: sqlx::Row> sqlx::FromRow<'a, R> for Record
where
    &'a str: sqlx::ColumnIndex<R>,
    i64: sqlx::decode::Decode<'a, R::Database>,
    i64: sqlx::types::Type<R::Database>,
    sqlx::types::Json<Metadata>: sqlx::decode::Decode<'a, R::Database>, // <- New bound
    sqlx::types::Json<Metadata>: sqlx::types::Type<R::Database>, // <- New bound
{
    fn from_row(row: &'a R) -> sqlx::Result<Self> {
        // Code generated by `try_from`.
        let counter = row.try_get("counter")
            .and_then(|v| <u64 as ::std::convert::TryFrom::<i64>>::try_from(v)
                .map_err(|e| ::sqlx::Error::ColumnNotFound("FromRow: try_from failed".to_string())
            ))?;

        // NEW: Code generated by json. Uses `try_get` with `sqlx::types::Json` instead of `Metadata`.
        let metadata: Metadata = row.try_get::<sqlx::types::Json<_>, _>("metadata").map(|x| x.0)?;

        Ok(Record {
            counter,
            metadata,
        })
    }
}

Interactions with the already existing attributes

  • rename and default. They work as before, no interference with json.
  • flatten. I don't think it makes sense to use both flatten and json on the same field, so this combination is rejected.
  • try_from. If we have a field#[sqlx(json, try_from = "X")] field: Y, the value will be first deserialized to X and then converted using std::convert::TryFrom to Y.

@jltorresm
Copy link

Would love to see this happen!

@abonander
Copy link
Collaborator

@95ulisse Why is wrapping metadata in Json<_> unacceptable? It forwards almost all relevant traits and also implements Deref/DerefMut so you can access the fields of Metadata through it?

I'm wary of having more than one thing to do something, cause that's just more to teach.

@95ulisse
Copy link
Contributor Author

95ulisse commented Feb 9, 2023

Why is wrapping metadata in Json<_> unacceptable?

@abonander It's not unacceptable, it's just a bit inconvenient in some situations. I think of this #[sqlx(json)] as a little helper to make deriving FromRow easier on the same level of #[sqlx(try_from)]. It's not strictly essential, but it's helpful as it avoids writing a bunch of glue code.

Just to keep the example of a generic metadata field, I often find myself writing a model like this one that I keep passing around in my application:

pub struct SomeModel {
    pub id: Uuid,
    pub metadata: HashMap<string, string>,
}

As it is right now, I can't just #[derive(FromRow)] and call it a day. I see three solutions to this:

  1. Wrap that HashMap in Json<_>.
  2. Write my own wrapper and implement Decode and Type manually (deferring to Json<_> internally).
  3. Introduce an intermediate struct SomeModelDb with the same fields but with the map wrapped in Json<_> and then implement From<SomeModelDb> for SomeModel.

And that's why I'm not a huge fan of each of them:

  1. IMHO it just feels wrong to let an sqlx type leak to higher abstraction levels of the application. It's not a matter of ergonomics as Json<_> is almost transparent as you said, but it's more of an encapsulation/isolation concern. HTTP/gRPC/whatever layers should not be aware of sqlx.
  2. That's definitely too much boilerplate to basically just reimplement Json<_>.
  3. This is the route I've been following until now in practice, but it causes a lot of duplication and boilerplate conversions.

Having an #[sqlx(json)] attribute would allow me to sidestep all these problems and have a single model that can be cleanly shared in all layers of the application.

As I said at the beginning, these are all just annoyances, not real blocking problems. If we could find a way to make it work with the existing try_from I'd be perfectly ok with it, but I just couldn't find a solution, so that's why this PR with a new json attribute. You can think of it as a "specialized" form of try_from for JSON data.

@abonander
Copy link
Collaborator

After thinking about this more, I will accept this as I want to land something similar for converting to/from types using their FromStr and Display impls, respectively. I agree it would be more convenient than the wrapper type.

@95ulisse do you mind rebasing and fixing the merge conflicts?

@95ulisse
Copy link
Contributor Author

@abonander I've rebased the PR and fixed the conflicts. I've also added a bit of docs to the FromRow trait itself to document the new json attribute.

@abonander abonander merged commit 9463b75 into launchbadge:main Jul 31, 2023
57 checks passed
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.

None yet

3 participants