Extended formatted sql preconditions by table-/viewExists

[JulienMa94]

Impact

• Bug fix (non-breaking change which fixes expected existing functionality)
• Enhancement/New feature (adds functionality without impacting existing logic)
• Breaking change (fix or feature that would cause existing functionality to change)

Description

As described in (#3767) I extended the formatted sql parsing with a table-/viewExists precondition.

The user can add a tableExist precondition with the following syntax to his changeset.

--changeset "n marcq":"change 3" (stripComments:false splitStatements:false endDelimiter:X runOnChange:true runAlways:true contextFilter:y dbms:mysql runInTransaction:false failOnError:false)
--precondition-table-exists table:table1 schema:12345
create table table2 (
    id int primary key
);

or in the viewExists case like so

--changeset "n marcq":"change 3" (stripComments:false splitStatements:false endDelimiter:X runOnChange:true runAlways:true contextFilter:y dbms:mysql runInTransaction:false failOnError:false)
--precondition-view-exists view:view1 schema:12345'
create table table1 (
    id int primary key
);

As the documentation (https://docs.liquibase.com/concepts/changelogs/preconditions.html) says the parameter tableName / viewName are required and the schemaName is an optional parameter.

For this change some new regex patterns where introduced in the AbstractFormattedChangeLogParser and used inside the FormattedSqlChangeLogParser Class to extract the precondition type & parameters.

Addtionally two groovy tests targeting the parsing logic were added.

Additional Context

Some manuell testing with a postgres db was done to verify the changes on my local setup.

Case 1: Table Exists precondition fails because of a missing table

In the screenshot below we can see that the example.sql contains two changesets. The first creates a regular table. The second changeset tests with a precondition if a table is existing and creates a second table if the precondition passes.
On the right we can see a freshly started postgres database.

Precondition test failed because we searched for a non existing table.
Result only one table was created.

Case 2: Table Exists precondition successfully passed and runs the changeset

In the screenshot below we can see that the example.sql contains two changesets. The first creates a regular table. The second changeset tests with a precondition if a table is existing and creates a second table if the precondition passes.
On the right we can see a freshly started postgres database.

Precondition passes because we found a corresponding table.
Result reflects that two tables were created.

If any other preconditions would be beneficial for the formatted sql syntax, let me know I am happy to contribute some additional conditions.

Fixes #3767

[MalloD12]

There are a few of these messages in the parseTableExistsCondition() and in parseViewExistsCondition(). I imagine these were added for testing purposes when you developed these methods, if that's the case can we please remove them? But if you add them trying to log status or any specific action, please use a logger and add a message of what you want to track.

[JulienMa94]

I will remove them. They only were added for some testing purposes.

[MalloD12]

Hi @JulienMa94,

Thank you for creating this PR, I like this enhancement. I think overall code changes look good to me, but performing a bit of manual testing I noticed when trying to deploy a changeset like the one below using a precondition-view-exists clause, but without specifying a view name (which is a required field):

--changeset "n voxland":"change 6" (stripComments:false splitStatements:false endDelimiter:X runOnChange:true runAlways:true contextFilter:y runInTransaction:false failOnError:false)
--precondition-view-exists
create table table4 (
    id int primary key
);

Precondition is not correctly parsed, hence ignored, and change is deployed anyway without returning any error. The same applies to precondition-table-exists.

Can you please take a look at it?

Also, I think it would be nice if we could add some of these negative tests (without specifying required fields or providing null values), I think having a few more scenarios would give us some more confidence.

Thanks,
Daniel.

[JulienMa94]

Hi @MalloD12,

thx for your review I will look into this cases as soon I will have some time & add additional test cases 👍🏼

Greetings
Julien

[MalloD12]

Hi @MalloD12,

thx for your review I will look into this cases as soon I will have some time & add additional test cases 👍🏼

Greetings Julien

Sounds good, thank you for all your help!

Cheers,
Daniel.

[JulienMa94]

Hi @MalloD12,

I was able to reproduce the mentioned case above for table-exists, view-exists and also for the existing precondition sqlCheck.
This behaviour occured because no Regex Pattern matched in the AbstractFormattedChangeLogParser.

protected final String PRECONDITION_REGEX = String.format("\\s*%s[\\s]*precondition\\-([a-zA-Z0-9-]+) (.*)", getSingleLineCommentSequence());

The processing of any precondition is ignored at this time when no parameters are provided by the user. Leading to this misleading behaviour.

My suggestion is to enhance the AbstractFormattedChangeLogParser with a regex which captures the invalid regex values and calls an abstract Method handleInvalidEmptyPreconditionCase which is implemented by the FormattedSqlChangeLogParser.

AbstractFormattedChangeLogParser

Abstract Method

protected abstract void handleInvalidEmptyPreconditionCase(ChangeLogParameters changeLogParameters, ChangeSet changeSet, Matcher preconditionMatcher) throws ChangeLogParseException;

Invalid empty precondition regex

protected final String INVALID_EMPTY_PRECONDITION_REGEX = String.format("\\s*%s[\\s]*precondition\\-([a-zA-Z0-9-]+)", getSingleLineCommentSequence());

FormattedSqlChangeLogParser

@Override
    protected void handleInvalidEmptyPreconditionCase(ChangeLogParameters changeLogParameters, ChangeSet changeSet, Matcher preconditionMatcher) throws ChangeLogParseException {
        if (preconditionMatcher.groupCount() == 1) {
            String name = StringUtil.trimToNull(preconditionMatcher.group(1));
            if (name != null) {
                if ("sql-check".equals(name)) {
                    throw new ChangeLogParseException("Precondition sql check failed because of missing required Parameter expectedResult and sql.");
                } else if ("table-exists".equals(name)) {
                    throw new ChangeLogParseException("Precondition table exists failed because of missing required table name parameter.");
                } else if ("view-exists".equals(name)) {
                    throw new ChangeLogParseException("Precondition view exists failed because of missing required view name parameter.");
                } else {
                    throw new ChangeLogParseException("The '" + name + "' precondition type is not supported.");
                }
            }
        }
    }

In this case we handle an empty precondition case by throwing an ChangeLogParseException and inform the user about what parameters were missing.
I added three more tests to handle this case of an empty precondition for sqlCheck, tableExists & viewExists.

Greetings
Julien

[MalloD12]

Hi @MalloD12,

I was able to reproduce the mentioned case above for table-exists, view-exists and also for the existing precondition sqlCheck. This behaviour occured because no Regex Pattern matched in the AbstractFormattedChangeLogParser.

protected final String PRECONDITION_REGEX = String.format("\\s*%s[\\s]*precondition\\-([a-zA-Z0-9-]+) (.*)", getSingleLineCommentSequence());

The processing of any precondition is ignored at this time when no parameters are provided by the user. Leading to this misleading behaviour.

My suggestion is to enhance the AbstractFormattedChangeLogParser with a regex which captures the invalid regex values and calls an abstract Method handleInvalidEmptyPreconditionCase which is implemented by the FormattedSqlChangeLogParser.

AbstractFormattedChangeLogParser

Abstract Method

protected abstract void handleInvalidEmptyPreconditionCase(ChangeLogParameters changeLogParameters, ChangeSet changeSet, Matcher preconditionMatcher) throws ChangeLogParseException;

Invalid empty precondition regex

protected final String INVALID_EMPTY_PRECONDITION_REGEX = String.format("\\s*%s[\\s]*precondition\\-([a-zA-Z0-9-]+)", getSingleLineCommentSequence());

FormattedSqlChangeLogParser

@Override
    protected void handleInvalidEmptyPreconditionCase(ChangeLogParameters changeLogParameters, ChangeSet changeSet, Matcher preconditionMatcher) throws ChangeLogParseException {
        if (preconditionMatcher.groupCount() == 1) {
            String name = StringUtil.trimToNull(preconditionMatcher.group(1));
            if (name != null) {
                if ("sql-check".equals(name)) {
                    throw new ChangeLogParseException("Precondition sql check failed because of missing required Parameter expectedResult and sql.");
                } else if ("table-exists".equals(name)) {
                    throw new ChangeLogParseException("Precondition table exists failed because of missing required table name parameter.");
                } else if ("view-exists".equals(name)) {
                    throw new ChangeLogParseException("Precondition view exists failed because of missing required view name parameter.");
                } else {
                    throw new ChangeLogParseException("The '" + name + "' precondition type is not supported.");
                }
            }
        }
    }

In this case we handle an empty precondition case by throwing an ChangeLogParseException and inform the user about what parameters were missing. I added three more tests to handle this case of an empty precondition for sqlCheck, tableExists & viewExists.

Greetings Julien

Nice, thanks for implementing these new changes. I'll take some time to look into the new changes tomorrow.

Thanks,
Daniel.

[MalloD12]

Approved.

I made a very small change because there was one of the new tests failing. Manually verified all the scenarios are working, so in my opinion we are ready to go with this changes.

Thank you for all your work on this PR.

Daniel.

[tati-qalified]

The functionality is correct for databases H2, PostgreSQL and MSSQL. I think it's safe to assume it will work in general.
The print statements are still present, so they should be removed before merging to main.

[MalloD12]

The functionality is correct for databases H2, PostgreSQL and MSSQL. I think it's safe to assume it will work in general. The print statements are still present, so they should be removed before merging to main.

Good point! @JulienMa94 if you don't mind I'll clean them up.

Thanks,
Daniel.

[JulienMa94]

The functionality is correct for databases H2, PostgreSQL and MSSQL. I think it's safe to assume it will work in general. The print statements are still present, so they should be removed before merging to main.

Good point! @JulienMa94 if you don't mind I'll clean them up.

Thanks, Daniel.

Thanks Daniel! Sry for forgetting to remove the dev prints. I thought I already had removed them all.

Cheers,
Julien

[adrian-velonis1]

Internal doc: https://datical.atlassian.net/browse/PD-4389