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.

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

Jump to bottom

Improve addColumn handling in SQLite #1970

Merged
merged 7 commits into from
Nov 9, 2021
Merged

Improve addColumn handling in SQLite #1970

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b822f1c
Improve addColumn handling in SQLite
nvoxland Jul 7, 2021
961ce7f
Improve addColumn handling in SQLite
nvoxland Jul 7, 2021
9f8c253
Improve addColumn handling in SQLite
nvoxland Jul 7, 2021
74e91df
Merge remote-tracking branch 'origin/master' into sqlite-improvements
nvoxland Aug 26, 2021
b8fa371
Merge branch 'master' into sqlite-improvements
nvoxland Sep 3, 2021
6559bc1
SQLite and Firebird at least seem to sometimes return no catalogs. Re…
nvoxland Sep 3, 2021
f3b698b
Merge remote-tracking branch 'origin/master' into sqlite-improvements
nvoxland Oct 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions base.pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,12 @@
<version>6.0.6</version>
<scope>test</scope>
</dependency>
<!-- <dependency>-->
nvoxland marked this conversation as resolved.
Show resolved Hide resolved
<!-- <groupId>org.xerial</groupId>-->
<!-- <artifactId>sqlite-jdbc</artifactId>-->
<!-- <version>3.34.0</version>-->
<!-- <scope>test</scope>-->
<!-- </dependency>-->

<dependency>
<groupId>com.microsoft.sqlserver</groupId>
Expand Down
7 changes: 2 additions & 5 deletions liquibase-core/src/main/java/liquibase/change/ColumnConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,7 @@
import liquibase.statement.NotNullConstraint;
import liquibase.statement.SequenceCurrentValueFunction;
import liquibase.statement.SequenceNextValueFunction;
import liquibase.structure.core.Column;
import liquibase.structure.core.ForeignKey;
import liquibase.structure.core.PrimaryKey;
import liquibase.structure.core.Table;
import liquibase.structure.core.UniqueConstraint;
import liquibase.structure.core.*;
import liquibase.util.*;

import java.math.BigInteger;
Expand Down Expand Up @@ -153,6 +149,7 @@ public ColumnConfig(Column columnSnapshot) {
"(" +
fk.getPrimaryKeyColumns().get(0).getName() +
")");
constraints.setDeleteCascade(fk.getDeleteRule() != null && fk.getDeleteRule() == ForeignKeyConstraintType.importedKeyCascade);
nonDefaultConstraints = true;
}
}
Expand Down
10 changes: 10 additions & 0 deletions liquibase-core/src/main/java/liquibase/change/core/AddColumnChange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,16 @@ public SqlStatement[] generateStatements(Database database) {
if (constraintsConfig.getValidateForeignKey()!=null && !constraintsConfig.getValidateForeignKey()) {
foreignKeyConstraint.setValidateForeignKey(false);
}

if (constraintsConfig.isDeleteCascade() != null) {
foreignKeyConstraint.setDeleteCascade(constraintsConfig.isDeleteCascade());
}
if (constraintsConfig.isDeferrable() != null) {
foreignKeyConstraint.setDeferrable(constraintsConfig.isDeferrable());
}
if (constraintsConfig.isInitiallyDeferred() != null) {
foreignKeyConstraint.setInitiallyDeferred(constraintsConfig.isInitiallyDeferred());
}
constraints.add(foreignKeyConstraint);
}
}
Expand Down
29 changes: 17 additions & 12 deletions liquibase-core/src/main/java/liquibase/change/core/DropColumnChange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import liquibase.snapshot.InvalidExampleException;
import liquibase.snapshot.SnapshotGeneratorFactory;
import liquibase.statement.SqlStatement;
import liquibase.statement.core.AddColumnStatement;
import liquibase.statement.core.DropColumnStatement;
import liquibase.statement.core.ReorganizeTableStatement;
import liquibase.structure.core.Column;
Expand All @@ -21,6 +22,8 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;

/**
* Drops an existing column from a table.
Expand Down Expand Up @@ -122,13 +125,13 @@ public SqlStatement[] generateStatements(Database database) {
private SqlStatement[] generateMultipleColumns(Database database) throws DatabaseException {
List<SqlStatement> statements = new ArrayList<>();
List<DropColumnStatement> dropStatements = new ArrayList<>();
for (ColumnConfig column : columns) {
if (database instanceof SQLiteDatabase) {
// SQLite is special in that it needs multiple SQL statements (i.e. a whole table recreation!) to drop
// a single column.
statements.addAll(Arrays.asList(generateStatementsForSQLiteDatabase(database, column.getName())));
} else {

if (database instanceof SQLiteDatabase) {
// SQLite is special in that it needs multiple SQL statements (i.e. a whole table recreation!) to drop
// a single column.
statements.addAll(Arrays.asList(generateStatementsForSQLiteDatabase(database)));
} else {
for (ColumnConfig column : columns) {
dropStatements.add(new DropColumnStatement(getCatalogName(), getSchemaName(), getTableName(), column
.getName()));
}
Expand All @@ -150,7 +153,7 @@ private SqlStatement[] generateMultipleColumns(Database database) throws Databas
private SqlStatement[] generateSingleColumn(Database database) throws DatabaseException {
if (database instanceof SQLiteDatabase) {
// return special statements for SQLite databases
return generateStatementsForSQLiteDatabase(database, getColumnName());
return generateStatementsForSQLiteDatabase(database);
}

List<SqlStatement> statements = new ArrayList<>();
Expand Down Expand Up @@ -179,11 +182,13 @@ Table.class, getCatalogName(), getSchemaName(), getTableName(), getColumnName()

}

private SqlStatement[] generateStatementsForSQLiteDatabase(Database database, final String columnName) throws DatabaseException {
private SqlStatement[] generateStatementsForSQLiteDatabase(Database database) throws DatabaseException {
SqlStatement[] sqlStatements = null;
// Since SQLite does not support a drop column statement, use alter table visitor to copy the table
// except for the column (and index containing that column) to delete.

Set<String> removedColumnNames = columns.stream().map(ColumnConfig::getName).collect(Collectors.toSet());

SQLiteDatabase.AlterTableVisitor alterTableVisitor = new SQLiteDatabase.AlterTableVisitor() {
@Override
public ColumnConfig[] getColumnsToAdd() {
Expand All @@ -192,20 +197,20 @@ public ColumnConfig[] getColumnsToAdd() {

@Override
public boolean copyThisColumn(ColumnConfig column) {
return !column.getName().equals(columnName);
return !removedColumnNames.contains(column.getName());
}

@Override
public boolean createThisColumn(ColumnConfig column) {
return !column.getName().equals(columnName);
return !removedColumnNames.contains(column.getName());
}

@Override
public boolean createThisIndex(Index index) {
// don't create the index if it has the column we are dropping
boolean indexContainsColumn = false;
for (Column column : index.getColumns()) {
if (column.getName().equals(columnName)) {
if (removedColumnNames.contains(column.getName())) {
indexContainsColumn = true;
}
}
Expand Down
6 changes: 5 additions & 1 deletion liquibase-core/src/main/java/liquibase/serializer/core/yaml/YamlSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,12 @@ protected Object toMap(LiquibaseSerializable object) {
continue;
}

for (Object key : ((Map) value).keySet()) {
for (Object key : new HashSet<>(((Map) value).keySet())) {
Object mapValue = ((Map) value).get(key);
if (mapValue == null) {
((Map) value).remove(key);
}

if (mapValue instanceof LiquibaseSerializable) {
((Map) value).put(key, toMap((LiquibaseSerializable) mapValue));
} else if (mapValue instanceof Collection) {
Expand Down
5 changes: 5 additions & 0 deletions liquibase-core/src/main/java/liquibase/snapshot/jvm/JdbcSnapshotGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import liquibase.database.Database;
import liquibase.database.core.InformixDatabase;
import liquibase.database.core.PostgresDatabase;
import liquibase.database.core.SQLiteDatabase;
import liquibase.database.jvm.JdbcConnection;
import liquibase.diff.DiffStatusListener;
import liquibase.exception.DatabaseException;
Expand Down Expand Up @@ -160,6 +161,10 @@ protected String[] getDatabaseCatalogNames(Database database) throws SQLExceptio
}

}

if (returnList.size() == 0 && database instanceof SQLiteDatabase) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we expand this to other DBs? I found Firebird need same fix as it also fails with liquibase.exception.UnexpectedLiquibaseException: Found a null snapshotId for catalog null

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing if condition to include (database instanceof SQLiteDatabase || database instanceof FirebirdDatabase) helps with Firebird snapshot generation, but maybe this can be expanded further

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nvoxland -- please see @KushnirykOleh's comment above and provide guidance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is probably safe to include for all database, not just sqlite and firebird. I updated the code to just be if (returnlist.size() == 0)

returnList.add(database.getDefaultCatalogName());
}
return returnList.toArray(new String[returnList.size()]);
}

Expand Down
81 changes: 61 additions & 20 deletions liquibase-core/src/main/java/liquibase/sqlgenerator/core/AddColumnGeneratorSQLite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,18 @@
import liquibase.exception.ValidationErrors;
import liquibase.sql.Sql;
import liquibase.sqlgenerator.SqlGeneratorChain;
import liquibase.statement.ColumnConstraint;
import liquibase.statement.ForeignKeyConstraint;
import liquibase.statement.core.AddColumnStatement;
import liquibase.structure.core.Index;

import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;

/**
* Workaround for adding column on existing table for SQLite.
*
*/
public class AddColumnGeneratorSQLite extends AddColumnGenerator {

Expand All @@ -41,33 +45,64 @@ public boolean supports(AddColumnStatement statement, Database database) {
public Sql[] generateSql(final AddColumnStatement statement, Database database, SqlGeneratorChain sqlGeneratorChain) {
// Workaround implemented by replacing a table with a new one (duplicate)
// with a new column added

final List<AddColumnStatement> columns = new ArrayList<>(statement.getColumns());
if (columns.size() == 0) {
columns.add(statement);
}

Set<String> newColumnNames = columns.stream().map(AddColumnStatement::getColumnName).collect(Collectors.toSet());

Sql[] generatedSqls;
SQLiteDatabase.AlterTableVisitor alterTableVisitor = new SQLiteDatabase.AlterTableVisitor() {
@Override
public ColumnConfig[] getColumnsToAdd() {
ColumnConfig[] columnConfigs = new ColumnConfig[1];
ColumnConfig newColumn = new ColumnConfig();
newColumn.setName(statement.getColumnName());
newColumn.setType(statement.getColumnType());
newColumn.setAutoIncrement(statement.isAutoIncrement());
ConstraintsConfig constraintsConfig = new ConstraintsConfig();
if (statement.isPrimaryKey()) {
constraintsConfig.setPrimaryKey(true);
}
if (statement.isNullable()) {
constraintsConfig.setNullable(true);
}
if (statement.isUnique()) {
constraintsConfig.setUnique(true);

ColumnConfig[] columnConfigs = new ColumnConfig[columns.size()];

int i = 0;
for (AddColumnStatement column : columns) {
ColumnConfig newColumn = new ColumnConfig();
newColumn.setName(column.getColumnName());
newColumn.setType(column.getColumnType());
newColumn.setAutoIncrement(column.isAutoIncrement());
ConstraintsConfig constraintsConfig = new ConstraintsConfig();
if (column.isPrimaryKey()) {
constraintsConfig.setPrimaryKey(true);
}
if (column.isNullable()) {
constraintsConfig.setNullable(true);
}
if (column.isUnique()) {
constraintsConfig.setUnique(true);
}
newColumn.setConstraints(constraintsConfig);

for (ColumnConstraint constraint : column.getConstraints()) {
if (constraint instanceof ForeignKeyConstraint) {
final ForeignKeyConstraint fkConstraint = (ForeignKeyConstraint) constraint;
constraintsConfig.setReferencedTableCatalogName(fkConstraint.getReferencedTableCatalogName());
constraintsConfig.setReferencedTableSchemaName(fkConstraint.getReferencedTableSchemaName());
constraintsConfig.setReferencedTableName(fkConstraint.getReferencedTableName());
constraintsConfig.setReferencedColumnNames(fkConstraint.getReferencedColumnNames());
constraintsConfig.setReferences(fkConstraint.getReferences());

constraintsConfig.setForeignKeyName(fkConstraint.getForeignKeyName());
if (fkConstraint.isDeleteCascade()) {
constraintsConfig.setDeleteCascade(true);
}
}
}

columnConfigs[i++] = newColumn;
}
newColumn.setConstraints(constraintsConfig);
columnConfigs[0] = newColumn;

return columnConfigs;
}

@Override
public boolean copyThisColumn(ColumnConfig column) {
return !column.getName().equals(statement.getColumnName());
return !newColumnNames.contains(column.getName());
}

@Override
Expand All @@ -79,9 +114,15 @@ public boolean createThisColumn(ColumnConfig column) {
public boolean createThisIndex(Index index) {
return true;
}

};
generatedSqls = SQLiteDatabase.getAlterTableSqls(database, alterTableVisitor, statement.getCatalogName(),
statement.getSchemaName(), statement.getTableName());

final String catalogName = columns.get(0).getCatalogName();
final String schemaName = columns.get(0).getSchemaName();
final String tableName = columns.get(0).getTableName();

generatedSqls = SQLiteDatabase.getAlterTableSqls(database, alterTableVisitor, catalogName,
schemaName, tableName);

return generatedSqls;
}
Expand Down
8 changes: 4 additions & 4 deletions liquibase-core/src/main/java/liquibase/sqlgenerator/core/CopyRowsGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,14 @@ public Sql[] generateSql(CopyRowsStatement statement, Database database, SqlGene
}

if (database instanceof SQLiteDatabase) {
sql.append("INSERT INTO `").append(statement.getTargetTable()).append("` (");
sql.append("INSERT INTO ").append(database.escapeTableName(null, null, statement.getTargetTable())).append(" (");

for (int i = 0; i < statement.getCopyColumns().size(); i++) {
ColumnConfig column = statement.getCopyColumns().get(i);
if (i > 0) {
sql.append(",");
}
sql.append("`").append(column.getName()).append("`");
sql.append(database.escapeColumnName(null, null, statement.getTargetTable(), column.getName()));
}

sql.append(") SELECT ");
Expand All @@ -52,9 +52,9 @@ public Sql[] generateSql(CopyRowsStatement statement, Database database, SqlGene
if (i > 0) {
sql.append(",");
}
sql.append("`").append(column.getName()).append("`");
sql.append(database.escapeColumnName(null, null, statement.getSourceTable(), column.getName()));
}
sql.append(" FROM `").append(statement.getSourceTable()).append("`");
sql.append(" FROM ").append(database.escapeTableName(null, null, statement.getSourceTable()));
}

return new Sql[]{
Expand Down