fix: Allows unclosed quote characters to exist in comments #4993

AlanConfluent · 2020-04-03T22:20:29Z

Description

This allows support for unclosed quotes in comments. e.g. "SELECT -- It's a comment ".

There's actually a lot of code now that reparses the same line. The better longer term solution is to have a single parser and not delegate to DefaultParser, but I wanted to avoid making this too complex, since this fixes a small bug.

#4075

Testing done

mvn package

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

vpapavas · 2020-04-03T23:03:09Z

ksqldb-cli/src/main/java/io/confluent/ksql/cli/console/UnclosedQuoteChecker.java

+        quoteStart = -1;
+      }
+    }
+    int commentInd = line.indexOf(COMMENT);


This will thrown an NPE if the line is null

Maybe add a null check to all methods? Objects.requireNonNull

Added for the public method. In general, I'm a little wary of littering these sort of null checks within internal classes, but we seem to do that in a lot of areas in the codebase.

vpapavas · 2020-04-03T23:08:55Z

ksqldb-cli/src/main/java/io/confluent/ksql/cli/console/UnclosedQuoteChecker.java

+    for(int i = 0; line != null && i < line.length(); ++i) {
+      if (quoteStart < 0 && isQuoteChar(line, i)) {
+        quoteStart = i;
+      } else if (quoteStart >= 0 && line.charAt(quoteStart) == line.charAt(i) &&


This line.charAt(quoteStart) == line.charAt(i) check if the char is a quote? Why not use isQuoteChar(line, i)?

It's so you can have multiple quote types that contain the others e.g. "This could be 'valid'" and 'This could be "valid"'.

Given that we're just doing 'this type of quote', I'll simplify it.

vpapavas · 2020-04-03T23:12:38Z

ksqldb-cli/src/test/java/io/confluent/ksql/cli/console/UnclosedQuoteCheckerTest.java

+  @Test
+  public void shouldFindUnclosedQuote() {
+    // Given:
+    final String line = "some line 'this is in a comment";


The string is a bit misleading :P These are not in comments, right?

Hah, copied a bit too many times. I changed them to make sense, I believe.

vpapavas

Thank you @AlanConfluent ! LGTM

big-andy-coates

Hey @AlanConfluent,

It's been a long time single I worked on this code, so I'm very rusty. However, I've added a few comments and suggestions.

These kind of changes always make me nervous when I'm making them as there are just so many edge cases. Adding extensive test cases is my best advice: think of every combination and permutation and add a test case!

Finally, I believe the sql standard allows a single quote to be escaped with another single quote, i.e. 'fred''s thing' should be treated as fred's thing - I think you've missed this case in your code + tests.

big-andy-coates · 2020-04-06T18:06:06Z

ksqldb-cli/src/main/java/io/confluent/ksql/cli/console/KsqlLineParser.java

@@ -47,6 +47,10 @@ public ParsedLine parse(final String line, final int cursor, final ParseContext
      return parsed;
    }

+    if (UnclosedQuoteChecker.isUnclosedQuote(line)) {


I'm not thought too much about this, as its been a while since I've been in this code... but are you happy with being passed line and not parsed.line(), i.e. do you understand the difference? (I certainly can't remember of the top of my head! )

I've read through the source code of DefaultParser and parsed.line() appears to be just returning the line passed to it, so this should be fine. https://github.com/jline/jline3/blob/master/reader/src/main/java/org/jline/reader/impl/DefaultParser.java#L312

big-andy-coates · 2020-04-06T18:07:36Z

ksqldb-cli/src/main/java/io/confluent/ksql/cli/console/UnclosedQuoteChecker.java

+  private UnclosedQuoteChecker() {}
+
+  public static boolean isUnclosedQuote(final String line) {
+    requireNonNull(line, "line");


fyi: no need to null check an argument you're not storing in your object state.

And I think if you remove this you should also remove the line != null in the for loop.

I agree about not littering the code with null checks. Usually you would do it at exposed API layers, but not for internal code, since you can trust callers to know the invariants. What's the motivation in differentiating between storing it as state vs not?

Removed the null check below in the for loop.

big-andy-coates · 2020-04-06T18:13:38Z

ksqldb-cli/src/test/java/io/confluent/ksql/cli/console/UnclosedQuoteCheckerTest.java

+    final String line = "some line 'this is in a quote";
+
+    // Then:
+    Assert.assertTrue(UnclosedQuoteChecker.isUnclosedQuote(line));


nit: prefer hamcrest matchers (with static import) over junit asserts, as they are most expressive.

(though in this case, you're comparing bools, so there's not much in it!)

Done, switched to hamcrest

big-andy-coates · 2020-04-06T18:14:38Z

ksqldb-cli/src/test/java/io/confluent/ksql/cli/console/UnclosedQuoteCheckerTest.java

+import org.junit.Assert;
+import org.junit.Test;
+
+public class UnclosedQuoteCheckerTest {


Can you add some test cases please?

where -- appears _within a quoted string

where -- appears after a quoted string is closed?

where a quoted string contains a single quote, e.g. 'this string''contains a single quote'

some with double quotes in quoted strings just for the hell of it.

I had to add support for the twoQuoteChars in a row. I added tests for all of the above and others. Thanks for coming up with some good cases.

AlanConfluent

Hey @AlanConfluent,

It's been a long time single I worked on this code, so I'm very rusty. However, I've added a few comments and suggestions.

I agree that this code is a bit tricky which is probably why this bug has been around for a bit. Hopefully, I've tested it well.

These kind of changes always make me nervous when I'm making them as there are just so many edge cases. Adding extensive test cases is my best advice: think of every combination and permutation and add a test case!

I've added a number of tests, including those you suggested and others. I covered the edge cases I can think of, including combinations of escaping and quoting.

Finally, I believe the sql standard allows a single quote to be escaped with another single quote, i.e. 'fred''s thing' should be treated as fred's thing - I think you've missed this case in your code + tests.

I added the two quote case you mentioned. Thanks for pointing that out! I don't believe that was explicitly handled using the DefaultParser, but they effectively support it by just pairing up quotes. My code also worked without explicit support, but I think the explicit is clearer, so I've added that.

AlanConfluent · 2020-04-06T19:50:44Z

ksqldb-cli/src/main/java/io/confluent/ksql/cli/console/KsqlLineParser.java

@@ -47,6 +47,10 @@ public ParsedLine parse(final String line, final int cursor, final ParseContext
      return parsed;
    }

+    if (UnclosedQuoteChecker.isUnclosedQuote(line)) {


I've read through the source code of DefaultParser and parsed.line() appears to be just returning the line passed to it, so this should be fine. https://github.com/jline/jline3/blob/master/reader/src/main/java/org/jline/reader/impl/DefaultParser.java#L312

AlanConfluent · 2020-04-06T19:54:23Z

ksqldb-cli/src/main/java/io/confluent/ksql/cli/console/UnclosedQuoteChecker.java

+  private UnclosedQuoteChecker() {}
+
+  public static boolean isUnclosedQuote(final String line) {
+    requireNonNull(line, "line");


I agree about not littering the code with null checks. Usually you would do it at exposed API layers, but not for internal code, since you can trust callers to know the invariants. What's the motivation in differentiating between storing it as state vs not?

Removed the null check below in the for loop.

AlanConfluent · 2020-04-06T21:31:31Z

ksqldb-cli/src/test/java/io/confluent/ksql/cli/console/UnclosedQuoteCheckerTest.java

+import org.junit.Assert;
+import org.junit.Test;
+
+public class UnclosedQuoteCheckerTest {


I had to add support for the twoQuoteChars in a row. I added tests for all of the above and others. Thanks for coming up with some good cases.

AlanConfluent · 2020-04-06T21:41:12Z

ksqldb-cli/src/test/java/io/confluent/ksql/cli/console/UnclosedQuoteCheckerTest.java

+    final String line = "some line 'this is in a quote";
+
+    // Then:
+    Assert.assertTrue(UnclosedQuoteChecker.isUnclosedQuote(line));


Done, switched to hamcrest

…s a comment

AlanConfluent requested a review from a team as a code owner April 3, 2020 22:20

vpapavas reviewed Apr 3, 2020

View reviewed changes

vpapavas approved these changes Apr 4, 2020

View reviewed changes

vpapavas self-assigned this Apr 4, 2020

big-andy-coates approved these changes Apr 6, 2020

View reviewed changes

AlanConfluent commented Apr 6, 2020

View reviewed changes

AlanConfluent added 4 commits April 7, 2020 09:05

fix: Allows quote characters to exist in comments, e.g. SELECT -- It'…

ce898d3

…s a comment

Feedback and style

a9e0892

Try again

5e151b2

Feedback

4393477

AlanConfluent force-pushed the fix_allow_quote_char_in_comment branch from 55820d6 to 4393477 Compare April 7, 2020 16:05

Fixed validate

c2e84cc

AlanConfluent merged commit fd65021 into confluentinc:master Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Allows unclosed quote characters to exist in comments #4993

fix: Allows unclosed quote characters to exist in comments #4993

AlanConfluent commented Apr 3, 2020

vpapavas Apr 3, 2020

vpapavas Apr 3, 2020

AlanConfluent Apr 3, 2020

vpapavas Apr 3, 2020

AlanConfluent Apr 3, 2020

vpapavas Apr 3, 2020

AlanConfluent Apr 3, 2020

vpapavas left a comment

big-andy-coates left a comment

big-andy-coates Apr 6, 2020

AlanConfluent Apr 6, 2020

big-andy-coates Apr 6, 2020

AlanConfluent Apr 6, 2020

big-andy-coates Apr 6, 2020

AlanConfluent Apr 6, 2020

big-andy-coates Apr 6, 2020

AlanConfluent Apr 6, 2020

AlanConfluent left a comment

AlanConfluent Apr 6, 2020

AlanConfluent Apr 6, 2020

AlanConfluent Apr 6, 2020

AlanConfluent Apr 6, 2020

fix: Allows unclosed quote characters to exist in comments #4993

fix: Allows unclosed quote characters to exist in comments #4993

Conversation

AlanConfluent commented Apr 3, 2020

Description

Testing done

Reviewer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpapavas left a comment

Choose a reason for hiding this comment

big-andy-coates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanConfluent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment