parsing: Improve Parsing error handling

andimarek · andimarek · commit fb53067cc862 · 2015-09-17T04:11:25.000+02:00
Use antlr BailErrorStrategy to cancel immediately if any syntax problem is detected
Removed ErrorListener, because its not called anymore
diff --git a/src/main/java/graphql/GraphQL.java b/src/main/java/graphql/GraphQL.java
@@ -10,14 +10,14 @@
 import graphql.validation.ValidationError;
 import graphql.validation.Validator;
 import org.antlr.v4.runtime.RecognitionException;
+import org.antlr.v4.runtime.misc.ParseCancellationException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import java.util.concurrent.ExecutorService;
 
 import static graphql.Assert.assertNotNull;
 
@@ -62,8 +62,9 @@ public ExecutionResult execute(String requestString, String operationName, Objec
         Document document;
         try {
             document = parser.parseDocument(requestString);
-        } catch (RecognitionException e) {
-            SourceLocation sourceLocation = new SourceLocation(e.getOffendingToken().getLine(), e.getOffendingToken().getCharPositionInLine());
+        } catch (ParseCancellationException e) {
+            RecognitionException recognitionException = (RecognitionException) e.getCause();
+            SourceLocation sourceLocation = new SourceLocation(recognitionException.getOffendingToken().getLine(), recognitionException.getOffendingToken().getCharPositionInLine());
             InvalidSyntaxError invalidSyntaxError = new InvalidSyntaxError(sourceLocation);
             return new ExecutionResultImpl(Arrays.asList(invalidSyntaxError));
         }
diff --git a/src/main/java/graphql/parser/Parser.java b/src/main/java/graphql/parser/Parser.java
@@ -3,7 +3,9 @@
 import graphql.language.Document;
 import graphql.parser.antlr.GraphqlLexer;
 import graphql.parser.antlr.GraphqlParser;
-import org.antlr.v4.runtime.*;
+import org.antlr.v4.runtime.ANTLRInputStream;
+import org.antlr.v4.runtime.BailErrorStrategy;
+import org.antlr.v4.runtime.CommonTokenStream;
 import org.antlr.v4.runtime.atn.PredictionMode;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -21,30 +23,13 @@ public Document parseDocument(String input) {
 
         GraphqlParser parser = new GraphqlParser(tokens);
         parser.removeErrorListeners();
-        parser.addErrorListener(new ErrorListener());
         parser.getInterpreter().setPredictionMode(PredictionMode.SLL);
-        parser.setErrorHandler(new ErrorStrategy());
+        parser.setErrorHandler(new BailErrorStrategy());
         GraphqlParser.DocumentContext document = parser.document();
 
 
         GraphqlAntlrToLanguage antlrToLanguage = new GraphqlAntlrToLanguage();
         antlrToLanguage.visitDocument(document);
         return antlrToLanguage.result;
     }
-
-    private class ErrorStrategy extends DefaultErrorStrategy {
-
-        @Override
-        public void recover(org.antlr.v4.runtime.Parser recognizer, RecognitionException e) {
-            throw e;
-        }
-    }
-
-    private class ErrorListener extends BaseErrorListener {
-        @Override
-        public void syntaxError(Recognizer<?, ?> recognizer, Object offendingSymbol, int line, int charPositionInLine, String msg, RecognitionException e) {
-            log.error("graphql syntax error: line " + line + ":" + charPositionInLine + " " + msg);
-        }
-    }
-
 }
diff --git a/src/test/groovy/graphql/GraphQLTest.groovy b/src/test/groovy/graphql/GraphQLTest.groovy
@@ -110,4 +110,21 @@ class GraphQLTest extends Specification {
         errors[0].errorType == ErrorType.InvalidSyntax
         errors[0].sourceLocations == [new SourceLocation(1, 8)]
     }
+
+    def "query with invalid syntax 2"() {
+        given:
+        GraphQLSchema schema = newSchema().query(
+                newObject()
+                        .name("RootQueryType")
+                        .build()
+        ).build()
+
+        when:
+        def errors = new GraphQL(schema).execute('{ hello[](() }').errors
+
+        then:
+        errors.size() == 1
+        errors[0].errorType == ErrorType.InvalidSyntax
+        errors[0].sourceLocations == [new SourceLocation(1, 7)]
+    }
 }
diff --git a/src/test/groovy/graphql/parser/ParserTest.groovy b/src/test/groovy/graphql/parser/ParserTest.groovy
@@ -1,6 +1,7 @@
 package graphql.parser
 
 import graphql.language.*
+import org.antlr.v4.runtime.misc.ParseCancellationException
 import spock.lang.Specification
 import spock.lang.Unroll
 
@@ -64,7 +65,7 @@ class ParserTest extends Specification {
         when:
         Document document = new Parser().parseDocument(input)
         then:
-        isEqual(document,expectedResult)
+        isEqual(document, expectedResult)
 
     }
 
@@ -142,7 +143,7 @@ class ParserTest extends Specification {
 
         then:
         document.definitions.size() == 2
-        isEqual(document.definitions[0],queryDefinition)
+        isEqual(document.definitions[0], queryDefinition)
         isEqual(document.definitions[1], fragmentDefinition)
     }
 
@@ -310,7 +311,7 @@ class ParserTest extends Specification {
         Field helloField = document.definitions[0].selectionSet.selections[0]
 
         then:
-        isEqual(helloField,new Field("hello", [new Argument("arg", new StringValue("hello, world"))]))
+        isEqual(helloField, new Field("hello", [new Argument("arg", new StringValue("hello, world"))]))
     }
 
     @Unroll
@@ -338,4 +339,26 @@ class ParserTest extends Specification {
 
     }
 
+    def "extraneous input is an excpetion"() {
+        given:
+        def input = """
+        mutation event(\$var: SomeType[]!) { res: update(arg: \$var) {id} }
+        """
+        when:
+        new Parser().parseDocument(input)
+        then:
+        thrown(ParseCancellationException)
+    }
+
+    def "invalid syntax is an error"() {
+        given:
+        def input = """
+        mutation event(() }
+        """
+        when:
+        new Parser().parseDocument(input)
+        then:
+        thrown(ParseCancellationException)
+    }
+
 }
