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

Fixed local XSD lookup #2830

Merged
merged 2 commits into from
May 10, 2022
Merged

Fixed local XSD lookup #2830

merged 2 commits into from
May 10, 2022

Conversation

nvoxland
Copy link
Contributor

@nvoxland nvoxland commented May 6, 2022

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

There is an issue in the XSD lookup logic, where it always pulls the XSD file over the network if the primary ResourceAccessor fails to find it and the "fallback" one does.

When we'd get to the fallback code, the && SECURE_PARSING) would be false and make the if block fail regardless of whether it found resources or not.

The standard Liquibase integrations (CLI, Maven, etc.) set up the primary resource accessor so we never hit the problem code path, but custom setups could. The integration tests, for example, did hit the problem code.

@nvoxland
Copy link
Contributor Author

nvoxland commented May 6, 2022

Notes and test results:

Things to be aware of

  • With my internet broken, the integration tests failed without this fix and pass with it in
  • Added "autocandidate" since it's not something reachable from the CLI, maven, etc.
  • It's an isolated fix to fallback logic

Things to worry about

  • Nothing

@github-actions
Copy link

github-actions bot commented May 6, 2022

Unit Test Results

  4 512 files  ±    0    4 512 suites  ±0   30m 18s ⏱️ - 12m 14s
  4 403 tests +  14    4 189 ✔️ +  14     214 💤 ±0  0 ±0 
52 116 runs  +168  47 108 ✔️ +168  5 008 💤 ±0  0 ±0 

Results for commit 258891a. ± Comparison against base commit 3728b75.

♻️ This comment has been updated with latest results.

…file but it's found by the fallback one, LiquibaseEntityResolver always loads it over the network
Copy link
Contributor

@XDelphiGrl XDelphiGrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix is specific and targeted.
  • All automated tests passed.
  • New parser test added to validate entity resolver.
    • Thank you! These tests are comprehensive and excellent!
  • No further testing required.

APPROVED

@XDelphiGrl XDelphiGrl self-assigned this May 10, 2022
@kataggart kataggart added this to the NEXT milestone May 10, 2022
@nvoxland nvoxland merged commit b6fab96 into master May 10, 2022
@nvoxland nvoxland deleted the fix-entityresolver branch May 10, 2022 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants