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

Improve addColumn handling in SQLite #1970

Merged
merged 7 commits into from Nov 9, 2021
Merged

Improve addColumn handling in SQLite #1970

merged 7 commits into from Nov 9, 2021

Conversation

nvoxland
Copy link
Contributor

@nvoxland nvoxland commented Jul 7, 2021

Environment

Liquibase Version: 4.4.0
Database Vendor & Version: SQLite

Description

Fixed a variety of issues with SQLite in addColumn. These problems were found by existing liquibase-test-harness tests that are now passing.

Fixed handling of multi-column addColumn statements

Sqlite failed running liquibase update with an addColumn change that used multiple columns.

<addColumn tableName="authors">
            <column name="varcharColumn" type="varchar(25)" value="INITIAL_VALUE"/>
            <column name="intColumn" type="int" valueNumeric="5"/>
            <column name="dateColumn" type="date" valueDate="2020-09-21"/>
        </addColumn>

Repo Steps

  1. Create a changeset using the above change
  2. Run liquibase update

Without the fix, it did not correctly add all 3 columns.

Fixed onDelete cascade

The onDelete=cascade for argument was not respected in addColumn changes. And, if a table contains existing foreign keys with onDelete=cascade, adding columns to that table loses that setting.

Repo Steps:

Fixed multi-column dropColumn

Sqlite failed running liquibase update with an dropColumn change that used multiple columns.

<dropColumn tableName="authors">
            <column name="varcharColumn"/>
            <column name="intColumn"/>
            <column name="dateColumn"/>
        </addColumn>

Repo Steps

  1. Create a changeset using the above change
  2. Run liquibase update

Without the fix, it failed with a "column does not exist" error.

ALTERNATE:

  1. create a changeset with <addColumn> using multiple nested {{}}s.
  2. Run liquibase update to apply it
  3. Run liquibase rollbackCount 1 to roll it back

Without the fix, it failed with a "column does not exist" error.

JSON Snapshot Serializer should not serialize null values

Sqlite does not use a username, and liquibase snapshot --format json would include username: "null" which is not correct. Instead, null values should be not included.

Repo steps

  1. Run liquibase snapshot --format json

The output should not include "null" for username or any attributes.

AddColumn fails with mixed-case column names

Fixed an issue with how quoting was used in the CopyRowsGenerator logic that is ran as part of addColumn. The multi-column example from test harness was using mixed-case column names which tripped up on the way quoting was done before.

Repo Steps

  1. <createTable> with all objects quoted, and mixed casing on column names
  2. insert data into that table
  3. Create an <addColumn> to the table
  4. Run liquibase update

Prior to the fix, the update failed


Dev Handoff Notes (Internal Use)

Links

Testing

Dev Verification

Ran a variety of addColumn and dropColumn tests locally, as well as ensured the test harness did not hit these problems anymore.

Test Requirements (Liquibase Internal QA)

Manual Tests

Add and Drop Multi-Column Changesets

Execute the manual tests against any of the Liquibase certified database platforms. After you deploy the XML changesets for creating the table and adding columns, you can use liquibase generate-changelog to create the JSON and YML changelogs. Of course, you do not want to try and generate those changelogs after you run the dropColumn changeset!

XML

<changeSet  id="1::table"  author="Liquibase Pro User" labels="table">  
    <createTable  tableName="authors">  
        <column  name="id"  type="int"/>  
    </createTable>  
</changeSet>

<changeSet  id="2::data"  author="Liquibase Pro User" labels="data">  
    <insert tableName="authors">  
        <column  name="id"  value="10"/>  
    </insert>  
    <insert tableName="authors">  
        <column  name="id"  value="20"/>  
    </insert>  
    <insert tableName="authors">  
        <column  name="id"  value="30"/>  
    </insert>  
</changeSet>

<changeSet  id="3::multiColumnAdd"  author="Liquibase Pro User" labels="addColumns">  
    <addColumn tableName="authors">
        <column name="varcharColumn" type="varchar(25)" value="INITIAL_VALUE"/>
        <column name="intColumn" type="int" valueNumeric="5"/>
        <column name="dateColumn" type="date" valueDate="2020-09-21"/>
    </addColumn>
</changeSet>

<changeSet  id="4::multiColumnDrop"  author="Liquibase Pro User" labels="dropColumns">  
    <dropColumn tableName="authors">
        <column name="varcharColumn"/>
        <column name="intColumn"/>
        <column name="dateColumn"/>
    </addColumn>
</changeSet>

Verify an update of a multi-column addColumn changeset is successful.

  • liquibase update --changelog-file <yourchangelog.xml> --labels table,addColumns
  • XML
  • YML
  • JSON

Verify an update of a multi-column dropColumn changeset is successful.

  • liquibase update --changelog-file <yourchangelog.xml> --labels dropColumns
  • XML
  • YML
  • JSON

Manual Test: Null Username in Snapshot

Setup

  • This test case requires a SQLite database to duplicate the JDBC connection string that does not contain a username.
  • Use the liquibase/liquibase-test-harness to launch a SQLite Docker.
  • Configure a liquibase.properties file with a connection to the SQLite DB.
    • Connection string: jdbc:sqlite:./src/test/resources/sqlite/sqlite.db

Verify that a JSON snapshot does not have “null” for the username.

  • liquibase snapshot --format json

Manual Test: Casing

Verify adding columns to a table with mixed-case columns is successful.

  • liquibase update --changelog-file <yourchangelog.xml> --labels table,data,addColumns
  • XML
  • YML
  • JSON

Verify the data is present in the table after update is complete.

  • Connect to the database and run select * authors

Automated Tests

  • Add new functional automated test to the liquibase-pro-tests suite.
  • Write a multi-db functional test that validates a multi-column addColumn and a multi-column dropColumn that:
    • validates update of addColumn is successful
    • validates rollback of addColumn is successful
    • validates update of dropColumn is successful
    • validates rollback of dropColumn is successful

┆Issue is synchronized with this Jira Bug by Unito

- Added jdbc driver as test dependency
- Fixed handling of multi-column addColumn statements
- Fix handling of onDelete=cascade for existing columns and new columns
- Do not include "null" values in yaml/json snapshot serializers
- Correctly escape object names in CopyRowsGenerator
- Fixed dropColumn with multiple columns
@nvoxland nvoxland added this to To Do in Conditioning++ via automation Jul 7, 2021
@nvoxland nvoxland moved this from To Do to Code Review in Conditioning++ Jul 7, 2021
- SQLiteIntegrationTest is broken enough I had to take the dependency out so it doesn't run as part of the builds at this point.
@suryaaki2 suryaaki2 self-requested a review July 27, 2021 16:19
base.pom.xml Show resolved Hide resolved
@suryaaki2 suryaaki2 moved this from Code Review to Ready for Handoff (In JIRA) in Conditioning++ Jul 27, 2021
@@ -160,6 +161,10 @@ protected String cleanNameFromDatabase(String objectName, Database database) {
}

}

if (returnList.size() == 0 && database instanceof SQLiteDatabase) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we expand this to other DBs? I found Firebird need same fix as it also fails with liquibase.exception.UnexpectedLiquibaseException: Found a null snapshotId for catalog null

Copy link
Contributor

Choose a reason for hiding this comment

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

changing if condition to include (database instanceof SQLiteDatabase || database instanceof FirebirdDatabase) helps with Firebird snapshot generation, but maybe this can be expanded further

Copy link
Contributor

Choose a reason for hiding this comment

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

@nvoxland -- please see @KushnirykOleh's comment above and provide guidance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is probably safe to include for all database, not just sqlite and firebird. I updated the code to just be if (returnlist.size() == 0)

@nvoxland nvoxland changed the base branch from 4.4.x to master October 19, 2021 14:00
@sync-by-unito sync-by-unito bot assigned yodzhubeiskyi and unassigned nvoxland Oct 28, 2021
@suryaaki2 suryaaki2 merged commit 9ee66e8 into master Nov 9, 2021
Conditioning++ automation moved this from Ready for Handoff (In JIRA) to Done Nov 9, 2021
@suryaaki2 suryaaki2 deleted the sqlite-improvements branch November 9, 2021 21:15
@nvoxland nvoxland removed this from Done in Conditioning++ Dec 1, 2021
@nvoxland nvoxland added this to the v4.6.2 milestone Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

addColumn breaks existing foreign keys addColumn ignores deleteCascade
5 participants