do not strip "classpath:" when normalizing the path #5894

andrus · 2024-05-08T20:25:31Z

Impact

This may or may not be a breaking change.

Description

This PR addresses the issue #5878 . It ensures that path "normalization" never strips the classpath: prefix when creating DatabaseChangeLog for child resources. The solution is submitted per @tati-qalified advise.

A user-oriented description:

This PR introduces an alternative behavior for sub-resource path resolution, namely if a root resource path starts with the classpath: prefix, it would preserve that prefix when resolving sub-resource locations. This behavior is activated with liquibase.preserveClasspathPrefixInNormalizedPaths property (it must be set to true), or via LIQUIBASE_PRESERVE_CLASSPATH_PREFIX_IN_NORMALIZED_PATHS variable or --preserve-classpath-prefix-in-normalized-paths CLI option. This feature allows to implement custom ResourceAccessors that work with abstract URLs without being directly tied to the filesystem, such as the one in Bootique.io.

Things to be aware of

The altered method is static and is widely used across Liquibase. This may or may not be a breaking change.

Things to worry about

I don't fully understand the consequences of NOT stripping classpath: prefix when resolving relative paths for other implementations of ResourceAccessor. Quite possibly some of them will need to be updated as well. But it seems logical: in a hierarchy of resources, child resource resolution should follow the same algorithm as the root resource.

Additional Context

See #5878

.. preserving it is the only way to resolve relative paths with a custom ResourceAccessor

into andrus-preserve-classpath

MalloD12 · 2024-05-28T18:48:25Z

Hi @andrus,

How are you doing? We very much appreciate you trying to fix #5878 and create this PR for it, but we can't merge it as it is. This PR is breaking some tests, but besides that, I have been discussing it with the team and we think this is a breaking change and we don't want to change the current behavior.

I think what we can do is define a new global configuration (you can have a look at src/main/java/liquibase/GlobalConfiguration.java to see some existent examples) and depending on the value set for this configuration then we can decide to strip/don't strip classpath: from the path. I think this way we would prevent breaking somebody else code.

If you have any questions/concerns about it please let us know and me or someone else from the team would be keen to help with you. Last thing, if you are keen to do address these modifications could you please add some unit tests to test the code changes made.

Thanks,
Daniel.

andrus · 2024-05-28T18:54:23Z

Hi @MalloD12 , I suspected we can't just change the policy globally. The suggested idea of doing it conditionally via GlobalConfiguration property should work. Not sure how hard it will be to write a test on the LB site, but I will definitely be happy to test it on Bootique side (and I already have unit tests there).

MalloD12 · 2024-05-28T18:57:46Z

You can try adding the tests also in Liquibase repo and I can help with it, or you can share the scenarios you would like to add and I can assist you on adding them.

Thanks,
Daniel.

andrus · 2024-05-28T18:59:14Z

Let me poke around

tests

.. preserving it is the only way to resolve relative paths with a custom ResourceAccessor

andrus · 2024-06-01T17:55:29Z

@MalloD12, I updated the PR. Now it has two commits:

c0d203b - a unit test that shows how relative paths do not work for a certain type of dynamic ResourceAccessors
94950ca - my original "fix" that will be replaced with GlobalConfiguration property as you suggested.

Hope this helps with that property implementation.

andrus · 2024-09-03T12:32:42Z

@MalloD12 thanks for the review and suggestions! Looks like we need one more approval Who can help with that?

MalloD12 · 2024-09-03T14:29:33Z

Hey @andrus, yeap someone else from the team needs to review this as well. Another Dev and QA team will review it, we are releasing a patch this week, so I would think if everything is ok for the rest of the team this change will be part of our next release (4.30.0).

Thanks,
Daniel.

filipelautert

Thanks for the improvement @andrus !

filipelautert · 2024-09-04T13:04:10Z

@MalloD12 could you create a documentation ticket for this PR?

liquibase-standard/src/test/java/liquibase/resource/UrlDrivenResourceAccessor.java

MalloD12 · 2024-09-04T18:18:37Z

Hi @andrus,

Could you please do us the favor of updating this PR description with the addition and usage of the newly introduced flag, so our docs team can update our docs accordingly?

Thanks,
Daniel.

MalloD12 · 2024-09-04T23:15:20Z

PD-5137 has been created for docs update.

Thanks,
Daniel.

andrus · 2024-09-05T21:21:55Z

@MalloD12

Could you please do us the favor of updating this PR description with the addition and usage of the newly introduced flag, so our docs team can update our docs accordingly?

Just added a paragraph to the top description.

andrus · 2024-10-07T06:50:23Z

Hi folks. Do we need any extra approvals to get this merged?

filipelautert · 2024-10-07T10:50:33Z

Hi folks. Do we need any extra approvals to get this merged?

@andrus nope, all fine! I'm getting it merged. Thanks for the PR!

liquibase-standard/src/test/java/liquibase/resource/UrlDrivenResourceAccessorTest.java

This PR introduces an alternative behavior for sub-resource path resolution, namely if a root resource path starts with the classpath: prefix, it would preserve that prefix when resolving sub-resource locations. This behavior is activated with liquibase.preserveClasspathPrefixInNormalizedPaths property (it must be set to true), or via LIQUIBASE_PRESERVE_CLASSPATH_PREFIX_IN_NORMALIZED_PATHS variable or --preserve-classpath-prefix-in-normalized-paths CLI option. This feature allows to implement custom ResourceAccessors that work with abstract URLs without being directly tied to the filesystem, such as the one in Bootique.io.

* changed code, started with test * Base tests * Modify tests * working on the groovy test * test: change order of stream closing and file deletion (#6380) * DAT-18563 :: Unhide tag parameter for UpdateTestingRollback command (#6330) - Unhide tag parameter for UpdateTestingRollback command. - Update command test. - Added integration test. * Removed usage of deprecated isEmpty() method (#6205) Removed usage of deprecated isEmpty method Co-authored-by: Daniel Mallorga <75833793+MalloD12@users.noreply.github.com> Co-authored-by: Daniel Mallorga <dmallorga@liquibase.com> Co-authored-by: filipe <flautert@liquibase.org> * #6281 fix NPE when default value is null (#6287) Sample: ```java java.lang.NullPointerException: Cannot invoke "String.replaceAll(String, String)" because "defaultValue" is null at liquibase.snapshot.jvm.ColumnSnapshotGenerator.readDefaultValue(ColumnSnapshotGenerator.java:571) at liquibase.snapshot.jvm.ColumnSnapshotGenerator.readColumn(ColumnSnapshotGenerator.java:291) at liquibase.snapshot.jvm.ColumnSnapshotGenerator.addTo(ColumnSnapshotGenerator.java:204) at liquibase.snapshot.jvm.JdbcSnapshotGenerator.snapshot(JdbcSnapshotGenerator.java:78) at liquibase.snapshot.SnapshotGeneratorChain.snapshot(SnapshotGeneratorChain.java:49) ``` * #6264 avoid ignoring the caught exception (#6288) * delete unused class UnknownConfigurationType * init field DatabaseIncapableOfOperation.operation it was not assigned in constructor; thus it was always NULL. P.S. This field is not used, so it should be removed. * #6264 avoid ignoring the caught exception I found all places where IDEA shows warning "catch block may ignore exception", and fixed many of them. * Adressing issue #6331, small changes to pom.xml files to reduce warnings shown in building (#6332) Use ${project.version} instead of ${version}; same goes for ${build.finalName}, which is recommended to switch to ${project.build.finalName}. Finally, use version for `maven-jar-plugin` consistently. Co-authored-by: Carlo Dapor <carlo.dapor@mimacom.com> * DAT-18743: dry-run: add current_branch_name (#6382) * missing ending fi statement * add simple logic, if the branch is neither "release" nor "master," it sets branch to the current branch name. * populate the value of the branch in dry-run-release.yml and pass it as outputs * populate the value of the branch in dry-run-release.yml and pass it as outputs --------- Co-authored-by: Sayali M <sayali@Sayalis-MacBook-Pro> * DAT-18329: comment out pieces of code that builds the Mac Installer (#6379) * comment out pieces of code that build the Mac Installer * delete macOS from install4j software and results produced are commented out * remove mac references --------- Co-authored-by: Sayali M <sayali@Sayalis-MacBook-Pro> * Handle Oracle blank schema names DAT-18199 (#6305) * Handle Oracle blank schema names DAT-18199 * Added comment DAT-18199 * Tweak last fix DAT-18199 * Another fix for the last fix DAT-18199 --------- Co-authored-by: suryaaki2 <80348493+suryaaki2@users.noreply.github.com> * DAT-18191 and DAT-18192 - Fix classpath loading and incorrect configuration for addForeignKeyConstraint (#6109) * fix: do not call DatabaseFactory before classpath parameter is evaluated by command line * chore: document changes/ refactor code * chore: remove offending method * fix: how sqlite supports initiallyDeferred and deferred if it doesn't work with addForeignKey ?! * fix: typo * Handle blank change set ID DAT-18687 (#6359) * WIP DAT-18687 * Handle blank change set IDs DAT-18687 * Add JavaDoc and another test --------- Co-authored-by: suryaaki2 <80348493+suryaaki2@users.noreply.github.com> * allow replaceIfExists in Postgres procedures (DAT-18495) (#6350) * allow replaceIfExists in Postgres procedures * reduce code duplication * CreateProcedure definition test updated to include postgresql as a supported database. --------- Co-authored-by: obovsunivskyii <baqaua@gmail.com> Co-authored-by: Daniel Mallorga <dmallorga@liquibase.com> Co-authored-by: suryaaki2 <80348493+suryaaki2@users.noreply.github.com> * fix CreateProcedureChange not working with mariadb (DAT-18495) (#6389) * Bump groovy.version from 4.0.22 to 4.0.23 (#6319) Bumps `groovy.version` from 4.0.22 to 4.0.23. Updates `org.apache.groovy:groovy-bom` from 4.0.22 to 4.0.23 - [Commits](https://github.com/apache/groovy/commits) Updates `org.apache.groovy:groovy-all` from 4.0.22 to 4.0.23 - [Commits](https://github.com/apache/groovy/commits) --- updated-dependencies: - dependency-name: org.apache.groovy:groovy-bom dependency-type: direct:production update-type: version-update:semver-patch - dependency-name: org.apache.groovy:groovy-all dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Add support for include columns in indexes (#6138) * Update ABOUT.txt * trying to get include columns to work with indexes * Update ABOUT.txt * Now with passing tests * got creating indexes to work properly. Still need to get generate changelog to work * got the generate-changelog command working with indexes that include columns * cleanup unnecesary changes in the diff * Added include field to the xsd * updated to use booleanutils * switched to non deprecated method * Added unit test coverage for index include feature * put generator into an if statement based on database * put more sections behind ifs on db type * do not strip "classpath:" when normalizing the path (#5894) This PR introduces an alternative behavior for sub-resource path resolution, namely if a root resource path starts with the classpath: prefix, it would preserve that prefix when resolving sub-resource locations. This behavior is activated with liquibase.preserveClasspathPrefixInNormalizedPaths property (it must be set to true), or via LIQUIBASE_PRESERVE_CLASSPATH_PREFIX_IN_NORMALIZED_PATHS variable or --preserve-classpath-prefix-in-normalized-paths CLI option. This feature allows to implement custom ResourceAccessors that work with abstract URLs without being directly tied to the filesystem, such as the one in Bootique.io. * Handle Snowflake add NOT NULL constraint DAT-18798 (#6391) * Fix generated SQL for Snowflake NOT NULL constraint DAT-18798 * Handle Snowflake NOT NULL constraint DAT-18798 --------- Co-authored-by: obovsunivskyii <baqaua@gmail.com> * chore: changes to make the test work - as it will be a PreparedStatement it needs to be supportsBatchUpdates - and as we are offline we need to override it. * Remove print statement * changed code, started with test * Base tests * Modify tests * working on the groovy test * chore: changes to make the test work - as it will be a PreparedStatement it needs to be supportsBatchUpdates - and as we are offline we need to override it. * Remove print statement * Changed string for the oracle clob test * Update names.csv * chore: take isRelativeToChangelogFile into account when evaluating the file location. * chore: fix variable name and tests --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: tatibeans <tatiana.f.ganz@gmail.com> Co-authored-by: Steven Massaro <steven.massaro.web@gmail.com> Co-authored-by: Daniel Mallorga <75833793+MalloD12@users.noreply.github.com> Co-authored-by: Anton Erofeev <antonerofeev11@gmail.com> Co-authored-by: Daniel Mallorga <dmallorga@liquibase.com> Co-authored-by: filipe <flautert@liquibase.org> Co-authored-by: Andrei Solntsev <andrei.solntsev@gmail.com> Co-authored-by: catull <catull@users.noreply.github.com> Co-authored-by: Carlo Dapor <carlo.dapor@mimacom.com> Co-authored-by: Sayali Mohadikar <76010603+sayaliM0412@users.noreply.github.com> Co-authored-by: Sayali M <sayali@Sayalis-MacBook-Pro> Co-authored-by: Wesley Willard <wwillard@liquibase.com> Co-authored-by: suryaaki2 <80348493+suryaaki2@users.noreply.github.com> Co-authored-by: obovsunivskyii <baqaua@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Stephen Atwell <78044713+stephenatwell@users.noreply.github.com> Co-authored-by: Andrus Adamchik <aadamchik@gmail.com> Co-authored-by: filipe <flautert@liquibase.com> Co-authored-by: rberezen <ruslan.berezenskyi@gmail.com>

do not strip "classpath:" when normalizing the path

b74cba2

.. preserving it is the only way to resolve relative paths with a custom ResourceAccessor

andrus requested a review from filipelautert as a code owner May 8, 2024 20:25

andrus had a problem deploying to external May 8, 2024 20:25 — with GitHub Actions Failure

Merge branch 'master' into preserve-classpath

6eccdf8

filipelautert had a problem deploying to external May 9, 2024 17:21 — with GitHub Actions Failure

filipelautert had a problem deploying to external May 9, 2024 17:21 — with GitHub Actions Error

kevin-atx added the TypeBug label May 10, 2024

kevin-atx linked an issue May 10, 2024 that may be closed by this pull request

Path normalization breaks classpath-relative paths with a custom ResourceAccessor #5878

Closed

2 tasks

Merge branch 'master' into preserve-classpath

f2e94aa

MalloD12 had a problem deploying to external May 21, 2024 19:32 — with GitHub Actions Error

Merge branch 'preserve-classpath' of https://github.com/andrus/liquibase

8389c46

into andrus-preserve-classpath

MalloD12 self-requested a review May 28, 2024 18:48

MalloD12 self-assigned this May 28, 2024

MalloD12 added 4 commits May 28, 2024 18:04

Create UnparsedSql object specifying endDelimiter value.

d091918

Merge branch 'master' of https://github.com/liquibase/liquibase

3c81c7a

Merge branch 'master' of https://github.com/liquibase/liquibase

c076662

Merge branch 'master' of https://github.com/liquibase/liquibase

f930934

andrus force-pushed the preserve-classpath branch from f2e94aa to d32a3e9 Compare June 1, 2024 15:48

andrus had a problem deploying to external June 1, 2024 15:49 — with GitHub Actions Error

andrus added 2 commits June 1, 2024 13:50

do not strip "classpath:" when normalizing the path liquibase#5894

c0d203b

tests

do not strip "classpath:" when normalizing the path

94950ca

.. preserving it is the only way to resolve relative paths with a custom ResourceAccessor

andrus force-pushed the preserve-classpath branch from d32a3e9 to 94950ca Compare June 1, 2024 17:51

andrus temporarily deployed to external June 1, 2024 17:51 — with GitHub Actions Inactive

Merge branch 'master' of https://github.com/liquibase/liquibase

ac05ffb

filipelautert approved these changes Sep 4, 2024

View reviewed changes

filipelautert requested review from rberezen, suryaaki2 and tati-qalified September 4, 2024 13:03

filipelautert added the DocNeeded label Sep 4, 2024

tati-qalified reviewed Sep 4, 2024

View reviewed changes

liquibase-standard/src/test/java/liquibase/resource/UrlDrivenResourceAccessor.java Show resolved Hide resolved

tati-qalified approved these changes Sep 4, 2024

View reviewed changes

Merge branch 'master' into preserve-classpath

2ac9cf0

rberezen temporarily deployed to external September 5, 2024 13:24 — with GitHub Actions Inactive

rberezen had a problem deploying to external September 5, 2024 13:30 — with GitHub Actions Failure

rberezen approved these changes Sep 5, 2024

View reviewed changes

Merge branch 'master' into preserve-classpath

38d90a3

filipelautert temporarily deployed to external October 7, 2024 10:48 — with GitHub Actions Inactive

filipelautert added this to the 1NEXT milestone Oct 7, 2024

fix: restore public method

6a6488a

filipelautert temporarily deployed to external October 7, 2024 11:11 — with GitHub Actions Inactive

github-advanced-security bot found potential problems Oct 7, 2024

View reviewed changes

filipelautert merged commit ff672de into liquibase:master Oct 7, 2024
34 of 35 checks passed

do not strip "classpath:" when normalizing the path #5894

do not strip "classpath:" when normalizing the path #5894

Uh oh!

Conversation

andrus commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Impact

Description

Things to be aware of

Things to worry about

Additional Context

Uh oh!

MalloD12 commented May 28, 2024

Uh oh!

andrus commented May 28, 2024

Uh oh!

MalloD12 commented May 28, 2024

Uh oh!

andrus commented May 28, 2024

Uh oh!

andrus commented Jun 1, 2024

Uh oh!

andrus commented Sep 3, 2024

Uh oh!

MalloD12 commented Sep 3, 2024

Uh oh!

filipelautert left a comment

Choose a reason for hiding this comment

Uh oh!

filipelautert commented Sep 4, 2024

Uh oh!

Uh oh!

MalloD12 commented Sep 4, 2024

Uh oh!

MalloD12 commented Sep 4, 2024 • edited by jira bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrus commented Sep 5, 2024

Uh oh!

andrus commented Oct 7, 2024

Uh oh!

filipelautert commented Oct 7, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrus commented May 8, 2024 •

edited

Loading

MalloD12 commented Sep 4, 2024 •

edited by jira bot

Loading