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 sequenceExists check that became case sensitive for Postgresql #5911

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

davidecavestro
Copy link

@davidecavestro davidecavestro commented May 14, 2024

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

Fix regression on sequenceExists precondition, which became case sensitive from 4.24 on.
Replaces the equals operator with a ILIKE into the Postgresql specific SQL query defined as constant SQL_CHECK_POSTGRES_SEQUENCE_EXISTS, to also match sequences created (and checked for existence) with an uppercase name and automatically converted to lowercase by the dbms.

Additional Context

Fixes #5832

@MalloD12 MalloD12 changed the base branch from master to DAT-8444 May 28, 2024 02:43
@MalloD12 MalloD12 changed the base branch from DAT-8444 to master May 28, 2024 02:43
@MalloD12 MalloD12 self-requested a review May 28, 2024 02:45
@MalloD12
Copy link
Contributor

Hi @davidecavestro,

We appreciate your contribution and would be happy with you creating more PRs that solve issues from our backlog. Although, in this particular case I think the proposed solution is not totally accurate, the reason is if someone creates another sequence with a similar name to the previous one it was created it could match and. For example, if someone prior to deploy a changelog like the one shared in #5832 deployed a changeset like the one below:

<changeSet author="me" id="test-foo">
		<!-- create a new sequence --> 
		<createSequence sequenceName="MY_SQ_TEST_FOO"/>
</changeSet>

Then, the execution of the below changeset:

<changeSet author="me" id="test-bar">
		<preConditions onFail="HALT">
			<sequenceExists sequenceName="SQ_TEST_FOO"/>
		</preConditions>

		<createSequence sequenceName="SQ_TEST_BAR"/>
</changeSet>

could provide a wrong update output. In my opinion, what we can do for this Postgres case would be to just update the below line:

statement.setString(2, getSequenceName());

from SequenceExistsPrecondition.checkPostgresSequence() method by lowercasing the provided sequence name.

On the other hand, it would be nice if we can add some test coverage to test the provided solution, and as in this case this is a DB specific issue I would recommend you to at least add the Postgres integration test. What you can do is to add the provided example in #5832 to this src/test/resources/changelogs/pgsql/complete/root.changelog.xml.

Please let me know if you have any questions/concerns.

Thanks,
Daniel.

@davidecavestro
Copy link
Author

davidecavestro commented May 28, 2024

@MalloD12 sure, I'll definitely add some tests.
In the meantime please note that ILIKE ? doesn't use any wildcard - such as the percent sign - so it should be considered as a case-insensitive equals.
In other words I expect it just matches in a case insensitive fashion the specified sequence name (injected as-is through the ? param placeholder), while any prefixed or suffixed name shoudn't match.

@davidecavestro
Copy link
Author

@MalloD12 I've sketched a test for the false match and one for the proper match.
Please tell me if it's not the kind of tests you meant.

@MalloD12
Copy link
Contributor

Hi @davidecavestro,

Thanks for clarifying. When I saw the ILIKE I instantly thought of its common use with the % wildcard. I got you and this is perfectly fine with me. I appreciate you added those new test scenarios for our Postgres integration tests.

This PR looks good to me to move forward with it. I'll wait a bit more to let one of our Postgres test executions finish before approving this PR and leaving it in the queue so other members of the team can review/test.

Thanks,
Daniel.

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 and tests look good to me. Thank you Davide for this PR and for adding test coverage to it.

There are some test (from Test-Harness project) currently failing, but that's not related to the changes made here.

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

Successfully merging this pull request may close these issues.

sequenceExists became case sensitive from 4.24 on
4 participants