-
Notifications
You must be signed in to change notification settings - Fork 41.5k
Support a generalized way to configure Liquibase global properties through Spring Boot configuration #47289
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
base: main
Are you sure you want to change the base?
Conversation
2ab53f3
to
19761a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, @dmiska25. I've left some comments for your consideration.
...c/main/java/org/springframework/boot/liquibase/autoconfigure/LiquibaseAutoConfiguration.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/springframework/boot/liquibase/autoconfigure/LiquibaseAutoConfiguration.java
Outdated
Show resolved
Hide resolved
// Remove any previously registered instance of our provider class | ||
liquibaseConfiguration.getProviders() | ||
.stream() | ||
.filter((provider) -> provider.getClass() == EnvironmentConfigurationValueProvider.class) | ||
.toList() | ||
.forEach(liquibaseConfiguration::unregisterProvider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully this won't be necessary when using a @Bean
method. If it is necessary, one scenario where the @Bean
method may be called multiple times is in tests with slightly different configuration that result in the creation of multiple contexts. Removing one provider in favour of a new one may cause problems there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this mostly for tests. This is needed because the Liquibase configuration providers are static and when the LiquibaseAutoConfigurationTests run, they destroy Spring context in between tests, leaving the register with a stale configuration variable. I could handle this inside each test, but I think it might be wise to keep in source for downstream consumers too. I don't think this would be an issue in production runs, but I'll lean on your experience for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could avoid the problem by sub-classing SpringLiquibase
and overriding afterPropertiesSet
to create a child scope before calling super
. This child scope could hold the EnvironmentConfigurationValueProvider
and, hopefully, remove any problems of multiple contexts interfering with each other. Our existing subclass (DataSourceClosingSpringLiquibase
) would become a sub-class of the proposed new sub-class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I understand correctly, your suggestion is to execute the Spring Liquibase logic within a Liquibase child scope with the new provider defined? If so, I created this subclass like this. But, in testing, what I found out is that Liquibase still registers its providers globally, even if the provider was registered to a child scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I think one way it could be done is if the provider was defined by the scope(scope id passed in during construction). The provider can then request the current scope id at any point and then just return false for a lookup if its not the appropriate scope it was created with. My only thought is whether it's necessary to have this level of separation? Is it realistic that two Spring Liquibases would ever run at the same time? If it is, I could implement this, but if it's not, I'd lean to simplicity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having two run in parallel is unlikely, but having them run serially with potentially different configuration is definitely possible both in tests and in apps with a context hierarchy. There's also the Actuator endpoint to consider where we use StandardChangeLogHistoryService at that can be called at any time, not just during application context refresh. I haven't examined the code path of the history service to know if it references configuration values that would be affected by configuration value providers.
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
|
||
class EnvironmentConfigurationValueProviderTests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some class javadoc. See other …Tests
classes for examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. I wasn't sure if @Author
is appropriate for community members, so I left that out; If you want me to set that for myself or you, I can do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do add an @author
tag. It's absolutely appropriate for community members. It's warranted on the other classes that you have created or modified as well.
.../org/springframework/boot/liquibase/autoconfigure/EnvironmentConfigurationValueProvider.java
Outdated
Show resolved
Hide resolved
…-through Signed-off-by: Dylan Miska <djmiska25@gmail.com>
…on and align code standards to match Spring Boot convention. Signed-off-by: Dylan Miska <djmiska25@gmail.com>
19761a2
to
8ba8d5d
Compare
Summary
This PR introduces support for configuring Liquibase’s global properties directly through Spring Boot’s configuration system.
A new map property has been added:
At startup, these entries are passed through to Liquibase’s
ConfigurationValueProvider
. This aligns with how Spring Boot already supports pass-through properties for JPA (spring.jpa.properties.*
).Motivation
Liquibase supports an expanding set of global configuration options (e.g.,
duplicateFileMode
,logLevel
,searchPath
). Until now, these could only be set using JVM system properties or environment variables if they were not specified in the Spring Boot configuration.That approach created fragmentation: most Liquibase settings were configured under
spring.liquibase.*
, while global properties had to be managed separately. This change unifies configuration, allowing Spring users to manage all Liquibase options from one place.My motivating use case: we had multiple Liquibase changelog files and frequently saw an error indicating duplicates. The recommended fix was to set
liquibase.duplicateFileMode=WARN
. The workaround was to set it in Gradle system properties, but that made testing and environment alignment harder.Design
EnvironmentConfigurationValueProvider
bridges Spring’sEnvironment
to Liquibase’s configuration system.spring.liquibase.properties.*
are read exactly as-is (no relaxed binding or rewriting).Precedence Choice
The provider is registered with a precedence of 100, above defaults but below environment variables.
--
)Reference: Liquibase parameters documentation.
Testing
application.yml
andapplication.properties
.Documentation
spring.liquibase.properties
to clarify usage, with an example key.Implementation Notes
Fixes #47212