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

JSON serialization error panics with PostgreSQL #2730

Closed
FSMaxB opened this issue Sep 4, 2023 · 3 comments · Fixed by #3126
Closed

JSON serialization error panics with PostgreSQL #2730

FSMaxB opened this issue Sep 4, 2023 · 3 comments · Fixed by #3126
Labels

Comments

@FSMaxB
Copy link
Contributor

FSMaxB commented Sep 4, 2023

Bug Description

When using sqlx::Json in sqlx::query!, if serializing the value fails, it panics with

'failed to serialize to JSON for encoding on transmission to the database: Error("the enum variant Variant::Unknown cannot be serialized", line: 0, column: 0)'

The line where this happens is:

serde_json::to_writer(&mut **buf, &self.0)
.expect("failed to serialize to JSON for encoding on transmission to the database");

Not sure how to fix this without a breaking change because Encode::encode_by_ref returns IsNull and not a Result of some kind. I would still argue that this shouldn't panic though.

Minimal Reproduction

I created a small reproducer here: https://github.com/FSMaxB/sqlx-serialization-panic-reproducer

This is the same reproducer but without the full project setup.

use serde::{Deserialize, Serialize};
use sqlx::{Connection, PgConnection};
use sqlx::types::Json;

#[tokio::main(flavor = "current_thread")]
async fn main() -> anyhow::Result<()> {
    let mut connection = sqlx::PgConnection::connect("postgresql://postgres:postgres@localhost:14538").await?;

    // this goes through successfully
    insert_variant(&mut connection, Variant::A).await?;

    // this panics
    insert_variant(&mut connection, Variant::Unknown).await?;
    // And yes, writing code like this should be considered a bug, because this
    // can obviously never work with a non-serializable variant, but I would
    // strongly argue that this should not panic (especially since this is an easy
    // mistake to make)

    Ok(())
}

async fn insert_variant(connection: &mut PgConnection, variant: Variant) -> sqlx::Result<()> {
    sqlx::query!(
        "INSERT INTO variants (variant) VALUES ($1)",
        Json(variant) as _,
    ).execute(connection).await?;

    Ok(())
}

#[derive(Clone, Copy, Deserialize, Serialize)]
enum Variant {
    A,
    B,
    C,
    #[serde(other, skip_serializing)]
    Unknown,
}

I chose #[serde(skip_serializing)] to provoke the serialization error in this example, but it could also have been any other serialization error.

Info

  • SQLx version: 0.7.1
  • SQLx features enabled:
    • migrate
    • json
    • macros
    • postgres
    • runtime-tokio-rustls
  • Database server and version: PostgreSQL 15.4
  • Operating system: Linux 6.4.12-200.fc38.x86_64 x86_64
  • rustc --version: rustc 1.72.0 (5680fa18f 2023-08-23)
@FSMaxB FSMaxB added the bug label Sep 4, 2023
@FSMaxB
Copy link
Contributor Author

FSMaxB commented Mar 12, 2024

@abonander Is this something you would consider for the next breaking release?

I can try taking this on. So far I tried making encode and encode_ref return Result<IsNull, BoxDynError>, this bubbles up all the way and even ends up in bind(..). Maybe this can be solved by storing the error internally and only surfacing it on .execute(..) etc. instead of on every .bind(...).

@abonander
Copy link
Collaborator

Yep, we've started putting together 0.8.0 so now is the time to do it! The infallibility requirement has bitten us in a few other places as well (#3053).

I like the idea of storing the error internally until .execute() is called to avoid requiring every usage everywhere to be refactored; I'd add .try_bind() to eagerly get the error where the user wants it.

@FSMaxB
Copy link
Contributor Author

FSMaxB commented Apr 10, 2024

I've finally gotten around to finishing up my PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants