Skip to content

Conversation

filipelautert
Copy link
Collaborator

@filipelautert filipelautert commented Aug 1, 2024

Description

Consider that you have sql endDelimiter = "/" , and the following SQL:

a = var1 / 
var2;

How Liquibase should handle it? Should we split or divide? It can be understood as:

a = var1 / var2; --  'division behavior'

or

a = var1 ;   
var2; -- 'strict behavior'

Historically, we had:

Versions v4.9.0 to v4.14.x (only 5 months) were strict while all other versions are division. And with 4.29.0 we restored strict because of issue #5359 (this is a case where an upgrade is being done from Liquibase 4.9.1).

As we have 2 different behaviors we decided to support both by using the strict flag. So:

This is being documented so we don't change it again.

Fixes #6150

@filipelautert filipelautert force-pushed the fix-sql-enddelimeter-issues branch from 7d0237d to 52fd86c Compare August 2, 2024 17:37
@filipelautert filipelautert requested a deployment to internal August 5, 2024 18:01 — with GitHub Actions Abandoned
@filipelautert filipelautert force-pushed the fix-sql-enddelimeter-issues branch from 10ce265 to fd778f6 Compare August 5, 2024 20:03
@filipelautert filipelautert requested a review from rberezen August 13, 2024 19:35
@tati-qalified
Copy link
Contributor

@filipelautert the default behaviour is back to "division", which I understand is what we wanted.
However, when I try to use the --strict=true attribute, the run fails, and it doesn't seem to be related to the end delimiter at all.

I'm using the CLI and a basic XML changelog. Running liquibase --strict=true update.

Starting Liquibase at 09:40:06 using Java 17.0.7 (version [local build] #0 built at 2024-08-08 14:53+0000)
Liquibase Version: [local build]
Liquibase Open Source [local build] by Liquibase
ERROR: Exception Details
ERROR: Exception Primary Class:  IllegalArgumentException
ERROR: Exception Primary Reason:  Strict check failed due to undefined key(s) for 'internalRollbackOnError' command in file exists at path liquibase.properties':
 - 'changeLogFile'
 - 'password'
 - 'url'
 - 'username'
To define keys that could apply to any command, prefix it with 'liquibase.command.'
To disable strict checking, remove 'strict' from the file.
ERROR: Exception Primary Source:  H2 2.2.224 (2023-09-17)

Unexpected error running Liquibase: Strict check failed due to undefined key(s) for 'internalRollbackOnError' command in file exists at path liquibase.properties':
 - 'changeLogFile'
 - 'password'
 - 'url'
 - 'username'
To define keys that could apply to any command, prefix it with 'liquibase.command.'
To disable strict checking, remove 'strict' from the file.

Note that the exception is for internalRollbackOnError.
Any thoughts?

@rberezen
Copy link
Contributor

testing blocked by https://datical.atlassian.net/browse/DAT-18440

@filipelautert filipelautert modified the milestones: 1NEXT, 4.29.2 Aug 20, 2024
@filipelautert filipelautert changed the base branch from master to release August 20, 2024 18:18
@filipelautert
Copy link
Collaborator Author

Blocked by https://github.com/liquibase/liquibase-pro/pull/1909 . CC @tati-qalified

@rberezen
Copy link
Contributor

@tati-qalified @filipelautert this should be unblocked now

@tati-qalified
Copy link
Contributor

@rberezen @filipelautert using --strict=true doesn't fail anymore!

This changeset generates the same code, no matter if strict is true or false:

-- liquibase formatted sql
-- changeset AUTHOR:CHANGESET_NAME endDelimiter:GO rollbackEndDelimiter:GO runAlways:true runOnChange:true dbms:mssql
declare cursorToProcess cursor local
	read_only forward_only fast_forward
	for
		SELECT * FROM DATABASECHANGELOG;

open cursorToProcess;

Generates:

-- Changeset mssql.sql::CHANGESET_NAME::AUTHOR
declare cursorToProcess cursor local
	read_only forward_only fast_forward
	for
		SELECT * FROM DATABASECHANGELOG;

open cursorToProcess;
GO

I think we're good to go with this

@rberezen
Copy link
Contributor

rberezen commented Aug 23, 2024

@adrian-velonis1 @AMBERMW13
we need to update this section:

Newline behavior with a / end delimiter
In Liquibase 4.8.0 to 4.14.0, using the delimiter / (forward slash) in a PL/SQL block, for example as a division operator, causes Liquibase to fail on Oracle databases:

Caused by: liquibase.exception.LiquibaseException: liquibase.exception.MigrationFailedException: Migration failed for change set filepath/example-changeset.sql::example-id::example-author:
		Reason: liquibase.exception.DatabaseException: ORA-00900: invalid SQL statement
In Liquibase 4.15.0 to 4.28.0, Liquibase fixes this by requiring a newline before the / to treat it as a delimiter. Otherwise, the / is treated as a regular character. However, using a newline in the wrong place can lead to the error message "ORA-00922: missing or invalid option" on Oracle databases.

In Liquibase 4.29.0, Liquibase fixes this by requiring a newline after the / to treat it as a delimiter, not before. Otherwise, the / is treated as a regular character.

to

In Liquibase 4.29.2, the / is treated as a regular character when --strict=true and as a delimiter when --strict=false or not specified.
is it correct @filipelautert ?
https://docs.liquibase.com/change-types/enddelimiter-sql.html

@filipelautert
Copy link
Collaborator Author

In Liquibase 4.29.2, the / is treated as a regular character when --strict=true and as a delimiter when --strict=false or not specified.
is it correct @filipelautert ?
https://docs.liquibase.com/change-types/enddelimiter-sql.html

@rberezen yes, this seems a good description.

@filipelautert filipelautert merged commit b064029 into release Aug 23, 2024
41 of 42 checks passed
@filipelautert filipelautert deleted the fix-sql-enddelimeter-issues branch August 23, 2024 13:42
@adrian-velonis1
Copy link
Contributor

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

Successfully merging this pull request may close these issues.

EndDelimiter '/' not working as expected since 4.29
5 participants