Upgrade to Liquibase 4.0.0

[andrejpetras]

Hi,

I prepared the PR for the Liquibase v4 which has new "liquibase-core"

The main changes are:

• clean up LiquibaseProcessor - Liquibase services are now loaded with the Java ServiceLoader 🥳
• remove maven index for the Liquibase dependency
• remove unnecessary native image substitute
• remove OSGI maven dependency
• add new Liquibase resources to native image

I did few test on my laptop and compared it to 1.6.1 I got same or better time. My test application: https://github.com/andrejpetras/quarkus-liquibase-test

Quarkus 1.6.1

• developer mode: ~4s (reload)
• native image
  ** clean-at-start: true - native (powered by Quarkus 1.6.1.Final) started in 4.764s. Listening on: http://0.0.0.0:8080
  ** clean-at-start: false - native (powered by Quarkus 1.6.1.Final) started in 2.341s. Listening on: http://0.0.0.0:8080

Quarkus 999-SNAPSHOT

• developer mode: ~4s (reload)
• native image
  ** clean-at-start: true - native (powered by Quarkus 999-SNAPSHOT) started in 2.958s. Listening on: http://0.0.0.0:8080
  ** clean-at-start: false - native (powered by Quarkus 999-SNAPSHOT) started in 0.580s. Listening on: http://0.0.0.0:8080

I might not test all the possible scenarios which can this extension used in. I am open for any suggestions or examples which we need to add to the tests or test with test application.

Andrej

[famod]

Very nice!

So we had this discussion about waiting for 4.0.1 (just to be safe) in that other dependabot PR #10878.
Do you propose to merge this before 4.0.1 and hope for a 4.0.1 release before Quarkus 1.8 is released?

Btw, these are the issues slated for 4.0.1 so far: https://github.com/liquibase/liquibase/issues?q=is%3Aissue+is%3Aopen+label%3ATarget4.0.1

[andrejpetras]

@famod
Because of the big change in the Liquibase v4, I was not sure if this would be easy to implement.
It looks like it is much better than v3, and we have working Quarkus version with Liquibase v4.

Yes, I agree we can wait for the 4.0.1, there is no rush on this.
I will update this PR when we have Liquibase .4.0.1.

[gastaldi]

I think it's better to merge this PR now and let Dependabot do the easy work of bumping to a patch version later, but if you guys think it's safer to wait for 4.0.1, I'm fine with that 😉

[gastaldi]

LGTM, deleting code is always good 😄

[famod]

if you guys think it's safer to wait for 4.0.1, I'm fine with that

As there is only one bugfix planned for 4.0.1 so far (which is a rather special one IMO, the rest is enhancements), I am +/- 0 for merging now. 🤷‍♂️

[gastaldi]

@famod now I am curious. Got a link to this special bug?

[famod]

liquibase/liquibase#1277

With "special" I meant that it seems like a corner case, not a general showstopper or so.

[gsmet]

Nice work @andrejpetras , thanks for taking care of this!

Let's merge it, if something critical comes up, I'm sure they will release a 4.0.1 right away.

[famod]

@andrejpetras I just tried out the current master and I've spotted a problem:

We are using a custom @Observes StartupEvent bean to perform custom migrations (we don't use the built-in auto-migration).
In this migrator bean, we are using liquibase.resource.ClassLoaderResourceAccessor (with the TCCL) to find our changelog files.
This is working nicely in Quarkus 1.6.1, 1.7.0.CR1 and 1.7.0.CR2 but not with master. Can you imagine that something in this PR might have broken this?
If I find the time I'll try the commit directly before this PR and the commit directly after the merge to rule out other changes.

[andrejpetras]

@famod
could you please post example code?
I will try to reproduce it and also put it in the test application or tests. We should have covered this for next upgrade.

[famod]

@andrejpetras

I was able to confirm that this PR breaks our special use case. I guess something has changed in Liquibase 4.

could you please post example code?

Something like:

        final ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
        final ResourceAccessor resourceAccessor = new ClassLoaderResourceAccessor(contextClassLoader);
        final Set<String> changeLogs = Optional.ofNullable(resourceAccessor.list(null, "db/", true, false, false))
                .stream()
                .flatMap(Collection::stream)
                .filter(i -> i.toLowerCase().contains("changelog-"))
                .collect(Collectors.toSet());

This should (normally) find files like src/main/resources/db/changelog-foo.xml.

[andrejpetras]

@famod
It looks like the method list of the ResourceAccessor class in Liquibase 4 has been completely rewritten.

[famod]

Ok, thanks for checking. I'll have a look at this.

Disclaimer: This is a corner case and non-standard usage of the liquibase extension. Therefore I do not propose to roll back or "fix" this PR in any way.

[famod]

FTR, parameters of ResourceAccessor.list() switched places:
old:
(String relativeTo, String path, boolean includeFiles, boolean includeDirectories, boolean recursive)
new:
(String relativeTo, String path, boolean recursive, boolean includeFiles, boolean includeDirectories)

[gastaldi]

FTR, parameters of ResourceAccessor.list() switched places:

LOL, this is unbelievable 😆

[gastaldi]

But apparently this has 0 impact since we don't implement ResourceAccessor or call its list() method, right?

[famod]

LOL, this is unbelievable

Yeah, pretty dodgy. 🤦

But apparently this has 0 impact since we don't implement ResourceAccessor or call its list() method, right?

Not for Quarkus, but in my project we have to use a custom @Startup bean that invokes Liquibase and for that it uses some Quarkus Liquibase extension classes. In that bean we are using this method which is why I had this problem: #11147 (comment)

I just wanted to leave a clue for others that might be in the same position. 🙂

[famod]

Turns out that list() is broken for our use case in regular JVM mode (runner-jar). Dev mode and tests are fine though after switching the parameters. *sigh*
Appears to be a problem in Liquibase 4 because it also breaks when backporting this PR to Quarkus 1.7.4 (locally).