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

CORE-931: addColumn should support not-null constraint with an initia… #928

Merged
merged 13 commits into from Dec 3, 2021

Conversation

RG9
Copy link
Contributor

@RG9 RG9 commented Oct 22, 2019

Liquibase generates invalid SQL on addColumn changesets specified with a not-nullable column. The bug manifests when update is executed against a table that is populated with data. Bug is reproducible in 4.6.0.

Steps to Reproduce

Dev Notes

nvoxland this pull request should resolve JIRA ticket https://liquibase.jira.com/browse/CORE-931

Test Requirements (Internal Liquibase QA)

This test requires a Postgres instance; version does not matter. The test-harness will execute the test against the following versions: 9, 9.5, 10, 11, 12, 13. The attached changelog includes a changeset that replicates the bug as well as a changeset that was the workaround to the bug prior to the fix; we want to make sure both versions of adding a not-nullable constraint both work after the fix.

Manual Tests

Verify liquibase updatesql generates valid SQL for an addColumn with a notnullable constraint.

liquibase update-sql --changelog-file lb45-changelog.xml

  • Generated SQL for the changeset 2::addColumn is:
    ALTER TABLE lb45 ALTER COLUMN "columnWithInitialValue" SET NOT NULL;
  • Generated SQL for the changeset 3::addColumnAndAddNotNullConstraint is:
    ALTER TABLE lb45 ALTER COLUMN "columnWithConstraintInSeparateChange" SET NOT NULL;

Verify update is successful for adding a column with a not-nullable constraint on a populated table.

liquibase update --changelog-file lb45-changelog.xml

  • There is no error that column "columnWithInitialValue" contains null values.
  • Column is added to the table lb45.

Automated Tests

  • No new functional automated tests required.
  • A liquibase/liquibase-test-harness test is required.
    • Add an XML changelog with the following changesets included in the manual test changelog.
      • 1::createTable
      • 1::insertData
      • 2::addColumn
      • liquibase-test-harness/src/main/resources/liquibase/harness/change/changelogs/postgresql.
    • Add the JSON snapshot for test validation:
      • src/main/resources/liquibase/harness/change/expectedSnapshot/
        • You only need to validate the JSON for the changeset 2::addColumn
    • Add the generated SQL for test validation:
      • src/main/resources/liquibase/harness/change/expectedSql/
        • You only need to validate the SQL for the changeset 2::addColumn
    • Validate that a column with a not-nullable constraint can be added to a populated Postgres table.

┆Issue is synchronized with this Jira Bug by Unito

@datical-jenkins datical-jenkins changed the title CORE-931: addColumn should support not-null constraint with an initia… LB-45 ⁃ CORE-931: addColumn should support not-null constraint with an initia… Mar 4, 2020
@datical-jenkins datical-jenkins changed the title LB-45 ⁃ CORE-931: addColumn should support not-null constraint with an initia… CORE-931: addColumn should support not-null constraint with an initia… Mar 5, 2020
@ro-rah
Copy link

ro-rah commented Apr 27, 2020

Hi @RG9 !

Thanks for your pull request!

Here’s what happens next:

A member of the Liquibase team will take a look at your contribution and may suggest
changes, additional tests, or provide other feedback. The PR will be prioritized
according to our internal development and testing capacity.

We’ll let you know when it’s ready to move to the next step or if any changes are needed.

-Ronak

@ro-rah ro-rah added RiskLow Trivial changes in spelling, documentation changes, focused bug fixes, etc. Sprint7.5 and removed StatusDiscovery labels Apr 27, 2020
@ro-rah
Copy link

ro-rah commented Apr 27, 2020

Labeled Risk Low because unit tests added. Moving to Conditioning internally.

@mariochampion mariochampion moved this from To Do to Ready for Handoff (In JIRA) in Conditioning++ Nov 8, 2021
@StevenMassaro
Copy link
Contributor

@RG9 I'm going to update your fork with the latest changes from liquibase/master and resolve the merge conflict.

@StevenMassaro StevenMassaro changed the base branch from master to 4.4.x November 17, 2021 19:30
@StevenMassaro StevenMassaro changed the base branch from 4.4.x to master November 17, 2021 19:30
@sync-by-unito
Copy link

sync-by-unito bot commented Nov 17, 2021

➤ Steven Massaro commented:

Surya Aki Erzsebet Carmean Nathan Voxland This change looks to be dev complete to me. It is ready for QA sizing.

StevenMassaro and others added 2 commits November 18, 2021 07:22
The creation of the databasechangeloglock table was not threadsafe. If the creation of the table failed, Liquibase would exit. This change introduces a retry loop with a random wait between failed iterations so that multiple instances of Liquibase can be run concurrently on a fresh database.
Correctly default strict setting and use global configuration DAT-8547
@StevenMassaro StevenMassaro merged commit 3f62f95 into liquibase:master Dec 3, 2021
Conditioning++ automation moved this from Ready for Handoff (In JIRA) to Done Dec 3, 2021
@nvoxland nvoxland removed this from Done in Conditioning++ Jan 6, 2022
@nvoxland nvoxland added this to the v4.7.0 milestone Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DBPostgres ImpactLow IntegrationAny RiskLow Trivial changes in spelling, documentation changes, focused bug fixes, etc. Severity3 SyncTicket ThemeDBObjects TypeBug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet