PR suggestions - now uses 1 based indexes

graphql-java · Feb 7, 2019 · e310dc3 · e310dc3
1 parent cded477
commit e310dc3
Show file tree

Hide file tree

Showing 18 changed files with 157 additions and 107 deletions.
diff --git a/src/main/java/graphql/parser/ExtendedBailStrategy.java b/src/main/java/graphql/parser/ExtendedBailStrategy.java
@@ -39,7 +39,8 @@ InvalidSyntaxException mkMoreTokensException(Token token) {
         SourceAndLine sourceAndLine = multiSourceReader.getSourceAndLineFromOverallLine(token.getLine());
         int column = token.getCharPositionInLine();
 
-        SourceLocation sourceLocation = new SourceLocation(sourceAndLine.getLine(), column, sourceAndLine.getSourceName());
+        // graphql spec says line numbers start at 1
+        SourceLocation sourceLocation = new SourceLocation(sourceAndLine.getLine()+1, column, sourceAndLine.getSourceName());
         String sourcePreview = mkPreview(token.getLine());
         return new InvalidSyntaxException(sourceLocation,
                 "There are more tokens in the query that have not been consumed",
@@ -58,7 +59,8 @@ private InvalidSyntaxException mkException(Parser recognizer, RecognitionExcepti
             SourceAndLine sourceAndLine = multiSourceReader.getSourceAndLineFromOverallLine(tokenLine);
             offendingToken = currentToken.getText();
             sourcePreview = mkPreview(tokenLine);
-            sourceLocation = new SourceLocation(sourceAndLine.getLine(), column, sourceAndLine.getSourceName());
+            // graphql spec says line numbers start at 1
+            sourceLocation = new SourceLocation(sourceAndLine.getLine()+1, column, sourceAndLine.getSourceName());
         }
         return new InvalidSyntaxException(sourceLocation, null, sourcePreview, offendingToken, cause);
     }

diff --git a/src/main/java/graphql/parser/GraphqlAntlrToLanguage.java b/src/main/java/graphql/parser/GraphqlAntlrToLanguage.java
@@ -812,7 +812,9 @@ protected Description newDescription(GraphqlParser.DescriptionContext descriptio
     protected SourceLocation getSourceLocation(Token token) {
         MultiSourceReader.SourceAndLine sourceAndLine = multiSourceReader.getSourceAndLineFromOverallLine(token.getLine());
         int column = token.getCharPositionInLine() + 1;
-        return new SourceLocation(sourceAndLine.getLine(), column, sourceAndLine.getSourceName());
+        // graphql spec says line numbers start at 1
+        int line = sourceAndLine.getLine() + 1;
+        return new SourceLocation(line, column, sourceAndLine.getSourceName());
     }
 
     protected SourceLocation getSourceLocation(ParserRuleContext parserRuleContext) {
@@ -845,7 +847,9 @@ protected List<Comment> getCommentOnChannel(List<Token> refChannel) {
             text = text.replaceFirst("^#", "");
             MultiSourceReader.SourceAndLine sourceAndLine = multiSourceReader.getSourceAndLineFromOverallLine(refTok.getLine());
             int column = refTok.getCharPositionInLine();
-            comments.add(new Comment(text, new SourceLocation(sourceAndLine.getLine(), column, sourceAndLine.getSourceName())));
+            // graphql spec says line numbers start at 1
+            int line = sourceAndLine.getLine() + 1;
+            comments.add(new Comment(text, new SourceLocation(line, column, sourceAndLine.getSourceName())));
         }
         return comments;
     }

diff --git a/src/main/java/graphql/parser/MultiSourceReader.java b/src/main/java/graphql/parser/MultiSourceReader.java
@@ -1,7 +1,7 @@
 package graphql.parser;
 
 import graphql.Assert;
-import graphql.Internal;
+import graphql.PublicApi;
 
 import java.io.IOException;
 import java.io.LineNumberReader;
@@ -19,7 +19,7 @@
  * It can also track all data in memory if you want to have all of the previous read data in
  * place at some point in time.
  */
-@Internal
+@PublicApi
 public class MultiSourceReader extends Reader {
 
     private final List<SourcePart> sourceParts;
@@ -81,6 +81,15 @@ public int getLine() {
         }
     }
 
+    /**
+     * This returns the source name and line number given an overall line number
+     *
+     * This is zeroes based like {@link java.io.LineNumberReader#getLineNumber()}
+     *
+     * @param overallLineNumber the over all line number
+     *
+     * @return the source name and relative line number to that source
+     */
     public SourceAndLine getSourceAndLineFromOverallLine(int overallLineNumber) {
         SourceAndLine sourceAndLine = new SourceAndLine();
         if (sourceParts.isEmpty()) {
@@ -120,6 +129,9 @@ public SourceAndLine getSourceAndLineFromOverallLine(int overallLineNumber) {
         return sourceAndLine;
     }
 
+    /**
+     * @return the line number of the current source.  This is zeroes based like {@link java.io.LineNumberReader#getLineNumber()}
+     */
     public int getLineNumber() {
         synchronized (this) {
             if (sourceParts.isEmpty()) {
@@ -132,6 +144,9 @@ public int getLineNumber() {
         }
     }
 
+    /**
+     * @return The name of the current source
+     */
     public String getSourceName() {
         synchronized (this) {
             if (sourceParts.isEmpty()) {
@@ -144,6 +159,9 @@ public String getSourceName() {
         }
     }
 
+    /**
+     * @return the overall line number of the all the sources.  This is zeroes based like {@link java.io.LineNumberReader#getLineNumber()}
+     */
     public int getOverallLineNumber() {
         return overallLineNumber;
     }
@@ -183,14 +201,17 @@ private static class SourcePart {
     }
 
 
-    public static Builder newMultiSourceLineReader() {
+    public static Builder newMultiSourceReader() {
         return new Builder();
     }
 
     public static class Builder {
         List<SourcePart> sourceParts = new ArrayList<>();
         boolean trackData = true;
 
+        private Builder() {
+        }
+
         public Builder reader(Reader reader, String sourceName) {
             SourcePart sourcePart = new SourcePart();
             sourcePart.lineReader = new LineNumberReader(Assert.assertNotNull(reader));

diff --git a/src/main/java/graphql/parser/Parser.java b/src/main/java/graphql/parser/Parser.java
@@ -11,6 +11,7 @@
 import org.antlr.v4.runtime.atn.PredictionMode;
 
 import java.io.IOException;
+import java.io.Reader;
 import java.io.UncheckedIOException;
 import java.util.List;
 
@@ -22,14 +23,21 @@ public Document parseDocument(String input) throws InvalidSyntaxException {
     }
 
     public Document parseDocument(String input, String sourceName) throws InvalidSyntaxException {
-        MultiSourceReader multiSourceReader = MultiSourceReader.newMultiSourceLineReader()
+        MultiSourceReader multiSourceReader = MultiSourceReader.newMultiSourceReader()
                 .string(input, sourceName)
                 .trackData(true)
                 .build();
         return parseDocument(multiSourceReader);
     }
 
-    public Document parseDocument(MultiSourceReader multiSourceReader) throws InvalidSyntaxException {
+    public Document parseDocument(Reader reader) throws InvalidSyntaxException {
+        MultiSourceReader multiSourceReader;
+        if (reader instanceof MultiSourceReader) {
+            multiSourceReader = (MultiSourceReader) reader;
+        } else {
+            multiSourceReader = MultiSourceReader.newMultiSourceReader()
+                    .reader(reader, null).build();
+        }
         CodePointCharStream charStream;
         try {
             charStream = CharStreams.fromReader(multiSourceReader);

diff --git a/src/main/java/graphql/schema/idl/SchemaParser.java b/src/main/java/graphql/schema/idl/SchemaParser.java
@@ -13,6 +13,7 @@
 import java.io.File;
 import java.io.IOException;
 import java.io.Reader;
+import java.io.StringReader;
 import java.io.StringWriter;
 import java.nio.file.Files;
 import java.util.ArrayList;
@@ -56,7 +57,7 @@ public TypeDefinitionRegistry parse(File file) throws SchemaProblem {
      */
     public TypeDefinitionRegistry parse(Reader reader) throws SchemaProblem {
         try (Reader input = reader) {
-            return parse(read(input));
+            return parseImpl(input);
         } catch (IOException e) {
             throw new RuntimeException(e);
         }
@@ -72,6 +73,10 @@ public TypeDefinitionRegistry parse(Reader reader) throws SchemaProblem {
      * @throws SchemaProblem if there are problems compiling the schema definitions
      */
     public TypeDefinitionRegistry parse(String schemaInput) throws SchemaProblem {
+        return parseImpl(new StringReader(schemaInput));
+    }
+
+    public TypeDefinitionRegistry parseImpl(Reader schemaInput) {
         try {
             Parser parser = new Parser();
             Document document = parser.parseDocument(schemaInput);

diff --git a/src/test/groovy/graphql/DataFetcherWithErrorsAndDataTest.groovy b/src/test/groovy/graphql/DataFetcherWithErrorsAndDataTest.groovy
@@ -108,7 +108,7 @@ class DataFetcherWithErrorsAndDataTest extends Specification {
         result.errors.size() == 1
         result.errors[0].path == ["root", "parent", "child", "badField"]
         result.errors[0].message == "badField is bad"
-        result.errors[0].locations == [new SourceLocation(6, 27)]
+        result.errors[0].locations == [new SourceLocation(7, 27)]
 
         result.data["root"]["parent"]["child"]["goodField"] == "good"
         result.data["root"]["parent"]["child"]["badField"] == null
@@ -191,10 +191,10 @@ class DataFetcherWithErrorsAndDataTest extends Specification {
         result.errors.size() == 2
         result.errors[0].path == ["root", "parent", "child", "goodField"]
         result.errors[0].message == "goodField is bad"
-        result.errors[0].locations == [new SourceLocation(7, 31)]
+        result.errors[0].locations == [new SourceLocation(8, 31)]
         result.errors[1].path == ["root", "parent", "child", "badField"]
         result.errors[1].message == "badField is bad"
-        result.errors[1].locations == [new SourceLocation(7, 31)]
+        result.errors[1].locations == [new SourceLocation(8, 31)]
 
         result.data["root"]["parent"]["child"]["goodField"] == null
         result.data["root"]["parent"]["child"]["badField"] == null
@@ -268,11 +268,11 @@ class DataFetcherWithErrorsAndDataTest extends Specification {
         result.errors.size() == 2
         result.errors[0].path == ["parent", "child"]
         result.errors[0].message == "error 1"
-        result.errors[0].locations == [new SourceLocation(6, 27)]
+        result.errors[0].locations == [new SourceLocation(7, 27)]
 
         result.errors[1].path == ["parent", "child"]
         result.errors[1].message == "error 2"
-        result.errors[1].locations == [new SourceLocation(6, 27)]
+        result.errors[1].locations == [new SourceLocation(7, 27)]
 
         result.data["parent"]["child"] == null
 
@@ -344,7 +344,7 @@ class DataFetcherWithErrorsAndDataTest extends Specification {
         result.errors.size() == 1
         result.errors[0].path == ["parent", "child", "badField"]
         result.errors[0].message == "badField is bad"
-        result.errors[0].locations == [new SourceLocation(6, 27)]
+        result.errors[0].locations == [new SourceLocation(7, 27)]
 
         result.data["parent"]["child"]["goodField"] == "good"
         result.data["parent"]["child"]["badField"] == null

diff --git a/src/test/groovy/graphql/GraphQLTest.groovy b/src/test/groovy/graphql/GraphQLTest.groovy
@@ -139,7 +139,7 @@ class GraphQLTest extends Specification {
         then:
         errors.size() == 1
         errors[0].errorType == ErrorType.InvalidSyntax
-        errors[0].locations == [new SourceLocation(0, 8)]
+        errors[0].locations == [new SourceLocation(1, 8)]
     }
 
     def "query with invalid syntax 2"() {
@@ -156,7 +156,7 @@ class GraphQLTest extends Specification {
         then:
         errors.size() == 1
         errors[0].errorType == ErrorType.InvalidSyntax
-        errors[0].locations == [new SourceLocation(0, 7)]
+        errors[0].locations == [new SourceLocation(1, 7)]
     }
 
     def "non null argument is missing"() {
@@ -179,7 +179,7 @@ class GraphQLTest extends Specification {
         then:
         errors.size() == 1
         errors[0].errorType == ErrorType.ValidationError
-        errors[0].locations == [new SourceLocation(0, 3)]
+        errors[0].locations == [new SourceLocation(1, 3)]
         (errors[0] as ValidationError).validationErrorType == ValidationErrorType.MissingFieldArgument
     }
 

diff --git a/src/test/groovy/graphql/Issue739.groovy b/src/test/groovy/graphql/Issue739.groovy
@@ -89,7 +89,7 @@ class Issue739 extends Specification {
         varResult.errors[0].errorType == ErrorType.ValidationError
         varResult.errors[0].message == "Variable 'input' has an invalid value. Expected type 'Map' but was 'Integer'." +
                 " Variables for input objects must be an instance of type 'Map'.";
-        varResult.errors[0].locations == [new SourceLocation(0, 11)]
+        varResult.errors[0].locations == [new SourceLocation(1, 11)]
 
         when:
         variables = ["input": ["baz": "hi", "boom": "hi"]]
@@ -108,6 +108,6 @@ class Issue739 extends Specification {
         varResult.errors.size() == 1
         varResult.errors[0].errorType == ErrorType.ValidationError
         varResult.errors[0].message == "Variable 'boom' has an invalid value. Expected type 'Int' but was 'String'."
-        varResult.errors[0].locations == [new SourceLocation(0, 11)]
+        varResult.errors[0].locations == [new SourceLocation(1, 11)]
     }
 }
diff --git a/src/test/groovy/graphql/IssueNonNullDefaultAttribute.groovy b/src/test/groovy/graphql/IssueNonNullDefaultAttribute.groovy
@@ -54,7 +54,7 @@ class IssueNonNullDefaultAttribute extends Specification {
         result.errors.size() == 1
         result.errors[0].errorType == ErrorType.ValidationError
         result.errors[0].message == "Validation error of type WrongType: argument 'characterNumber' with value 'NullValue{}' must not be null @ 'name'"
-        result.errors[0].locations == [new SourceLocation(3, 26)]
+        result.errors[0].locations == [new SourceLocation(4, 26)]
         result.data == null
 
     }

diff --git a/src/test/groovy/graphql/NullVariableCoercionTest.groovy b/src/test/groovy/graphql/NullVariableCoercionTest.groovy
@@ -67,7 +67,7 @@ class NullVariableCoercionTest extends Specification {
         varResult.errors.size() == 1
         varResult.errors[0].errorType == ErrorType.ValidationError
         varResult.errors[0].message == "Field 'baz' of variable 'input' has coerced Null value for NonNull type 'String!'"
-        varResult.errors[0].locations == [new SourceLocation(0, 11)]
+        varResult.errors[0].locations == [new SourceLocation(1, 11)]
     }
 }