-
Notifications
You must be signed in to change notification settings - Fork 23
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.
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
GQLGW-2564: Static Args Work #461
Changes from 11 commits
49ecee5
ae478f4
3bd713d
92c7041
ff205f8
181f1c4
9a58595
745d88f
0a95127
95728d0
39fe797
c5b0014
e3ff70e
9eeb347
55d6896
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,17 @@ | ||
package graphql.nadel.dsl | ||
|
||
import graphql.language.Value | ||
|
||
// todo this should be a union or sealed class thing | ||
data class RemoteArgumentSource( | ||
val argumentName: String?, // for OBJECT_FIELD | ||
val pathToField: List<String>?, | ||
val sourceType: SourceType?, | ||
val staticValue: Value<*>?, | ||
val sourceType: SourceType, | ||
) { | ||
enum class SourceType { | ||
ObjectField, | ||
FieldArgument, | ||
StaticArgument | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,10 +8,12 @@ import graphql.Scalars.GraphQLString | |
import graphql.language.EnumTypeDefinition | ||
import graphql.language.FieldDefinition | ||
import graphql.language.ImplementingTypeDefinition | ||
import graphql.language.StringValue | ||
import graphql.nadel.Service | ||
import graphql.nadel.dsl.FieldMappingDefinition | ||
import graphql.nadel.dsl.RemoteArgumentSource.SourceType.FieldArgument | ||
import graphql.nadel.dsl.RemoteArgumentSource.SourceType.ObjectField | ||
import graphql.nadel.dsl.RemoteArgumentSource.SourceType.StaticArgument | ||
import graphql.nadel.dsl.TypeMappingDefinition | ||
import graphql.nadel.dsl.UnderlyingServiceHydration | ||
import graphql.nadel.engine.blueprint.hydration.NadelBatchHydrationMatchStrategy | ||
|
@@ -260,6 +262,7 @@ private class Factory( | |
when (it.valueSource) { | ||
is NadelHydrationActorInputDef.ValueSource.ArgumentValue -> null | ||
is FieldResultValue -> it.valueSource.queryPathToField | ||
is NadelHydrationActorInputDef.ValueSource.StaticValue -> null | ||
} | ||
}, | ||
) | ||
|
@@ -358,6 +361,7 @@ private class Factory( | |
when (val hydrationValueSource: NadelHydrationActorInputDef.ValueSource = it.valueSource) { | ||
is NadelHydrationActorInputDef.ValueSource.ArgumentValue -> emptyList() | ||
is FieldResultValue -> selectSourceFieldQueryPaths(hydrationValueSource) | ||
is NadelHydrationActorInputDef.ValueSource.StaticValue -> emptyList() | ||
} | ||
}).toSet() | ||
|
||
|
@@ -483,7 +487,11 @@ private class Factory( | |
?: error("No field defined at: ${hydratedFieldParentType.name}.${pathToField.joinToString(".")}"), | ||
) | ||
} | ||
else -> error("Unsupported remote argument source type: '$argSourceType'") | ||
StaticArgument -> { | ||
NadelHydrationActorInputDef.ValueSource.StaticValue( | ||
value = remoteArgDef.remoteArgumentSource.staticValue!! | ||
) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again later the naming here is likely to be AstArgument and NadelHydrationActorInputDef.ValueSource.AstValue |
||
} | ||
|
||
NadelHydrationActorInputDef( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import graphql.nadel.engine.transform.result.json.JsonNodeExtractor | |
import graphql.nadel.engine.util.emptyOrSingle | ||
import graphql.nadel.engine.util.flatten | ||
import graphql.nadel.engine.util.javaValueToAstValue | ||
import graphql.nadel.engine.util.makeNormalizedInputValue | ||
import graphql.nadel.engine.util.mapFrom | ||
import graphql.nadel.hooks.ServiceExecutionHooks | ||
import graphql.normalized.ExecutableNormalizedField | ||
|
@@ -44,7 +45,7 @@ internal object NadelBatchHydrationInputBuilder { | |
instruction.actorInputValueDefs.mapNotNull { actorFieldArg -> | ||
when (val valueSource = actorFieldArg.valueSource) { | ||
is NadelHydrationActorInputDef.ValueSource.ArgumentValue -> { | ||
val argValue = hydrationField.normalizedArguments[valueSource.argumentName] | ||
val argValue: NormalizedInputValue? = hydrationField.normalizedArguments[valueSource.argumentName] | ||
?: valueSource.defaultValue | ||
if (argValue != null) { | ||
actorFieldArg to argValue | ||
|
@@ -54,8 +55,19 @@ internal object NadelBatchHydrationInputBuilder { | |
} | ||
// These are batch values, ignore them | ||
is NadelHydrationActorInputDef.ValueSource.FieldResultValue -> null | ||
is NadelHydrationActorInputDef.ValueSource.StaticValue -> { | ||
val staticValue: NormalizedInputValue? = makeNormalizedInputValue( | ||
type = actorFieldArg.actorArgumentDef.type, | ||
value = valueSource.value, | ||
) | ||
if (staticValue != null) { | ||
actorFieldArg to staticValue | ||
} else { | ||
null | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if it ended up null - should it be a map of name to null? Why does this like can you do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So looking into it, this function doesn't actually return null. This syntax was just a result of copying the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
so the answer is actually it doesn't. Good catch! |
||
} | ||
} | ||
}, | ||
} | ||
sbarker2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
package graphql.nadel.schema | ||
|
||
import graphql.scalars.ExtendedScalars | ||
import graphql.GraphQLContext | ||
import graphql.Scalars | ||
import graphql.Scalars.GraphQLString | ||
|
@@ -10,7 +11,9 @@ import graphql.language.DirectiveDefinition.newDirectiveDefinition | |
import graphql.language.EnumTypeDefinition.newEnumTypeDefinition | ||
import graphql.language.EnumValueDefinition.newEnumValueDefinition | ||
import graphql.language.InputObjectTypeDefinition.newInputObjectDefinition | ||
import graphql.language.ObjectValue | ||
import graphql.language.StringValue | ||
import graphql.language.Value | ||
import graphql.nadel.dsl.FieldMappingDefinition | ||
import graphql.nadel.dsl.RemoteArgumentDefinition | ||
import graphql.nadel.dsl.RemoteArgumentSource | ||
|
@@ -72,7 +75,7 @@ object NadelDirectives { | |
) | ||
.inputValueDefinition( | ||
name = "value", | ||
type = nonNull(GraphQLString), | ||
type = nonNull(ExtendedScalars.Json), | ||
This comment was marked as resolved.
Sorry, something went wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nah, this used to only have the options "$source.blah" or "$args.blah" which were always a string. |
||
) | ||
.build() | ||
|
||
|
@@ -251,8 +254,7 @@ object NadelDirectives { | |
val hydrations = fieldDefinition.getAppliedDirectives(hydratedDirectiveDefinition.name) | ||
.asSequence() | ||
.map { directive -> | ||
val argumentValues = resolveArgumentValue<List<Any>>(directive.getArgument("arguments")) | ||
val arguments = createRemoteArgs(argumentValues) | ||
val arguments = createRemoteArgs(directive.getArgument("arguments").argumentValue.value as ArrayValue) | ||
|
||
val inputIdentifiedBy = directive.getArgument("inputIdentifiedBy") | ||
val identifiedByValues = resolveArgumentValue<List<Any>>(inputIdentifiedBy) | ||
|
@@ -330,19 +332,19 @@ object NadelDirectives { | |
) | ||
} | ||
|
||
private fun createRemoteArgs(arguments: List<Any>): List<RemoteArgumentDefinition> { | ||
private fun createRemoteArgs(arguments: ArrayValue): List<RemoteArgumentDefinition> { | ||
fun Map<String, String>.requireArgument(key: String): String { | ||
return requireNotNull(this[key]) { | ||
"${nadelHydrationArgumentDefinition.name} definition requires '$key' to be not-null" | ||
} | ||
} | ||
|
||
return arguments | ||
return arguments.values | ||
.map { arg -> | ||
@Suppress("UNCHECKED_CAST") // trust GraphQL type system and caller | ||
val argMap = arg as Map<String, String> | ||
val remoteArgName = argMap.requireArgument("name") | ||
val remoteArgValue = argMap.requireArgument("value") | ||
val argMap = arg as ObjectValue | ||
val remoteArgName = (argMap.objectFields.single { it.name == "name" }.value as StringValue).value | ||
val remoteArgValue = argMap.objectFields.single { it.name == "value" }.value | ||
val remoteArgumentSource = createRemoteArgumentSource(remoteArgValue) | ||
RemoteArgumentDefinition(remoteArgName, remoteArgumentSource) | ||
} | ||
|
@@ -363,21 +365,40 @@ object NadelDirectives { | |
} | ||
} | ||
|
||
private fun createRemoteArgumentSource(value: String): RemoteArgumentSource { | ||
val values = listFromDottedString(value) | ||
|
||
return when (values.first()) { | ||
"\$source" -> RemoteArgumentSource( | ||
private fun createRemoteArgumentSource(value: Value<*>): RemoteArgumentSource { | ||
if (value is StringValue) { | ||
sbarker2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
val values = listFromDottedString(value.value) | ||
return when (values.first()) { | ||
"\$source" -> RemoteArgumentSource( | ||
argumentName = null, | ||
pathToField = values.subList(1, values.size), | ||
staticValue = null, | ||
sourceType = SourceType.ObjectField, | ||
) | ||
|
||
"\$argument" -> RemoteArgumentSource( | ||
argumentName = values.subList(1, values.size).single(), | ||
pathToField = null, | ||
staticValue = null, | ||
sourceType = SourceType.FieldArgument, | ||
) | ||
|
||
else -> | ||
RemoteArgumentSource( | ||
argumentName = null, | ||
pathToField = null, | ||
staticValue = value, | ||
sourceType = SourceType.StaticArgument, | ||
) | ||
sbarker2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
else { | ||
sbarker2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return RemoteArgumentSource( | ||
argumentName = null, | ||
pathToField = values.subList(1, values.size), | ||
sourceType = SourceType.ObjectField, | ||
) | ||
"\$argument" -> RemoteArgumentSource( | ||
argumentName = values.subList(1, values.size).single(), | ||
pathToField = null, | ||
sourceType = SourceType.FieldArgument, | ||
) | ||
else -> throw IllegalArgumentException("$value must begin with \$source. or \$argument.") | ||
staticValue = value, | ||
sourceType = SourceType.StaticArgument, | ||
) | ||
sbarker2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
|
@@ -420,12 +441,14 @@ object NadelDirectives { | |
|
||
var argumentName: String? = null | ||
var path: List<String>? = null | ||
var value: Value<*>? = null | ||
when (argumentType) { | ||
SourceType.ObjectField -> path = values | ||
SourceType.FieldArgument -> argumentName = values.single() | ||
SourceType.StaticArgument -> value = value | ||
} | ||
|
||
return RemoteArgumentSource(argumentName, path, argumentType) | ||
return RemoteArgumentSource(argumentName, path, value, argumentType) | ||
} | ||
|
||
fun createFieldMapping(fieldDefinition: GraphQLFieldDefinition): FieldMappingDefinition? { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -234,4 +234,4 @@ internal class NadelHydrationValidation( | |
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we dont have to do it for this PR - but I think this well become just
astValue
when we do the mix or static or dynamic work laterThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good