Skip to content

Use deterministic order for type policy keyFields and field policy keyArgs #243

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

Closed
wants to merge 2 commits into from
Closed

Use deterministic order for type policy keyFields and field policy keyArgs #243

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
7f446ab
Use deterministic order for type policy keyFields and field policy ke…
rohandhruva Oct 22, 2025
cd0be1f
Add test, use SortedSet in internal data classs
rohandhruva Oct 22, 2025
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
2 changes: 1 addition & 1 deletion ...in/com/apollographql/cache/apollocompilerplugin/internal/AddKeyFieldsDocumentTransform.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ internal object AddKeyFieldsExecutableDocumentTransform : ExecutableDocumentTran
val parentTypeKeyFields = keyFields[parentType] ?: emptySet()
newSelections.filterIsInstance<GQLField>().forEach {
// Disallow fields whose alias conflicts with a key field, or is "__typename"
if (parentTypeKeyFields.contains(it.alias) || it.alias == "__typename") {
if (it.alias != null && (parentTypeKeyFields.contains(it.alias) || it.alias == "__typename")) {
throw SourceAwareException(
error = "Apollo: Field '${it.alias}: ${it.name}' in $parentType conflicts with key fields",
sourceLocation = it.sourceLocation
Expand Down
2 changes: 1 addition & 1 deletion .../kotlin/com/apollographql/cache/apollocompilerplugin/internal/CacheSchemaCodeGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ internal class CacheSchemaCodeGenerator(
withIndent {
addStatement("keyFields = setOf(")
withIndent {
typePolicy.keyFields.forEach { keyField ->
typePolicy.keyFields.sorted().forEach { keyField ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we use a List I think it'd make more sense to keep the user-defined order set in @typePolicy.

addStatement("%S, ", keyField)
}
}
Expand Down
5 changes: 3 additions & 2 deletions .../src/main/kotlin/com/apollographql/cache/apollocompilerplugin/internal/getTypePolicies.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ import com.apollographql.apollo.ast.GQLTypeDefinition
import com.apollographql.apollo.ast.Schema
import com.apollographql.apollo.ast.Schema.Companion.TYPE_POLICY
import com.apollographql.apollo.ast.SourceAwareException
import java.util.SortedSet

internal data class TypePolicy(
val keyFields: Set<String>,
val keyFields: SortedSet<String>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can just use List. Technically SortedSet makes more sense, but I'd rather avoid the JVM-only type.

Note: the equivalent TypePolicy in com.apollographql.cache.normalized.api also needs to be a List instead of a Set (that's a breaking change but that's all right at the moment).

val embeddedFields: Set<String>,
)

Expand Down Expand Up @@ -83,7 +84,7 @@ private fun Schema.validateAndComputeTypePolicy(

private fun GQLDirective.toTypePolicy(): TypePolicy {
return TypePolicy(
keyFields = extractFields("keyFields"),
keyFields = extractFields("keyFields").toSortedSet(),
embeddedFields = extractFields("embeddedFields")
)
}
43 changes: 37 additions & 6 deletions .../test/kotlin/com/apollographql/cache/apollocompilerplugin/internal/GetTypePoliciesTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,43 @@ class GetTypePoliciesTest {
).getOrThrow()

val expected = mapOf(
"User" to TypePolicy(keyFields = setOf("id"), embeddedFields = emptySet()),
"Animal" to TypePolicy(keyFields = setOf("kingdom", "species"), embeddedFields = emptySet()),
"Lion" to TypePolicy(keyFields = setOf("kingdom", "species"), embeddedFields = emptySet()),
"HasId" to TypePolicy(keyFields = setOf("id"), embeddedFields = emptySet()),
"Circle" to TypePolicy(keyFields = setOf("id"), embeddedFields = emptySet()),
"Square" to TypePolicy(keyFields = setOf("radius"), embeddedFields = emptySet()),
"User" to TypePolicy(keyFields = sortedSetOf("id"), embeddedFields = emptySet()),
"Animal" to TypePolicy(keyFields = sortedSetOf("kingdom", "species"), embeddedFields = emptySet()),
"Lion" to TypePolicy(keyFields = sortedSetOf("kingdom", "species"), embeddedFields = emptySet()),
"HasId" to TypePolicy(keyFields = sortedSetOf("id"), embeddedFields = emptySet()),
"Circle" to TypePolicy(keyFields = sortedSetOf("id"), embeddedFields = emptySet()),
"Square" to TypePolicy(keyFields = sortedSetOf("radius"), embeddedFields = emptySet()),
)

assertEquals(expected, schema.getTypePolicies())
}

@Test
fun ensureTypePolicyKeyFieldsAreSorted() {
// language=GraphQL
val schema = """
type Query {
animal: Animal
}

type Animal @typePolicy(keyFields: "kingdom species genus class domain") {
kingdom: String!
species: String!
genus: String!
domain: String!
class: String!
}
""".trimIndent()
.parseAsGQLDocument().getOrThrow()
.validateAsSchema(
SchemaValidationOptions(
addKotlinLabsDefinitions = true,
foreignSchemas = emptyList()
)
).getOrThrow()

val expected = mapOf(
"Animal" to TypePolicy(keyFields = sortedSetOf("class", "domain", "genus", "kingdom", "species"), embeddedFields = emptySet()),
)

assertEquals(expected, schema.getTypePolicies())
Expand Down
4 changes: 2 additions & 2 deletions ...lized-cache/src/commonMain/kotlin/com/apollographql/cache/normalized/api/CacheResolver.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,10 +261,10 @@ fun FieldPolicyCacheResolver(
) = object : CacheResolver {
override fun resolveField(context: ResolverContext): Any? {
val keyArgs = context.field.argumentValues(context.variables) { it.definition.isKey }
val keyArgsValues = keyArgs.values
if (keyArgsValues.isEmpty()) {
if (keyArgs.values.isEmpty()) {
return DefaultCacheResolver.resolveField(context)
}
val keyArgsValues = keyArgs.entries.sortedBy { it.key }.map { it.value }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this works only if the key arguments of a field have the same names as the key fields of the type. So for example:

extend type User @typePolicy(keyFields: "firstName lastName")
extend type Query @fieldPolicy(forField: "user", keyArgs: "firstName lastName")
{
  user(firstName: "john", lastName: "smith") {
    firstName
    lastName
  } 
}
  • The CacheKeyGenerator will return User:john+smith
  • The CacheResolver will return User:john+smith ✅

But

extend type User @typePolicy(keyFields: "firstName lastName")
extend type Query @fieldPolicy(forField: "user", keyArgs: "a b")
{
  user(b: "john", a: "smith") {
    firstName
    lastName
  } 
}
  • The CacheKeyGenerator will return User:john+smith
  • The CacheResolver will return User:smith+john ❌

In the end I think it would make sense to use the same orders as defined in @typePolicy and @fieldPolicy. But that isn't currently possible because that information is lost in compilation! Instead, for the key args, we use the order in which they are passed to the field. So in the example above we must select user(firstName: "john", lastName: "smith") and not user(lastName: "smith", firstName: "john").

But this soon will be possible, because for the key args, I intend to not use CompiledField but a Map generated in the Cache class. With that we'll have the order information.

var type = context.field.type
if (type is CompiledNotNullType) {
type = type.ofType
Expand Down