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

Liquibase sql comment validation bug #3017

Closed
stephen-lev opened this issue Jun 29, 2022 · 3 comments · Fixed by #3037
Closed

Liquibase sql comment validation bug #3017

stephen-lev opened this issue Jun 29, 2022 · 3 comments · Fixed by #3037

Comments

@stephen-lev
Copy link

Environment

Liquibase Version: 4.10.0

Liquibase Integration & Version: maven
Liquibase Extension(s) & Version: irrelevant

Database Vendor & Version: irrelevant

Operating System Type & Version: irrelevant

Infrastructure Type/Provider: irrelevant

Description

Since the #2761 pull request the validation of formatted SQL changelogs changed, as far as i can comprehend the intention was partly to detect invalid patterns in the SQL files, however this resulted in a side effect, right now it will fail on any sql comment which has the word 'property' in it and it is not a liquibase property definition.
c30dddf
For every line the decision looks like this: If it is a valid property definition then parse it, else if it matches to 'altPropertyOneDashPattern' it is invalid and exception is thrown with an error message which is completely useles because it does not tell you the filename of the SQL file.

Steps To Reproduce

I provide you an example made with a little modification of your VALID_CHANGELOG from the FormattedSqlChangeLogParserTest.groovy test:

private static final String SHOULD_BE_VALID_CHANGELOG = """
--liquibase formatted sql

--property name:idProp value:1
--property name:authorProp value:nvoxland
--property nAmE:tableNameProp value:table1
--property name:runwith value: sqlplus


--changeset \${authorProp}:\${idProp}
select * from \${tableNameProp};


--changeset "n voxland":"change 2" (stripComments:false splitStatements:false endDelimiter:X runOnChange:true runAlways:true context:y dbms:mysql runInTransaction:false failOnError:false)
-- this is a problematic comment with the word property in it, it will cause the validator to throw an exception
create table table1 (
    id int primary key
);"""

Actual Behavior

SQL comments containing the word 'property' will cause the FormattedSqlChangeLogParser to fail with a very vague error message which don't even mention the file name.

Expected/Desired Behavior

I believe that such comments should be allowed, prior to 4.10.0 version there was no such problem, also the error message should tell me at least the file name of the SQL file which contains the error.

Screenshots (if appropriate)

image

Thank you for your work and I hope that i can help to improve this project with this issue.

@kataggart
Copy link
Contributor

@stephen-lev thanks for the details and feedback; we will definitely review

@AdrienHallet
Copy link

To add on the issue, I encountered a similar behavior when using a comment that contains the word 'comment';

Unexpected formatting at line 7. Formatted SQL changelogs require known formats, such as '--comment <comment>' and others to be recognized and run. Learn all the options at https://docs.liquibase.com/concepts/changelogs/sql-format.html

Which I guess is caused by the same parsing error. Also present in liquibase 4.11.0

@nvoxland
Copy link
Contributor

This has been fixed with #3037 in 4.14.0, including for things like -- comment too

Conditioning++ automation moved this from To Do to Done Jul 27, 2022
@nvoxland nvoxland linked a pull request Jul 27, 2022 that will close this issue
3 tasks
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 a pull request may close this issue.

4 participants