search

liquibase / liquibase-groovy-dsl

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

Support placeholders in imports #26

Closed dgeissl closed 7 years ago

dgeissl commented 7 years ago

I have tested a XML-File like this:

<databaseChangeLog
  xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog
         http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.1.xsd">

    <!-- defaults that may be overwritten with commandLine parameters -->
    <property name="targetSystem" value="MSY" />
    <property name="targetVersion" value="1.2.0" />

    <include file="changeLog/${targetSystem}/alter${targetSystem}-DB-${targetVersion}.xml" />

</databaseChangeLog>

transformed to groovy:

databaseChangeLog() {
    property(name:'targetSystem', value:'MSY')
    property(name:'targetVersion', value:'1.2.0')

    include(file: 'changeLog/${targetSystem}/alter${targetSystem}-DB-${targetVersion}.xml')
}

but then i get:

Unexpected error running Liquibase: changeLog/${targetSystem}/alter${targetSystem}-DB-${targetVersion}.xml does not exist

replacing single with double quotes does not work either as groovy complains that there is no property called targetSystem. Did i use the property closures right, there's only documentation about the property(file:'') syntax.

stevesaliman commented 7 years ago

This issue is related to a significant difference between the way the XML parser handles includes and the way the Groovy parser does it.

Basically, the XML parser defers processing of the include elements to Liquibase, which has access to the properties you've put into the databaseChangelog. The Groovy parser processes include itself as soon as they are encountered, which means it doesn't have easy access to Liquibase properties in the changelog.

I can get into the more technical details of why this is, but you can probably solve your problem by using the following changelog:

def targetSystem = 'MSY'
def targetVersion = '1.2.0'
databaseChangeLog() {
    property(name: 'targetSystem', value: targetSystem)
    property(name: 'targetVersion', value: targetVersion)

    include(file: "changeLog/${targetSystem}/alter${targetSystem}-DB-${targetVersion}.xml")
}

The two property statements inside the databaseChangeLog are not necessary for the include to work, they are only needed if the targetSystem and targetVersion are used somewhere besides the include statement.

dgeissl commented 7 years ago

Hi @stevesaliman,

thank you for the basic idea, but the goal i had, was to be able to override the parameters via CLI. e.g. ./liquibase status -DtargetVersion=1.3.0 but trying that with your example does not work and i had no success with the following change:

def targetSystem = System.getProperty('targetSystem','MSY')
def targetVersion = System.getProperty('targetVersion','1.2.0')
databaseChangeLog() {
    property(name: 'targetSystem', value: targetSystem)
    property(name: 'targetVersion', value: targetVersion)

    print targetVersion // still outputs 1.2.0 if -DtargetVersion=1.3.0 is used

    include(file: "changeLog/${targetSystem}/alter${targetSystem}-DB-${targetVersion}.xml")
}

I can see the -DtargetVersion=1.3.0 only in the SystemProperty "sun.java.command" and not as individual key in the System.getProperties().each {k,v -> println "$k = $v"}. Not sure if i am doing something wrong, the Liquibase CLI Processor kicks in or i am missing something with groovy basics.

If you like to share, i m also interested in the reasons of why the groovy-dsl does not hand over include to Liquibase. One downside i see (as stated in the includeAll documentation) is loosing the opportunity of mixing groovy with other language changeLogs, right?

dgeissl commented 7 years ago

I've had some look around and found the comments mentioning the placeholder problems: DatabaseChangeLogDelegate#changeSet DatabaseChangeLogDelegate#include DatabaseChangeLogDelegate#includeAll

just a wild guess, wouldn't it be sufficient to insert databaseChangeLog.expandExpressions(filename, databaseChangeLog) in DatabaseChangeLogDelegate#includeChangeLog to fix at some of the issues?

Maybe there should be a more general aproach for other parameters too, but i m not sure how to do this properly.

stevesaliman commented 7 years ago

I'm not sure why -DtargetVersion=1.3.0 isn't working for you. I tried doing something similar on my end and it appears to do the right thing. Perhaps the command line had a lowercase 'v' and the property is expecting uppercase?

As for why I don't hand off the include to Liquibase, it has to do with the way Liquibase parses files. When the XML parser reads in a file, it creates a tree of ParsedNode objects. An include is one type of ParsedNode. Next, Liquibase converts the tree of ParsedNode into the various Liquibase objects, expands the include node at that time to create another tree to process and add to the change log. In general, I really like the idea of ParsedNodes, but the Liquibase 3.x implementation has a flaw in it that I don't like - it silently ignores attributes it doesn't recognize and reports success even though it isn't doing what the developer asked it to do. You can see https://liquibase.jira.com/browse/CORE-1968 for more details on that issue.

The net result of this is that I don't use ParsedNodes. I make the Liquibase objects directly, which means I have to handle the includes myself.

As for the databaseChangeLog.expandExpressions(filename, databaseChangeLog) I seem to remember trying that once before and running into some sort of problem, but I don't remember what it might have been. It may be time for me to look at it again...

stevesaliman commented 7 years ago

I'm not sure what the original issue was with the expanded expressions, but adding it into the current code seems to be working now.

I'll try to get a new build out in the next few days with that fix.

dgeissl commented 7 years ago

Thank you for the detailed information. Can't wait for your changes to arrive.

As for the checking of non supported parameters, i guess that's because liquibase is originally build around the xml file wich has it's own validation by the xml schema.

dgeissl commented 7 years ago

I tested the current master version, including https://github.com/liquibase/liquibase-groovy-dsl/commit/60a3b28fe0d678ba9f94b0504ab66b9379b2316e

databaseChangeLog() {
    property(name: 'targetSystem', value: 'MSY')
    property(name: 'targetVersion', value: '1.2.0')

    include(file: "changeLog/${targetSystem}/alter${targetSystem}-DB-${targetVersion}.xml")
}

worked like a charm. Also parameter overriding by a command line argument was fine.

stevesaliman commented 7 years ago

Release 1.2.2 is out with the fix for this issue.

stevesaliman commented 7 years ago

FYI, if you use the DSL via the Liquibase Gradle plugin, the 1.2.2 release of that plugin has a bug and should not be used. Gradle users should use version 1.2.3 of the Gradle plugin.