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 unique constraint and primary key generator issues for Postgres and H2 #5555

Merged
merged 8 commits into from Mar 5, 2024

Conversation

Guschtel
Copy link
Contributor

@Guschtel Guschtel commented Feb 4, 2024

…the "using index" clause for the h2 database

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

Fixes #5410
Fixes #1384

Things to be aware of

Things to worry about

Additional Context

…the "using index" clause for the h2 database

Signed-off-by: Guschtel <github@guschtel.de>
@Guschtel
Copy link
Contributor Author

Guschtel commented Feb 4, 2024

Fixes #5410

@MalloD12 can you please advise if that is what you are expecting?

If so, I can have a look at #1384 in a similar way and add tests.

@MalloD12
Copy link
Contributor

MalloD12 commented Feb 5, 2024

Hi @Guschtel,

Code changes look good to me. The only thing I would ask is whether it's possible to instead of creating a Java class for tests we do it as a groovy test class. For new test suites, we tried to use that language.

Test would look like:

 @Unroll
 def  "test unique constraint using index for #database database"() {
 given:
        AddUniqueConstraintStatement statement = new AddUniqueConstraintStatement(null, null, TABLE_NAME, new ColumnConfig[]{new ColumnConfig().setName(COLUMN_NAME), new ColumnConfig().setName(COLUMN_NAME2)}, CONSTRAINT_NAME);
        statement.setForIndexName(INDEX_NAME);
then:
            assertEquals(expectedSql, this.generatorUnderTest.generateSql(statement, database, null)[0].toSql(), "Failed for " + database.getShortName()))
where:
database | expectedSql
new H2Database() |  "ALTER TABLE AddUQTest ADD CONSTRAINT UQ_TEST UNIQUE (colToMakeUQ, colToMakeUQ2)"
....       
    }

Thanks,
Daniel.

Signed-off-by: Guschtel <github@guschtel.de>
Allows forIndexName on Postgresql Databases for AddUniqueConstraint and AddPrimaryKey

Signed-off-by: Guschtel <github@guschtel.de>
@Guschtel
Copy link
Contributor Author

Guschtel commented Feb 10, 2024

Fixes #1384 and #5410

@MalloD12 have a look again, I tried to change the tests to Spock tests. First time using Spock, so please be kind if there's room to improve. :D
I didn't really find a way to parameterize the tests as i'm used to do it with JUnit, so i flattened out all possible cases.

I Also added the changes from #1384 and testcases for that.

@Guschtel Guschtel marked this pull request as ready for review February 10, 2024 18:36
@kevin-atx kevin-atx changed the title Fixes #5410 Fix for the Implementation of unique constraint generator so that it omits … Fixes #5410 H2 database - fix unique constraint generator so that it omits the "using index" clause Feb 13, 2024
Copy link
Contributor

@MalloD12 MalloD12 left a comment

Choose a reason for hiding this comment

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

Approved.

Code changes look good to me. Thank you @Guschtel for submitting this PR to fix two issues. Also, thank you for changing the tests format and use Spock (Groovy).

Build and most of the tests have been successfully executed (except functional test which seems to be failing for a non-related reason).

Thanks,
Daniel.

@MalloD12 MalloD12 changed the title Fixes #5410 H2 database - fix unique constraint generator so that it omits the "using index" clause Fix unique constraint and primary key generator issues for Postgres and H2 Feb 26, 2024
@filipelautert filipelautert merged commit 4c8d904 into liquibase:master Mar 5, 2024
30 of 32 checks passed
@filipelautert filipelautert added this to the 1NEXT milestone Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
4 participants