fix(java client): allow java client to accept statements with more than one semicolon #7243
Conversation
ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/impl/DdlDmlRequestValidators.java
Outdated
Show resolved
Hide resolved
| @@ -34,7 +37,10 @@ static boolean validateExecuteStatementRequest( | |||
| return false; | |||
| } | |||
|
|
|||
| if (sql.indexOf(";") != sql.lastIndexOf(";")) { | |||
| if (Arrays.stream(sql.split(QUOTED_STRING_OR_SEMICOLON)) | |||
| .mapToInt(part -> part.split(";", -1).length - 1) | |||
There was a problem hiding this comment.
Why is -1 being passed as the second argument rather than using the default 0? We shouldn't reject statements with extra white space after the semicolon. Might even be nice to trim the resulting pieces to see if there really are multiple pieces on either side of the semicolon.
There was a problem hiding this comment.
This counts the number of semicolons in outside of strings. The -1 means that empty strings before or after a semicolon are counted. The name of the regex is a misnomer, which has been fixed.
There was a problem hiding this comment.
Thanks. To check my understanding (again): the purpose of the -1 is to ensure that there are always 2 pieces on either side of the semicolon, which means the length of the (split) array minus 1 is the number of semicolons in the statement? Can we add a short comment into the code to explain this for future readers?
Would be great to also add a short javadoc explaining exactly what this method checks. For example, this method does not validate that there are no hanging statements (text at the end not ended with a semicolon). This is fine since the client passes the original text to the server and the server will throw if such a statement is received, but it's not immediately clear to a reader that this is intentional rather than an oversight.
There was a problem hiding this comment.
Yup, that is correct. Will add a javadoc.
ksqldb-api-client/src/test/java/io/confluent/ksql/api/client/ClientTest.java
Outdated
Show resolved
Hide resolved
ksqldb-api-client/src/test/java/io/confluent/ksql/api/client/ClientTest.java
Outdated
Show resolved
Hide resolved
ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/impl/DdlDmlRequestValidators.java
Outdated
Show resolved
Hide resolved
| public void shouldExecuteSingleStatementWithMultipleSemicolons() throws Exception { | ||
| // Given | ||
| final CommandStatusEntity entity = new CommandStatusEntity( | ||
| "CREATE STREAM FOO AS CONCAT(A, `wow;`) FROM `BAR`; ", |
There was a problem hiding this comment.
Let's also update this example to reflect proper usage of quoted strings, to avoid confusion (contributed to my question above).
| "CREATE STREAM FOO AS CONCAT(A, `wow;`) FROM `BAR`; ", | |
| "CREATE STREAM FOO AS CONCAT(A, 'wow;') FROM `BAR`; ", |
ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/impl/DdlDmlRequestValidators.java
Outdated
Show resolved
Hide resolved
…an one semicolon (confluentinc#7243) * fix(java client): allow java client to accept statements with more than one semicolon * address review comments * add javadoc * address review comments * Update DdlDmlRequestValidators.java
…an one semicolon (confluentinc#7243) * fix(java client): allow java client to accept statements with more than one semicolon * address review comments * add javadoc * address review comments * Update DdlDmlRequestValidators.java
Description
The
executeStatementJava client method fails if the given statement contains a string with semicolons. This PR updates the validator logic to ignore semicolons in strings when counting semicolons.Testing done
Added a unit test
Reviewer checklist