Ensure backward-compatibility as part of the LiquibaseConfiguration API refactoring

[nvoxland]

Overview

In #1691, we made a large refactoring to the liquibase.configuration package which will break API compatibility with anyone using the old version.

After the refactoring is complete, collect up the changes to the API and determine which should have deprecated methods re-introduced to provide compatibility with existing extensions and integrations.

Code that will remain backward compatible

Code that looks up fields via getter methods

Existing code that looks up "Configuration" classes and calls get methods to read those configurations will continue to work without change.

Example:

LiquibaseConfiguration.getInstance().getConfiguration(GlobalConfiguration.class).getOutputEncoding()

Code that calls configuration setter methods

We no longer have setXYZ methods on the Configuration objects. Instead, wanted values to get set on the scope.

Example:

LiquibaseConfiguration.getInstance().getConfiguration(GlobalConfiguration.class).setOutputEncoding("UTF-8")

We will introduce a new ConfigurationValueSource which has a singleton map and can be used to store values set this way. Values set in this way will override all other sources EXCEPT for the new Scope-based method which replaces this pattern.

Custom configuration definitions

Example:

getContainer().addProperty(OUTPUT_ENCODING, String.class)
        .setDescription("Encoding to output text in. Defaults to file.encoding system property or UTF-8")
        .setDefaultValue("UTF-8")
        .addAlias("file.encoding");

Code that directly references the configuration's key name

Example:

String outputEncodingKey = GlobalConfiguration.OUTPUT_ENCODING;

Branch Policy

This compatibility work will be done in the LB-1222 branch.

[nvoxland]

With the API changes we have to make, we aren't able to keep 100% compatibility with the existing code.

In particular:

• The "setter" functions cannot stay as-is because we no longer have a simple configuration value store
• The way new properties are defined changed. Including the getContainer() part of getContainer().addProperty()... not existing anymore

The other breaking API changes which could have a deprecated backwards-compatible functions/variables re-introduced:

• GlobalConfiguration moved to a new package
• LiquibaseConfiguration.getInstance() is removed in favor of Scope.getSingleton(LiquibaseConfiguration)
• Removed getters on Configuration subclasses
• The public static variables containing configuration names, like GlobalConfiguration.SHOULD_RUN are now the full definition objects, not strings but re-used the same variable names.

Looking at our own extensions, as an example of how the existing configuration APIs are used:

• liquibase-compat calls LiquibaseConfiguration.getInstance().getConfiguration(GlobalConfiguration.class).getOutputEncoding()
• liquibase-cosmosdb calls the same sort of pattern, but for getDatabaseChangeLogLockWaitTime() and getDatabaseChangeLogLockPollRate()
• liquibase-mongodb defines a new MongoConfiguration class using the old style as well as looks up GlobalConfiguration properties like cosmosdb does

Trying to find open source 3rd party code that uses the configuration API:

• micronaut-projects/micronaut-liquibase uses LiquibaseConfiguration.getInstance().getConfiguration(GlobalConfiguration.class).getOutputEncoding();

I checked the following repos and found no use of the configuration APIs:

• Quarkus
• OpenMRS
• Portofino
• Grails database-migration
• keycloak
• Flowable App Engine
• JHipster
• Eclipse Kapua
• Zenbones smallmind
• jamesnetherton/wildfly-liquibase

Suggestion

1. Fix: Re-introduce the getInstance().getConfiguration() methods and create a deprecated subclass of GlobalConfiguration in the old package. This allows all the LiquibaseConfiguration.getInstance().getConfiguration(GlobalConfiguration.class) style calls to work.
2. Fix: Re-introduce deprecated versions of all the getOutputEncoding() type getters, but re-implemented to use the new value lookup logic
3. Keep broken: setter method calls
4. Keep broken: new property definition/setup
5. Keep broken: direct calls to the public static String OUTPUT_ENCODING = "outputEncoding" property names

Fixing 1 and 2 is straightforward to do and addresses the one 3rd party usage I found as well as 2 out of 3 of our extensions, so likely most people doing "normal" extension/integration stuff.

By leaving 3 and 4 broken, it means that anyone doing more complex configuration usage like defining your own configuration subclasses or manipulating the configuration state will need to be updated to work with 4.4.0. But, I think that will be a small to empty set of users, and they will be getting into the "can't be 100% compatible regardless" point anyway so not worth working to fix part when it's not going to fully work anyway.

5 is something we could fix by renaming the new version of the public static properties to something like OUTPUT_ENCODING_DEF and re-introducing the old version back as a string. But, since the new version of the API usage has users interacting directly with those fields, the extra _DEF suffix will be making all that future code more wordy and unwieldy. Since I found no usage of the direct usage of those fields (because that is what the getters are for) I don't think it is worth re-introducing those back.

Thoughts?

[sync-by-unito]

➤ Mario Champion commented:

for future readers, i m good with nathan’s proposal for what we fix and dont (at least immediately) so comment if you have a different take.

[sync-by-unito]

➤ Kevin Chappell commented:

Nathan Voxland - could you add examples of 3, 4, and 5 above? I want us to figure out a way to maintain backward compatibility so we make it easy for everyone to use the new Liquibase version.

[sync-by-unito]

➤ Mario Champion commented:

Note: good to keep in mind that API compatibility and user compatibility are two different concerns. I keep forgetting to be specific enough.

[sync-by-unito]

➤ Kevin Chappell commented:

Nathan Voxland I believe that an extra week of time to maintain backward-compatibility is worth it. We have a significant problem with people not moving to new versions of Liquibase. Anything we can do to remove friction and make it safe and seamless to adopt the latest version is worth it. We’ve seen improvements to Hub signups from removing friction. I suspect we’ll see the same here, if we do it well.

[sync-by-unito]

➤ Mario Champion commented:

very much agree if it is like a week, or even more. our one week, versus people being stuck, is worth it.

[nvoxland]

Because we moved liquibase.configuration.GlobalConfiguration to liquibase.GlobalConfiguration, I was able to keep the new definitions as liquibase.GlobalConfiguration.OUTPUT_ENCODING etc. without adding the _DEF suffix, since the deprecated string versions are on liquibase.configuration.GlobalConfiguration and they are both static

[nvoxland]

All the compatibility support should be in #1776 along side the overall refactoring work.

[sync-by-unito]

➤ Erzsebet Carmean commented:

All functional tests passed on the Liquibase internal Jenkins server. Thanks, Nathan Voxland! Moving to ready to merge.

[sync-by-unito]

➤ Erzsebet Carmean commented:

Moving back to UAT. The ticket is getting whiplash!

CC:: Mario Champion, Nehal Dixit

[sync-by-unito]

➤ Nehal Dixit commented:

Does UAT-ing this ticket cause any issues or blockers? Mike Olivas Tsvi Zandany ??

[sync-by-unito]

➤ Mike Olivas commented:

I just downloaded the #14 build and replaced my liquibase 431 with 4.4.0-SNAPSHOT build.
Ran liquibase history.

C:\Users\Administrator\git\CyclopsBI\Liquibase>liquibase history
Liquibase Pro 4.4.0-LB-1222-SNAPSHOT by Datical licensed to support until Wed Sep 22 23:59:59 CDT 2021
####################################################

_ _ _ _

| | () () |

| | _ __ _ _ _ | |_ __ _ ___ ___

| | | |/ _ | | | | | '_ \ / _ / __|/ _ \

| || | (| | || | | |) | (| _ \ __/

_/|_, |_,||./ _,|/__|

| |

|_|

Get documentation at docs.liquibase.com

Get certified courses at learn.liquibase.com

Free schema change activity reports at

https://hub.liquibase.com

####################################################
Starting Liquibase at 11:16:47 (version 4.4.0-LB-1222-SNAPSHOT #14 built at 2021-03-30 19:04+0000)
Unexpected error running Liquibase: java.lang.RuntimeException: Cannot find database driver: com.microsoft.sqlserver.jdb
c.SQLServerDriver
For more information, please use the --logLevel flag

[sync-by-unito]

➤ Nehal Dixit commented:

UAT failed moving it back into development
cc Nathan Voxland Mario Champion Erzsebet Carmean

[sync-by-unito]

➤ Mike Olivas commented:

After fixing the driver and adding it to lib, the rest of the use cases went great. This code driver went fine!