modifySql ignoring isApplyToRollback() property (again?)

Liquibase Development
1

Using 3.1.1, 3.2.2 and the current 3.3.0-SNAPSHOT, I recently tried to have custom SQL appended to a create table statement but not to the associated rollback.  The custom SQL in the append element is appended to the rollback SQL regardless of whether applyToRollback is true or false on the modifySql element.

I put together a patch for this, but I figured I would take a look at Jira and see if it was currently a reported issue.  I see that it has been reported twice, once 5 years ago (https://liquibase.jira.com/browse/CORE-457), and again about 2 years ago (https://liquibase.jira.com/browse/CORE-1348).

The first time, a patch was submitted (which as near as I can tell, was virtually identical to what I was going to do - iterate over all the visitors and only pass the applyToRollback=true ones to the executor.execute() call in the rollback method of the ChangeSet.java class).  So, presumably this worked at some point.  Again, about 2 years ago this was reported (CORE-1348) but the comments indicated that this feature worked in the 3.x codebase and would not be back-ported to 2.x.

I looked through the history of the ChangeSet.java class on GitHub and I couldn’t find a version of this file that contained the patch (or anything similar to it) from CORE-457.  Is it worth filing a new defect on Jira for this and submitting the patch I just put together, or was this functionality removed for a reason?  Should this functionality be implemented somewhere other than the rollback method of the ChangeSet.java file?

Any info anyone could provide would be most appreciated.

Daniel Morton

2

Ok… replying to my own post above… After a bit more digging I see that the AbstractJdbcDatabase.executeRollbackStatements contains the changes submitted as part of CORE-457.  However, that code path is only hit when custom rollback steps are not provided (only the Liquibase generated rollback steps will be executed).  When custom rollback steps are provided, the ExecutorService.execute method is called instead, which skips the code where the superfluous visitors are culled.

Unless there is a good reason why this should be the case, I will file a bug and submit a pull request to ensure that the applyToRollback attribute is respected in either execution path.

Regards,

Daniel Morton