Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix bug in AstPrinter preventing query short form from being used #2799

Merged
merged 1 commit into from May 17, 2022
Merged

Fix bug in AstPrinter preventing query short form from being used #2799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/graphql/language/AstPrinter.java
View file
Open in desktop
Expand Up @@ -294,7 +294,7 @@ private NodePrinter<OperationDefinition> operationDefinition() {

// Anonymous queries with no directives or variable definitions can use
// the query short form.
if (isEmpty(name) && isEmpty(directives) && isEmpty(varDefinitions) && op.equals("QUERY")) {
if (isEmpty(name) && isEmpty(directives) && isEmpty(varDefinitions) && op.equals("query")) {
out.append(selectionSet);
} else {
out.append(spaced(op, smooshed(name, varDefinitions), directives, selectionSet));
Expand Down
8 changes: 4 additions & 4 deletions src/test/groovy/graphql/analysis/QueryTransformerTest.groovy
View file
Open in desktop
Expand Up @@ -80,7 +80,7 @@ class QueryTransformerTest extends Specification {

then:
printAstCompact(newDocument) ==
"query {root {fooA {midA-modified {leafA} midB {leafB}} fooB {midA-modified {leafA} midB {leafB}}}}"
"{root {fooA {midA-modified {leafA} midB {leafB}} fooB {midA-modified {leafA} midB {leafB}}}}"
}

def "transform query delete midA nodes"() {
Expand All @@ -102,7 +102,7 @@ class QueryTransformerTest extends Specification {

then:
printAstCompact(newDocument) ==
"query {root {fooA {midB {leafB}} fooB {midB {leafB}}}}"
"{root {fooA {midB {leafB}} fooB {midB {leafB}}}}"
}

def "transform query add midA sibling"() {
Expand All @@ -124,7 +124,7 @@ class QueryTransformerTest extends Specification {

then:
printAstCompact(newDocument) ==
"query {root {fooA {midA {leafA} addedField}}}"
"{root {fooA {midA {leafA} addedField}}}"
}

def "transform query delete fragment spread and inline fragment"() {
Expand Down Expand Up @@ -170,7 +170,7 @@ class QueryTransformerTest extends Specification {
then:

printAstCompact(newDocument) ==
"query {root {fooA {midB {leafB}} fooB {midB {leafB}}}} fragment frag on Foo {midA {leafA}}"
"{root {fooA {midB {leafB}} fooB {midB {leafB}}}} fragment frag on Foo {midA {leafA}}"
}

def "transform query does not traverse named fragments when started from query"() {
Expand Down
14 changes: 7 additions & 7 deletions src/test/groovy/graphql/execution/preparsed/persisted/ApolloPersistedQuerySupportTest.groovy
View file
Open in desktop
Expand Up @@ -69,7 +69,7 @@ class ApolloPersistedQuerySupportTest extends Specification {
def documentEntry = apolloSupport.getDocument(ei, engineParser)
def doc = documentEntry.getDocument()
then:
printAstCompact(doc) == "query {oneTwoThree}"
printAstCompact(doc) == "{oneTwoThree}"
persistedQueryCache.keyCount[hashOne] == 1
persistedQueryCache.parseCount[hashOne] == 1

Expand All @@ -79,7 +79,7 @@ class ApolloPersistedQuerySupportTest extends Specification {
doc = documentEntry.getDocument()

then:
printAstCompact(doc) == "query {oneTwoThree}"
printAstCompact(doc) == "{oneTwoThree}"
persistedQueryCache.keyCount[hashOne] == 2
persistedQueryCache.parseCount[hashOne] == 1 // only compiled once cause we had it
}
Expand All @@ -94,7 +94,7 @@ class ApolloPersistedQuerySupportTest extends Specification {
def documentEntry = apolloSupport.getDocument(ei, engineParser)
def doc = documentEntry.getDocument()
then:
printAstCompact(doc) == "query {normal}"
printAstCompact(doc) == "{normal}"
persistedQueryCache.keyCount.size() == 0
persistedQueryCache.parseCount.size() == 0
}
Expand All @@ -104,11 +104,11 @@ class ApolloPersistedQuerySupportTest extends Specification {
def apolloSupport = new ApolloPersistedQuerySupport(persistedQueryCache)

when:
def ei = mkEI(hashOne, "query {normal}")
def ei = mkEI(hashOne, "{normal}")
def documentEntry = apolloSupport.getDocument(ei, engineParser)
def doc = documentEntry.getDocument()
then:
printAstCompact(doc) == "query {oneTwoThree}"
printAstCompact(doc) == "{oneTwoThree}"
persistedQueryCache.keyCount[hashOne] == 1
persistedQueryCache.parseCount[hashOne] == 1

Expand Down Expand Up @@ -140,14 +140,14 @@ class ApolloPersistedQuerySupportTest extends Specification {
def documentEntry = apolloSupport.getDocument(ei, engineParser)
def doc = documentEntry.getDocument()
then:
printAstCompact(doc) == "query {oneTwoThree}"
printAstCompact(doc) == "{oneTwoThree}"

when:
ei = mkEI(hashTwo, PERSISTED_QUERY_MARKER)
documentEntry = apolloSupport.getDocument(ei, engineParser)
doc = documentEntry.getDocument()
then:
printAstCompact(doc) == "query {fourFiveSix}"
printAstCompact(doc) == "{fourFiveSix}"

when:
ei = mkEI("nonExistent", PERSISTED_QUERY_MARKER)
Expand Down
6 changes: 3 additions & 3 deletions src/test/groovy/graphql/execution/preparsed/persisted/InMemoryPersistedQueryCacheTest.groovy
View file
Open in desktop
Expand Up @@ -40,19 +40,19 @@ class InMemoryPersistedQueryCacheTest extends Specification {
def getDoc = inMemCache.getPersistedQueryDocument(hash, ei, onMiss)
def doc = getDoc.document
then:
printAstCompact(doc) == "query {oneTwoThreeFour}"
printAstCompact(doc) == "{oneTwoThreeFour}"
}

def "When there's a cache miss, uses the query from known queries if the execution input's query is the APQ Marker"() {
def hash = "somehash"
def inMemCache = InMemoryPersistedQueryCache.newInMemoryPersistedQueryCache()
.addQuery(hash, "query {foo bar baz}")
.addQuery(hash, "{foo bar baz}")
.build()
def ei = mkEI(hash, PersistedQuerySupport.PERSISTED_QUERY_MARKER)
when:
def getDoc = inMemCache.getPersistedQueryDocument(hash, ei, onMiss)
def doc = getDoc.document
then:
printAstCompact(doc) == "query {foo bar baz}"
printAstCompact(doc) == "{foo bar baz}"
}
}
10 changes: 5 additions & 5 deletions src/test/groovy/graphql/language/AstPrinterTest.groovy
View file
Open in desktop
Expand Up @@ -199,7 +199,7 @@ scalar DateTime
String output = printAst(document)

expect:
output == """query {
output == """{
empireHero: hero(episode: EMPIRE) {
name
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we want this behavior. I get that that printAstCompact might lead to { f } if there is no args etc.. (is more compact) but the normal printAst probably should use the slightly longer for.

@andimarek what are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was ported from graphql.js originally so in a sense could do what they do

And they do this

src/language/printer.js

  OperationDefinition(node) {
    const op = node.operation;
    const name = node.name;
    const varDefs = wrap('(', join(node.variableDefinitions, ', '), ')');
    const directives = join(node.directives, ' ');
    const selectionSet = node.selectionSet;
    // Anonymous queries with no directives or variable definitions can use
    // the query short form.
    return !name && !directives && !varDefs && op === 'query'
      ? selectionSet
      : join([op, join([name, varDefs]), directives, selectionSet], ' ');
  },

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which is inline with what this PR does

Copy link

@luiznaac luiznaac Aug 1, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbakerman I recently updated to the newest release (19.0) and found out that a test of mine was failing due to this modification. There's no parameter which we could pass nor a specific function we could call to get this behavior. IMHO, it's misleading to just assume the client wants the query in short form just because there're no names, vars nor directives.... shouldn't we introduce a more explicit way to achieve it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @luiznaac for calling this out. We discussed this and decided that this is a change we want to make, but we didn't document this explicitly in the release notes. I just updated them to make this clear.

We also discussed introducing an option, but we decided against it as it is semantically the same.

Expand Down Expand Up @@ -235,7 +235,7 @@ fragment comparisonFields on Character {
String output = printAst(document)

expect:
output == """query {
output == """{
leftComparison: hero(episode: EMPIRE) {
...comparisonFields
}
Expand Down Expand Up @@ -563,7 +563,7 @@ extend input Input @directive {
String output = AstPrinter.printAstCompact(document)

expect:
output == '''query {aliasOfFoo:foo(arg1:"val1",args2:"val2") @isCached {hello} world @neverCache @okThenCache} fragment FX on SomeType {aliased:field(withArgs:"argVal",andMoreArgs:"andMoreVals")}'''
output == '''{aliasOfFoo:foo(arg1:"val1",args2:"val2") @isCached {hello} world @neverCache @okThenCache} fragment FX on SomeType {aliased:field(withArgs:"argVal",andMoreArgs:"andMoreVals")}'''
}

def "print ast with inline fragment without type condition"() {
Expand All @@ -581,8 +581,8 @@ extend input Input @directive {
String outputFull = AstPrinter.printAst(document)

expect:
outputCompact == '''query {foo {... {hello}}}'''
outputFull == '''query {
outputCompact == '''{foo {... {hello}}}'''
outputFull == '''{
foo {
... {
hello
Expand Down
2 changes: 1 addition & 1 deletion src/test/groovy/graphql/language/AstSignatureTest.groovy
View file
Open in desktop
Expand Up @@ -110,7 +110,7 @@ fragment X on SomeType {
}
}"""

def expectedQuery = """query {
def expectedQuery = """{
allIssues(arg1: "", arg2: 0) {
id
}
Expand Down
4 changes: 2 additions & 2 deletions src/test/groovy/graphql/language/AstSorterTest.groovy
View file
Open in desktop
Expand Up @@ -97,7 +97,7 @@ class AstSorterTest extends Specification {

'''

def expectedQuery = '''query {
def expectedQuery = '''{
unamedQueryX
unamedQueryY
unamedQueryZ
Expand Down Expand Up @@ -340,7 +340,7 @@ input InputZ {
}
'''

def expectedQuery = '''query {
def expectedQuery = '''{
field(arg: {x : {a : "vala", b : "valb", c : "valc"}, y : "valy", z : "valz"})
}
'''
Expand Down