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

add support for the TRAN keyword (T-SQL) #4099

Merged

Conversation

AlexGruebel
Copy link
Contributor

@AlexGruebel AlexGruebel commented Apr 6, 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

This PR fixes #4098. It add support for the T-SQL Keyword "TRAN".

Currently liquibase splits sql statements only, if there are not between a "BEGIN" and "END" statement. Since most database start transactions with the "BEGIN TRANSACTION" command, the StringUtil class which is responsible for splitting statements, does ignore the "BEGIN" keyword if it is followed by the keyword "TRANSACTION".

T-SQL (the sql dialect from mssql-server) also supports a shorter version for "TRANSACTION" => "TRAN". The current version of liquibase does not check for keyword "TRAN". This pull request adds support for this keyword.

Things to be aware of

  • I just added the new keyword "TRAN" to the if statement, with intention to change as little as possible

Things to worry about

  • the current code also supports the keyword "TRANS", I don't know any database which supports the keyword "TRANS" to start an transaction, maybe this was just a typo and can be removed. But I don't know maybe there is a reason for this:).

Additional Context

@nvoxland
Copy link
Contributor

nvoxland commented Apr 7, 2023

Initial review:

Change is what I'd expect. Thanks!

Things to be aware of:

  • Nothing

Things to worry about:

  • Nothing

@MalloD12 MalloD12 assigned filipelautert and unassigned MalloD12 Apr 28, 2023
@filipelautert filipelautert added sprint-50 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.

Thanks for the fix @AlexGruebel

@MalloD12 MalloD12 removed the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label May 12, 2023
@MalloD12 MalloD12 changed the base branch from master to 3.3.x May 12, 2023 18:34
@MalloD12 MalloD12 changed the base branch from 3.3.x to master May 12, 2023 18:34
@MalloD12 MalloD12 added the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label May 12, 2023
@MalloD12 MalloD12 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 12, 2023
@MalloD12 MalloD12 added the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label May 12, 2023
@XDelphiGrl
Copy link
Contributor

XDelphiGrl commented May 16, 2023

Rerunning test harness here: https://github.com/liquibase/liquibase-test-harness/actions/runs/4995573646 PASS

Note that this execution reran only the SQL Server database tests; all others passed (except for the endemic Titan issues on our HSQL and Firebird DBs).

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.

SQL Server supports the keyword TRANSACTION and the shorter variant, TRAN. This PR extends Liquibase to recognize TRAN and correctly apply delimiters. This prevents SQL execution errors due to improper "chunking" of SQL statements.

  • New integration test added with TRAN.
  • Functional and test harness executions passing.
  • No additional testing required.

APPROVED

@filipelautert filipelautert merged commit 3560103 into liquibase:master May 17, 2023
31 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DBMSSQLServer SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions sprint-50 TypeBug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

The T-SQL keyword "TRAN" is not supported in liquibase splitstatmenets mssql
7 participants