Reintroduce stored procedures to drop columns on MySql #2403
Conversation
…ures_mysql_drop_column
|
@rbygrave were you already able to have a look at this? I would appreciate some feedback. |
| @@ -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) { | |||
There was a problem hiding this comment.
This does not look correct - useMigrationStoredProcedures must NOT be here.
| @@ -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) { | |||
There was a problem hiding this comment.
This does not look correct for Postgres at least - useMigrationStoredProcedures check must NOT be here.
| 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()) { |
There was a problem hiding this comment.
Might be worth refactoring this if/else
| } else { | ||
| this.runDdl = config.isDdlRun(); | ||
| this.ddlAutoCommit = databasePlatform.isDdlAutoCommit(); | ||
| this.useMigrationStoredProcedures = config.getDatabasePlatform().isUseMigrationStoredProcedures(); |
There was a problem hiding this comment.
config.getDatabasePlatform() -> databasePlatform
…cluded + simplify skip logic
|
Thank you @rbygrave for taking time to review and merge this. The changes look fine to me, I guess some of the problems were relicts from the previous implementation and so it was good to have them cleaned up 👍🏿. |
|
Yes, apologies that it took so long to get to that. I had a review partially done but not submitted for a few months there - oh well, sorted now :) |
Hello Rob,
this PR would re-introduce using stored procedures to drop columns in migration scripts on MySql database platforms as originally introduced in #1451. With #1802 the original code was reverted for MySql, because some migration runners other than Ebean's could not interpret the init-script. I have modified the orignal change so that Ebean's default behaviour stays as it is (generating simple
alter table ... drop ...calls) but now there is an option available to generate the init-script as well as use the stored procedure to drop the columns while before dropping all constraints.I guess that for quite a lot of people using MySql (or MariaDB) with Ebean and its migration runner, this would be a major improvement and less hassle when dropping columns.
Additionally I added a test, which checks for the correct procedure calls in generated SQL and I didn't have to change anything for the other migration tests.
If this would get merged, I would also like to provide documentation for the website, so that there is a place where you can actually stumble upon this feature or reference to in case of a question e.g. on the forum. I already sketched out a paragraph for the documentation here, but I would open a PR once this one is finalized.
Looking forward to your feedback and all the best!
Jonas