launchbadge / sqlx

🧰 The Rust SQL Toolkit. An async, pure Rust SQL crate featuring compile-time checked queries without a DSL. Supports PostgreSQL, MySQL, and SQLite.
Apache License 2.0
13.29k stars 1.26k forks source link

Query macro documentation understates lack of nullability checking on bind parameters #2642

Open okkero opened 1 year ago

okkero commented 1 year ago

Bug Description

When using query! to insert data into a non-null column, but supplying a nullable value, the code compiles fine. I was expecting it to not compile, since it is a clear violation of the schema. Perhaps it is intended behavior, or a known limitation, but I couldn't find it documented anywhere, so I was a bit confused.

Minimal Reproduction

Create a table with a single non-null column. I did create table foo (data text not null);. Try to insert a null value into the table using query!:

sqlx::query!("insert into foo (data) values (null)")
    .execute(&pool)
    .await?;

Or using an Option:

sqlx::query!("insert into foo (data) values ($1)", None::<String>)
    .execute(&pool)
    .await?;

The code compiles fine, but outputs the following error when run:

Error: error returned from database: null value in column "data" of relation "foo" violates not-null constraint

Caused by:
    null value in column "data" of relation "foo" violates not-null constraint

Shouldn't this fail at compile time?

Info

abonander commented 1 year ago

This is not a bug; null checking of parameters has never been supported.

There's unfortunately just no way to get the required information without analyzing the query ourselves, which is not trivial: https://github.com/launchbadge/sqlx/blob/main/FAQ.md#why-cant-sqlx-just-look-at-my-database-schemamigrations-and-parse-the-sql-itself

The lack of null checking is mentioned in the documentation, albeit a little understated: https://docs.rs/sqlx/latest/sqlx/macro.query.html#nullability-bind-parameters

Option::None will be bound as NULL, so if binding a type behind Option be sure your query can support it.

I'll leave this open as a documentation bug.

okkero commented 1 year ago

Ah, yes I see it now. I think I read that part but didn't quite make the connection to the issues I was facing, but it makes more sense now. Thank you.