Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,7 @@ build/
.vscode/

### Mac OS ###
.DS_Store
.DS_Store

/sonar-scanner-*
/samples/.scannerwork/
6 changes: 3 additions & 3 deletions BUILDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 \
Expand Down
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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/)
* [Squawk](https://squawkhq.com/)
1 change: 1 addition & 0 deletions samples/ban-alter-domain-with-add-constraint/file1-ok.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER DOMAIN domain_name_1 SET DEFAULT 1;
1 change: 1 addition & 0 deletions samples/ban-alter-domain-with-add-constraint/file2-ok.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER DOMAIN domain_name_2 SET NOT NULL;
1 change: 1 addition & 0 deletions samples/ban-alter-domain-with-add-constraint/file3-ok.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER DOMAIN domain_name_3 DROP CONSTRAINT other_domain_name;
1 change: 1 addition & 0 deletions samples/ban-alter-domain-with-add-constraint/file4-ok.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER DOMAIN domain_name_4 RENAME CONSTRAINT constraint_name TO other_constraint_name;
1 change: 1 addition & 0 deletions samples/ban-alter-domain-with-add-constraint/file5-ok.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER DOMAIN domain_name_5 RENAME TO other_domain_name;
1 change: 1 addition & 0 deletions samples/ban-alter-domain-with-add-constraint/file6-ok.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER DOMAIN domain_name_6 VALIDATE CONSTRAINT constraint_name;
1 change: 1 addition & 0 deletions samples/ban-alter-domain-with-add-constraint/file7-ok.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER DOMAIN domain_name_7 OWNER TO you;
1 change: 1 addition & 0 deletions samples/ban-alter-domain-with-add-constraint/file8-ok.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER DOMAIN domain_name_8 SET SCHEMA foo;
1 change: 1 addition & 0 deletions samples/ban-alter-domain-with-add-constraint/file9.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER DOMAIN domain_name ADD CONSTRAINT constraint_name CHECK (value > 0);
1 change: 1 addition & 0 deletions samples/ban-create-domain-with-constraint/file1-ok.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CREATE DOMAIN domain_name_1 AS TEXT;
1 change: 1 addition & 0 deletions samples/ban-create-domain-with-constraint/file2.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CREATE DOMAIN domain_name_3 AS NUMERIC(15,5) CHECK (value > 0);
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -204,6 +218,8 @@ public static List<VisitorCheck> allChecks(){
new AddForeignKeyVisitorCheck(),
new AddFieldWithDefaultVisitorCheck(),
new BanCharFieldVisitorCheck(),
new BanCreateDomainWithConstraintCheck(),
new BanAlterDomainWithConstraintCheck(),
new PreferTextFieldVisitorCheck(),
new VacuumFullVisitorCheck(),
new ClusterVisitorCheck(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -112,6 +113,11 @@ public void visit(CreateSchemaStmt createSchemaStmt) {

}

@Override
public void visit(CreateDomainStmt createDomainStmt) {

}

@Override
public void visit(AlterDomainStmt alterDomainStmt) {

Expand Down Expand Up @@ -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()){
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
Original file line number Diff line number Diff line change
@@ -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)
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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<RuleKey, Map<String, Issue>> issueMap = groupByRuleAndFile(contextTester.allIssues());

final Map<String, Issue> 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<RuleKey, Map<String, Issue>> issueMap = groupByRuleAndFile(contextTester.allIssues());

final Map<String, Issue> 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();

Expand Down