Take default values into account when computing field keys #5384
Conversation
✅ Deploy Preview for apollo-android-docs canceled.
|
| public final fun getName ()Ljava/lang/String; | ||
| public final fun getValue ()Ljava/lang/Object; | ||
| public final fun getValue ()Lcom/apollographql/apollo3/api/Optional; |
There was a problem hiding this comment.
Breaking change.
| @@ -305,7 +306,7 @@ public final class com/apollographql/apollo3/api/CompiledField : com/apollograph | |||
| public final fun getType ()Lcom/apollographql/apollo3/api/CompiledType; | |||
| public final fun nameWithArguments (Lcom/apollographql/apollo3/api/Executable$Variables;)Ljava/lang/String; | |||
| public final fun newBuilder ()Lcom/apollographql/apollo3/api/CompiledField$Builder; | |||
| public final fun resolveArgument (Ljava/lang/String;Lcom/apollographql/apollo3/api/Executable$Variables;)Ljava/lang/Object; | |||
| public final fun resolveArgument (Ljava/lang/String;Lcom/apollographql/apollo3/api/Executable$Variables;)Lcom/apollographql/apollo3/api/Optional; | |||
There was a problem hiding this comment.
Same here. An argument with a variable value is possibly absent if that variable is absent as well and unless we change the signature, there's no way to account for that. We could go through deprecation cycles but given this is a low level API, I'm hoping this is fine.
| @@ -351,7 +352,6 @@ public final class com/apollographql/apollo3/api/CompiledGraphQL { | |||
| public static final fun -notNull (Lcom/apollographql/apollo3/api/CompiledType;)Lcom/apollographql/apollo3/api/CompiledNotNullType; | |||
| public static final fun isComposite (Lcom/apollographql/apollo3/api/CompiledNamedType;)Z | |||
| public static final fun keyFields (Lcom/apollographql/apollo3/api/CompiledNamedType;)Ljava/util/List; | |||
| public static final fun resolveVariables (Ljava/lang/Object;Lcom/apollographql/apollo3/api/Executable$Variables;)Ljava/lang/Object; | |||
There was a problem hiding this comment.
Another breaking change. This one was an accidental public I'd say
| * | ||
| * Anything else is undefined | ||
| */ | ||
| typealias ApolloJsonElement = Any? |
There was a problem hiding this comment.
Interesting that this doesn't show in apiDump. I guess it's source-breaking only.
In all cases, I wish I've done that earlier to be able to point in KDoc to what Any? can contain. Naming is very open to suggestions.
There was a problem hiding this comment.
ApolloJsonElement sounds good to me 👍. I remember doing a similar thing (typealias JsonMap = Map<String, Any?>) in DeferredJsonMerger which made the code less messy.
| KotlinSymbols.BooleanAdapter, | ||
| defaultValue.value, | ||
| KotlinSymbols.NullableAnyAdapter, | ||
| defaultValue.codeBlock(), |
There was a problem hiding this comment.
This is the most important change. serializeVariables now serializes all the variables if asked to (instead of just the boolean variables before)
| /** | ||
| * Pull all arguments from the schema as we need them to compute the cache key | ||
| */ | ||
| val typeDefinition = schema.typeDefinition(parentType) | ||
| val actualArguments = fieldDefinition.arguments.map { schemaArgument -> | ||
| val operationArgument = arguments.firstOrNull { it.name == schemaArgument.name } | ||
|
|
||
| val keyArgs = typeDefinition.keyArgs(name, schema) | ||
| val paginationArgs = typeDefinition.paginationArgs(name, schema) | ||
|
|
||
| /** | ||
| * When passed explicitly, the argument values are coerced (but not their default value) | ||
| */ | ||
| val userValue = operationArgument?.value?.coerceInExecutableContextOrThrow(schemaArgument.type, schema) | ||
| IrArgument( | ||
| name = schemaArgument.name, | ||
| value = (userValue ?: schemaArgument.defaultValue)?.toIrValue(), | ||
| isKey = keyArgs.contains(schemaArgument.name), | ||
| isPagination = paginationArgs.contains(schemaArgument.name) | ||
| ) | ||
| } |
There was a problem hiding this comment.
This is the other important change. The CompiledSelections now contain even more schema information.
| * | ||
| * Anything else is undefined | ||
| */ | ||
| typealias ApolloJsonElement = Any? |
There was a problem hiding this comment.
ApolloJsonElement sounds good to me 👍. I remember doing a similar thing (typealias JsonMap = Map<String, Any?>) in DeferredJsonMerger which made the code less messy.
See the tests in CacheArgumentTest. Allows to take defaultValues in cache keys into account for queries like so:
Also take this opportunity to fix
GQLIntValueandGQLFLoatValueto make them support arbitrary precision numbers.While these 2 things are not directly linked, they are related as
defaultValueswere the only places where we would use theGQLIntValueandGQLFLoatValuevalues.Closes #5186
I'm not 100% certain yet about the exhaustivity and correctness of the coercion in different cases (see also graphql/graphql-spec#793) but this should provide a good base to build on.