liquibase / liquibase-groovy-dsl

The official Groovy DSL for Liquibase
Other
83 stars 34 forks source link

initialization of Change property defaults not consistent with liquibase core #28

Closed dgeissl closed 7 years ago

dgeissl commented 7 years ago

With xml you can not simply use the OutputChange, but that's no problem in groovy as we are not limited by the xml validation:

databaseChangeLog() {
    changeSet(id: '0', author:'unknown', runAlways: true, runOnChange: true){
        output(message: 'hello world', target: "STDOUT")
    }
}

however the liquibase.change.core.OutputChange does not require the target property to be set

    @DatabaseChangeProperty(description = "Target for message. Possible values: STDOUT, STDERR, FATAL, WARN, INFO, DEBUG. Default value: STDERR", exampleValue = "STDERR")
    public String getTarget() {
        if (target == null) {
            return "STDERR";
        }
        return target;
    }

if i extend the OutputChange class and override the getSerializedObjectNamespace method to use a different xml namespace, i can use it in xml and do not need to provide the target. If i use the output change with groovy-dsl directly (and without target) i get the following error:

$ ./liquibase --logLevel=INFO --changeLogFile=changeLog/root.groovy update
INFO 07.02.17 09:23: liquibase: Successfully acquired change log lock
INFO 07.02.17 09:23: liquibase: Reading from S83CFLS.LIQUIBASE_LOG
SEVERE 07.02.17 09:23: liquibase: changeLog/root.groovy: changeLog/root.groovy::0::unknown: Change Set changeLog/root.groovy::0::unknown failed.  Error: Unknown target:
INFO 07.02.17 09:23: liquibase: changeLog/root.groovy::0::unknown: Successfully released change log lock
Unexpected error running Liquibase: Unknown target:

SEVERE 07.02.17 09:23: liquibase: changeLog/root.groovy::0::unknown: Unknown target:
liquibase.exception.MigrationFailedException: Migration failed for change set changeLog/root.groovy::0::unknown:
     Reason: liquibase.exception.UnexpectedLiquibaseException: Unknown target:
        at liquibase.changelog.ChangeSet.execute(ChangeSet.java:619)
        at liquibase.changelog.visitor.UpdateVisitor.visit(UpdateVisitor.java:51)
        at liquibase.changelog.ChangeLogIterator.run(ChangeLogIterator.java:79)
        at liquibase.Liquibase.update(Liquibase.java:214)
        at liquibase.Liquibase.update(Liquibase.java:192)
        at liquibase.integration.commandline.Main.doMigration(Main.java:1130)
        at liquibase.integration.commandline.Main.run(Main.java:188)
        at liquibase.integration.commandline.Main.main(Main.java:103)
Caused by: liquibase.exception.UnexpectedLiquibaseException: Unknown target:
        at liquibase.change.core.OutputChange$1.generate(OutputChange.java:73)
        at liquibase.sqlgenerator.core.RuntimeGenerator.generateSql(RuntimeGenerator.java:19)
        at liquibase.sqlgenerator.core.RuntimeGenerator.generateSql(RuntimeGenerator.java:10)
        at liquibase.sqlgenerator.SqlGeneratorChain.generateSql(SqlGeneratorChain.java:30)
        at liquibase.sqlgenerator.SqlGeneratorFactory.generateSql(SqlGeneratorFactory.java:225)
        at liquibase.executor.AbstractExecutor.applyVisitors(AbstractExecutor.java:25)
        at liquibase.executor.jvm.JdbcExecutor.access$500(JdbcExecutor.java:36)
        at liquibase.executor.jvm.JdbcExecutor$ExecuteStatementCallback.doInStatement(JdbcExecutor.java:295)
        at liquibase.executor.jvm.JdbcExecutor.execute(JdbcExecutor.java:55)
        at liquibase.executor.jvm.JdbcExecutor.execute(JdbcExecutor.java:113)
        at liquibase.database.AbstractJdbcDatabase.execute(AbstractJdbcDatabase.java:1277)
        at liquibase.database.AbstractJdbcDatabase.executeStatements(AbstractJdbcDatabase.java:1259)
        at liquibase.changelog.ChangeSet.execute(ChangeSet.java:582)
        ... 7 more

So i debuged the code and found that target is a empty string (that s the way the class initializes the property) and not null or "STDERR". When watching the setTargetin OutputChange, i can see that the ChangeFactoryinitializes properties with the exampleValue from the annotation when it calls createChangeParameterMetadata but it seems not to be called when used from the groovy-dsl plugin.

So this looks like a combination of a unclear default value in OutputChange and inconsistent behavior of the groovy-dsl plugin that may affect other Changes too.

dgeissl commented 7 years ago

I've had a closer look at how the xml process workes out and my guess about the exampleValue was wrong. With xml the setTarget method is called by AbstractChange#load wich iterates over all ChangeParameterMetaData. If a value is not provided it simply sets null. Seems that would be some bigger rewrite of ChangeSetDelegate#makeChangeFromMap.

Another thing that comes to my mind when inspecting the code is that custom changes (implemented as individual dsl elements) isn't supported at the moment, right? https://github.com/liquibase/liquibase-groovy-dsl/blob/master/src/main/groovy/org/liquibase/groovy/delegate/ChangeSetDelegate.groovy#L550

stevesaliman commented 7 years ago

I'll have to take a closer look at the Output change over the weekend.

As for the custom changes, you are correct that only changes that map directly to Liquibase core changes are currently supported. When Liquibase 4 comes out, I plan to use Liquibase's ParsedNode functionality to translate any arbitrary groovy method found in the DSL into a matching ParsedNode, which Liquiabse would then validate. This would make the DSL just a thin wrapper around Liquibase, and we'd be able to support updates to Liquibase without updating the DSL.

We started looking into Liquibase 4 a year ago, but I haven't seen any commits to that branch for at least 6 months...

stevesaliman commented 7 years ago

AbstractChange#load is part of the ParsedNode parsing which I can't use as described in Issue #26.

I think the real bug here is in Liquibase itself. If Liquibase wants the default value of target to be "STDERR", it should initialize the attribute accordingly and let others override it if needed. Or at the very least, it should be initialized to null instead of the empty string. getTarget could even be changed to check for null or an empty string. That said, I will put a workaround for that into the groovy DSL. I'll also take a quick sweep through the current changes to see if any others have this problem.

stevesaliman commented 7 years ago

At a quick glance, it doesn't look like any other properties of any other changes suffer from this problem. I created Issue 2998 in the Liquibase Jira system to track the underlying issue with the OutputChange.

stevesaliman commented 7 years ago

Release 1.2.2 is out with a workaround for this issue.