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

SpringLiquibase ResourceAccessor is now overridable #936

Merged
merged 1 commit into from Sep 18, 2020
Merged

SpringLiquibase ResourceAccessor is now overridable #936

merged 1 commit into from Sep 18, 2020

Conversation

WellingR
Copy link
Contributor

@WellingR WellingR commented Nov 16, 2019

SpringLiquibase createResourceOpener() now returns a ResourceAccessor, which allows it to be overridden with any kind of ResourceAccessor.

The reason for this change is that I would like SpringLiquibase to be able use the regular ClassLoaderResourceAccessor. Since it appears that SpringLiquibase handles classpath resources differently, which makes it incompatible with other methods of running liquibase (i.e. command line or maven-plugin).

This change itself does not change the behaviour of SpringLiquibase at all, it only allows classes overriding it to use a different ResourceAccessor

Notes:
It should be possible to merge this change to master too, however it appears that changes have already been made, such that the SpringResourceAccessor no longer overrides any methods from ClassLoaderResourceAccessor, making their behaviour identical.

…, which allows it to be overridden with any kind of ResourceAccessor
Copy link
Contributor

@SteveDonie SteveDonie left a comment

Choose a reason for hiding this comment

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

Looks like a safe change. Will consider for 3.8.3

@WellingR
Copy link
Contributor Author

@SteveDonie it looks like 3.8.3 and 3.8.4 have been released, but it seems that this change was not included.
I would apprecriate it if this would be included in an upcoming release

@WellingR
Copy link
Contributor Author

Ping @SteveDonie

@datical-jenkins datical-jenkins changed the title SpringLiquibase ResourceAccessor is now overridable LB-40 ⁃ SpringLiquibase ResourceAccessor is now overridable Mar 4, 2020
@datical-jenkins datical-jenkins changed the title LB-40 ⁃ SpringLiquibase ResourceAccessor is now overridable SpringLiquibase ResourceAccessor is now overridable Mar 5, 2020
@ro-rah
Copy link

ro-rah commented Apr 22, 2020

Hi @WellingR , sorry for the delay, I am looking to get this PR conditioned and scheduled.

Would you be willing to add unit test to the PR as well so we could prioritize merging this PR faster?

Looks like we stubbed out the tests here:
liquibase-core/src/test/java/liquibase/integration/spring/SpringLiquibaseTest.java

Thanks,

Ronak

@WellingR
Copy link
Contributor Author

@ro-rah what do you want me to test?
All this change does is relax a type declaration which is verified by the java compiler.

@ro-rah
Copy link

ro-rah commented Apr 22, 2020

Hi @WellingR , that is a good question. I am not a unit test expert but I was thinking is it possible to assert if the type declaration returned by SpringLiquibase is ClassLoaderResourceAccessor and not ResourceAccessor?

Or maybe I am thinking about it all wrong, perhaps we have a integration test that shows that SpringLiquibase is compatible with other ways of running liquibase.

Or, if none of those are appropriate, I would love suggestions on how we test out this fix :)

Thanks!

Ronak

@ro-rah ro-rah added RiskMedium Changes that require more testing or that affect many different code paths. StatusConditioning and removed StatusDiscovery labels May 21, 2020
@ro-rah
Copy link

ro-rah commented May 21, 2020

riskMed to figure out if we need tests.

@mariochampion mariochampion changed the base branch from 3.8.x to 3.10.x June 18, 2020 15:26
@nvoxland
Copy link
Contributor

There isn't really anything to test with it, it's just making a return type from a method more broad.

However, the change in signature is going to be an API incompatible change for anyone already extending SpringLiquibase, so this can't go into a patch release like 3.10.1. It'll have to go into 4.0

@WellingR
Copy link
Contributor Author

FYI, this change does not affect those who are already extending this class. Since the return type is more broad, java does allow subclasses to declare and return a subtype (such as the existing SpringResourceOpener ).

So unless I am missing something, this change is source (API) compatible. However I am not completely sure about binary compatibility of this signature change.

@WellingR
Copy link
Contributor Author

@nvoxland wasn't this going to be part of 4.0.0?

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

Successfully merging this pull request may close these issues.

None yet

6 participants