Skip to content

deprecated default value method produces bad results #3286

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

Merged
dondonz merged 2 commits into from
Aug 21, 2023
Merged

deprecated default value method produces bad results #3286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f112054
deprecated default value method produces bad results
bbakerman Aug 9, 2023
b79540b
Fixed the ValuesResolverConversion to handle programmatically set nulls
bbakerman Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/main/java/graphql/execution/ValuesResolverConversion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,17 @@ static Object valueToLiteralImpl(GraphqlFieldVisibility fieldVisibility,
if (valueMode == NORMALIZED) {
return assertShouldNeverHappen("can't infer normalized structure");
}
return ValuesResolverLegacy.valueToLiteralLegacy(
Value<?> value = ValuesResolverLegacy.valueToLiteralLegacy(
inputValueWithState.getValue(),
type,
graphqlContext,
locale);
//
// the valueToLiteralLegacy() nominally cant know if null means never set or is set to a null value
// but this code can know - its is SET to a value so, it MUST be a Null Literal
// this method would assert at the end of it if inputValueWithState.isNotSet() were true
//
return value == null ? NullValue.of() : value;
Copy link
Member Author

@bbakerman bbakerman Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The comment outlines why this is safe - but what you cant see in this diff is if (inputValueWithState.isInternal()) being called. Since this is true then the value MUST have been set into the input value and hence we can turn the legacy null into a AST NullValue safely.

}
if (inputValueWithState.isLiteral()) {
return inputValueWithState.getValue();
Expand Down
86 changes: 54 additions & 32 deletions src/test/groovy/graphql/introspection/IntrospectionTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package graphql.introspection


import graphql.TestUtil
import graphql.schema.DataFetcher
import graphql.schema.FieldCoordinates
Expand Down Expand Up @@ -438,7 +439,7 @@ class IntrospectionTest extends Specification {

def "test AST printed introspection query is equivalent to original string"() {
when:
def oldIntrospectionQuery = "\n" +
def oldIntrospectionQuery = "\n" +
" query IntrospectionQuery {\n" +
" __schema {\n" +
" queryType { name }\n" +
Expand Down Expand Up @@ -540,12 +541,11 @@ class IntrospectionTest extends Specification {
" }\n" +
"\n"

def newIntrospectionQuery = IntrospectionQuery.INTROSPECTION_QUERY;
def newIntrospectionQuery = IntrospectionQuery.INTROSPECTION_QUERY

then:
oldIntrospectionQuery.replaceAll("\\s+","").equals(
newIntrospectionQuery.replaceAll("\\s+","")
)
oldIntrospectionQuery.replaceAll("\\s+", "") ==
newIntrospectionQuery.replaceAll("\\s+", "")
}

def "test parameterized introspection queries"() {
Expand Down Expand Up @@ -582,44 +582,66 @@ class IntrospectionTest extends Specification {

def parseExecutionResult = {
[
it.data["__schema"]["types"].find{it["name"] == "Query"}["fields"].find{it["name"] == "notDeprecated"}["description"] != null, // descriptions is true
it.data["__schema"]["types"].find{it["name"] == "UUID"}["specifiedByURL"] != null, // specifiedByUrl is true
it.data["__schema"]["directives"].find{it["name"] == "repeatableDirective"}["isRepeatable"] != null, // directiveIsRepeatable is true
it.data["__schema"]["description"] != null, // schemaDescription is true
it.data["__schema"]["types"].find { it['name'] == 'InputType' }["inputFields"].find({ it["name"] == "inputField" }) != null // inputValueDeprecation is true
it.data["__schema"]["types"].find { it["name"] == "Query" }["fields"].find { it["name"] == "notDeprecated" }["description"] != null, // descriptions is true
it.data["__schema"]["types"].find { it["name"] == "UUID" }["specifiedByURL"] != null, // specifiedByUrl is true
it.data["__schema"]["directives"].find { it["name"] == "repeatableDirective" }["isRepeatable"] != null, // directiveIsRepeatable is true
it.data["__schema"]["description"] != null, // schemaDescription is true
it.data["__schema"]["types"].find { it['name'] == 'InputType' }["inputFields"].find({ it["name"] == "inputField" }) != null // inputValueDeprecation is true
]
}

when:
def allFalseExecutionResult = graphQL.execute(
def allFalseExecutionResult = graphQL.execute(
IntrospectionQueryBuilder.build(
IntrospectionQueryBuilder.Options.defaultOptions()
.descriptions(false)
.specifiedByUrl(false)
.directiveIsRepeatable(false)
.schemaDescription(false)
.inputValueDeprecation(false)
.typeRefFragmentDepth(5)
IntrospectionQueryBuilder.Options.defaultOptions()
.descriptions(false)
.specifiedByUrl(false)
.directiveIsRepeatable(false)
.schemaDescription(false)
.inputValueDeprecation(false)
.typeRefFragmentDepth(5)
)
)
)
then:
!parseExecutionResult(allFalseExecutionResult).any()
allFalseExecutionResult.data["__schema"]["types"].find{it["name"] == "Query"}["fields"].find{it["name"] == "tenDimensionalList"}["type"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"] == null // typeRefFragmentDepth is 5
!parseExecutionResult(allFalseExecutionResult).any()
allFalseExecutionResult.data["__schema"]["types"].find { it["name"] == "Query" }["fields"].find { it["name"] == "tenDimensionalList" }["type"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"] == null // typeRefFragmentDepth is 5

when:
def allTrueExecutionResult = graphQL.execute(
def allTrueExecutionResult = graphQL.execute(
IntrospectionQueryBuilder.build(
IntrospectionQueryBuilder.Options.defaultOptions()
.descriptions(true)
.specifiedByUrl(true)
.directiveIsRepeatable(true)
.schemaDescription(true)
.inputValueDeprecation(true)
.typeRefFragmentDepth(7)
IntrospectionQueryBuilder.Options.defaultOptions()
.descriptions(true)
.specifiedByUrl(true)
.directiveIsRepeatable(true)
.schemaDescription(true)
.inputValueDeprecation(true)
.typeRefFragmentDepth(7)
Copy link
Member Author

@bbakerman bbakerman Aug 10, 2023

Choose a reason for hiding this comment

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

mostly just reformatting

)
)
)
then:
parseExecutionResult(allTrueExecutionResult).every()
allTrueExecutionResult.data["__schema"]["types"].find { it["name"] == "Query" }["fields"].find { it["name"] == "tenDimensionalList" }["type"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"] == null // typeRefFragmentDepth is 7
}

def "issue 3285 - deprecated defaultValue on programmatic args prints AST literal as expected"() {
def queryObjType = newObject().name("Query")
.field(newFieldDefinition().name("f").type(GraphQLString)
.argument(newArgument().name("arg").type(GraphQLString).defaultValue(null)))
.build()
def schema = newSchema().query(queryObjType).build()
def graphQL = newGraphQL(schema).build()


when:
def executionResult = graphQL.execute(IntrospectionQuery.INTROSPECTION_QUERY)
then:
parseExecutionResult(allTrueExecutionResult).every()
allTrueExecutionResult.data["__schema"]["types"].find{it["name"] == "Query"}["fields"].find{it["name"] == "tenDimensionalList"}["type"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"] == null // typeRefFragmentDepth is 7
executionResult.errors.isEmpty()

def types = executionResult.data['__schema']['types'] as List
def queryType = types.find { it['name'] == 'Query' }
def fField = (queryType['fields'] as List)[0]
def arg = (fField['args'] as List)[0]
arg['name'] == "arg"
arg['defaultValue'] == "null" // printed AST
Copy link
Member Author

@bbakerman bbakerman Aug 10, 2023

Choose a reason for hiding this comment

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

test is here - the introspection AST printed value is "null" and not ""

}
}
18 changes: 18 additions & 0 deletions src/test/groovy/graphql/schema/idl/SchemaPrinterTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2247,6 +2247,24 @@ type Query {
type TestObjectB {
field: String
}
'''
}

def "issue 3285 - deprecated defaultValue on programmatic args prints as expected"() {
def queryObjType = newObject().name("Query")
.field(newFieldDefinition().name("f").type(GraphQLString)
.argument(newArgument().name("arg").type(GraphQLString).defaultValue(null)))
.build()
def schema = GraphQLSchema.newSchema().query(queryObjType).build()


when:
def options = defaultOptions().includeDirectiveDefinitions(false)
def sdl = new SchemaPrinter(options).print(schema)
then:
sdl == '''type Query {
f(arg: String = null): String
}
'''
}
}