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

Do not split SQL on delimiters within BEGIN/END blocks #1589

Merged
merged 4 commits into from Jul 28, 2022
Merged

Conversation

nvoxland
Copy link
Contributor

@nvoxland nvoxland commented Dec 15, 2020

Description

Currently we apply the endDelimiter logic in parsing sql throughout the SQL. But stored logic, views, and other SQL will often have otherwise-delimiters inside their "body" which is delineated with BEGIN/END statements.

This updates the parsing logic to not look for delimiters while inside a BEGIN/END statement, thereby removing a lot of the need to specify "endDelimiter" in the first place.

NOTE: It will not count BEGIN/END TRANSACTION blocks as "begin/end" blocks

Fixes LB-958
Fixes #1553

Things to be aware of:

  • Added unit tests for the new types of sql to look for

Things to worry about:

  • Nothing

@molivasdat molivasdat linked an issue Dec 15, 2020 that may be closed by this pull request
@molivasdat molivasdat added DBAll ImpactMedium IntegrationAny RiskMedium Changes that require more testing or that affect many different code paths. Severity3 TypeEnhancement labels Dec 15, 2020
- Handle nested begin/end statements

LB-958
@molivasdat
Copy link
Contributor

@nvoxland Thanks for making the updates for nested BEGIN END statements. At least one more use case to consider that I missed earlier. What about BEGIN TRANS or BEGIN TRANSACTION statements?
https://www.mssqltips.com/sqlservertutorial/3305/what-does-begin-tran-rollback-tran-and-commit-tran-mean/
Not sure if folks still use them or those types of SQL statements will flow through this changed code.

@liquibase liquibase deleted a comment from sync-by-unito bot Mar 22, 2021
@nvoxland nvoxland changed the base branch from 4.3.x to master July 26, 2022 18:39
@github-actions
Copy link

github-actions bot commented Jul 26, 2022

Unit Test Results

  4 620 files  ±  0    4 620 suites  ±0   31m 57s ⏱️ - 5m 42s
  4 597 tests +  3    4 378 ✔️  -   1     219 💤 +4  0 ±0 
54 336 runs  +36  49 312 ✔️ +32  5 024 💤 +4  0 ±0 

Results for commit 7b23a4b. ± Comparison against base commit 65c65ba.

♻️ This comment has been updated with latest results.

@liquibase liquibase deleted a comment from codecov bot Jul 26, 2022
@nvoxland
Copy link
Contributor Author

Good point on Begin/end transaction statements, @molivasdat . It doesn't seem like something people should generally do, but I added a check for them in case.

@kataggart kataggart added this to To Do in Conditioning++ via automation Jul 27, 2022
@XDelphiGrl
Copy link
Contributor

@nvoxland, thank you for this improvement! Stored logic delimiters are one of the most frustrating bits of changeset configuration to get right. I think this will be helpful.

Copy link
Contributor

@XDelphiGrl XDelphiGrl left a comment

Choose a reason for hiding this comment

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

  • Enhancement improves handling of stored logic SQL that contains internally-delimited statements wrapped in BEGIN...END.
    • Liquibase no longer attempts to parse the statements within BEGIN...END keywords using the endDelimiter defined on the changeset. Statements with in BEGIN...END are taken as is for execution against the database.
  • New unit tests added to validate several BEGIN...END parsing use cases.
  • No additional testing required.

APPROVED

Passing Functional Tests
Passing Test Harness Execution

@nvoxland nvoxland merged commit 671fa1e into master Jul 28, 2022
Conditioning++ automation moved this from To Do to Done Jul 28, 2022
@nvoxland nvoxland deleted the LB-958 branch July 28, 2022 15:11
@kataggart
Copy link
Contributor

@nvoxland Do we have docs that need to be updated/improved with this change? Thanks!

@dozsa
Copy link

dozsa commented Mar 31, 2023

This created a regression for files containing begin and commit . Those now fail as their statements are not properly separated anymore. And unfortunately the way this was implemented it is not possible to know the closing statement for a begin statement, to only apply for begin-end and not other closing statements.

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

Successfully merging this pull request may close these issues.

Do not split SQL on delimiters within BEGIN/END blocks
6 participants