sbdchd / squawk

🐘 linter for PostgreSQL, focused on migrations
https://squawkhq.com
GNU General Public License v3.0
541 stars 37 forks source link

CREATE TABLE and CREATE INDEX do not trigger prefer-robust-statements warnings individually #325

Open febpetroni opened 7 months ago

febpetroni commented 7 months ago

Version: v0.24.2

Config file:

excluded_rules = [
    "adding-field-with-default", 
    "adding-not-nullable-field", 
    "ban-drop-column", 
    "ban-drop-table", 
    "ban-drop-not-null", 
    "changing-column-type", 
    "renaming-table", 
    "renaming-column", 
    "require-concurrent-index-creation", 
    "require-concurrent-index-deletion", 
    "prefer-big-int", 
    "prefer-bigint-over-int", 
    "prefer-bigint-over-smallint", 
    "prefer-timestamptz" 
]

Migration file 1:

CREATE TABLE some_table(
    id UUID PRIMARY KEY,
    version INTEGER NOT NULL,
    created_at TIMESTAMP WITHOUT TIME ZONE NOT NULL,
    updated_at TIMESTAMP WITHOUT TIME ZONE NOT NULL
);

Output:

Found 0 issues in 1 file 🎉

Expected:

warning: prefer-robust-stmts

   1 | CREATE TABLE some_table(
   2 |     id UUID PRIMARY KEY,
   3 |     version INTEGER NOT NULL,
   4 |     created_at TIMESTAMP WITHOUT TIME ZONE NOT NULL,
   5 |     updated_at TIMESTAMP WITHOUT TIME ZONE NOT NULL
   6 | );

  help: Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statement supports it.

Migration file 2:

CREATE INDEX some_table_idx ON some_table (version);

Output:

Found 0 issues in 1 file 🎉

Expected:

warning: prefer-robust-stmts

   1 | CREATE INDEX some_table_idx ON some_table (version);

  help: Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statement supports it.

However, if both statements are in the same migration file, it works as expected:

Migration file 3:

CREATE TABLE some_table(
    id UUID PRIMARY KEY,
    version INTEGER NOT NULL,
    created_at TIMESTAMP WITHOUT TIME ZONE NOT NULL,
    updated_at TIMESTAMP WITHOUT TIME ZONE NOT NULL
);
CREATE INDEX some_table_idx ON some_table (version);

Output (as expected):

some_clever_test.sql:1:0: warning: prefer-robust-stmts

   1 | CREATE TABLE some_table(
   2 |     id UUID PRIMARY KEY,
   3 |     version INTEGER NOT NULL,
   4 |     created_at TIMESTAMP WITHOUT TIME ZONE NOT NULL,
   5 |     updated_at TIMESTAMP WITHOUT TIME ZONE NOT NULL
   6 | );

  help: Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statement supports it.

some_clever_test.sql:7:1: warning: prefer-robust-stmts

   7 | CREATE INDEX some_table_idx ON some_table (version);

  help: Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statement supports it.
chdsbd commented 7 months ago

Hi @febpetroni

I think this behavior is expected.

prefer-robust-statements works on a single file level. The rule ensures that the statements in a file can be run multiple times in case of a partial failure.

If a file only has one statement, it can be run multiple times without issue.

If there are multiple statements, then more care is needed. Either wrapping a transaction or adding an if not exists clause

febpetroni commented 7 months ago

Hi @chdsbd , thanks for the quick response!

I think I understand what you mean... if you're using a migration tool, like flyway, a one-line migration will either work and be persisted (so that it won't actually run again), or will fail and must be fixed; meanwhile for migrations with more than one line and without transactional scope, we will have partial commits that won't let us run the whole migration again.

I had the impression that prefer-robust-statements would ask to use if exists / if not exists as a good practice, regardless of the number of lines in the migration. Thanks for clarifying!