The <property /> definition for column type does not work in version 4.8.0, which works well in 3.x.x

[JosephCen]

Environment

Liquibase Version: 4.6.1

Liquibase Integration & Version: spring boot

Liquibase Extension(s) & Version: <n/a>

Database Vendor & Version: postgres-12 in container 'postgres:12-alpine'

Operating System Type & Version: Mac OS

Description

An exception java.lang.ArrayIndexOutOfBoundsException happened when run with below change log.

<databaseChangeLog ...>

    <property name="bytesarray_type" value="BYTEA" global="false" dbms="postgresql" />
    <property name="bytesarray_type" value="java.sql.Types.BLOB" global="false" dbms="hana" />

    <changeSet author="i521084" id="1583641912012-4">
        <createTable tableName="asyncevent">
            <column name="id" type="BIGINT">
                <constraints primaryKey="true" primaryKeyName="asyncevent_pkey"/>
            </column>
            <column name="messagebodybytes" type="${bytesarray_type}"/>
            <!-- ... -->
        </createTable>
    </changeSet>
</databaseChangeLog>

Steps To Reproduce

1. Using postgres DB in docker image 'postgres:12-alpine'.
2. Using a changelog as above.
3. Run the changelog with 'liquibase.integration.spring.SpringLiquibase.performUpdate'.
4. The exception will happened with handle above 'create table' change set.

Actual Behavior

An exception ArrayIndexOutOfBoundsException happened.

Expected/Desired Behavior

Relevant table should be created with above change log. And the same change log works well under Liquibase version 3.x.x.

Screenshots (if appropriate)

If applicable, add screenshots to help explain your problem.

[JosephCen]

This issue is same to #2231. But NOT fixed well in release 4.8.0.

[JosephCen]

In release 4.8.0, there is a 'isDuplicate(...)' function added to check whether new added parameter is existing. But the logic of 'isDuplicate(...)' may not correct.

Relevant code:
https://github.com/liquibase/liquibase/blob/v4.8.0/liquibase-core/src/main/java/liquibase/changelog/ChangeLogParameters.java#L143

Please take a look at below screen-shot:

(From above screen-shot, the 'param' & 'parameter' are different. But 'isDuplicate(...)' function will return 'true'.)

I guess the root cause is that the key of a parameter is not checked in below function:
https://github.com/liquibase/liquibase/blob/v4.8.0/liquibase-core/src/main/java/liquibase/changelog/ChangeLogParameters.java#L355
(I think the key of parameter is NOT checked in above logic.)

[JosephCen]

@nvoxland Could you please take a look at above issue? I think it is duplicated with #2231 and the fix is not perfect.

[kataggart]

@JosephCen thanks for the detail here; I will make sure we review this as soon as possible. Thanks again!

[dwieland]

The java.lang.ArrayIndexOutOfBoundsException happens when the property placeholder could not be replaced for some reason. In that case the ${bytesarray_type} is forwarded as is to the DataTypeFactory. The DataTypeFactory identifies the curly braces as some kind of embedded information in the type, i.e. int{autoIncrement:true}, and fails accessing the expected "value" after splitting the content of the curly braces by colon.

But: the example you provided works as long as postgresql or hana are used as database.

Question for the maintainers: Is this behaviour actually expected? In my opinion there are only 2 sane solutions:

1. Fail as soon as a missing property needs to be expanded
2. Expand missing properties as an empty string

Just leaving the property placeholder in there and failing in later stages is not a good solution tbh.

[nvoxland]

I think the original "what used to work on 3.x.x isn't working anymore" bug reported here IS a duplicate of #2231, and the logic in that is working correctly still.

I think what's @dwieland saying is the current state: as long as you are using a database which has a property value defined, it will work but if you use one that doesn't have a defined value, you get the ArrayIndexOutOfBoundsException exception still.

If Liquibase finds something that looks like a changelog property but doesn't match a defined property, we assume it's not actually one and just pass it along as-is. In the case of a datatype, though, our logic that parses the datatype to understand what it is gets confused by the format of something like ${invalid} and needs to simply pass it along unchanged.

I created #2772 which solves that problem.

BUT: is anyone still seeing problems a property that SHOULD be set correctly but is still throwing the ArrayIndexOutOfBoundsException? Or can I close this issue as resolved and let PR 2772 work it's way through on it's own?

[JosephCen]

@nvoxland I think it is not just a friendly exception message. The root cause is that 2 properties will be duplicated if they have same target DB and value.

Please look at below liquibase script:

    <property name="bytesarray_type" value="BYTEA" global="false" dbms="postgresql" />        <!-- (1) -->
    <property name="bytesarray_type" value="java.sql.Types.BLOB" global="false" dbms="hana" />
    <property name="messageproperties_type" value="BYTEA" global="false" dbms="postgresql" /> <!-- (2) -->
    <property name="messageproperties_type" value="NCLOB" global="false" dbms="hana" />

The script is expected to support 2 type DB. For postgres, 'BYTEA' column should be used for column for 'bytearray' and 'messageproperties'.
But in hana DB, these 2 columns should used different DB type.

But due to below isDuplicate logic. (1) & (2) will be classified as duplicated due to they have same target DB, value and label (no label). Finally, only one of (1) & (2) will be left.

            return StringUtil.equalsIgnoreCaseAndEmpty(contextString, otherContextString) &&
                   StringUtil.equalsIgnoreCaseAndEmpty(labelsString, otherLabelsString) &&
                   StringUtil.equalsIgnoreCaseAndEmpty(databases, otherDatabases);

ChangeLogParameter::isDuplicate

I think that key of these 2 parameters should be add to duplicated check as well.

[nvoxland]

I think the issue you mention @JosephCen is addressed in a larger refactoring I did in #2656. We'll double check that, though.

[kataggart]

@nvoxland did you have a chance to look to see if #2656 resolves this? Thanks!

[nvoxland]

Yes it should. I'll mark this as closed now