Spring Boot: better support relativeToChangelogfile when ResourceLoaders return FilteredReactiveWebContextResources

[erik-meuwese-topicus]

Pull Request Type

• Bug fix (non-breaking change which fixes an issue.)
• Enhancement/New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Description

Subclasses of FilteredReactiveWebContextResource and potentially others, make exists() always returns false in order to avoid exposing the whole classpath in a non-servlet environment. which impacts Liquibase's logic in figuring out relative paths.

The change to SpringResourceAccessor to rely on exists() was introduced in Liquibase 4.0, so this will fix some changelogs that worked in 3.x but not any versions.

Improves spring support as mentioned in #2281 for at least some configurations.

Example Fixed Scenario

file:/...../target/classes/liquibase/init/test.xml

<?xml version="1.0" encoding="UTF-8"?>
<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.5.xsd">
    <changeSet>
        <sqlFile path="test.sql" relativeToChangelogFile="true" splitStatements="false"/>
    </changeSet>
</databaseChangeLog>

file:/...../target/classes/liquibase/init/test.sql

Liquibase 4.9.1 fails to find test.sql relative to test.xml which contains the changelog

Actual Behavior

org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'liquibase' defined in class path resource [org/springframework/boot/autoconfigure/liquibase/LiquibaseAutoConfiguration$LiquibaseConfiguration.class]: Invocation of init method failed; nested exception is liquibase.exception.LiquibaseException: liquibase.exception.UnexpectedLiquibaseException: java.io.IOException: The file test.sql was not found in
    - file:/...../target/classes/
    - file:/...../target/classes/
Specifying files by absolute path was removed in Liquibase 4.0. Please use a relative path or add '/' to the classpath parameter.

Expected/Desired Behavior

2022-04-14 16:06:45.705  INFO 36588 --- [  restartedMain] liquibase.database                       : Set default schema name to public
2022-04-14 16:06:45.910  INFO 36588 --- [  restartedMain] liquibase.lockservice                    : Successfully acquired change log lock
2022-04-14 16:06:46.688  INFO 36588 --- [  restartedMain] liquibase.changelog                      : Reading from public.databasechangelog
Running Changeset: liquibase/init/test.xml::test-003::tester
2022-04-14 16:06:46.930  INFO 36588 --- [  restartedMain] liquibase.changelog                      : SQL in file test.sql executed
2022-04-14 16:06:46.938  INFO 36588 --- [  restartedMain] liquibase.changelog                      : ChangeSet liquibase/init/test.xml::test-003::tester ran successfully in 80ms
2022-04-14 16:06:46.966  INFO 36588 --- [  restartedMain] liquibase.lockservice                    : Successfully released change log lock

[nvoxland]

@erik-meuwese-topicus thanks for tracking down and describing the exists() problem in #2281 (comment) and making the PR for it.

I am not able to reproduce the problem to see your change fix it due to not getting spring set up in whatever configuration exposes it, but I understand how this can solve it.

I did push a change where I extracted the DefaultResourceLoader creation to the constructor and made a new getResource(String) method to wrap the logic you added to make it more extendable by people with other odd resource loader configurations. I also didn't want to completely abandon the regular resourceLoader in case it's able to find things not in the classpath. Can you check that my changes didn't break your fix, @erik-meuwese-topicus ?

I also updated the description to have it a bit more focused on the exists() problem vs. a more generic "fixes spring boot". Let me know if there is anything I got wrong in that change.

I also took the "Fixes" link to 2281 off since I'm not sure this fully fixes that overly generic "addresses spring issues" issue, and I don't want merging this to close the issue.

[nvoxland]

Code review and test results:

Things to be aware of

• I couldn't reproduce the problem to see the fix work, but I was able to tell the change didn't break the setup I had
• Fix improves the handling within certain spring boot setups without impacting others by falling back to the old logic if the new lookup doesn't find a resource

Things to worry about

• No tests added, but I need to spend more time that we have right now to figure out the right way to integrate spring testing into our suite. Unit tests didn't seem like they'd add enough value to come up with.

[github-actions]

Unit Test Results

  4 512 files  ±0    4 512 suites  ±0   36m 46s ⏱️ + 2m 39s
  4 414 tests ±0    4 200 ✔️ ±0     214 💤 ±0  0 ❌ ±0 
52 248 runs  ±0  47 240 ✔️ ±0  5 008 💤 ±0  0 ❌ ±0 

Results for commit ddb7b71. ± Comparison against base commit a574d1e.

♻️ This comment has been updated with latest results.

[XDelphiGrl]

• Code change introduces an appropriate redundancy check for ensuring Liquibase loads the expected classpath.
• Change is limited to Spring Boot.

Please Note! @nvoxland (Liquibase Founder) and several other Liquibase Engineers reviewed the code change and deem it low risk. However, there are no automated functional tests in place for Spring Boot. Be advised there is a larger possibility of unexpected side-effect due to the change than for similar changes with robust testing in place.

[XDelphiGrl]

I submitted a PR approval but just noticed that the build did not complete. Requesting that the build run and pass all developer-level tests prior to merging this fix.

[XDelphiGrl]

Build passed!

APPROVED

[erik-meuwese-topicus]

[nvoxland] this fixed our issue, thanks