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

SQL Unique constraint generation for h2 should not contain "USING INDEX" clause #5410

Closed
2 tasks done
Guschtel opened this issue Dec 20, 2023 · 4 comments · Fixed by #5555
Closed
2 tasks done

SQL Unique constraint generation for h2 should not contain "USING INDEX" clause #5410

Guschtel opened this issue Dec 20, 2023 · 4 comments · Fixed by #5555

Comments

@Guschtel
Copy link
Contributor

Guschtel commented Dec 20, 2023

Search first

  • I searched and no similar issues were found

Description

When generating a diff changelog with liquibase, the output generation of unique constraints contains "USING INDEX" for a h2 database output sql file when the reference database was a oracle database. This could be improved by adding a condition !h2Database to the generation code.

The generated code is:

CREATE UNIQUE INDEX FALL_FKRQ_IX ON FALL(FALL_KEY, QUARTAL);
ALTER TABLE FALL ADD CONSTRAINT UC_FALL_KEY_QUARTAL UNIQUE (FALL_KEY, QUARTAL) USING INDEX FALL_FKRQ_IX;

But it should be

ALTER TABLE FALL ADD CONSTRAINT UC_FALL_KEY_QUARTAL UNIQUE (FALL_KEY, QUARTAL);

It works fine, when using a h2 database as a reference database.

The usecase we have is to automatically generate incremental diff changeset for various databases (using the liquibase gradle plugin) based on our Spring / JPA Entities compared to a reference Database (oracle).
So we compare the Spring JPA Entites with the Oracle Database and write changelog files for different database types.

Steps To Reproduce

@Table(
        name = "FALL",
        uniqueConstraints = {
                @UniqueConstraint(
                        name = "UC_FALL_KEY_QUARTAL",
                        columnNames = {"FALL_KEY", "QUARTAL"}
                )
        }
)
  • Use a spring url as a reference url (hibernate:spring:...)
  • Use a oracle jdbc url as a url
  • Set the changelog filename to my-filename.h2.sql
  • Generate diff changelog

Expected/Desired Behavior

Liquibase omits the "USING INDEX" clause when the database detected based on the filename is h2.

Liquibase Version

4.24.0

Database Vendor & Version

Oracle, H2

Liquibase Integration

gradle

Liquibase Extensions

No response

OS and/or Infrastructure Type/Provider

Windows 10

Additional Context

The fix would be to change

if (statement.getForIndexName() != null) {
            sql += " USING INDEX ";
            sql += database.escapeObjectName(statement.getForIndexCatalogName(), statement.getForIndexSchemaName(),
                statement.getForIndexName(), Index.class);
            isInUsingIndexClause = true;
        }

to

if (statement.getForIndexName() != null && !(database instanceof H2Database)) {
            sql += " USING INDEX ";
            sql += database.escapeObjectName(statement.getForIndexCatalogName(), statement.getForIndexSchemaName(),
                statement.getForIndexName(), Index.class);
            isInUsingIndexClause = true;
        }

in liquibase.sqlgenerator.core.AddUniqueConstraintGenerator#generateSql

Are you willing to submit a PR?

  • I'm willing to submit a PR (Thank you!)
@Guschtel
Copy link
Contributor Author

Might be related: #1384

@tati-qalified
Copy link
Contributor

Hi @Guschtel! Thank you for reporting this issue.
If you could go ahead and create a PR, our devs will review it and guide you if necessary. If you add a fix to the related issue you've mentioned, that would be great, but otherwise mention the issue in your PR so we can work on it.

Thanks again,
Tatiana

@MalloD12
Copy link
Contributor

Hi @Guschtel,

Your proposed code change looks good to me. Please go ahead and apply these changes when you wish. I think it would be nice to also add some H2 integration test/s for this so we have some test coverage for this case. I can help you with it if you have any questions.

You can also add go ahead and apply the required changes to fix #1384 as well in the same PR if you are happy to address that issue as well.

Thanks,
Daniel.

@Guschtel
Copy link
Contributor Author

Guschtel commented Feb 4, 2024

I haven't forgotten this issue :) I'll definitely look into it as soon as time allows 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants