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 H2 database case-sensitive identifier issues + upgrades JUnit tests cases to version 5 #4052

Merged
merged 10 commits into from
Aug 25, 2023
Merged

Fix H2 database case-sensitive identifier issues + upgrades JUnit tests cases to version 5 #4052

merged 10 commits into from
Aug 25, 2023

Conversation

mches
Copy link
Contributor

@mches mches commented Mar 30, 2023

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

On H2,

  1. Restore compatiblity with Liquibase 4.6.2 with respect to reserved words for H2 1.x.
  2. Fold identifiers that must be quoted due to being an illegal identifier or reserved word to uppercase so that all names are in a consistent case.
  3. Allow override of the defaults by way of objectQuotingStrategy on the change log or change set, liquibase.preserveSchemaCase configuration property, modifySql, or extension.

Fixes #2385
Fixes #3289

Things to be aware of

  • Imported the JUnit 5 BOM for better coordination of dependency versions, as a separate commit.
  • Corrected whitespace errors, e.g. tab indentation, in JUnit 4 database tests a separate commit.
    • IntelliJ IDEA ignores this commit for Git Blame by default.
    • It's also possible to ignore this commit in other tools by adding it to .git-blame-ignore-revs.
  • Converted JUnit 4 database tests to JUnit 5, as a separate commit.
  • Upgraded Groovy from 2.4 to 2.5, as a separate commit.
    • This allows use of the tap closure.
  • Configured Maven to also execute tests named *Spec, as a seprate commit.
    • This allows H2DatabaseTest can H2DatabaseSpec to coexist, so that the existing test doesn't have to be renamed or converted, along with it's class hierachy, to free up the name.
  • Established baseline specs for H2 and all other core databases, as a separate commit.
    • Tests were passing after each commit.
    • This makes it easier to see where potential bugs exist or when bugs are fixed or behavior changed.

Things to worry about

Some existing users' change sets or databases may require modification if they're targeting a long-lived embedded H2 database and have chosen names that require implicit quoting such as reserved words, e.g. user, group, groups, or value, or names with spaces.

Additional Context

Spring Boot users upgrading to 2.7.x and using jOOQ Open Source Edition often have to downgrade H2 to 1.4.x. See spring-projects/spring-boot#31168, https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-2.7-Release-Notes#jooq.

If desired, I can create separate PRs for

  1. Testing improvements
  2. H2 1.x reserved words
  3. Identifier case folding or preservation with respect to objectQuotingStrategy

@filipelautert filipelautert added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Mar 30, 2023
@nvoxland
Copy link
Contributor

nvoxland commented Apr 7, 2023

Initial review:

Thanks for the fix, and the other general cleanup in there to, @mches . I appreciate the separate commits for each step you did.

The conversion to JUnit 5 tests is nice. We've generally been converting from junit 4 tests to spock tests, but that takes time for each existing test when there is just a small change you'd like to make. Having them be the nicer junit 5 will help the times when it's not a good time to redo the whole test, but still want to take advantage of nicer syntax.

I also like the separate reserved word list for v1 vs v2. That helps us support both versions, which we need to continue doing.

In your last commit, you override quoteObject() which I don't think is the right spot for the fix. That method is generally used as "quote the object" where the caller is expecting it to be quoted. I see it's missing javadoc specifying that, though... So it seems like the "should we quote?" logic needs to be pushed upstream to determine whether the quoteObject method should be called or not? I didn't step through where you are seeing the problem to know where that is not working, but is there a better place?

Things to be aware of:

  • Nothing

@mches
Copy link
Contributor Author

mches commented Apr 8, 2023

Thank you for your time and feedback @nvoxland. Of course you're right about the purpose of each of the methods. I took that to heart and came up with a better organization of the logic. Hope you like it.

@kevin-atx kevin-atx added TypeBug RiskMedium Changes that require more testing or that affect many different code paths. DBH2 labels Apr 10, 2023
@mches mches changed the title Fix H2 case-sensitive identifier issues Fix H2, MySQL, MariaDB case-sensitive identifier issues Apr 11, 2023
@mches mches changed the title Fix H2, MySQL, MariaDB case-sensitive identifier issues Fix H2, MariaDB, MySQL case-sensitive identifier issues Apr 11, 2023
@mches mches changed the title Fix H2, MariaDB, MySQL case-sensitive identifier issues Fix H2 and other database case-sensitive identifier issues Apr 11, 2023
@mches mches changed the title Fix H2 and other database case-sensitive identifier issues Fix H2 database case-sensitive identifier issues Apr 13, 2023
@filipelautert filipelautert self-assigned this May 9, 2023
@filipelautert filipelautert added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions sprint2023-51 and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels May 9, 2023
Copy link
Collaborator

@filipelautert filipelautert left a comment

Choose a reason for hiding this comment

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

This PR is restoring compatibility with H2 1.x reserved words, and as an "extra" it also upgrades JUnit tests cases to version 5.
Thanks for those fix and improvements @mches . Also, I added some "merge branch master" commits - please feel free to rebase to get rid of them .

@filipelautert filipelautert added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels May 16, 2023
@suryaaki2 suryaaki2 requested a review from karepu July 20, 2023 19:20
@suryaaki2 suryaaki2 removed the request for review from karepu July 20, 2023 19:21
@filipelautert filipelautert requested review from karepu and removed request for karepu July 20, 2023 19:24
@mches
Copy link
Contributor Author

mches commented Jul 26, 2023

Resolved the conflict in liquibase-extension-testing/src/main/groovy/liquibase/extension/testing/command/CommandTests.groovy

@mches
Copy link
Contributor Author

mches commented Aug 25, 2023

Rebased onto latest master

@filipelautert filipelautert added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Aug 25, 2023
@filipelautert
Copy link
Collaborator

Thanks for the rebase! I'll get it merged today.

@filipelautert filipelautert merged commit 2fef580 into liquibase:master Aug 25, 2023
33 of 36 checks passed
@filipelautert filipelautert added this to the 1NEXT milestone Aug 25, 2023
@mches mches deleted the fix-h2-case-sensitive-identifier-issues branch August 27, 2023 01:28
@nealeu
Copy link

nealeu commented Aug 29, 2023

How do we take this change out for a test ride before the release? Should I be able to get it from Github packages without an access token for your repo?

@filipelautert
Copy link
Collaborator

Hey @nealeu , are you able to download it from here -> https://github.com/liquibase/liquibase/suites/15586269682/artifacts/891121515 ?

@shsmysore
Copy link

My H2 database has a table with one of the column name - key. (lower case reserved keyword)
How can I stop liquibase from automatically converting it to uppercase?

I tried adding below line in the changeset, but its not working.

<property name="preserveSchemaCase" value="true"/>

@mches
Copy link
Contributor Author

mches commented Oct 10, 2023

My H2 database has a table with one of the column name - key. (lower case reserved keyword) How can I stop liquibase from automatically converting it to uppercase?

I tried adding below line in the changeset, but its not working.

<property name="preserveSchemaCase" value="true"/>

Liquibase is uppercasing the reserved word key before quoting it in order to have it be treated case-insensitively. If you still want the key column to be lower-case, and therefore case-sensitive, then use something like this.

  <changeSet …>
    …
    <modifySql>
      <regExpReplace replace="&quot;KEY&quot;" with="&quot;key&quot;"/>
    </modifySql>
  </changeSet>

If you want to preserve the case of every identifier, then use someting like this.

  <changeSetobjectQuotingStrategy="QUOTE_ALL_OBJECTS">
    …
  </changeSet>

@shrralis
Copy link

@mches thanks for providing solutions!
The option with modifySql works great for me!
Anyway, it looks like it doesn't work for loadData, right?

@mches
Copy link
Contributor Author

mches commented Oct 15, 2023

@mches thanks for providing solutions! The option with modifySql works great for me! Anyway, it looks like it doesn't work for loadData, right?

@shrralis Yes, whether a bug or undocumented limitation, Liquibase doesn't seem to apply SQL visitors like modifySql to prepared statements like it does to raw SQL statements. The loadData change often ends up implicitly using prepared statements.

Knowing that limitiation, you may be able to work around it with something like this, but in some cases prepared statements are required.

  <changeSet …>
    <loadDatausePreparedStatements="false"/>
    <modifySql>
      <regExpReplace replace="&quot;KEY&quot;" with="&quot;key&quot;"/>
    </modifySql>
  </changeSet>

You could also use object quoting strategy, but then also uppercase the CSV file's headers other than the illegal identifiers.

  <changeSetobjectQuotingStrategy="QUOTE_ALL_OBJECTS">
    <loadData …/>
  </changeSet>
key,FIRST,LAST
0,john,doe
1,eric,smith
2,cat,jones

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DBH2 ReleaseMajor Fix needs to be part of an x.y release, not an x.y.z patch release RiskMedium Changes that require more testing or that affect many different code paths. SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions sprint2023-51 TypeBug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Liquibase 4.8 breaks H2 1.x compatibility Liquibase 4.7.0 upgrade causing issue with table name