Skip to content

fix(sql-check): failed to detect pk dropping when drop it as constraint #1879

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.

Sign up for GitHub

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

Merged
yhilmare merged 3 commits into from
Mar 18, 2024
Merged

fix(sql-check): failed to detect pk dropping when drop it as constraint #1879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
75a1da4
fix errror and add i18n tips
yhilmare Mar 13, 2024
be4dcdd
fix sql check unit test error
yhilmare Mar 14, 2024
6b6cf72
fix npe error
yhilmare Mar 14, 2024
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
2 changes: 1 addition & 1 deletion server/odc-core/src/main/resources/i18n/BusinessMessages.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,7 @@ com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.object-name-using-r
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.object-name-using-reserved-words.name=Reserved words should not be used as object names
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.object-name-using-reserved-words.description=Reserved words should not be used as object names

com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.offline-schema-change-exists.message=An offline structure change statement is detected, which may take a long time. It is recommended to use lock-free structure change to execute the statement.
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.offline-schema-change-exists.message={0} is an offline structure change statement, which may take a long time. It is recommended to use lock-free structure change to execute this statement.
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.offline-schema-change-exists.name=Offline structure change statement exists
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.offline-schema-change-exists.description=The offline structure change statement may involve full data modification, which will take a long time and may affect online business. It is recommended to use lock-free structure change to perform this operation.

Expand Down
2 changes: 1 addition & 1 deletion server/odc-core/src/main/resources/i18n/BusinessMessages_zh_CN.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.object-name-using-r
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.object-name-using-reserved-words.name=保留字不应该作为对象名
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.object-name-using-reserved-words.description=保留字不应该作为对象名

com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.offline-schema-change-exists.message=检测到 offline 结构变更语句，可能耗时较久，建议使用无锁结构变更执行该语句
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.offline-schema-change-exists.message={0} 是 offline 结构变更语句，可能耗时较久，建议使用无锁结构变更执行该语句
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.offline-schema-change-exists.name=offline 结构变更语句存在
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.offline-schema-change-exists.description=offline 结构变更语句可能涉及全量数据修改，会造成比较久耗时，可能会影响到线上业务，推荐使用无锁结构变更执行该操作

Expand Down
2 changes: 1 addition & 1 deletion server/odc-core/src/main/resources/i18n/BusinessMessages_zh_TW.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.object-name-using-r
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.object-name-using-reserved-words.name=保留字不應該作為對象名
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.object-name-using-reserved-words.description=保留字不應該作為對象名

com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.offline-schema-change-exists.message=偵測到 offline 結構變更語句，可能耗時較久，建議使用無鎖定結構變更執行該語句
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.offline-schema-change-exists.message={0} 是 offline 結構變更語句，可能耗時較久，建議使用無鎖定結構變更執行該語句
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.offline-schema-change-exists.name=offline 結構變更語句存在
com.oceanbase.odc.builtin-resource.regulation.rule.sql-check.offline-schema-change-exists.description=offline 結構變更語句可能涉及全量資料修改，會造成比較久耗時，可能會影響線上業務，推薦使用無鎖定結構變更執行此操作

Expand Down
22 changes: 12 additions & 10 deletions ...c/main/java/com/oceanbase/odc/service/sqlcheck/rule/BaseObjectNameUsingReservedWords.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,18 @@ public List<CheckViolation> check(@NonNull Statement statement, @NonNull SqlChec
statement, getType(), new Object[] {createTable.getTableName()}));
}
violations.addAll(builds(statement, createTable.getPartition()));
violations.addAll(createTable.getTableElements().stream().flatMap(tableElement -> {
if (tableElement instanceof ColumnDefinition) {
return builds(statement, (ColumnDefinition) tableElement).stream();
} else if (tableElement instanceof OutOfLineIndex) {
return builds(statement, (OutOfLineIndex) tableElement).stream();
} else if (tableElement instanceof OutOfLineConstraint) {
return builds(statement, (OutOfLineConstraint) tableElement).stream();
}
return Stream.empty();
}).collect(Collectors.toList()));
if (CollectionUtils.isNotEmpty(createTable.getTableElements())) {
violations.addAll(createTable.getTableElements().stream().flatMap(tableElement -> {
if (tableElement instanceof ColumnDefinition) {
return builds(statement, (ColumnDefinition) tableElement).stream();
} else if (tableElement instanceof OutOfLineIndex) {
return builds(statement, (OutOfLineIndex) tableElement).stream();
} else if (tableElement instanceof OutOfLineConstraint) {
return builds(statement, (OutOfLineConstraint) tableElement).stream();
}
return Stream.empty();
}).collect(Collectors.toList()));
}
} else if (statement instanceof AlterTable) {
AlterTable alterTable = (AlterTable) statement;
violations.addAll(alterTable.getAlterTableActions().stream().flatMap(action -> {
Expand Down
109 changes: 85 additions & 24 deletions ...-service/src/main/java/com/oceanbase/odc/service/sqlcheck/rule/MySQLOfflineDdlExists.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Optional;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.apache.commons.collections4.CollectionUtils;
import org.springframework.jdbc.core.JdbcOperations;
Expand All @@ -42,6 +43,7 @@
import com.oceanbase.tools.sqlparser.statement.createtable.CreateTable;
import com.oceanbase.tools.sqlparser.statement.createtable.GenerateOption.Type;
import com.oceanbase.tools.sqlparser.statement.createtable.InLineConstraint;
import com.oceanbase.tools.sqlparser.statement.createtable.OutOfLineConstraint;
import com.oceanbase.tools.sqlparser.statement.expression.ColumnReference;
import com.oceanbase.tools.sqlparser.statement.truncate.TruncateTable;

Expand Down Expand Up @@ -79,25 +81,25 @@ public List<CheckViolation> check(@NonNull Statement statement, @NonNull SqlChec
violations.addAll(changeColumnInLocation(statement, action));
violations.addAll(addAutoIncrementColumn(statement, action));
violations.addAll(changeColumnToAutoIncrement(statement, createTable, action));
violations.addAll(changeColumnType(statement, createTable, action));
violations.addAll(changeColumnToPK(statement, action));
violations.addAll(changeColumnDataType(statement, createTable, action));
violations.addAll(changeColumnToPrimaryKey(createTable, statement, action));
violations.addAll(addOrDropStoredVirtualColumn(statement, action));
violations.addAll(dropColumn(statement, action));
violations.addAll(addOrDropPK(statement, action));
violations.addAll(addOrDropPrimaryKey(createTable, statement, action));
violations.addAll(changeCharsetOrCollation(statement, action));
violations.addAll(modifyPartition(statement, action));
violations.addAll(changePartition(statement, action));
violations.addAll(dropPartition(statement, action));
violations.addAll(truncatePartition(statement, action));
return violations.stream();
}).collect(Collectors.toList());
} else if (statement instanceof TruncateTable) {
return Collections.singletonList(SqlCheckUtil.buildViolation(statement.getText(),
statement, getType(), new Object[] {}));
statement, getType(), new Object[] {"TRUNCATE TABLE"}));
} else if (statement instanceof DropStatement) {
DropStatement dropStatement = (DropStatement) statement;
if ("TABLE".equals(dropStatement.getObjectType())) {
return Collections.singletonList(SqlCheckUtil.buildViolation(statement.getText(),
statement, getType(), new Object[] {}));
statement, getType(), new Object[] {"DROP TABLE"}));
}
}
return Collections.emptyList();
Expand All @@ -106,27 +108,29 @@ public List<CheckViolation> check(@NonNull Statement statement, @NonNull SqlChec
protected List<CheckViolation> addColumnInLocation(Statement statement, AlterTableAction action) {
return addColumn(action, definition -> {
if (definition.getLocation() != null) {
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(), new Object[] {});
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(),
new Object[] {"ADD COLUMN IN THE MIDDLE (BEFORE/AFTER/FIRST)"});
}
return null;
});
}

protected List<CheckViolation> changeColumnType(Statement statement, CreateTable target,
protected List<CheckViolation> changeColumnDataType(Statement statement, CreateTable target,
AlterTableAction action) {
return changeColumn(action, changed -> {
ColumnDefinition origin = extractColumnDefFrom(target, changed.getColumnReference());
if (origin == null || Objects.equals(origin.getDataType(), changed.getDataType())) {
return null;
}
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(), new Object[] {});
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(),
new Object[] {"MODIFY COLUMN DATA TYPE"});
});
}

protected List<CheckViolation> modifyPartition(Statement statement, AlterTableAction action) {
protected List<CheckViolation> changePartition(Statement statement, AlterTableAction action) {
if (action.getModifyPartition() != null) {
return Collections.singletonList(SqlCheckUtil.buildViolation(statement.getText(),
action, getType(), new Object[] {}));
action, getType(), new Object[] {"MODIFY PARTITION"}));
}
return Collections.emptyList();
}
Expand All @@ -135,7 +139,7 @@ protected List<CheckViolation> dropPartition(Statement statement, AlterTableActi
if (CollectionUtils.isNotEmpty(action.getDropPartitionNames())
|| CollectionUtils.isNotEmpty(action.getDropSubPartitionNames())) {
return Collections.singletonList(SqlCheckUtil.buildViolation(statement.getText(),
action, getType(), new Object[] {}));
action, getType(), new Object[] {"DROP PARTITION"}));
}
return Collections.emptyList();
}
Expand All @@ -144,15 +148,15 @@ protected List<CheckViolation> truncatePartition(Statement statement, AlterTable
if (CollectionUtils.isNotEmpty(action.getTruncatePartitionNames())
|| CollectionUtils.isNotEmpty(action.getTruncateSubPartitionNames())) {
return Collections.singletonList(SqlCheckUtil.buildViolation(statement.getText(),
action, getType(), new Object[] {}));
action, getType(), new Object[] {"TRUNCATE PARTITION"}));
}
return Collections.emptyList();
}

protected List<CheckViolation> dropColumn(Statement statement, AlterTableAction action) {
if (CollectionUtils.isNotEmpty(action.getDropColumns())) {
return Collections.singletonList(SqlCheckUtil.buildViolation(statement.getText(),
action, getType(), new Object[] {}));
action, getType(), new Object[] {"DROP COLUMN"}));
}
return Collections.emptyList();
}
Expand All @@ -161,7 +165,8 @@ protected List<CheckViolation> addAutoIncrementColumn(Statement statement, Alter
return addColumn(action, definition -> {
if (definition.getColumnAttributes() != null
&& Boolean.TRUE.equals(definition.getColumnAttributes().getAutoIncrement())) {
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(), new Object[] {});
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(),
new Object[] {"ADD AUTO-INCREMENT COLUMN"});
}
return null;
});
Expand All @@ -171,30 +176,61 @@ protected List<CheckViolation> addOrDropStoredVirtualColumn(Statement statement,
return addColumn(action, definition -> {
if (definition.getGenerateOption() != null
&& Type.STORED.equals(definition.getGenerateOption().getType())) {
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(), new Object[] {});
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(),
new Object[] {"ADD/DROP STORED GENERATED COLUMN"});
}
return null;
});
}

protected List<CheckViolation> addOrDropPK(Statement statement, AlterTableAction action) {
protected List<CheckViolation> addOrDropPrimaryKey(CreateTable createTable,
Statement statement, AlterTableAction action) {
List<CheckViolation> violations = new ArrayList<>();
if (action == null) {
return violations;
}
if (CollectionUtils.isNotEmpty(action.getDropConstraintNames()) && createTable != null) {
List<String> pkConstraintNames = createTable.getColumnDefinitions().stream().flatMap(d -> {
if (d.getColumnAttributes() == null) {
return Stream.empty();
}
return d.getColumnAttributes().getConstraints().stream()
.filter(c -> c.isPrimaryKey() && c.getConstraintName() != null)
.map(InLineConstraint::getConstraintName);
}).collect(Collectors.toList());
if (CollectionUtils.isNotEmpty(createTable.getConstraints())) {
pkConstraintNames.addAll(createTable.getConstraints().stream().flatMap(c -> {
if (!c.isPrimaryKey() || c.getConstraintName() == null) {
return Stream.empty();
}
return Stream.of(c.getConstraintName());
}).collect(Collectors.toList()));
}
List<String> droppedNames = action.getDropConstraintNames().stream()
.map(this::unquoteIdentifier).collect(Collectors.toList());
pkConstraintNames = pkConstraintNames.stream()
.map(this::unquoteIdentifier).collect(Collectors.toList());
if (CollectionUtils.containsAny(droppedNames, pkConstraintNames)) {
violations.add(SqlCheckUtil.buildViolation(statement.getText(), action, getType(),
new Object[] {"DROP PRIMARY KEY"}));
}
}
if (Boolean.TRUE.equals(action.getDropPrimaryKey())) {
violations.add(SqlCheckUtil.buildViolation(statement.getText(), action, getType(), new Object[] {}));
violations.add(SqlCheckUtil.buildViolation(statement.getText(), action, getType(),
new Object[] {"DROP PRIMARY KEY"}));
}
if (action.getAddConstraint() != null && action.getAddConstraint().isPrimaryKey()) {
violations.add(SqlCheckUtil.buildViolation(statement.getText(), action, getType(), new Object[] {}));
violations.add(SqlCheckUtil.buildViolation(statement.getText(), action, getType(),
new Object[] {"ADD PRIMARY KEY"}));
}
return violations;
}

protected List<CheckViolation> changeColumnInLocation(Statement statement, AlterTableAction action) {
return changeColumn(action, definition -> {
if (definition.getLocation() != null) {
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(), new Object[] {});
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(),
new Object[] {"REARRANGE (BEFORE/AFTER/FIRST)"});
}
return null;
});
Expand All @@ -213,26 +249,51 @@ protected List<CheckViolation> changeColumnToAutoIncrement(Statement statement,
&& Boolean.TRUE.equals(changed.getColumnAttributes().getAutoIncrement())) {
return null;
}
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(), new Object[] {});
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(),
new Object[] {"MODIFY TO AUTO-INCREMENT COLUMN"});
});
}

protected List<CheckViolation> changeColumnToPK(Statement statement, AlterTableAction action) {
protected List<CheckViolation> changeColumnToPrimaryKey(CreateTable createTable,
Statement statement, AlterTableAction action) {
return changeColumn(action, definition -> {
if (definition.getColumnAttributes() == null
|| CollectionUtils.isEmpty(definition.getColumnAttributes().getConstraints())
|| definition.getColumnAttributes().getConstraints().stream()
.noneMatch(InLineConstraint::isPrimaryKey)) {
return null;
}
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(), new Object[] {});
String column = unquoteIdentifier(definition.getColumnReference().getColumn());
Optional<ColumnDefinition> optional = createTable.getColumnDefinitions().stream()
.filter(def -> Objects.equals(unquoteIdentifier(def.getColumnReference().getColumn()), column))
.findAny();
if (!optional.isPresent()) {
return null;
}
ColumnDefinition colDef = optional.get();
if (colDef.getColumnAttributes() != null
&& CollectionUtils.isNotEmpty(colDef.getColumnAttributes().getConstraints())
&& colDef.getColumnAttributes().getConstraints().stream()
.anyMatch(InLineConstraint::isPrimaryKey)) {
return null;
}
List<String> pkColumns = createTable.getConstraints().stream()
.filter(OutOfLineConstraint::isPrimaryKey)
.flatMap(c -> c.getColumns().stream()
.map(sc -> unquoteIdentifier(sc.getColumn().toString())))
.collect(Collectors.toList());
if (pkColumns.contains(column)) {
return null;
}
return SqlCheckUtil.buildViolation(statement.getText(), action, getType(),
new Object[] {"MODIFY COLUMN AS PRIMARY KEY"});
});
}

protected List<CheckViolation> changeCharsetOrCollation(Statement statement, AlterTableAction action) {
if (action.getCharset() != null || action.getCollation() != null) {
return Collections.singletonList(SqlCheckUtil.buildViolation(statement.getText(),
action, getType(), new Object[] {}));
action, getType(), new Object[] {"CONVERT CHAR SET"}));
}
return Collections.emptyList();
}
Expand Down
Loading