Skip to content

check if the next char out of string length #6436

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Nov 19, 2024
Merged

check if the next char out of string length #6436

merged 14 commits into from
Nov 19, 2024

Conversation

fntz
Copy link
Contributor

@fntz fntz commented Oct 17, 2024 •
edited by MalloD12
Loading

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 #6424

@fntz fntz requested a review from filipelautert as a code owner October 17, 2024 20:30
@fntz fntz changed the title check if the next char out of length check if the next char out of string length Oct 17, 2024
@MalloD12 MalloD12 self-requested a review October 24, 2024 16:03
@MalloD12
Copy link
Collaborator

MalloD12 commented Oct 24, 2024 •
edited
Loading

Hi Mike (@fntz),

Thank you for submitting a PR to fix #6424! Code changes added look good to me. I would only ask for adding some additional tests for this given issue. I was thinking we could add the changeset example provided in the description of the issue into the common.tests.changelog.xml file that contains a list of changesets that integration tests will deploy, that specific file will be executed by some of the different DBs that Liquibase support.

Example:

<changeSet id="createProcedureWithCommentedBodyTest" author="testUser">
		<createProcedure>
			/*CREATE PROC... */
		</createProcedure>
</changeSet>

Thanks,
Daniel.

@MalloD12 MalloD12 self-assigned this Oct 24, 2024
@MalloD12
Copy link
Collaborator

MalloD12 commented Nov 8, 2024

Hi Mike (@fntz),

Is there anything I can help you with on this PR? Are you ok to perform the suggested changes? If you are ok, I can perform those changes for you.

Thanks,
Daniel.

@MalloD12
Copy link
Collaborator

MalloD12 commented Nov 12, 2024 •
edited
Loading

Hi @fntz,

I've added a test scenario that contains the createProcedure change with an empty SQL body that will be executed by our integration tests. This given changeset is failing for some DBs like db2, hsql, oracle, and firebird. Some of the errors are like the one below:

Error:    DB2IntegrationTest>AbstractIntegrationTest.testUpdateTwice:547 » CommandExecution liquibase.exception.LiquibaseException: liquibase.exception.MigrationFailedException: Migration failed for changeset changelogs/common/common.tests.changelog.xml::createProcedureWithOnlyBlockCommentAsBody::mallod:
     Reason: liquibase.exception.DatabaseException: [jcc][10234][10927][4.33.31] SQL passed with no tokens. ERRORCODE=-4462, SQLSTATE=null [Failed SQL: (-4462) ]

See more of the execution details here.

which means that creteProcedure change is trying to execute the SQL body, but since this is a block comment it fails for the mentioned DBs.

MalloD12
MalloD12 approved these changes Nov 14, 2024
View reviewed changes
Copy link
Collaborator

@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, this helpful to avoid StringIndexOutOfBoundsException error happening again. Additionally to the unit test added we have added a test scenario that will be executed as part of our Integration tests. This test helped us to identify there are some DBs (like Oracle, HSQL, etc) that doesn't accept an empty/block of comment for the SQL body, because the given DB executor will try to execute that body, and it will fail because there are no SQL token present on it. We have decided to move forward with it leaving on user judgement that just a block of comment is not what some others DBs expect when creating a procedure.

@filipelautert filipelautert merged commit 4314b44 into liquibase:master Nov 19, 2024
37 of 39 checks passed
@filipelautert filipelautert added this to the 1NEXT milestone Nov 19, 2024
catull pushed a commit to catull/liquibase that referenced this pull request Nov 20, 2024
@fntz @MalloD12
check if the next char out of string length (liquibase#6436)
384db41 
* check if the next char out of length

* use correct string length

* Common integration test added for procedure with commented body scenario.

* Added dbms property to only run this test against a some specific DBs.

* Revert "Added dbms property to only run this test against a some specific DBs."

This reverts commit 1e54204.

* Added dbms property to only run this test against a some specific DBs.

---------

Co-authored-by: Daniel Mallorga <dmallorga@liquibase.com>
Co-authored-by: Daniel Mallorga <75833793+MalloD12@users.noreply.github.com>
@tati-qalified tati-qalified removed their request for review November 20, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filipelautert filipelautert filipelautert approved these changes

@rberezen rberezen rberezen approved these changes

@MalloD12 MalloD12 MalloD12 approved these changes

@suryaaki2 suryaaki2 Awaiting requested review from suryaaki2

Assignees

@MalloD12 MalloD12

Labels
manuallyTested runFunctional&HarnessTests TypeBug
Projects
Liquibase Open Source
Archived in project
Milestone
v4.31.0
Development

Successfully merging this pull request may close these issues.

createProcedure fails with exception if contains only comment as body
5 participants
@fntz @MalloD12 @filipelautert @rberezen @tati-qalified