Add configuration options for missing property handling

[dwieland]

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

Adds a new liquibase.missingPropertyMode setting with options of PRESERVE, EMPTY, and ERROR. The default value is PRESERVE which follows the old behavior of keeping the unexpanded property in the changelog file. EMPTY will replace it with an empty value, and ERROR will exit parsing with an error.

Example: if a changelog as Using value ${myValue} here and myValue is not defined, the result will be:

• PRESERVE: Using value ${myValue} here
• EMPTY: Using value here
• ERROR: Exits with error

This PR also does a cleanup/refactoring of the changelog property handling and the classes/methods associated with it to make the mode more maintainable and usable, as well as better avoid bugs such as #2586 and the ArrayIndexOutOfBoundsException issue listed below

ArrayIndexOutOfBoundsException Issue

Overview

The initial issue I encountered was a strange ArrayIndexOutOfBoundsException when running Liquibase. After some analysis I found out that it was related to #2586 - I tried to use specific varchar specifications for Oracle (VARCHAR(100 CHAR)) while keeping compatibility with MySQL. So I specified the type like this: type: VARCHAR(100 ${sql.type.varchar.char}), triggering the above issue as I forgot the set the property explicitly for MySQL.

After investigation I found that property placeholders are left untouched if the property could not be found in the current scope. After that the DataTypeFactory inspects the data type and assumes some embedded information based on the occurrence of a {. It tries to split the string in key and value and fails horribly doing so.

This PR tries to address multiple aspects of the issue. It allows to configure the behavior when trying to expand a placeholder of an unknown property - keeping backwards compatibility by default. The two new modes are: "replace with empty string" and "throw meaningful exception". It also tries to give a more meaningful exception when some placeholder reaches the DataTypeFactory.

Steps To Reproduce

Use a placeholder in createTable.columns.column[].type that is not backed by a property.

Actual Behavior

java.lang.ArrayIndexOutOfBoundsException is thrown, leaving the normal user clueless about what actually happened.

Expected/Desired Behavior

A meaningful error is presented to the user. The behavior in the described case is configurable.

Please tell me if additional unit or integration tests are needed.
I can update the documentation if I get some hints.

Fast Track PR Acceptance Checklist:

• Build is successful and all new and existing tests pass
• Added Unit Test(s)
• Added Integration Test(s)
• Added Test Harness Test(s)
• Documentation Updated

[kataggart]

@dwieland This is an interesting proposal. I'd love to know a bit more about the end use case/conditions you are dealing with that inspired you to create this if you wouldn't mind sharing. Thanks!

[JosephCen]

I guess that key should be compare in method 'isDuplicate' in class ChangeLogParameter here.

From below screen-shot, that these 2 ChangeLogParameters with different key. But according to logic, they are duplicated. I think the logic is in-corrected. ChangeLogParameter with different key means different parameter.

Refer below screen-shot:

[nvoxland]

Thanks for the PR!

In reviewing it, I took it over a bit to do a more extensive cleanup of the property handling code. We had been trying to just do some workaround patches in the property handling vs. the needed refactoring to avoid taking time to retest existing functionality.

But, your switch from the regexp-based searching to scanning makes me want to make sure we do a pass through the overall functionality already so adding in the other changes I'd been holding of on.

Changes I made to your original PR functionality:

• I renamed the missingPropertyMode options from LEGACY, EMPTY, THROW to PRESERVE, EMPTY, ERROR to be more descriptive and less java-y
• The "error" setting throws a ChangeLogParseException vs. an UnexpectedLiquibaseException
  I'm also going to update the description to be more focused on the overall change and new feature vs. the bug around datatypes

That seem ok to you @dwieland ?

Beyond that, it was:

• Adding missing javadocs/improving error messages
• Re-implemented the global vs. local parameters implementation in ChangeLogParameters, clearing out all the "TODO: This is ugly" comments that it resolved
• Created new set() vs setLocal() functions that made it more obvious what the passed DatabaseChangeLog was for and not
• Adjusted some class/function visibility based on how internal they should be
• Fixed issue with parameters being local by default in formatted sql when they are global everywhere else by default
• Cleaned up existing unit tests and added more

Notes and test results:

Things to be aware of

• Made some breaking API changes to classes/methods that were either previously broken or were ones that there was really no good reason for people to have been using.
• I preserved API compatibility in any methods that seemed at all likely people would be using

Things to worry about

• While I did a round of testing myself and added more automated tests, the changelog parameter logic can be complex and this rewrites a good chunk of it. So I'd like another set of eyes on it to see if I missed anything

[dwieland]

That seem ok to you @dwieland ?

That's totally fine. Thanks for looking into it!

[github-actions]

Unit Test Results

  4 512 files  ±    0    4 512 suites  ±0   38m 48s ⏱️ + 6m 19s
  4 453 tests +  34    4 239 ✔️ +  38     214 💤  - 4  0 ❌ ±0 
52 716 runs  +408  47 708 ✔️ +412  5 008 💤  - 4  0 ❌ ±0 

Results for commit dbe132a. ± Comparison against base commit 4ab687b.

♻️ This comment has been updated with latest results.

[nvoxland]

The failing functional test is unrelated to this change, so moving this along to test.

Things to be aware of

• The functional tests have existing tests of property support. My initial changes failed one of these tests (a value which has a property value as well)

Things to worry about

• This ended up a major refactoring of our property support, so we need good testing of the property behavior in general to make sure nothing was lost in the change.

[FBurguer]

For this PR, i checked if the new option --missing-property-mode with each param on hanaDB and MySQL, also I used the SQL script done by @JosephCen on issue #2586 with the modification they commented there.

PRESERVE: Using value ${myValue} here

• HanaDb
• Mysql
  UpdateSQL Output: CREATE TABLE asyncevent (id BIGINT NOT NULL, messagebodybytes ${BYTESARRAY_TYPE} NULL, CONSTRAINT asyncevent_pkey PRIMARY KEY (id));

EMPTY: Using value here

• HanaDb
• Mysql
  UpdateSQL Output: CREATE TABLE asyncevent (id BIGINT NOT NULL, messagebodybytes NULL, CONSTRAINT asyncevent_pkey PRIMARY KEY (id));

ERROR: Exits with error

• HanaDb
• Mysql
  UpdateSQL Output: Unexpected error running Liquibase: Could not resolve expression ${bytesarray_type} in file changelog.xml

It looks like liquibase with the option --missing-property-mode has the right behavior listed above.

Test Environment:
OS: Windows 10
Java 11

[kataggart]

@nvoxland can you please also update the docs? Thanks!

[minasgull]

Thanks for this enhancement. I run into its absence a few days ago and started looking for workarounds. But now i just wait till next release. Thank you a lot