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

Macros: add option to surface warning notices as errors #3133

Open
northmaybe opened this issue Mar 18, 2024 · 2 comments
Open

Macros: add option to surface warning notices as errors #3133

northmaybe opened this issue Mar 18, 2024 · 2 comments
Labels

Comments

@northmaybe
Copy link

Bug Description

Given an already existing table, e.g:

CREATE TABLE person (
	first_name TEXT NOT NULL 
);

when adding a second column to the table, and adding a NOT NULL constraint, e.g.

ALTER TABLE person ADD COLUMN last_name TEXT; -- Add column without constraint for existing rows
UPDATE person SET last_name = ''; -- Add default value for existing rows
ALTER TABLE person ALTER COLUMN last_name SET NOT NULL; -- Add constraint

sqlx::query! will not throw a compile time error for the following query:

sqlx::query!(
    r#"INSERT INTO person (first_name)
    VALUES ($1)"#,
    "John"
);

While the new column itself is detected (e.g. for select queries), any later added NOT NULL constraints are apparently completely ignored.

Minimal Reproduction

Minimal github project

Info

  • SQLx version: 0.7.4
  • SQLx features enabled: ["postgres", "runtime-tokio"]
  • Database server and version: psql (PostgreSQL) 15.2 (Debian 15.2-1.pgdg110+1)
  • Operating system: Arch Linux 6.6.2-arch1-1 x86_64
  • rustc --version: rustc 1.76.0 (07dca489a 2024-02-04)
@northmaybe northmaybe added the bug label Mar 18, 2024
@abonander
Copy link
Collaborator

This is not a bug. If Postgres returns a successful parse result, we have no way of knowing the query is invalid. SQLx doesn't null-check bind parameters or do any sort of analysis on the query itself, only the metadata that Postgres returns.

In this situation, Postgres is likely returning a non-fatal warning message alongside the parse result, which we currently don't have a good way to surface. Unfortunately, emitting lint warnings from proc macros has been unstable for 6 years and doesn't look to be getting stabilized anytime soon. And turning any non-fatal warnings into compiler errors for all users would be an unacceptable breaking change.

We've talked about implementing a Clippy-like linter for SQLx, but that's a whole project in and of itself.

We could use the sqlx_macros_unstable/procmacro2_semver_exempt cfg-flags to conditionally enable the use of the proc_macro::Diagnostic API, but I have a feeling most people don't use those flags,

I think instead, we could make it optional to elevate these warnings to errors, either using an environment variable (e.g. SQLX_DENY_WARNING_NOTICES=1) or by setting a key in a config file committed to the project, e.g. a sqlx.toml.

@northmaybe
Copy link
Author

Despite having already read issues about SQLx not null-checking, I had the illusion it did in my case and only altering the table was the issue. Yeah, should've definitely rechecked that.

Either way, the latter idea to provide an option to elevate warnings to errors sounds very useful to me. I think the ability to store the option to a file is crucial for reliability (opposed to remembering setting the environment variable every new session).

If using an environment variable includes the option to write it into e.g. the .env file, that sounds like the easier implementation.

@abonander abonander added macros and removed bug labels Mar 31, 2024
@abonander abonander changed the title PSQL: NOT NULL constraints of later added columns to existing tables are not detcted by sqlx::query! Macros: add option to surface warning notices as errors Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants