search

liquibase / liquibase-groovy-dsl

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

enabling loading of extenssions #36

Closed amexboy closed 6 years ago

amexboy commented 6 years ago

Unless there is a complex logic we need to do to parse the groovy change, we can look up the liquibases registry to find the appropriate change.

What I did was to look up the change in the methodMissing callback as that is the perfect place to look for it.

Also minor changes to the build file to enable publishing to mavenLocal

stevesaliman commented 6 years ago

Thank you for the contribution. I'll take a look at it as soon as I get the chance.

I am curious about one thing: What does the maven-publish plugin do that we can't already do with gradlew install?

amexboy commented 6 years ago

Thank you for the contribution. I'll take a look at it as soon as I get the chance.

I am curious about one thing: What does the maven-publish plugin do that we can't already do with gradlew install?

I couldn't figure out how to publish to my local maven. Should I remove?

stevesaliman commented 6 years ago

I had some time to take a deeper look at the code.

I really like the idea of getting changes from the registry. In fact, I think all changes should be fetched from the registry. This could be done easily enough by putting the registry lookup in its own helper and having all methods, including methodMissing use that helper. This would work for most use cases, but it does leave a couple of challenges...

First, how would we process external changes that have nested elements? The DSL represents nested elements as closures, but I can't think of any way to up a delegate for the closures in external changes. For now this could be a documented limitation since it lets us process more than we used to be able to handle.

Second, the new DSL code will ignore certain invalid change set code. For example, addAutoIncrement(tableName: 'myTable') { def x = 4 } currently complains because a closure is not valid for the addAutoIncrement change, but the new code would not complain. I don't like this because the code should not report success if it doesn't do everything it was asked to do. I think I could work around that problem by having a list of changes that support closures and throwing an exception if a given change has a closure and isn't on the list.

There are a couple of other bugs in the DSL I need to look into, but then I can look into this further. I think we can make this work.

amexboy commented 6 years ago

Hi,

Thanks for the review. I have thought about the first challenge. I can think of a messy way but it's a long shot.

As for the second problem, I believe the validation should be left for the Change implementations. I just skimmed through the code and I think we can reject the changeset in org.liquibase.groovy.delegate.ChangeSetDelegate#makeColumnarChangeFromMap if it has a closure and doesn't have either a where clause or some columns. IMHO I don't think to let users define a closure that does nothing is not a bad thing.

Thanks for the review.

On Sat, Oct 6, 2018 at 10:12 PM Steven C. Saliman notifications@github.com wrote:

I had some time to take a deeper look at the code.

I really like the idea of getting changes from the registry. In fact, I think all changes should be fetched from the registry. This could be done easily enough by putting the registry lookup in its own helper and having all methods, including methodMissing use that helper. This would work for most use cases, but it does leave a couple of challenges...

First, how would we process external changes that have nested elements? The DSL represents nested elements as closures, but I can't think of any way to up a delegate for the closures in external changes. For now this could be a documented limitation since it lets us process more than we used to be able to handle.

Second, the new DSL code will ignore certain invalid change set code. For example, addAutoIncrement(tableName: 'myTable') { def x = 4 } currently complains because a closure is not valid for the addAutoIncrement change, but the new code would not complain. I don't like this because the code should not report success if it doesn't do everything it was asked to do. I think I could work around that problem by having a list of changes that support closures and throwing an exception if a given change has a closure and isn't on the list.

There are a couple of other bugs in the DSL I need to look into, but then I can look into this further. I think we can make this work.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/liquibase/liquibase-groovy-dsl/pull/36#issuecomment-427599135, or mute the thread https://github.com/notifications/unsubscribe-auth/AG_trmxup7hQvrSV5rInFQWU2o6poWnPks5uiQC2gaJpZM4WoTWz .

stevesaliman commented 6 years ago

Here is what I think I want to do:

  1. Create a method that takes a change name and returns an instance of a change. The exact class would be looked up from the registry.

  2. Change all methods that currently instantiate a Change class to call this new helper method. This means that a lot of methods that sent or received Class arguments would be refactored to use a name instead.

  3. Change methodMissing to try to get a change from the registry. If it can't find one, or the arguments passed to methodMissing are more than just a map, then we could fail as before.

IMHO, I think when users put something in a changelog they intend for Liquibase to do something with it, and Liquibase should not report success if it didn't run everything the user intended.

I should have some time over the weekend to work on this.

stevesaliman commented 6 years ago

I've implemented this fix, and resolved a few other issues at the same time:

  1. I implemented a lookupChange method as discussed before.

  2. I changed makeColumnarChange and makeChangeFromMap to use the new method.

  3. I implemented a methodMissing method. It will only handle changes with no arguments, or a single map argument. Anything else will throw an exception to let users know that they've asked us to process a change we can't parse. I also fixed a bug that was causing errors in the way changes were added to a rollback.

  4. I eliminated all of the standard changes that take a single map and had no special processing. They can flow through methodMissing now.

  5. I fixed the test that was failing because of the unexpected debug output from the Liquibase registry lookup - I did it by adding a logback-test.xml file to send the output to stderr instead of stdout.

  6. I moved the sample extension from src/main to src/test so we weren't shipping it with the DSL, but we could still use it to test the processing of extension based changes.

The changes are pushed, but I want to look into a possible Spring Boot bug before I make a release.

amexboy commented 6 years ago

Hi Steven

I'm sorry I couldn't help last weekend. I'm using it with spring boot, I've not had a problem. But let me know if you need help.

Amanuel Nega (Software Engineer) +251 947 01 25 21

On Sun, Oct 14, 2018 at 9:45 PM Steven C. Saliman notifications@github.com wrote:

I've implemented this fix, and resolved a few other issues at the same time:

1.

I implemented a lookupChange method as discussed before. 2.

I changed makeColumnarChange and makeChangeFromMap to use the new method. 3.

I implemented a methodMissing method. It will only handle changes with no arguments, or a single map argument. Anything else will throw an exception to let users know that they've asked us to process a change we can't parse. I also fixed a bug that was causing errors in the way changes were added to a rollback. 4.

I eliminated all of the standard changes that take a single map and had no special processing. They can flow through methodMissing now. 5.

I fixed the test that was failing because of the unexpected debug output from the Liquibase registry lookup - I did it by adding a logback-test.xml file to send the output to stderr instead of stdout. 6.

I moved the sample extension from src/main to src/test so we weren't shipping it with the DSL, but we could still use it to test the processing of extension based changes.

The changes are pushed, but I want to look into a possible Spring Boot bug before I make a release.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/liquibase/liquibase-groovy-dsl/pull/36#issuecomment-429651347, or mute the thread https://github.com/notifications/unsubscribe-auth/AG_trsMj9IjBaGA3DmDixykHitzD0bMNks5uk4ZdgaJpZM4WoTWz .

stevesaliman commented 6 years ago

I just released version 2.0.2 of the Groovy DSL with the changes that allow extension provided changes to be loaded. Thank you for all of your help on this issue.