Allow @typePolicy directive on interfaces and unions

[lukel97]

Closes #3356 (if this is the desirable behaviour!)
This fixes a couple of things:
It changes the @typePolicy directive definition in apollo.graphqls to allow it to be used on interfaces.
And in order for it to be picked up, this extends GQLInterfaceTypeExtension with a directives field.
Finally, it also checks for interfaces as well as objects when checking the key fields

@martinbonnin

[netlify]

👷 Deploy request for apollo-android-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	7de9b02

[martinbonnin]

Do you mind adding UNION while we're at it? They're conceptually very close to interfaces

Suggested change

	directive @typePolicy(keyFields: String!) on OBJECT | INTERFACE
	directive @typePolicy(keyFields: String!) on OBJECT | INTERFACE | UNION

[martinbonnin]

Thank you so much for this, this is really cool! 2 small comments but look good to me overall!

[lukel97]

@martinbonnin Where would be the best place to throw an exception, and what exception type should I use for a user facing error, i.e. ApolloException? I’ve currently added it inside schema.kt within the keyFields function, but it uses error which looks like it’s meant for internal errors. I could alternatively check and throw exceptions in check_key_fields.kt

[martinbonnin]

what exception type should I use for a user facing error

Good question. We're historically been quite liberal in what exceptions are thrown in the Gradle codegen side of things. While the runtime side of things is using ApolloException everywhere, the codegen side of things uses a bunch of different exceptions. Ideally we should log Issues so that the codegen can output all issues at once without the user having to fix one issue, recompile, fix the next one, etc...

That being said, Issue is in the apollo-ast module and checkKeyFields is in the Apollo-specific apollo-compiler module. I guess this is why it just used "error" because it was easier edit: nevermind, both are in the apollo-ast module so not sure why we don't use Issues everywhere.

For the purpose of this PR, feel free to throw IllegalStateException or just error and this should be fine. ApolloException is for runtime only. I don't even think it's in the compiler classpath.

Where would be the best place to throw an exception

I think the best place is in GQLDocument.validateAsSchema. This validation doesn't need to know the operation, it can operate on the schema, which is way faster. Make a mapping of TypeDefinition -> keyFields and check that inside a given type hierarchy, all the keyFields are the same? Ultimately, we might want to factor in this code, i.e. precompile the mapping just after getting the schema and reuse that one from checkKeyFields (instead of computing it lazily as we do now). But that can be done in a follow up PR

[lukel97]

@martinbonnin Reworked it to report an issue: Had to extract out the keyFields logic from Schema.kt to gqltypedefinition.kt since the validation step occurs before the schema object has been created

Should also work on unions now too

[martinbonnin]

The IntrospectionTest fails because it has an invalid schema (where there are both an interface and an input object named Character). You can just remove the error:

apollo-kotlin/apollo-ast/src/main/kotlin/com/apollographql/apollo3/ast/gqltypedefinition.kt

Line 101 in f28d91c

else -> error("Type '$name' cannot have key fields")

    is GQLUnionTypeDefinition -> directives.toKeyFields() ?: emptySet()
-    else -> error("Type '$name' cannot have key fields")
+   else -> emptySet() // If we come here, the schema is invalid and this should be caught by other validation rules

[martinbonnin]

And integration-tests had some tests using the previous logic of "overriding" the key fields. Sorry about this, things were a bit "in flight" (which is part of the reason why we disabled @typePolicy on interfaces). Let me know if it's too cumbersome, I can take it from there.

[lukel97]

Whoops, didn’t realise the integration tests were failing. Fixing now

[lukel97]

Potentially stupid question: How can I run the integration tests locally? Is there a specific gradle task for them or do I need to run the allTests task? IntelliJ isn’t suggesting to run them from within the IDE either

[martinbonnin]

You'll need to open the tests project in IDE. It's a composite build that will import the integration tests.

Or you can run the tests from the command line with:

./gradlew -p tests allTest

[martinbonnin]

Many thanks again for looking into this. I just pushed a small optimisation to cache the keyFields in the Schema and a test for the case where a type inherits two interfaces with different keyfields. Will merge once tests are all ✅

[BoD]

(Nitpick) maybe the comment could be moved to the method above.

[martinbonnin]

I added another comment on the method above and kept this one as it's the entry point of validation.

[BoD]

🙏 👍

[martinbonnin]

Thanks for the contribution!

[lukel97]

Hi, thanks for patching up the PR! I was able to pull it from the snapshots repository which was great, but it had some surprising behaviour with my own schema, my extras.graphqls is just this one line:

extend interface Node @typePolicy(keyField: "id”)

And my server schema has types like this:

interface Node {}
interface UniformResourceLocatable {}
# and some more interfaces etc.
type Foo implements Node, UniformResourceLocatable {}

With the latest version of this PR the compiler plugin errors because Foo inherits different keys from different interfaces:

Cause: e: /Users/luke/Source/minm-android/app/src/main/graphql/schema.graphqls: (10, 1): Apollo: Type 'Album' cannot inherit different keys from different interfaces:
LibraryItem: []
Node: [id]
UniformResourceLocatable: []

Even though Node is the only interface with a @typePolicy directive, the other interfaces technically have keyFields of []. I’m not really sure why this is happening: the distinct key fields should just be the one (id):

apollo-kotlin/apollo-ast/src/main/kotlin/com/apollographql/apollo3/ast/internal/SchemaValidationScope.kt

Lines 206 to 216 in 2187cce

	val interfacesKeyFields = interfaces.map { keyFields(allTypeDefinition[it]!!, allTypeDefinition, keyFieldsCache) }
	val distinct = interfacesKeyFields.distinct()
	if (distinct.size > 1) {
	val extra = interfaces.indices.map {
	"${interfaces[it]}: ${interfacesKeyFields[it]}"
	}.joinToString("\n")
	registerIssue(
	message = "Apollo: Type '${typeDefinition.name}' cannot inherit different keys from different interfaces:\n$extra",
	sourceLocation = typeDefinition.sourceLocation
	)
	}

[martinbonnin]

Oops, my bad, I forgot to exclude the 'empty' key fields special case. I'll make a patch in a few

[martinbonnin]

Fix is there, sorry for this. (the list was a list of Sets so calling distinct on it would still include the empty set)

Interestingly, this does not distinguish between interface Foo @typePolicy(keyFields: "") and just interface Foo. It could be interesting to throw a validation error on keyFields: "" as I don't think there's a use case for this.