From 0ac3fa409a0e019226250bbae4b0fae22aa156f5 Mon Sep 17 00:00:00 2001 From: Filipe Roque Date: Fri, 4 Apr 2025 23:50:33 +0100 Subject: [PATCH] Add rules to ban domains with constraints --- .gitignore | 5 +- BUILDING.md | 6 +-- README.md | 4 +- .../file1-ok.sql | 1 + .../file2-ok.sql | 1 + .../file3-ok.sql | 1 + .../file4-ok.sql | 1 + .../file5-ok.sql | 1 + .../file6-ok.sql | 1 + .../file7-ok.sql | 1 + .../file8-ok.sql | 1 + .../file9.sql | 1 + .../file1-ok.sql | 1 + .../file2.sql | 1 + .../postgres/PostgresSqlQualityProfile.java | 4 ++ .../postgres/PostgresSqlRulesDefinition.java | 16 ++++++ .../visitors/AbstractVisitorCheck.java | 8 +++ .../BanAlterDomainWithConstraintCheck.java | 26 ++++++++++ .../BanCreateDomainWithConstraintCheck.java | 26 ++++++++++ .../sonar/postgres/visitors/VisitorCheck.java | 2 + .../ban-alter-domain-with-add-constraint.md | 32 ++++++++++++ .../ban-create-domain-with-constraint.md | 32 ++++++++++++ .../sonar/postgres/PostgresSqlSensorTest.java | 49 +++++++++++++++++++ 23 files changed, 216 insertions(+), 5 deletions(-) create mode 100644 samples/ban-alter-domain-with-add-constraint/file1-ok.sql create mode 100644 samples/ban-alter-domain-with-add-constraint/file2-ok.sql create mode 100644 samples/ban-alter-domain-with-add-constraint/file3-ok.sql create mode 100644 samples/ban-alter-domain-with-add-constraint/file4-ok.sql create mode 100644 samples/ban-alter-domain-with-add-constraint/file5-ok.sql create mode 100644 samples/ban-alter-domain-with-add-constraint/file6-ok.sql create mode 100644 samples/ban-alter-domain-with-add-constraint/file7-ok.sql create mode 100644 samples/ban-alter-domain-with-add-constraint/file8-ok.sql create mode 100644 samples/ban-alter-domain-with-add-constraint/file9.sql create mode 100644 samples/ban-create-domain-with-constraint/file1-ok.sql create mode 100644 samples/ban-create-domain-with-constraint/file2.sql create mode 100644 src/main/java/com/premiumminds/sonar/postgres/visitors/BanAlterDomainWithConstraintCheck.java create mode 100644 src/main/java/com/premiumminds/sonar/postgres/visitors/BanCreateDomainWithConstraintCheck.java create mode 100644 src/main/resources/com/premiumminds/sonar/postgres/ban-alter-domain-with-add-constraint.md create mode 100644 src/main/resources/com/premiumminds/sonar/postgres/ban-create-domain-with-constraint.md diff --git a/.gitignore b/.gitignore index b425f09..5fbab0e 100644 --- a/.gitignore +++ b/.gitignore @@ -32,4 +32,7 @@ build/ .vscode/ ### Mac OS ### -.DS_Store \ No newline at end of file +.DS_Store + +/sonar-scanner-* +/samples/.scannerwork/ diff --git a/BUILDING.md b/BUILDING.md index 9d8fdbf..4b42973 100644 --- a/BUILDING.md +++ b/BUILDING.md @@ -46,7 +46,7 @@ docker run -d \ --name sonarqube \ -e SONAR_ES_BOOTSTRAP_CHECKS_DISABLE=true \ -p 9000:9000 \ - -v $(pwd)/target/sonar-postgres-plugin-1.2-SNAPSHOT.jar:/opt/sonarqube/extensions/plugins/sonar-postgres-plugin-1.2-SNAPSHOT.jar \ + -v $(pwd)/target/sonar-postgres-plugin-1.4-SNAPSHOT.jar:/opt/sonarqube/extensions/plugins/sonar-postgres-plugin-1.4-SNAPSHOT.jar \ sonarqube:lts-community xdg-open http://localhost:9000/ docker logs -f sonarqube @@ -66,8 +66,8 @@ mvn sonar:sonar \ ```shell wget https://binaries.sonarsource.com/Distribution/sonar-scanner-cli/sonar-scanner-cli-5.0.1.3006-linux.zip unzip sonar-scanner-cli-5.0.1.3006-linux.zip -cd sonar-scanner-5.0.1.3006-linux -bin/sonar-scanner \ +cd samples +../sonar-scanner-5.0.1.3006-linux/bin/sonar-scanner \ -Dsonar.login=admin \ -Dsonar.password=admin1 \ -Dsonar.host.url=http://localhost:9000 \ diff --git a/README.md b/README.md index 992ace0..6304b9e 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,8 @@ Check [BUILDING.md](BUILDING.md) * [adding-serial-primary-key-field](src/main/resources/com/premiumminds/sonar/postgres/adding-serial-primary-key-field.md) * [ban-char-field](src/main/resources/com/premiumminds/sonar/postgres/ban-char-field.md) * [ban-drop-database](src/main/resources/com/premiumminds/sonar/postgres/ban-drop-database.md) + * [ban-alter-domain-with-add-constraint](src/main/resources/com/premiumminds/sonar/postgres/ban-alter-domain-with-add-constraint.md) + * [ban-create-domain-with-constraint](src/main/resources/com/premiumminds/sonar/postgres/ban-create-domain-with-constraint.md) * [changing-column-type](src/main/resources/com/premiumminds/sonar/postgres/changing-column-type.md) * [cluster](src/main/resources/com/premiumminds/sonar/postgres/cluster.md) * [concurrently](src/main/resources/com/premiumminds/sonar/postgres/concurrently.md) @@ -38,4 +40,4 @@ Check [BUILDING.md](BUILDING.md) * [vacuum-full](src/main/resources/com/premiumminds/sonar/postgres/vacuum-full.md) ## Acknowledgements - * [Squawk](https://squawkhq.com/) \ No newline at end of file + * [Squawk](https://squawkhq.com/) diff --git a/samples/ban-alter-domain-with-add-constraint/file1-ok.sql b/samples/ban-alter-domain-with-add-constraint/file1-ok.sql new file mode 100644 index 0000000..d7d97a8 --- /dev/null +++ b/samples/ban-alter-domain-with-add-constraint/file1-ok.sql @@ -0,0 +1 @@ +ALTER DOMAIN domain_name_1 SET DEFAULT 1; diff --git a/samples/ban-alter-domain-with-add-constraint/file2-ok.sql b/samples/ban-alter-domain-with-add-constraint/file2-ok.sql new file mode 100644 index 0000000..654d382 --- /dev/null +++ b/samples/ban-alter-domain-with-add-constraint/file2-ok.sql @@ -0,0 +1 @@ +ALTER DOMAIN domain_name_2 SET NOT NULL; diff --git a/samples/ban-alter-domain-with-add-constraint/file3-ok.sql b/samples/ban-alter-domain-with-add-constraint/file3-ok.sql new file mode 100644 index 0000000..f00efc4 --- /dev/null +++ b/samples/ban-alter-domain-with-add-constraint/file3-ok.sql @@ -0,0 +1 @@ +ALTER DOMAIN domain_name_3 DROP CONSTRAINT other_domain_name; diff --git a/samples/ban-alter-domain-with-add-constraint/file4-ok.sql b/samples/ban-alter-domain-with-add-constraint/file4-ok.sql new file mode 100644 index 0000000..601a1be --- /dev/null +++ b/samples/ban-alter-domain-with-add-constraint/file4-ok.sql @@ -0,0 +1 @@ +ALTER DOMAIN domain_name_4 RENAME CONSTRAINT constraint_name TO other_constraint_name; diff --git a/samples/ban-alter-domain-with-add-constraint/file5-ok.sql b/samples/ban-alter-domain-with-add-constraint/file5-ok.sql new file mode 100644 index 0000000..40a4d7d --- /dev/null +++ b/samples/ban-alter-domain-with-add-constraint/file5-ok.sql @@ -0,0 +1 @@ +ALTER DOMAIN domain_name_5 RENAME TO other_domain_name; diff --git a/samples/ban-alter-domain-with-add-constraint/file6-ok.sql b/samples/ban-alter-domain-with-add-constraint/file6-ok.sql new file mode 100644 index 0000000..e022e9e --- /dev/null +++ b/samples/ban-alter-domain-with-add-constraint/file6-ok.sql @@ -0,0 +1 @@ +ALTER DOMAIN domain_name_6 VALIDATE CONSTRAINT constraint_name; diff --git a/samples/ban-alter-domain-with-add-constraint/file7-ok.sql b/samples/ban-alter-domain-with-add-constraint/file7-ok.sql new file mode 100644 index 0000000..35636ab --- /dev/null +++ b/samples/ban-alter-domain-with-add-constraint/file7-ok.sql @@ -0,0 +1 @@ +ALTER DOMAIN domain_name_7 OWNER TO you; diff --git a/samples/ban-alter-domain-with-add-constraint/file8-ok.sql b/samples/ban-alter-domain-with-add-constraint/file8-ok.sql new file mode 100644 index 0000000..e5d334b --- /dev/null +++ b/samples/ban-alter-domain-with-add-constraint/file8-ok.sql @@ -0,0 +1 @@ +ALTER DOMAIN domain_name_8 SET SCHEMA foo; diff --git a/samples/ban-alter-domain-with-add-constraint/file9.sql b/samples/ban-alter-domain-with-add-constraint/file9.sql new file mode 100644 index 0000000..41a056f --- /dev/null +++ b/samples/ban-alter-domain-with-add-constraint/file9.sql @@ -0,0 +1 @@ +ALTER DOMAIN domain_name ADD CONSTRAINT constraint_name CHECK (value > 0); diff --git a/samples/ban-create-domain-with-constraint/file1-ok.sql b/samples/ban-create-domain-with-constraint/file1-ok.sql new file mode 100644 index 0000000..0e6ddac --- /dev/null +++ b/samples/ban-create-domain-with-constraint/file1-ok.sql @@ -0,0 +1 @@ +CREATE DOMAIN domain_name_1 AS TEXT; diff --git a/samples/ban-create-domain-with-constraint/file2.sql b/samples/ban-create-domain-with-constraint/file2.sql new file mode 100644 index 0000000..9a25d10 --- /dev/null +++ b/samples/ban-create-domain-with-constraint/file2.sql @@ -0,0 +1 @@ +CREATE DOMAIN domain_name_3 AS NUMERIC(15,5) CHECK (value > 0); diff --git a/src/main/java/com/premiumminds/sonar/postgres/PostgresSqlQualityProfile.java b/src/main/java/com/premiumminds/sonar/postgres/PostgresSqlQualityProfile.java index b520ce5..de77c16 100644 --- a/src/main/java/com/premiumminds/sonar/postgres/PostgresSqlQualityProfile.java +++ b/src/main/java/com/premiumminds/sonar/postgres/PostgresSqlQualityProfile.java @@ -6,7 +6,9 @@ import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_ADDING_SERIAL_PRIMARY_KEY_FIELD; import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_ADD_FIELD_WITH_DEFAULT; import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_ADD_FOREIGN_KEY; +import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_BAN_ALTER_DOMAIN_WITH_CONSTRAINT; import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_BAN_CHAR_FIELD; +import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_BAN_CREATE_DOMAIN_WITH_CONSTRAINT; import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_BAN_DROP_DATABASE; import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_CHANGING_COLUMN_TYPE; import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_CLUSTER; @@ -42,6 +44,8 @@ public void define(Context context) { activateRule(profile, RULE_SETTING_NOT_NULLABLE_FIELD); activateRule(profile, RULE_ADDING_SERIAL_PRIMARY_KEY_FIELD); activateRule(profile, RULE_BAN_CHAR_FIELD); + activateRule(profile, RULE_BAN_ALTER_DOMAIN_WITH_CONSTRAINT); + activateRule(profile, RULE_BAN_CREATE_DOMAIN_WITH_CONSTRAINT); activateRule(profile, RULE_BAN_DROP_DATABASE); activateRule(profile, RULE_CHANGING_COLUMN_TYPE); activateRule(profile, RULE_CONSTRAINT_MISSING_NOT_VALID); diff --git a/src/main/java/com/premiumminds/sonar/postgres/PostgresSqlRulesDefinition.java b/src/main/java/com/premiumminds/sonar/postgres/PostgresSqlRulesDefinition.java index 422cbe4..3eec065 100644 --- a/src/main/java/com/premiumminds/sonar/postgres/PostgresSqlRulesDefinition.java +++ b/src/main/java/com/premiumminds/sonar/postgres/PostgresSqlRulesDefinition.java @@ -6,7 +6,9 @@ import com.premiumminds.sonar.postgres.visitors.AddFieldWithDefaultVisitorCheck; import com.premiumminds.sonar.postgres.visitors.AddForeignKeyVisitorCheck; import com.premiumminds.sonar.postgres.visitors.AddingSerialPrimaryKeyfieldvisitorCheck; +import com.premiumminds.sonar.postgres.visitors.BanAlterDomainWithConstraintCheck; import com.premiumminds.sonar.postgres.visitors.BanCharFieldVisitorCheck; +import com.premiumminds.sonar.postgres.visitors.BanCreateDomainWithConstraintCheck; import com.premiumminds.sonar.postgres.visitors.BanDropDatabaseVisitorCheck; import com.premiumminds.sonar.postgres.visitors.ChangingColumnTypeVisitorCheck; import com.premiumminds.sonar.postgres.visitors.ClusterVisitorCheck; @@ -42,6 +44,8 @@ public class PostgresSqlRulesDefinition implements RulesDefinition { public static final RuleKey RULE_SETTING_NOT_NULLABLE_FIELD = RuleKey.of(REPOSITORY, "setting-not-nullable-field"); public static final RuleKey RULE_ADDING_SERIAL_PRIMARY_KEY_FIELD = RuleKey.of(REPOSITORY, "adding-serial-primary-key-field"); public static final RuleKey RULE_BAN_CHAR_FIELD = RuleKey.of(REPOSITORY, "ban-char-field"); + public static final RuleKey RULE_BAN_CREATE_DOMAIN_WITH_CONSTRAINT = RuleKey.of(REPOSITORY, "ban-create-domain-with-constraint"); + public static final RuleKey RULE_BAN_ALTER_DOMAIN_WITH_CONSTRAINT = RuleKey.of(REPOSITORY, "ban-alter-domain-with-add-constraint"); public static final RuleKey RULE_BAN_DROP_DATABASE = RuleKey.of(REPOSITORY, "ban-drop-database"); public static final RuleKey RULE_CHANGING_COLUMN_TYPE = RuleKey.of(REPOSITORY, "changing-column-type"); public static final RuleKey RULE_CONSTRAINT_MISSING_NOT_VALID = RuleKey.of(REPOSITORY, "constraint-missing-not-valid"); @@ -105,6 +109,16 @@ public void define(Context context) { .setType(RuleType.BUG) .setMarkdownDescription(getClass().getResource("ban-char-field.md")); + repository.createRule(RULE_BAN_CREATE_DOMAIN_WITH_CONSTRAINT.rule()) + .setName("ban-create-domain-with-constraint") + .setType(RuleType.BUG) + .setMarkdownDescription(getClass().getResource("ban-create-domain-with-constraint.md")); + + repository.createRule(RULE_BAN_ALTER_DOMAIN_WITH_CONSTRAINT.rule()) + .setName("ban-alter-domain-with-add-constraint") + .setType(RuleType.BUG) + .setMarkdownDescription(getClass().getResource("ban-alter-domain-with-add-constraint.md")); + repository.createRule(RULE_BAN_DROP_DATABASE.rule()) .setName("ban-drop-database rule") .setType(RuleType.BUG) @@ -204,6 +218,8 @@ public static List allChecks(){ new AddForeignKeyVisitorCheck(), new AddFieldWithDefaultVisitorCheck(), new BanCharFieldVisitorCheck(), + new BanCreateDomainWithConstraintCheck(), + new BanAlterDomainWithConstraintCheck(), new PreferTextFieldVisitorCheck(), new VacuumFullVisitorCheck(), new ClusterVisitorCheck(), diff --git a/src/main/java/com/premiumminds/sonar/postgres/visitors/AbstractVisitorCheck.java b/src/main/java/com/premiumminds/sonar/postgres/visitors/AbstractVisitorCheck.java index 0b118c3..48af092 100644 --- a/src/main/java/com/premiumminds/sonar/postgres/visitors/AbstractVisitorCheck.java +++ b/src/main/java/com/premiumminds/sonar/postgres/visitors/AbstractVisitorCheck.java @@ -12,6 +12,7 @@ import com.premiumminds.sonar.postgres.protobuf.ClusterStmt; import com.premiumminds.sonar.postgres.protobuf.ColumnDef; import com.premiumminds.sonar.postgres.protobuf.Constraint; +import com.premiumminds.sonar.postgres.protobuf.CreateDomainStmt; import com.premiumminds.sonar.postgres.protobuf.CreateExtensionStmt; import com.premiumminds.sonar.postgres.protobuf.CreateFunctionStmt; import com.premiumminds.sonar.postgres.protobuf.CreateSchemaStmt; @@ -112,6 +113,11 @@ public void visit(CreateSchemaStmt createSchemaStmt) { } + @Override + public void visit(CreateDomainStmt createDomainStmt) { + + } + @Override public void visit(AlterDomainStmt alterDomainStmt) { @@ -253,6 +259,8 @@ public void analyze(RawStmt rawStmt) { visit(stmt.getAlterSeqStmt()); } else if (stmt.hasCreateSchemaStmt()){ visit(stmt.getCreateSchemaStmt()); + } else if (stmt.hasCreateDomainStmt()) { + visit(stmt.getCreateDomainStmt()); } else if (stmt.hasAlterDomainStmt()) { visit(stmt.getAlterDomainStmt()); } else if (stmt.hasCreateTableAsStmt()){ diff --git a/src/main/java/com/premiumminds/sonar/postgres/visitors/BanAlterDomainWithConstraintCheck.java b/src/main/java/com/premiumminds/sonar/postgres/visitors/BanAlterDomainWithConstraintCheck.java new file mode 100644 index 0000000..4d0e436 --- /dev/null +++ b/src/main/java/com/premiumminds/sonar/postgres/visitors/BanAlterDomainWithConstraintCheck.java @@ -0,0 +1,26 @@ +package com.premiumminds.sonar.postgres.visitors; + +import com.premiumminds.sonar.postgres.protobuf.AlterDomainStmt; +import org.sonar.api.rule.RuleKey; +import org.sonar.check.Rule; + +import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_BAN_ALTER_DOMAIN_WITH_CONSTRAINT; + +@Rule(key = "ban-alter-domain-with-add-constraint") +public class BanAlterDomainWithConstraintCheck extends AbstractVisitorCheck { + + @Override + public void visit(AlterDomainStmt alterDomainStmt) { + + if (alterDomainStmt.getSubtype().equals("C")){ + newIssue("Domains with constraints have poor support for online migrations"); + } + + super.visit(alterDomainStmt); + } + + @Override + protected RuleKey getRule() { + return RULE_BAN_ALTER_DOMAIN_WITH_CONSTRAINT; + } +} diff --git a/src/main/java/com/premiumminds/sonar/postgres/visitors/BanCreateDomainWithConstraintCheck.java b/src/main/java/com/premiumminds/sonar/postgres/visitors/BanCreateDomainWithConstraintCheck.java new file mode 100644 index 0000000..75f457c --- /dev/null +++ b/src/main/java/com/premiumminds/sonar/postgres/visitors/BanCreateDomainWithConstraintCheck.java @@ -0,0 +1,26 @@ +package com.premiumminds.sonar.postgres.visitors; + +import com.premiumminds.sonar.postgres.protobuf.CreateDomainStmt; +import org.sonar.api.rule.RuleKey; +import org.sonar.check.Rule; + +import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_BAN_CREATE_DOMAIN_WITH_CONSTRAINT; + +@Rule(key = "ban-create-domain-with-constraint") +public class BanCreateDomainWithConstraintCheck extends AbstractVisitorCheck { + + @Override + public void visit(CreateDomainStmt createDomainStmt) { + + if (createDomainStmt.getConstraintsCount() > 0){ + newIssue("Domains with constraints have poor support for online migrations"); + } + + super.visit(createDomainStmt); + } + + @Override + protected RuleKey getRule() { + return RULE_BAN_CREATE_DOMAIN_WITH_CONSTRAINT; + } +} diff --git a/src/main/java/com/premiumminds/sonar/postgres/visitors/VisitorCheck.java b/src/main/java/com/premiumminds/sonar/postgres/visitors/VisitorCheck.java index 1e940b5..d6eca78 100644 --- a/src/main/java/com/premiumminds/sonar/postgres/visitors/VisitorCheck.java +++ b/src/main/java/com/premiumminds/sonar/postgres/visitors/VisitorCheck.java @@ -9,6 +9,7 @@ import com.premiumminds.sonar.postgres.protobuf.ClusterStmt; import com.premiumminds.sonar.postgres.protobuf.ColumnDef; import com.premiumminds.sonar.postgres.protobuf.Constraint; +import com.premiumminds.sonar.postgres.protobuf.CreateDomainStmt; import com.premiumminds.sonar.postgres.protobuf.CreateExtensionStmt; import com.premiumminds.sonar.postgres.protobuf.CreateFunctionStmt; import com.premiumminds.sonar.postgres.protobuf.CreateSchemaStmt; @@ -46,6 +47,7 @@ public interface VisitorCheck { void visit(CreateTableAsStmt createTableAsStmt); void visit(CreateStmt createStmt); void visit(CreateSchemaStmt createSchemaStmt); + void visit(CreateDomainStmt createDomainStmt); void visit(AlterDomainStmt alterDomainStmt); void visit(AlterEnumStmt alterEnumStmt); void visit(AlterTableCmd alterTableCmd); diff --git a/src/main/resources/com/premiumminds/sonar/postgres/ban-alter-domain-with-add-constraint.md b/src/main/resources/com/premiumminds/sonar/postgres/ban-alter-domain-with-add-constraint.md new file mode 100644 index 0000000..e7be3ea --- /dev/null +++ b/src/main/resources/com/premiumminds/sonar/postgres/ban-alter-domain-with-add-constraint.md @@ -0,0 +1,32 @@ +== problem + +Postgres [domains](https://www.postgresql.org/docs/current/sql-createdomain.html), which associate a data type with an optional check constraint, have poor support for online migrations +when associated with a check constraint. + +The purpose of domains is to make the named type-plus-constraint reusable, but this means that any change to the domain's constraint +requires _all_ columns that use the domain to be revalidated. And, because Postgres can't reason well about arbitrary constraints, +they increase the chances of a change requiring an expensive table rewrite. + +A couple relevant quotes from a Postgres developer include: + +> No, that's not going to work: coercing to a domain that has any +> constraints is considered to require a rewrite. + +And: + +> In any case, the point remains that domains are pretty inefficient +> compared to native types like varchar(12); partly because the system +> can’t reason very well about arbitrary check constraints as compared +> to simple length constraints, and partly because the whole feature +> just isn’t implemented very completely or efficiently. So you’ll be +> paying *a lot* for some hypothetical future savings. + + +== solution + +Either avoid domains altogether, or (most importantly) avoid adding constraints to domains. Instead, put the [constraint](https://www.postgresql.org/docs/current/sql-createdomain.html) +on the desired column(s) directly. + +== links + +[The mailing list thread from which the above quotes are sourced](https://www.postgresql.org/message-id/flat/CADVWZZKjhV9fLpewPdQMZx7V6kvGJViwMEDrPAv9m50rGeK9UA%40mail.gmail.com) diff --git a/src/main/resources/com/premiumminds/sonar/postgres/ban-create-domain-with-constraint.md b/src/main/resources/com/premiumminds/sonar/postgres/ban-create-domain-with-constraint.md new file mode 100644 index 0000000..e7be3ea --- /dev/null +++ b/src/main/resources/com/premiumminds/sonar/postgres/ban-create-domain-with-constraint.md @@ -0,0 +1,32 @@ +== problem + +Postgres [domains](https://www.postgresql.org/docs/current/sql-createdomain.html), which associate a data type with an optional check constraint, have poor support for online migrations +when associated with a check constraint. + +The purpose of domains is to make the named type-plus-constraint reusable, but this means that any change to the domain's constraint +requires _all_ columns that use the domain to be revalidated. And, because Postgres can't reason well about arbitrary constraints, +they increase the chances of a change requiring an expensive table rewrite. + +A couple relevant quotes from a Postgres developer include: + +> No, that's not going to work: coercing to a domain that has any +> constraints is considered to require a rewrite. + +And: + +> In any case, the point remains that domains are pretty inefficient +> compared to native types like varchar(12); partly because the system +> can’t reason very well about arbitrary check constraints as compared +> to simple length constraints, and partly because the whole feature +> just isn’t implemented very completely or efficiently. So you’ll be +> paying *a lot* for some hypothetical future savings. + + +== solution + +Either avoid domains altogether, or (most importantly) avoid adding constraints to domains. Instead, put the [constraint](https://www.postgresql.org/docs/current/sql-createdomain.html) +on the desired column(s) directly. + +== links + +[The mailing list thread from which the above quotes are sourced](https://www.postgresql.org/message-id/flat/CADVWZZKjhV9fLpewPdQMZx7V6kvGJViwMEDrPAv9m50rGeK9UA%40mail.gmail.com) diff --git a/src/test/java/com/premiumminds/sonar/postgres/PostgresSqlSensorTest.java b/src/test/java/com/premiumminds/sonar/postgres/PostgresSqlSensorTest.java index 9f13bbf..b2f0095 100644 --- a/src/test/java/com/premiumminds/sonar/postgres/PostgresSqlSensorTest.java +++ b/src/test/java/com/premiumminds/sonar/postgres/PostgresSqlSensorTest.java @@ -23,7 +23,9 @@ import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_ADDING_SERIAL_PRIMARY_KEY_FIELD; import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_ADD_FIELD_WITH_DEFAULT; import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_ADD_FOREIGN_KEY; +import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_BAN_ALTER_DOMAIN_WITH_CONSTRAINT; import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_BAN_CHAR_FIELD; +import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_BAN_CREATE_DOMAIN_WITH_CONSTRAINT; import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_BAN_DROP_DATABASE; import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_CHANGING_COLUMN_TYPE; import static com.premiumminds.sonar.postgres.PostgresSqlRulesDefinition.RULE_CLUSTER; @@ -93,6 +95,8 @@ void testAllRules() { RULE_ADDING_SERIAL_PRIMARY_KEY_FIELD, RULE_BAN_CHAR_FIELD, RULE_BAN_DROP_DATABASE, + RULE_BAN_CREATE_DOMAIN_WITH_CONSTRAINT, + RULE_BAN_ALTER_DOMAIN_WITH_CONSTRAINT, RULE_CHANGING_COLUMN_TYPE, RULE_CONSTRAINT_MISSING_NOT_VALID, RULE_DISALLOWED_UNIQUE_CONSTRAINT, @@ -890,6 +894,51 @@ void onlyLowerCaseNames() { assertEquals(4, fileMap.size()); } + @Test + void banCreateDomainWithConstraints() { + createFile(contextTester, "file1-ok.sql", "CREATE DOMAIN domain_name_1 AS TEXT;"); + createFile(contextTester, "file2.sql", "CREATE DOMAIN domain_name_3 AS NUMERIC(15,5) CHECK (value > 0);"); + + final RuleKey rule = RULE_BAN_CREATE_DOMAIN_WITH_CONSTRAINT; + PostgresSqlSensor sensor = getPostgresSqlSensor(rule); + sensor.execute(contextTester); + + final Map> issueMap = groupByRuleAndFile(contextTester.allIssues()); + + final Map fileMap = issueMap.get(rule); + + assertEquals("Domains with constraints have poor support for online migrations", + fileMap.get(":file2.sql").primaryLocation().message()); + + assertEquals(1, fileMap.size()); + } + + @Test + void banAlterDomainWithConstraints() { + createFile(contextTester, "file1-ok.sql", "ALTER DOMAIN domain_name_1 SET DEFAULT 1;"); + createFile(contextTester, "file2-ok.sql", "ALTER DOMAIN domain_name_2 SET NOT NULL;"); + createFile(contextTester, "file3-ok.sql", "ALTER DOMAIN domain_name_3 DROP CONSTRAINT other_domain_name;"); + createFile(contextTester, "file4-ok.sql", "ALTER DOMAIN domain_name_4 RENAME CONSTRAINT constraint_name TO other_constraint_name;"); + createFile(contextTester, "file5-ok.sql", "ALTER DOMAIN domain_name_5 RENAME TO other_domain_name;"); + createFile(contextTester, "file6-ok.sql", "ALTER DOMAIN domain_name_6 VALIDATE CONSTRAINT constraint_name;"); + createFile(contextTester, "file7-ok.sql", "ALTER DOMAIN domain_name_7 OWNER TO you;"); + createFile(contextTester, "file8-ok.sql", "ALTER DOMAIN domain_name_8 SET SCHEMA foo;"); + createFile(contextTester, "file9.sql", "ALTER DOMAIN domain_name ADD CONSTRAINT constraint_name CHECK (value > 0);"); + + final RuleKey rule = RULE_BAN_ALTER_DOMAIN_WITH_CONSTRAINT; + PostgresSqlSensor sensor = getPostgresSqlSensor(rule); + sensor.execute(contextTester); + + final Map> issueMap = groupByRuleAndFile(contextTester.allIssues()); + + final Map fileMap = issueMap.get(rule); + + assertEquals("Domains with constraints have poor support for online migrations", + fileMap.get(":file9.sql").primaryLocation().message()); + + assertEquals(1, fileMap.size()); + } + private PostgresSqlSensor getPostgresSqlSensor(RuleKey... ruleKey) { ActiveRulesBuilder activeRulesBuilder = new ActiveRulesBuilder();