Skip to content
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

Fix #2780: AddColumnChange with schema and NOT NULL constraint fails #2781

Merged
merged 5 commits into from
May 31, 2022

Conversation

MartinRied
Copy link
Contributor

@MartinRied MartinRied commented Apr 21, 2022

Environment

Liquibase Version: 4.9.1

Liquibase Integration & Version: Spring 5.3.15

Liquibase Extension(s) & Version: none

Database Vendor & Version: H2 1.2.212

Operating System Type & Version: Windows 11

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

Fixes #2780

With commit 3f62f95 the AddColumnChange was modified to not include NOT NULL constraints in the initial ALTER TABLE ... ADD statement but instead use an "embedded" AlterColumnChange to generate an ALTER TABLE ... ALTER COLUMN statement to be executed separately.

However when creating that AlterColumnChange, the schema property from the original AddColumnChange (as well as a number of other properties from the corresponding ConstraintsConfig) are not copied over to the new AlterColumnChange which means that when using a non-default schema the ALTER TABLE ... ALTER COLUMN targets the wrong schema.

Steps To Reproduce

    <changeSet id="0" author="example">
        <sql>create schema my_non_default_schema</sql>
    </changeSet>

    <changeSet author="admin" id="1">
        <createTable tableName="commands" schemaName="my_non_default_schema">
            <column name="command_id" type="NUMBER" autoIncrement="true">
                <constraints nullable="false" primaryKey="true" unique="false"/>
            </column>
            <column name="table_id" type="INTEGER">
                <constraints nullable="false" primaryKey="false" unique="false"/>
            </column>
            <column name="command_name" type="VARCHAR2(30)">
                <constraints nullable="false" primaryKey="false" unique="false"/>
            </column>
            <column name="command_exec" type="VARCHAR2(4000)">
                <constraints nullable="false" primaryKey="false" unique="false"/>
            </column>
        </createTable>
        <rollback>
            <dropTable tableName="commands" schemaName="my_non_default_schema"/>
        </rollback>
    </changeSet>
    <changeSet id="2" author="example">
        <addColumn tableName="commands" schemaName="my_non_default_schema">
            <!-- Add the command_type column, using "EXEC" for existing records -->
            <column name="command_type" type="VARCHAR2(30)" value="EXEC" defaultValue="EXEC">
                <constraints nullable="false" primaryKey="false" unique="false"/>
            </column>
        </addColumn>
    </changeSet>

Actual Behavior

Before Liquibase 4.7.0 the above change sets works fine.
Starting with Liquibase 4.7.0 the second change set fails at the SQL statement that is generated to add the NOT NULL constraint:

Reason: liquibase.exception.DatabaseException: Table "COMMANDS" not found; SQL statement:
ALTER TABLE PUBLIC.commands ALTER COLUMN command_type SET NOT NULL

The non-default schema given in the addColumn-Change is obviously not used when generating that statement.

Expected/Desired Behavior

The SQL statement generated by the second change set changes to:

ALTER TABLE my_non_default_schema.commands ALTER COLUMN command_type SET NOT NULL

This is executed without errors.

Screenshots (if appropriate)

n.a.

Additional Context

n.a.

Fast Track PR Acceptance Checklist:

  • Build is successful and all new and existing tests pass
    There are failures in liquibase.util.csv.CSVReaderTest and liquibase.util.grammar.SimpleSqlGrammarTest, but these look unrelated to the modifications to me.
    All other tests in liquibase-core pass.
  • Added Unit Test(s) -
    existing Unit Test liquibase-core/src/test/java/liquibase/sqlgenerator/core/AddColumnGeneratorTest.java has been updated to include the schema property.
  • Added Integration Test(s)
  • Added Test Harness Test(s)
  • Documentation Updated

TheDECKER and others added 3 commits April 21, 2022 09:05
Update the AddColumnChange test case to include a non-default schema & catalog, covering those two fields of the AddColumnChange.
The changeSet field from the AbstractChange is not covered by the test.
The original implementation delegating the creation of a NOT NULL constraint within the AddColumnChange to the AddNotNullConstraintChange was flawed in that it ignored some properties present both in the ConstraintsConfig and in the AddNotNullConstraintChange.
…I'm not quite certain of the expected outcome.
@kataggart kataggart linked an issue Apr 21, 2022 that may be closed by this pull request
@kataggart kataggart added this to To Do in Conditioning++ via automation Apr 21, 2022
Copy link
Contributor

@nvoxland nvoxland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review and test results:

Things to be aware of

  • Code changes makes sense for the change
  • I was able to reproduce the problem before the fix, and see it work after the fix
  • I didn't add a test-harness test since I don't think it handles multi-schema tests yet
  • Unit test was updated to ensure the schema is passed along if sent

Things to worry about

  • Nothing

@github-actions
Copy link

github-actions bot commented May 2, 2022

Unit Test Results

  4 512 files  ±  0    4 512 suites  ±0   34m 43s ⏱️ - 1m 53s
  4 414 tests  - 92    4 196 ✔️  - 96     218 💤 +4  0 ±0 
52 248 runs  ±  0  47 236 ✔️  -   4  5 012 💤 +4  0 ±0 

Results for commit 7f83cff. ± Comparison against base commit a574d1e.

♻️ This comment has been updated with latest results.

@nvoxland nvoxland removed their assignment May 26, 2022
Copy link
Contributor

@XDelphiGrl XDelphiGrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix passes schemaName property to the addNotNullConstraintChange.
    • Fix also sets the validate property for addNotNullConstraintChange.
  • Unit tests added.
  • No additional test required.

APPROVED

Passing Test Harness Execution
Passing Functional Tests

@nvoxland nvoxland merged commit 7c21cf5 into liquibase:master May 31, 2022
Conditioning++ automation moved this from To Do to Done May 31, 2022
@kataggart kataggart added this to the NEXT milestone Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

AddColumnChange with schema and NOT NULL constraint fails
6 participants