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

Flyway 5 regression when line in multi line string starts with single line comment #1927

Closed
manikantag opened this issue Feb 15, 2018 · 7 comments

Comments

@manikantag
Copy link

@manikantag manikantag commented Feb 15, 2018

What version of Flyway are you using?

5.0.7

Which client are you using? (Command-line, Java API, Maven plugin, Gradle plugin, SBT plugin, ANT tasks)

Command-line & Java API

What database are you using (type & version)?

MySQL 5.7, SQL Server, Oracle, DB2

What operating system are you using?

Linux

What did you do?

(Please include the content causing the issue, any relevant configuration settings, and the command you ran)

V1_test_code.sql

CREATE TABLE `certstore` (
  `cert` text,
  `last_modified_date` datetime DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

insert into certstore (cert) values ('old cert data');

update certstore set cert = '-----BEGIN CERTIFICATE-----
MIIEIjCCAwqgAwIBAgIIAd68xDltoBAwDQYJKoZIhvcNAQEFBQAwYjELMAkGA1UE
BhMCVVMxEzARBgNVBAoTCkFwcGxlIEluYy4xJjAkBgNVBAsTHUFwcGxlIENlcnRp
ZmljYXRpb24gQXV0aG9yaXR5MRYwFAYDVQQDEw1BcHBsZSBSb290IENBMB4XDTEz
MDIwNzIxNDg0N1oXDTIzMDIwNzIxNDg0N1owgZYxCzAJBgNVBAYTAlVTMRMwEQYD
VQQKDApBcHBsZSBJbmMuMSwwKgYDVQQLDCNBcHBsZSBXb3JsZHdpZGUgRGV2ZWxv
cGVyIFJlbGF0aW9uczFEMEIGA1UEAww7QXBwbGUgV29ybGR3aWRlIERldmVsb3Bl
ciBSZWxhdGlvbnMgQ2VydGlmaWNhdGlvbiBBdXRob3JpdHkwggEiMA0GCSqGSIb3
DQEBAQUAA4IBDwAwggEKAoIBAQDKOFSmy1aqyCQ5SOmM7uxfuH8mkbw0U3rOfGOA
YXdkXqUHI7Y5/lAtFVZYcC1+xG7BSoU+L/DehBqhV8mvexj/avoVEkkVCBmsqtsq
Mu2WY2hSFT2Miuy/axiV4AOsAX2XBWfODoWVN2rtCbauZ81RZJ/GXNG8V25nNYB2
NqSHgW44j9grFU57Jdhav06DwY3Sk9UacbVgnJ0zTlX5ElgMhrgWDcHld0WNUEi6
Ky3klIXh6MSdxmilsKP8Z35wugJZS3dCkTm59c3hTO/AO0iMpuUhXf1qarunFjVg
0uat80YpyejDi+l5wGphZxWy8P3laLxiX27Pmd3vG2P+kmWrAgMBAAGjgaYwgaMw
HQYDVR0OBBYEFIgnFwmpthhgi+zruvZHWcVSVKO3MA8GA1UdEwEB/wQFMAMBAf8w
HwYDVR0jBBgwFoAUK9BpR5R2Cf70a40uQKb3R01/CF4wLgYDVR0fBCcwJTAjoCGg
H4YdaHR0cDovL2NybC5hcHBsZS5jb20vcm9vdC5jcmwwDgYDVR0PAQH/BAQDAgGG
MBAGCiqGSIb3Y2QGAgEEAgUAMA0GCSqGSIb3DQEBBQUAA4IBAQBPz+9Zviz1smwv
j+4ThzLoBTWobot9yWkMudkXvHcs1Gfi/ZptOllc34MBvbKuKmFysa/Nw0Uwj6OD
Dc4dR7Txk4qjdJukw5hyhzs+r0ULklS5MruQGFNrCk4QttkdUGwhgAqJTleMa1s8
Pab93vcNIx0LSiaHP7qRkkykGRIZbVf1eliHe2iK5IaMSuviSRSqpd1VAKmuu0sw
ruGgsbwpgOYJd+W+NKIByn/c4grmO7i77LpilfMFY0GCzQ87HUyVpNur+cmV6U/k
TecmmYHpvPm0KdIBembhLoz2IYrF+Hjhga6/05Cdqa3zr/04GpZnMBxRpVzscYqC
tGwPDBUf
-----END CERTIFICATE-----', last_modified_date=NOW();

commit;
What did you expect to see?

The above V1_test_code.sql file should run without any issues as it is working as expected in 4.2.0, but is failing in 5.0.x because of a regression.

What did you see instead?

My analysis and fix:

This is a regression caused in Flyway 5.0.x codebase.

Regression was introduced in this line: https://github.com/flyway/flyway/blob/flyway-5.0.7/flyway-core/src/main/java/org/flywaydb/core/internal/database/SqlStatementBuilder.java#L199

if (endWithOpenMultilineStringLiteral() || insideMultiLineComment || isSingleLineComment(lineSimplified)) {
    statement.append(line);
    return;
}

But the same logic in 4.2.0 is: https://github.com/flyway/flyway/blob/flyway-4.2.0/flyway-core/src/main/java/org/flywaydb/core/internal/dbsupport/SqlStatementBuilder.java#L193

if (endWithOpenMultilineStringLiteral() || insideMultiLineComment) {
    statement.append(line);
    return;
}

In 5.0.x code, code is checking if the line starts with line comment (--) and if it does, then Flyway is thinking the that line is not terminating line and amusing there would be following lines in the same statement. But it is not checking if that comment chars are inside a multi line string literal value, which is exactly our case.

I've fixed it by adding extra check endWithOpenMultilineStringLiteral() for isSingleLineComment(lineSimplified) condition check, like below:

if (endWithOpenMultilineStringLiteral() || insideMultiLineComment || 
        ( endWithOpenMultilineStringLiteral() && isSingleLineComment(lineSimplified) )) {
    statement.append(line);
    return;
}

But isSingleLineComment() is used in other place also. Not sure if the same gaurd is required in that place also.

@mahidbdw
Copy link

@mahidbdw mahidbdw commented Feb 15, 2018

Thanks Mani for identifying the code which is causing this issue.

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Feb 15, 2018

Thanks for the report and the detailed analysis. The example you provided is for MySQL. Are you seeing this for the other 3 databases you mentioned as well?

@manikantag
Copy link
Author

@manikantag manikantag commented Feb 15, 2018

I've not tested with other DBs for this specific scenario. But as the fix is in common class, I assume it will affect others too.

@manikantag
Copy link
Author

@manikantag manikantag commented Feb 15, 2018

I also thought of adding the safe guard check in 'isSingleLineComment()' method itself, but I'm not sure if that could cause another regression, which I doubt though.

axelfontaine added a commit to flyway/flywaydb.org that referenced this issue Feb 17, 2018
@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Feb 17, 2018

Thanks for reporting. Fixed.

@manikantag
Copy link
Author

@manikantag manikantag commented Feb 17, 2018

Thanks for quick fix.

Just for my info - is 'isSingleLineComment()' not at required?

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Feb 17, 2018

@manikantag It would appear not. I couldn't find a single scenario where it actually made sense.

dohrayme pushed a commit to dohrayme/flyway that referenced this issue Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.