Skip to content

#1914 Use default values for fields of input types when default argument value is provided#2085

Merged
bbakerman merged 5 commits into
graphql-java:masterfrom
priyaaggarwal24:1914
Nov 1, 2020
Merged

#1914 Use default values for fields of input types when default argument value is provided#2085
bbakerman merged 5 commits into
graphql-java:masterfrom
priyaaggarwal24:1914

Conversation

@priyaaggarwal24

@priyaaggarwal24 priyaaggarwal24 commented Oct 28, 2020

Copy link
Copy Markdown
Contributor

Adds support for honoring the default values declared in input type definition (Issue)

Example 1

Schema:

type Query {
    sayHello(arg: Arg! = {}): String
}

input Arg {
    foo: String = "Bar"
}

Query:

query {
    sayHello
}

arg value before change:
{}

arg value after change:
{foo: "Bar"}

Example 2

Schema:

type Query {
    sayHello(arg: Arg! = {field1: "F1ValOverride"}): String
}

input Arg {
    field1: String = "F1V"
    field2: String = "F2V"
}

Query:

query {
    sayHello
}

arg value before change:
{field1: "F1ValOverride"}

arg value after change:
{field1: "F1ValOverride", field2: "F2V"}

buildValue(of.getValue(), objectType.getField(of.getName()).getType())));
objectType.getFieldDefinitions().forEach(
f -> {
final Value<?> fieldValueFromDefaultObjectValue = defaultValue.getObjectFields().stream()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Performance of this statement can be improved by keeping a map of Object field's name and the field itself in ObjectValue class.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

fields of objects are typically < 10 say - iterating them is not a real problem. Also schema building is KN OWN to be a heavy thing they can be slower (where slower is not really slow) - So there are no perf savings to be had in schema generation as a rule

That is not to say it should be terrible - but that when people come to us about performance its NOT in the schema gen side of things

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks


type Query {
outputField(inputArg: InputType = {age : 666, name : "nameViaArg"}, inputBoolean: Boolean = true, inputInt: Int = 1, inputString: String = "viaArgString"): OutputType
outputField(inputArg: InputType = {age : 666, complex : {boolean : true, int : 666, string : "string"}, name : "nameViaArg", rocks : true}, inputBoolean: Boolean = true, inputInt: Int = 1, inputString: String = "viaArgString"): OutputType

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Introspection of schema now considers the default values defined in Input types.

@bbakerman bbakerman added this to the 16.0 milestone Oct 29, 2020
@bbakerman

Copy link
Copy Markdown
Member

Given:

sayHello(arg: Arg! = {foo : null}): String

what's the value? I expect null not the default in this case

I havent look in the code yet or the tests but can you answer this please either way

@priyaaggarwal24

Copy link
Copy Markdown
Contributor Author

I, too, expect null value, @bbakerman Let me add a test for this use case as well.

buildValue(of.getValue(), objectType.getField(of.getName()).getType())));
objectType.getFieldDefinitions().forEach(
f -> {
final Value<?> fieldValueFromDefaultObjectValue = defaultValue.getObjectFields().stream()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

fields of objects are typically < 10 say - iterating them is not a real problem. Also schema building is KN OWN to be a heavy thing they can be slower (where slower is not really slow) - So there are no perf savings to be had in schema generation as a rule

That is not to say it should be terrible - but that when people come to us about performance its NOT in the schema gen side of things

map.put(f.getName(), fieldValueFromDefaultObjectValue != null ? buildValue(fieldValueFromDefaultObjectValue, f.getType()): f.getDefaultValue());
}
);
return map;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

final Value<?> fieldValueFromDefaultObjectValue = defaultValue.getObjectFields().stream().filter(dvf -> dvf.getName().equals(f.getName())).map(ObjectField::getValue).findFirst().orElse(null);

ends up really hard to read. I like to put in simple methods to represent this

final Value<?> fieldValueFromDefaultObjectValue = buildFieldDefaultValue(defaultValue,...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bbakerman Done. Refactored the long statement into a method.


then:
result.errors.isEmpty()
result.data == [sayHello: "F1ValOverride & F2V"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we get another test in place for when the inner field is named BUT the value is null

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

do you something like
"default values in input objects are overridden when null value is provided in input"?

or do you mean the default value provided for a field in input type is null?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bbakerman Added a test where one of the fields in input type have null value (below this test)

@bbakerman bbakerman removed this from the 16.0 milestone Oct 30, 2020
@bbakerman bbakerman added this to the 16.0 milestone Nov 1, 2020
@bbakerman bbakerman merged commit 3295066 into graphql-java:master Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants