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.

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

Reintroduce stored procedures to drop columns on MySql #2403

Merged
Merged
Show file tree
Hide file tree
Changes from 2 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
18 changes: 18 additions & 0 deletions ebean-api/src/main/java/io/ebean/config/PlatformConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ public class PlatformConfig {

private boolean caseSensitiveCollation = true;

private boolean useMigrationStoredProcedures = false;

/**
* Modify the default mapping of standard types such as default precision for DECIMAL etc.
*/
Expand All @@ -90,6 +92,7 @@ public PlatformConfig(PlatformConfig platformConfig) {
this.geometrySRID = platformConfig.geometrySRID;
this.dbUuid = platformConfig.dbUuid;
this.caseSensitiveCollation = platformConfig.caseSensitiveCollation;
this.useMigrationStoredProcedures = platformConfig.useMigrationStoredProcedures;
this.allQuotedIdentifiers = platformConfig.allQuotedIdentifiers;
this.databaseInetAddressVarchar = platformConfig.databaseInetAddressVarchar;
this.customDbTypeMappings = platformConfig.customDbTypeMappings;
Expand Down Expand Up @@ -139,6 +142,20 @@ public void setCaseSensitiveCollation(boolean caseSensitiveCollation) {
this.caseSensitiveCollation = caseSensitiveCollation;
}

/**
* Return true if force use of helper stored procedures for migrations.
*/
public boolean isUseMigrationStoredProcedures() {
return useMigrationStoredProcedures;
}

/**
* Set true to force use of helper stored procedures for migrations.
*/
public void setUseMigrationStoredProcedures(boolean useMigrationStoredProcedures) {
this.useMigrationStoredProcedures = useMigrationStoredProcedures;
}

/**
* Return true if Postgres FOR UPDATE should use the NO KEY option.
*/
Expand Down Expand Up @@ -314,6 +331,7 @@ public void loadSettings(PropertiesWrapper p) {
databaseBooleanFalse = p.get("databaseBooleanFalse", databaseBooleanFalse);
databaseInetAddressVarchar = p.getBoolean("databaseInetAddressVarchar", databaseInetAddressVarchar);
caseSensitiveCollation = p.getBoolean("caseSensitiveCollation", caseSensitiveCollation);
useMigrationStoredProcedures = p.getBoolean("useMigrationStoredProcedures", useMigrationStoredProcedures);

DbUuid dbUuid = p.getEnum(DbUuid.class, "dbuuid", null);
if (dbUuid != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ public enum OnQueryOnly {

protected boolean supportsSavepointId = true;

protected boolean useMigrationStoredProcedures = false;

/**
* The behaviour used when ending a read only transaction at read committed isolation level.
*/
Expand Down Expand Up @@ -237,6 +239,7 @@ public PersistenceException translate(String message, SQLException e) {
public void configure(PlatformConfig config) {
this.sequenceBatchSize = config.getDatabaseSequenceBatchSize();
this.caseSensitiveCollation = config.isCaseSensitiveCollation();
this.useMigrationStoredProcedures = config.isUseMigrationStoredProcedures();
configureIdType(config.getIdType());
configure(config, config.isAllQuotedIdentifiers());
}
Expand Down Expand Up @@ -343,6 +346,13 @@ public boolean isSupportsSavepointId() {
return supportsSavepointId;
}

/**
* Return true if migrations should use stored procedures.
*/
public boolean isUseMigrationStoredProcedures() {
return useMigrationStoredProcedures;
}

/**
* Return true if the platform supports LIMIT with sql update.
*/
Expand Down Expand Up @@ -573,6 +583,10 @@ public void setSupportsResultSetConcurrencyModeUpdatable(boolean supportsResultS
this.supportsResultSetConcurrencyModeUpdatable = supportsResultSetConcurrencyModeUpdatable;
}

public void setUseMigrationStoredProcedures(final boolean useMigrationStoredProcedures) {
this.useMigrationStoredProcedures = useMigrationStoredProcedures;
}

/**
* Normally not needed - overridden in CockroachPlatform.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,9 @@ protected String withForUpdate(String sql, Query.LockWait lockWait, Query.LockTy
// for update are hints on from clause of base table
return sql;
}

@Override
public boolean isUseMigrationStoredProcedures() {
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public class DdlGenerator implements SpiDdlGenerator {
private final ScriptTransform scriptTransform;
private final Platform platform;
private final String platformName;
private final boolean useMigrationStoredProcedures;

private CurrentModel currentModel;
private String dropAllContent;
Expand All @@ -71,9 +72,11 @@ public DdlGenerator(SpiEbeanServer server) {
log.warn("DDL can't be run on startup with TenantMode " + config.getTenantMode());
this.runDdl = false;
this.ddlAutoCommit = false;
this.useMigrationStoredProcedures = false;
} else {
this.runDdl = config.isDdlRun();
this.ddlAutoCommit = databasePlatform.isDdlAutoCommit();
this.useMigrationStoredProcedures = config.getDatabasePlatform().isUseMigrationStoredProcedures();
Copy link
Member

Choose a reason for hiding this comment

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

config.getDatabasePlatform() -> databasePlatform

}
this.scriptTransform = createScriptTransform(config);
this.baseDir = initBaseDir();
Expand Down Expand Up @@ -187,7 +190,7 @@ private DdlRunner createDdlRunner(boolean expectErrors, String scriptName) {

protected void runDropSql(Connection connection) throws IOException {
if (!createOnly) {
if (extraDdl && jaxbPresent) {
if (extraDdl && jaxbPresent && useMigrationStoredProcedures) {
Copy link
Member

Choose a reason for hiding this comment

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

This does not look correct - useMigrationStoredProcedures must NOT be here.

String extraApply = ExtraDdlXmlReader.buildExtra(platform, true);
if (extraApply != null) {
runScript(connection, false, extraApply, "extra-ddl");
Expand All @@ -210,7 +213,7 @@ protected void runCreateSql(Connection connection) throws IOException {
if (extraDdl && jaxbPresent) {
if (currentModel().isTablePartitioning()) {
String extraPartitioning = ExtraDdlXmlReader.buildPartitioning(platform);
if (extraPartitioning != null && !extraPartitioning.isEmpty()) {
if (extraPartitioning != null && !extraPartitioning.isEmpty() && useMigrationStoredProcedures) {
Copy link
Member

Choose a reason for hiding this comment

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

This does not look correct for Postgres at least - useMigrationStoredProcedures check must NOT be here.

runScript(connection, false, extraPartitioning, "builtin-partitioning-ddl");
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,19 +392,25 @@ private void configurePlatforms() {
private void generateExtraDdl(File migrationDir, DatabasePlatform dbPlatform, boolean tablePartitioning) throws IOException {
if (dbPlatform != null) {
if (tablePartitioning && includeBuiltInPartitioning) {
generateExtraDdlFor(migrationDir, dbPlatform, ExtraDdlXmlReader.readBuiltinTablePartitioning());
generateExtraDdlFor(migrationDir, dbPlatform, ExtraDdlXmlReader.readBuiltinTablePartitioning(), true);
}
generateExtraDdlFor(migrationDir, dbPlatform, ExtraDdlXmlReader.readBuiltin());
generateExtraDdlFor(migrationDir, dbPlatform, ExtraDdlXmlReader.read());
generateExtraDdlFor(migrationDir, dbPlatform, ExtraDdlXmlReader.readBuiltin(), true);
generateExtraDdlFor(migrationDir, dbPlatform, ExtraDdlXmlReader.read(), false);
}
}

private void generateExtraDdlFor(File migrationDir, DatabasePlatform dbPlatform, ExtraDdl extraDdl) throws IOException {
private void generateExtraDdlFor(File migrationDir, DatabasePlatform dbPlatform, ExtraDdl extraDdl, boolean isBuiltin) throws IOException {
if (extraDdl != null) {
List<DdlScript> ddlScript = extraDdl.getDdlScript();
for (DdlScript script : ddlScript) {
if (!script.isDrop() && matchPlatform(dbPlatform.getPlatform(), script.getPlatforms())) {
writeExtraDdl(migrationDir, script);
if (script.isInit()) {
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth refactoring this if/else

if (!isBuiltin || dbPlatform.isUseMigrationStoredProcedures()) {
writeExtraDdl(migrationDir, script);
}
} else {
writeExtraDdl(migrationDir, script);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@ public class MySqlDdl extends PlatformDdl {
// this flag is for compatibility. Use it with care.
private static final boolean USE_CHECK_CONSTRAINT = Boolean.getBoolean("ebean.mysql.useCheckConstraint");

private final boolean useMigrationStoredProcedures;

public MySqlDdl(DatabasePlatform platform) {
super(platform);
this.alterColumn = "modify";
this.dropUniqueConstraint = "drop index";
this.historyDdl = new MySqlHistoryDdl();
this.inlineComments = true;
this.useMigrationStoredProcedures = platform.isUseMigrationStoredProcedures();
}

/**
Expand All @@ -34,6 +37,15 @@ public String dropIndex(String indexName, String tableName, boolean concurrent)
return "drop index " + maxConstraintName(indexName) + " on " + tableName;
}

@Override
public void alterTableDropColumn(final DdlBuffer buffer, final String tableName, final String columnName) throws IOException {
if (this.useMigrationStoredProcedures) {
buffer.append("CALL usp_ebean_drop_column('").append(tableName).append("', '").append(columnName).append("')").endOfStatement();
} else {
super.alterTableDropColumn(buffer, tableName, columnName);
}
}

/**
* Return the drop foreign key clause.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,5 +112,88 @@ BEGIN
END
GO
</ddl-script>
<ddl-script name="create procs" platforms="mysql" init="true">-- Inital script to create stored procedures etc for mysql platform
DROP PROCEDURE IF EXISTS usp_ebean_drop_foreign_keys;

delimiter $$
--
-- PROCEDURE: usp_ebean_drop_foreign_keys TABLE, COLUMN
-- deletes all constraints and foreign keys referring to TABLE.COLUMN
--
CREATE PROCEDURE usp_ebean_drop_foreign_keys(IN p_table_name VARCHAR(255), IN p_column_name VARCHAR(255))
BEGIN
DECLARE done INT DEFAULT FALSE;
DECLARE c_fk_name CHAR(255);
DECLARE curs CURSOR FOR SELECT CONSTRAINT_NAME from information_schema.KEY_COLUMN_USAGE
WHERE TABLE_SCHEMA = DATABASE() and TABLE_NAME = p_table_name and COLUMN_NAME = p_column_name
AND REFERENCED_TABLE_NAME IS NOT NULL;
DECLARE CONTINUE HANDLER FOR NOT FOUND SET done = TRUE;

OPEN curs;

read_loop: LOOP
FETCH curs INTO c_fk_name;
IF done THEN
LEAVE read_loop;
END IF;
SET @sql = CONCAT('ALTER TABLE ', p_table_name, ' DROP FOREIGN KEY ', c_fk_name);
PREPARE stmt FROM @sql;
EXECUTE stmt;
END LOOP;

CLOSE curs;
END
$$

DROP PROCEDURE IF EXISTS usp_ebean_drop_column;

delimiter $$
--
-- PROCEDURE: usp_ebean_drop_column TABLE, COLUMN
-- deletes the column and ensures that all indices and constraints are dropped first
--
CREATE PROCEDURE usp_ebean_drop_column(IN p_table_name VARCHAR(255), IN p_column_name VARCHAR(255))
BEGIN
CALL usp_ebean_drop_foreign_keys(p_table_name, p_column_name);
SET @sql = CONCAT('ALTER TABLE ', p_table_name, ' DROP COLUMN ', p_column_name);
PREPARE stmt FROM @sql;
EXECUTE stmt;
END
$$
</ddl-script>

<ddl-script name="create procs" platforms="hana" init="true">-- Inital script to create stored procedures etc for the hana platform
delimiter $$
--
-- PROCEDURE: usp_ebean_drop_foreign_keys TABLE, COLUMN
-- deletes all constraints and foreign keys referring to TABLE.COLUMN
--
CREATE OR REPLACE PROCEDURE usp_ebean_drop_foreign_keys(IN table_name NVARCHAR(256), IN column_name NVARCHAR(256))
AS
BEGIN
DECLARE foreign_key_names TABLE(CONSTRAINT_NAME NVARCHAR(256), TABLE_NAME NVARCHAR(256));
DECLARE i INT;

foreign_key_names = SELECT CONSTRAINT_NAME, TABLE_NAME FROM SYS.REFERENTIAL_CONSTRAINTS WHERE SCHEMA_NAME=CURRENT_SCHEMA AND TABLE_NAME=UPPER(:table_name) AND COLUMN_NAME=UPPER(:column_name);

FOR I IN 1 .. RECORD_COUNT(:foreign_key_names) DO
EXEC 'ALTER TABLE "' || ESCAPE_DOUBLE_QUOTES(:foreign_key_names.TABLE_NAME[i]) || '" DROP CONSTRAINT "' || ESCAPE_DOUBLE_QUOTES(:foreign_key_names.CONSTRAINT_NAME[i]) || '"';
END FOR;

END;
$$

delimiter $$
--
-- PROCEDURE: usp_ebean_drop_column TABLE, COLUMN
-- deletes the column and ensures that all indices and constraints are dropped first
--
CREATE OR REPLACE PROCEDURE usp_ebean_drop_column(IN table_name NVARCHAR(256), IN column_name NVARCHAR(256))
AS
BEGIN
CALL usp_ebean_drop_foreign_keys(table_name, column_name);
EXEC 'ALTER TABLE "' || UPPER(ESCAPE_DOUBLE_QUOTES(table_name)) || '" DROP ("' || UPPER(ESCAPE_DOUBLE_QUOTES(column_name)) || '")';
END;
$$
</ddl-script>
</extra-ddl>
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package io.ebeaninternal.dbmigration;

import io.ebean.Database;
import io.ebean.DatabaseFactory;
import io.ebean.annotation.Platform;
import io.ebean.config.DatabaseConfig;
import io.ebean.config.PlatformConfig;
import org.junit.jupiter.api.Test;

import java.io.File;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;

import static org.assertj.core.api.Assertions.assertThat;

/**
* Class to test the alternative drop behaviour using stored procedures for MySql databases .
*
* @author Jonas P&ouml;hler, FOCONIS AG
*/
public class MysqlGenerateMigrationTest {

@Test
public void testMysqlStoredProcedures() throws Exception {
DefaultDbMigration migration = new DefaultDbMigration();
migration.setIncludeIndex(true);
// We use src/test/resources as output directory (so we see in GIT if files will change)
migration.setPathToResources("src/test/resources");

migration.addPlatform(Platform.MYSQL, "mysql");

final PlatformConfig platformConfig = new PlatformConfig();
platformConfig.setUseMigrationStoredProcedures(true);

DatabaseConfig config = new DatabaseConfig();
config.setName("migrationtest");
config.loadFromProperties();
config.setPlatformConfig(platformConfig);
config.setRegister(false);
config.setDefaultServer(false);
config.getProperties().put("ebean.migration.migrationPath", "db/migration/mysql");

config.setPackages(Arrays.asList("misc.migration.mysql_v1_0"));
Database server = DatabaseFactory.create(config);
migration.setServer(server);
migration.setMigrationPath("mysql/procedures");

// First, we clean up the output-directory
assertThat(migration.migrationDirectory().getAbsolutePath()).contains("procedures");
Files.walk(migration.migrationDirectory().toPath())
.filter(Files::isRegularFile)
.map(Path::toFile).forEach(File::delete);

// then we generate migration scripts for v1_0
assertThat(migration.generateMigration()).isEqualTo("1.0__initial");

config.setPackages(Arrays.asList("misc.migration.mysql_v1_1"));
server.shutdown();
server = DatabaseFactory.create(config);
migration.setServer(server);
migration.setMigrationPath("mysql/procedures");
assertThat(migration.generateMigration()).isEqualTo("1.1");

System.setProperty("ddl.migration.pendingDropsFor", "1.1");
assertThat(migration.generateMigration()).isEqualTo("1.2__dropsFor_1.1");

final Path sqlFile = migration.migrationDirectory().toPath()
.resolve("mysql/1.2__dropsFor_1.1.sql");

assertThat(sqlFile).isNotEmptyFile();
assertThat(Files.readAllLines(sqlFile, StandardCharsets.UTF_8))
.contains("CALL usp_ebean_drop_column('migtest_e_basic', 'status2');")
.contains("CALL usp_ebean_drop_column('migtest_e_basic', 'description');");

}
}
Loading