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

#1531: Fix for CREATE PROCEDURE in SAP HANA dialect #1532

Merged
merged 9 commits into from Mar 3, 2017

Conversation

@ace130-github
Copy link

@ace130-github ace130-github commented Feb 15, 2017

This is a preliminary fix for #1531.

Some things might be reconsidered:

  • I'm not sure that the test and its associated resource are in the correct place
  • I wonder if it's appropriate to just throw a RuntimeException in case the syntax expectations are not met. Actually, I can't think of a more appropriate way to handle this situation. In any case, there might be a better suited subclass of RuntimeException.
  • There are some assumptions (ie BEGIN at the beginning, END; at the end of a line) that could possibly be at least documented.
  • And finally, there are a couple of other CREATE x-Statements (CREATE FUNCTION, ...) that might also benefit from that change, they could just be added to the if...-Statement.

Please let me know if the fix is going in the right direction, then I can work a little more on these items.

hanaws-1 added 5 commits Feb 15, 2017
…dures with embedded semicola.

Now recognizes that the statement ends only after the semicolon behind the terminating END;
Feat/support procedures with semicola

Patched Hana SqlStatementBuilder along with a test case

See merge request !1

private static final String PROCEDURE_TEST_SCRIPT_LOCATION = "/org/flywaydb/core/internal/dbsupport/saphana/procedures/procedure.sql";

private static String readResource(String name) {
Copy link
Contributor

@axelfontaine axelfontaine Feb 16, 2017

This can be replaced with new ClassPathResource(...).loadAsString("UTF-8")

Copy link
Author

@ace130-github ace130-github Feb 16, 2017

OK

@@ -44,22 +40,32 @@ protected String cleanToken(String token) {

@Override
protected Delimiter changeDelimiterIfNecessary(String line, Delimiter delimiter) {
if (StringUtils.countOccurrencesOf(statementStart, " ") < 4) {
Copy link
Contributor

@axelfontaine axelfontaine Feb 16, 2017

This change actually breaks the case where CREATE and PROCEDURE are on separate lines

Copy link
Author

@ace130-github ace130-github Feb 16, 2017

I found the approach useful to determine the start of the statement we're building. But it only worked in very limited cases. I tried to extend it by ignoring multiple blank lines, line comments and the beginning of block comments.

I also added tests specifically for the case you mentioned.


// TODO: When a line contains a CREATE X and and END statement, then the order is significant.
// TODO: There are a couple more statements allowing for nested begin/end blocks
if (line.startsWith("CREATE PROCEDURE") || line.startsWith("CREATE TRIGGER")) {
Copy link
Contributor

@axelfontaine axelfontaine Feb 16, 2017

Why is this nesting necessary? Can you point to a valid example SQL or better: add a test?

Copy link
Author

@ace130-github ace130-github Feb 16, 2017

Yes, BEGIN...END; blocks can be nested.
I have attached a test for that case.

assertEquals("Should recognize exactly three statements", 3, statements.size());
}

private static final String PROCEDURE_TEST_SCRIPT_LOCATION = "/org/flywaydb/core/internal/dbsupport/saphana/procedures/procedure.sql";
Copy link
Contributor

@axelfontaine axelfontaine Feb 16, 2017

Just inline this.

Copy link
Author

@ace130-github ace130-github Feb 16, 2017

OK

@ace130-github
Copy link
Author

@ace130-github ace130-github commented Feb 16, 2017

Thanks for your valuable feedback, I'll take some time to look into this.

-- a procedure with nested begin-end; blocks
-- also taken from
-- SAP HANA SQLScript Reference - SAP Library at:
-- http://help-legacy.sap.com/saphelp_hanaplatform/helpdata/en/f0/a6dceac8b94cca98dd2741ac6541b8/content.htm?frameset=/en/c6/558a64245942ebb52f6a6ddb3e4278/frameset.htm
Copy link
Contributor

@axelfontaine axelfontaine Feb 16, 2017

I can't see an example with nested blocks there. Where did you see it?

Copy link
Author

@ace130-github ace130-github Feb 16, 2017

Sorry, I used the "share" link on the SAP Help page, which seems not to include the full link.
I updated the link in file.

@ace130-github
Copy link
Author

@ace130-github ace130-github commented Feb 16, 2017

I resolved all your comments and added a couple of tests for these scenarios. In addition, I'll leave some replies to your comments explaining what I did.

I can still think of a thousand ways to break this functionality:

  • Have BEGINs not at the beginning and END;s not the end of a line, or both together in one line
  • CREATE PROCEDURE statements in "inner" lines of block comments
  • ...

In order to solve all these issues properly, we would have to have some kind of parser that's fully aware of the syntax (we're already implementing a part of it in this functionality). But this would be a major change in the base class, and would be misplaced to implement just in this subclass.

So I think we have covered fairly common cases which is better than nothing.

insideBeginEndBlock = false;
beginEndNestedDepth--;
if (beginEndNestedDepth < 0) {
throw new FlywayException("Syntax error in SQL script: unpaired END; statement");
Copy link
Contributor

@axelfontaine axelfontaine Feb 16, 2017

I wouldn't worry so much about this. A warning is probably enough here. If the script is incorrect (in itself, not due to our parsing) the DB will tell the user.

Copy link
Author

@ace130-github ace130-github Feb 17, 2017

OK, I replaced the exception with a warning in the logs,
Glad to see the pull request was added to the upcoming milestone.

@axelfontaine axelfontaine added this to the Flyway 5.0.0 milestone Feb 16, 2017
@axelfontaine axelfontaine added this to the Flyway 4.1.2 milestone Feb 17, 2017
@axelfontaine axelfontaine removed this from the Flyway 5.0.0 milestone Feb 17, 2017
@axelfontaine axelfontaine merged commit 4595315 into flyway:master Mar 3, 2017
dohrayme pushed a commit to dohrayme/flyway that referenced this issue Feb 3, 2020
flyway#1531: Fix for CREATE PROCEDURE in SAP HANA dialect
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants