Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

includeAll() resolve FileSystemResource when run with spring-boot:run… #1645

Closed

Conversation

pramoth
Copy link

@pramoth pramoth commented Jan 21, 2021

…. But SpringResourceAccessor.openStream() alway add 'classpath:' prefix to resource string event if it has 'file:' url string already. it produce string like 'classpth:file:xxxxx' that wrong path. So it fail when getInputStream()

I fix by detect if it already has 'file:' no need to add extra 'classpath:'

Environment

Liquibase Version: 4.2.2

Liquibase Integration & Version: maven

Liquibase Extension(s) & Version: 4.2.2

Database Vendor & Version: All

Operating System Type & Version: Linux

Pull Request Type

  • [x ] 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

A clear and concise description of the issue being addressed. Additional guidance here.

  • Describe the actual problematic behavior.
  • Ensure private information is redacted

when mvn spring-boot:run includeAll() will load all file with ResourcePatternUtils.getResourcePatternResolver(resourceLoader).getResources(searchPath) it return FileSystemResource ( file: prefix )
when SpringResourceAccessor.openStream() is call for each file it add additional classpath: prefix (in method finalizeSearchPath() ). it will look like 'classpath:file:xxxx" which is wrong path hence it produce FileNotFoundException ( exception was swallow).

I fixed by detect if url with file: prefix then skip add classpath: to url.

Steps To Reproduce

List the steps to reproduce the behavior.

  • Please be precise and ensure private information is redacted

  • Include things like

    • Files used - sql scripts, changelog file(s), property file(s), config files, POM Files
    • Exact commands used - CLI, maven, gradle, spring boot, servlet, etc.

    add <includeAll path="some-directory" errorIfMissingOrEmpty="true"> to changelog file and run mvn-spring-boot:run it will cause error

    but it run fine with same configuration with `java -jar Xxx.jar'

Actual Behavior

A clear and concise description of what happens in the software before this pull request.

When use <includeAll path="some-directory" errorIfMissingOrEmpty="true"> with mvn spring-boot:run cause an error ( but it run fine with java -jar )
error message :

Caused by: liquibase.exception.ChangeLogParseException: The file file:/home/pramoth/work/project/e-aquafeed/e-aquafeed-web/src/main/resources/db/changelog/data-master/0000_aquatic_animal_group.sql was not found in
   - Spring resources
Specifying files by absolute path was removed in Liquibase 4.0. Please use a relative path or add '/' to the classpath parameter.
       at liquibase.parser.core.sql.SqlChangeLogParser.parse(SqlChangeLogParser.java:40)
       at liquibase.changelog.DatabaseChangeLog.include(DatabaseChangeLog.java:586)
       at liquibase.changelog.DatabaseChangeLog.includeAll(DatabaseChangeLog.java:544)
       ... 35 common frames omitted

Log also wrong because it run fine with java -jar hence message not relevant

Expected/Desired Behavior

A clear and concise description of what happens in the software after this pull request.
mvn spring-boot:run ---> PASS
java -jat Xxx.jar --> PASS

Screenshots (if appropriate)

If applicable, add screenshots to help explain your problem.

Additional Context

Add any other context about the problem here.

Fast Track PR Acceptance Checklist:

Need Help?

Come chat with us on our discord channel

…. But SpringResourceAccessor.openStream() alway add 'classpath:' prefix to resource string event if it has 'file:' url string already. it produce string like 'classpth:file:xxxxx' that wrong path. So it fail when getInputStream()

I fix by detect if it already has 'file:' no need to add extra 'classpath:'
@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #1645 (d7e9fd9) into master (6986dff) will increase coverage by 0.00%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1645   +/-   ##
=========================================
  Coverage     48.20%   48.20%           
- Complexity     7960     7961    +1     
=========================================
  Files           796      796           
  Lines         38705    38706    +1     
  Branches       6953     6953           
=========================================
+ Hits          18658    18660    +2     
+ Misses        17546    17545    -1     
  Partials       2501     2501           
Impacted Files Coverage Δ Complexity Δ
...ase/integration/spring/SpringResourceAccessor.java 70.49% <71.42%> (-1.18%) 16.00 <1.00> (ø)
...-core/src/main/java/liquibase/util/ObjectUtil.java 41.43% <0.00%> (+1.10%) 39.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6986dff...d7e9fd9. Read the comment docs.

@pramoth
Copy link
Author

pramoth commented Jan 21, 2021

I create demo project at https://github.com/pramoth/liquibase-mvn-run-fail-demo
clone and mvn spring-boot:run

@molivasdat
Copy link
Contributor

Hi @pramoth Is this a possible fix for #1595 and #1657?
Thanks for writing this up.
Here’s what happens next:

A member of the Liquibase team will take a look at your contribution and may suggest:

The PR will be prioritized according to our internal development and testing capacity.

We’ll let you know when it’s ready to move to the next step or if any changes are needed.

@molivasdat molivasdat added DBAll IntegrationMaven RiskMedium Changes that require more testing or that affect many different code paths. Severity3 TypeBug labels Jan 28, 2021
@molivasdat
Copy link
Contributor

Hi @pramoth This is superceded by this fix.
#1665
I validated it fixes your demo scenario, thanks for creating the demo bug repo. It helped validate 1665 will fix your issue as well.

@sync-by-unito
Copy link

sync-by-unito bot commented Feb 10, 2021

➤ Nathan Voxland commented:

I'll double check the code in this PR to make sure there isn't anything we still want to bring in vs #1665

@sync-by-unito
Copy link

sync-by-unito bot commented Feb 10, 2021

➤ Mario Champion commented:

hey Nathan Voxland -- after you check into this one make sure to let Erzsebet Carmean know so she can add test points.

@nvoxland
Copy link
Contributor

It looks like #1665 has the same fixes as this PR, so closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DBAll IntegrationMaven RiskMedium Changes that require more testing or that affect many different code paths. Severity3 TypeBug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants