Skip to content

Commit

Permalink
Add ParserOption to ignore single-line comments (#2788)
Browse files Browse the repository at this point in the history
This commit adds the ability to ignore single-line comments during AST conversion.

Fixes #2767
  • Loading branch information
jord1e committed Apr 16, 2022
1 parent 2148cdf commit 66595fa
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 5 deletions.
3 changes: 3 additions & 0 deletions src/main/java/graphql/language/Comment.java
Expand Up @@ -4,6 +4,9 @@

import java.io.Serializable;

/**
* A single-line comment. These are comments that start with a {@code #} in source documents.
*/
@PublicApi
public class Comment implements Serializable {
public final String content;
Expand Down
7 changes: 6 additions & 1 deletion src/main/java/graphql/parser/GraphqlAntlrToLanguage.java
Expand Up @@ -82,6 +82,7 @@
@Internal
public class GraphqlAntlrToLanguage {

private static final List<Comment> NO_COMMENTS = ImmutableKit.emptyList();
private static final int CHANNEL_COMMENTS = 2;
private static final int CHANNEL_IGNORED_CHARS = 3;
private final CommonTokenStream tokens;
Expand Down Expand Up @@ -866,6 +867,10 @@ protected SourceLocation getSourceLocation(Token token) {
}

protected List<Comment> getComments(ParserRuleContext ctx) {
if (!parserOptions.isCaptureLineComments()) {
return NO_COMMENTS;
}

Token start = ctx.getStart();
if (start != null) {
int tokPos = start.getTokenIndex();
Expand All @@ -874,7 +879,7 @@ protected List<Comment> getComments(ParserRuleContext ctx) {
return getCommentOnChannel(refChannel);
}
}
return ImmutableKit.emptyList();
return NO_COMMENTS;
}


Expand Down
28 changes: 26 additions & 2 deletions src/main/java/graphql/parser/ParserOptions.java
Expand Up @@ -26,6 +26,7 @@ public class ParserOptions {
private static ParserOptions defaultJvmParserOptions = newParserOptions()
.captureIgnoredChars(false)
.captureSourceLocation(true)
.captureLineComments(true)
.maxTokens(MAX_QUERY_TOKENS) // to prevent a billion laughs style attacks, we set a default for graphql-java

.build();
Expand Down Expand Up @@ -66,12 +67,14 @@ public static void setDefaultParserOptions(ParserOptions options) {

private final boolean captureIgnoredChars;
private final boolean captureSourceLocation;
private final boolean captureLineComments;
private final int maxTokens;
private final ParsingListener parsingListener;

private ParserOptions(Builder builder) {
this.captureIgnoredChars = builder.captureIgnoredChars;
this.captureSourceLocation = builder.captureSourceLocation;
this.captureLineComments = builder.captureLineComments;
this.maxTokens = builder.maxTokens;
this.parsingListener = builder.parsingListener;
}
Expand All @@ -80,7 +83,7 @@ private ParserOptions(Builder builder) {
* Significant memory savings can be made if we do NOT capture ignored characters,
* especially in SDL parsing. So we have set this to false by default.
*
* @return true if ignored chars are captured in AST nodes
* @return true if ignored chars should be captured as AST nodes
*/
public boolean isCaptureIgnoredChars() {
return captureIgnoredChars;
Expand All @@ -91,14 +94,28 @@ public boolean isCaptureIgnoredChars() {
* Memory savings can be made if we do NOT set {@link graphql.language.SourceLocation}s
* on AST nodes, especially in SDL parsing.
*
* @return true if {@link graphql.language.SourceLocation}s are captured in AST nodes
* @return true if {@link graphql.language.SourceLocation}s should be captured as AST nodes
*
* @see graphql.language.SourceLocation
*/
public boolean isCaptureSourceLocation() {
return captureSourceLocation;
}

/**
* Single-line {@link graphql.language.Comment}s do not have any semantic meaning in
* GraphQL source documents, as such you may wish to ignore them.
* <p>
* This option does not ignore documentation {@link graphql.language.Description}s.
*
* @return true if {@link graphql.language.Comment}s should be captured as AST nodes
*
* @see graphql.language.SourceLocation
*/
public boolean isCaptureLineComments() {
return captureLineComments;
}

/**
* A graphql hacking vector is to send nonsensical queries that burn lots of parsing CPU time and burn
* memory representing a document that won't ever execute. To prevent this you can set a maximum number of parse
Expand Down Expand Up @@ -128,6 +145,7 @@ public static class Builder {

private boolean captureIgnoredChars = false;
private boolean captureSourceLocation = true;
private boolean captureLineComments = true;
private int maxTokens = MAX_QUERY_TOKENS;
private ParsingListener parsingListener = ParsingListener.NOOP;

Expand All @@ -137,6 +155,7 @@ public static class Builder {
Builder(ParserOptions parserOptions) {
this.captureIgnoredChars = parserOptions.captureIgnoredChars;
this.captureSourceLocation = parserOptions.captureSourceLocation;
this.captureLineComments = parserOptions.captureLineComments;
this.maxTokens = parserOptions.maxTokens;
this.parsingListener = parserOptions.parsingListener;
}
Expand All @@ -151,6 +170,11 @@ public Builder captureSourceLocation(boolean captureSourceLocation) {
return this;
}

public Builder captureLineComments(boolean captureLineComments) {
this.captureLineComments = captureLineComments;
return this;
}

public Builder maxTokens(int maxTokens) {
this.maxTokens = maxTokens;
return this;
Expand Down
27 changes: 25 additions & 2 deletions src/test/groovy/graphql/parser/ParserTest.groovy
Expand Up @@ -42,6 +42,7 @@ import graphql.language.VariableDefinition
import graphql.language.VariableReference
import org.antlr.v4.runtime.CommonTokenStream
import org.antlr.v4.runtime.ParserRuleContext
import spock.lang.Issue
import spock.lang.Specification
import spock.lang.Unroll

Expand Down Expand Up @@ -373,6 +374,28 @@ class ParserTest extends Specification {
helloField.comments.collect { c -> c.content } == [" this is some comment, which should be captured"]
}

@Issue("https://github.com/graphql-java/graphql-java/issues/2767")
def "parser does not transform comments to AST nodes when ParserOptions.captureLineComments(false)"() {
given:
def input = """
{ # this is some comment, which should be captured
hello(arg: "hello, world" ) # test
}
"""
def parserOptionsWithoutCaptureLineComments = ParserOptions.newParserOptions()
.captureLineComments(false)
.build()

when:
def document = new Parser().parseDocument(input, parserOptionsWithoutCaptureLineComments)
Field helloField = (document.definitions[0] as OperationDefinition).selectionSet.selections[0] as Field

then:
isEqual(helloField, new Field("hello", [new Argument("arg", new StringValue("hello, world"))]))
assert helloField.comments.isEmpty() // No single-line comments on lone fields
assert document.comments.isEmpty() // No single-line comments in entire document
}

@Unroll
def "parse floatValue #floatString"() {
given:
Expand Down Expand Up @@ -1118,7 +1141,7 @@ triple3 : """edge cases \\""" "" " \\"" \\" edge cases"""
then:
doc != null
count == 9
tokens == ["query" , "{", "f" , "(", "arg", ":", "1", ")", "}"]
tokens == ["query", "{", "f", "(", "arg", ":", "1", ")", "}"]

when: "integration test to prove it be supplied via EI"

Expand All @@ -1138,7 +1161,7 @@ triple3 : """edge cases \\""" "" " \\"" \\" edge cases"""
then:
er.errors.size() == 0
count == 9
tokens == ["query" , "{", "f" , "(", "arg", ":", "1", ")", "}"]
tokens == ["query", "{", "f", "(", "arg", ":", "1", ")", "}"]

}
}

0 comments on commit 66595fa

Please sign in to comment.