Skip to content
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

feat: Specify caching fields with typePolicy directive #554

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: Specify caching fields with typePolicy directive #554

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9c78c4
feat: Specify caching fields with typePolicy directive
x-sheep Dec 6, 2024
c40b446
Add keyFields to generated Interfaces
x-sheep Dec 17, 2024
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
Next Next commit
feat: Specify caching fields with typePolicy directive
  • Loading branch information
@x-sheep
x-sheep committed Dec 13, 2024
commit c9c78c42f86b7ad05a6585b58272afe013f5ed81
6 changes: 3 additions & 3 deletions Sources/AnimalKingdomAPI/animalkingdom-graphql/AnimalSchema.graphqls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,15 @@ input PetSearchFilters {
measurements: MeasurementsInput
}

interface Animal {
interface Animal @typePolicy(keyFields: "id") {
id: ID!
species: String!
height: Height!
predators: [Animal!]!
skinCovering: SkinCovering
}

interface Pet {
interface Pet @typePolicy(keyFields: "id") {
Copy link
Author

Choose a reason for hiding this comment

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

I added the directive to one of the schemas, but I didn't regenerate the example sources since that would add another 100 files to the Pull Request

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine while we do code review. But we should go ahead and generate those files and verify they look correct and compile prior to merging.

id: ID!
humanName: String
favoriteToy: String!
Expand Down Expand Up @@ -198,5 +198,5 @@ enum SkinCovering {
FUR
HAIR
FEATHERS
SCALES
SCALES
}
4 changes: 3 additions & 1 deletion Tests/ApolloCodegenInternalTestHelpers/MockGraphQLType.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@ public extension GraphQLObjectType {
_ name: String = "",
interfaces: [GraphQLInterfaceType] = [],
fields: [String: GraphQLField] = [:],
keyFields: [String] = [],
documentation: String? = nil
) -> GraphQLObjectType {
GraphQLObjectType(
name: GraphQLName(schemaName: name),
documentation: documentation,
fields: fields,
interfaces: interfaces
interfaces: interfaces,
keyFields: keyFields
)
}
}
Expand Down
44 changes: 40 additions & 4 deletions Tests/ApolloCodegenTests/CodeGeneration/Templates/ObjectTemplateTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@ class ObjectTemplateTests: XCTestCase {
name: String = "Dog",
customName: String? = nil,
interfaces: [GraphQLInterfaceType] = [],
keyFields: [String] = [],
documentation: String? = nil,
config: ApolloCodegenConfiguration = .mock()
) {
let objectType = GraphQLObjectType.mock(
name,
interfaces: interfaces,
keyFields: keyFields,
documentation: documentation
)
objectType.name.customName = customName
Expand Down Expand Up @@ -82,7 +84,7 @@ class ObjectTemplateTests: XCTestCase {
implementedInterfaces: [
TestSchema.Interfaces.Animal.self,
TestSchema.Interfaces.Pet.self
]
],
"""

// when
Expand All @@ -106,7 +108,7 @@ class ObjectTemplateTests: XCTestCase {
implementedInterfaces: [
Interfaces.Animal.self,
Interfaces.Pet.self
]
],
"""

// when
Expand All @@ -121,7 +123,7 @@ class ObjectTemplateTests: XCTestCase {
buildSubject()

let expected = """
implementedInterfaces: []
implementedInterfaces: [],
"""

// when
Expand All @@ -130,6 +132,39 @@ class ObjectTemplateTests: XCTestCase {
// then
expect(actual).to(equalLineByLine(expected, atLine: 3, ignoringExtraLines: true))
}

func test_render_givenKeyField_rendersKeyFieldArray() {
// given
buildSubject(keyFields: ["id"])

let expected = """
keyFields: ["id"]
"""

// when
let actual = renderSubject()

// then
expect(actual).to(equalLineByLine(expected, atLine: 4, ignoringExtraLines: true))
}

func test_render_givenMultipleKeyFields_rendersKeyFieldArray() {
// given
buildSubject(keyFields: ["id", "species"])

let expected = """
keyFields: [
"id",
"species"
]
"""

// when
let actual = renderSubject()

// then
expect(actual).to(equalLineByLine(expected, atLine: 4, ignoringExtraLines: true))
}

// MARK: Documentation Tests

Expand Down Expand Up @@ -213,7 +248,8 @@ class ObjectTemplateTests: XCTestCase {
// Renamed from GraphQL schema value: 'MyObject'
static let MyCustomObject = ApolloAPI.Object(
typename: "MyObject",
implementedInterfaces: [TestSchema.Interfaces.MyCustomInterface.self]
implementedInterfaces: [TestSchema.Interfaces.MyCustomInterface.self],
keyFields: nil
)
"""

Expand Down
51 changes: 51 additions & 0 deletions Tests/ApolloTests/CacheKeyResolutionTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,4 +209,55 @@ class CacheKeyResolutionTests: XCTestCase {
expect(actual).to(equal("GreekLetters:δ"))
}

func test__schemaConfiguration__givenSingleKeyField_shouldReturnKeyFieldValue() {
let Delta = Object(typename: "Dog", implementedInterfaces: [], keyFields: ["id"])

MockSchemaMetadata.stub_objectTypeForTypeName({ _ in Delta })

let object: JSONObject = [
"__typename": "Dog",
"id": "10",
"name": "Beagle"
]

let objectDict = NetworkResponseExecutionSource().opaqueObjectDataWrapper(for: object)
let actual = MockSchemaMetadata.cacheKey(for: objectDict)

expect(actual).to(equal("Dog:10"))
}

func test__schemaConfiguration__givenMultipleKeyFields_shouldReturnKeyFieldValues() {
let Delta = Object(typename: "Dog", implementedInterfaces: [], keyFields: ["id", "name"])

MockSchemaMetadata.stub_objectTypeForTypeName({ _ in Delta })

let object: JSONObject = [
"__typename": "Dog",
"id": "10",
"name": #"Be\ag+le"#,
"height": 20,
]

let objectDict = NetworkResponseExecutionSource().opaqueObjectDataWrapper(for: object)
let actual = MockSchemaMetadata.cacheKey(for: objectDict)

expect(actual).to(equal(#"Dog:10+Be\\ag\+le"#))
}

func test__schemaConfiguration__givenMissingKeyFields_shouldReturnNil() {
let Delta = Object(typename: "Dog", implementedInterfaces: [], keyFields: ["id", "name"])

MockSchemaMetadata.stub_objectTypeForTypeName({ _ in Delta })

let object: JSONObject = [
"__typename": "Dog",
"id": "10",
]

let objectDict = NetworkResponseExecutionSource().opaqueObjectDataWrapper(for: object)
let actual = MockSchemaMetadata.cacheKey(for: objectDict)

expect(actual).to(beNil())
}

}
11 changes: 10 additions & 1 deletion apollo-ios-codegen/Sources/ApolloCodegenLib/Templates/ObjectTemplate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,19 @@ struct ObjectTemplate: TemplateRenderer {
\(graphqlObject.name.typeNameDocumentation)
static let \(graphqlObject.render(as: .typename)) = \(config.ApolloAPITargetName).Object(
typename: "\(graphqlObject.name.schemaName)\",
implementedInterfaces: \(ImplementedInterfacesTemplate())
implementedInterfaces: \(ImplementedInterfacesTemplate()),
keyFields: \(KeyFieldsTemplate())
)
"""
}

private func KeyFieldsTemplate() -> TemplateString {
guard let fields = graphqlObject.keyFields, !fields.isEmpty else { return "nil" }

return """
[\(list: fields.map { "\"\($0)\"" })]
"""
}

private func ImplementedInterfacesTemplate() -> TemplateString {
return """
Expand Down
2 changes: 1 addition & 1 deletion apollo-ios-codegen/Sources/GraphQLCompiler/ApolloCodegenFrontendBundle.swift
View file
Open in desktop

Large diffs are not rendered by default.

7 changes: 6 additions & 1 deletion apollo-ios-codegen/Sources/GraphQLCompiler/GraphQLSchema.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,16 +239,20 @@ public final class GraphQLObjectType: GraphQLCompositeType, GraphQLInterfaceImpl
public private(set) var fields: [String: GraphQLField]!

public private(set) var interfaces: [GraphQLInterfaceType]!

public private(set) var keyFields: [String]!
Copy link
Contributor

Choose a reason for hiding this comment

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

I assumed we would implement this by adding keyFields to both GraphQLObjectType and GraphQLInterfaceType and then looking for them on the object, and if they don't exist, then going through the interfaces.

The way you are implementing this, we do that logic in the typePolicyDirectiveFor in the JS code, computing what keyFields each object would have and then applying them to all the objects. I do actually think that feels better!

There is a possibility of using the keyFields on interfaces with "unknown types", but it has enough complications that I think it's probably okay to leave that out of scope for now. But just some thoughts on how that could work for future reference:

If a type is added to a schema after the client has been shipped, but it implements an interface that had a defined typePolicy, we should theoretically be able to compute a cache key for it without even knowing about that object type. This would require us to hold onto the keyFields of the interface types themselves though, and then apply the logic for determining keyFields at runtime rather than at "code-gen-time".

This also runs into some minor edge cases (eg. if the type implements multiple interfaces that have conflicting @typePolicy directives) which would need to be addressed.

We also wouldn't be able to actually know all of the interface types that an unknown type implemented. We could only infer that if a type with an unknown __typename is returned by the server for a field of an interface type, then that type must implement that interface. But it could also implement other interfaces and be returned as the type for a field with another interface that has a different @typePolicy. In that case, the cache would not normalize the object properly because it would compute it with two different cache keys.

This is a super niche and likely very rare edge case, but I think it is enough of a consideration that we should not handle computing cache keys for unknown types via inheritance of an interface's @typePolicy at this time. I have a vision for a future in which we can do introspection queries and apply new type information to the schema at runtime. If that ever comes to fruition, we could reconsider this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should keyFields be optional, and if there are none we return nil instead of an empty array?

Copy link
Author

Choose a reason for hiding this comment

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

I made keyFields required, just to make sure the data is loaded correctly from the Javascript environment. The same field is Optional in the generated code.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a super niche and likely very rare edge case, but I think it is enough of a consideration that we should not handle computing cache keys for unknown types via inheritance of an interface's @typePolicy at this time.

Potential counter argument:

interface Node @typePolicy(keyFields: "id") {
  id: ID! 
}

In schemas with global ids, that's a convenient way do add id to all objects that inherit Node. Agree it raises a bunch of questions about potential inconsistencies. Maybe those consistencies could be checked on the server side, not sure.

FWIW, Kotlin currently keep tracks of key fields on interfaces at runtime too. Also because we don't have a global "typename -> object" dictionnary at runtime and didn't want to introduce it just for @typePolicy. So there's a small discrepency there but I think it's ok, maybe even good at this point. We can iterate various approaches and get field feedback before commiting to the final behaviour.

Copy link
Author

Choose a reason for hiding this comment

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

extend interface Node @typePolicy(keyFields: "id")

Every type I want to use in my specific iOS project has a base interface with the ID field, so it's important to me that the directive works on interfaces.

If a GQL server introduces a new type for an old endpoint used by a Client app that isn't up to date, it will now just miss the directive because the Object declaration doesn't exist. It's still possible to cache it anyway using Programmatic Cache Keys.

Copy link
Author

Choose a reason for hiding this comment

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

Should this be addressed in a separate PR? If yes, should it be done before or after merging this @typePolicy branch?

Copy link
Contributor

@AnthonyMDev AnthonyMDev Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we do need to add the keyFields to interface types and add that to the handling of keyFields for a type that is not a known generated type.

Even if we aren't handling all the cases of "unknown types" this is important because we are currently building a feature to allow many concrete types to not be generated (if they aren't directly referenced, but only can be returned for a field from an interface).

So this means that ALOT of known types would not normalize properly as well, as they would appear at run-time to be unrecognized types.

I really would have loved for us to be able to solve the issues with handling unknown types here in a more elegant way, but for now, I think this is what we need to do. Which, to be fair, is actually just mirroring what Apollo Kotlin does currently, and they haven't seen a lot of issues come up on that side. The only thing that's not working properly here is handling cache normalization for an unknown (or not-generated) type that, conforms to two interfaces with conflicting @typePolicy directives. But it's a very rare, minor edge case, and there isn't a great way to fix for that right now. So I think that we can't let that hold up pushing this feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. Also about this part:

The only thing that's not working properly here is handling cache normalization for an unknown (or not-generated) type that, conforms to two interfaces with conflicting @typePolicy directives.

Longer term, we could add linting/schema checks to catch that before the schema is actually put in production.

Copy link
Author

@x-sheep x-sheep Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This PR already throws an error if any type is found that has conflicting typePolicies through its inherited interfaces. As soon as the schema is updated, any new conflict will become apparent. The fact that it'll use the typePolicy of just one of the interfaces until then is unfortunate, but IMHO very unlikely.

I'm not aware of what the changes look like when Apollo iOS generates less code, but if the entire schema is still validated in the Javascript/IR step this will still catch conflicting policies (regardless of their usage by the client).

I can update this PR to add typePolicy information to the Interfaces as well as the Objects, so it will also run the above validation on interfaces without (known) object types.

Copy link
Author

Choose a reason for hiding this comment

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

I've now added keyFields to the generated Interface instances.


/// Initializer to be used for creating mock objects in tests only.
init(
name: GraphQLName,
documentation: String?,
fields: [String: GraphQLField],
interfaces: [GraphQLInterfaceType]
interfaces: [GraphQLInterfaceType],
keyFields: [String]
) {
self.fields = fields
self.interfaces = interfaces
self.keyFields = keyFields
super.init(name: name, documentation: documentation)
}

Expand All @@ -259,6 +263,7 @@ public final class GraphQLObjectType: GraphQLCompositeType, GraphQLInterfaceImpl
override func finalize(_ jsValue: JSValue, bridge: isolated JavaScriptBridge) {
self.fields = try! bridge.invokeMethod("getFields", on: jsValue)
self.interfaces = try! bridge.invokeMethod("getInterfaces", on: jsValue)
self.keyFields = jsValue["_apolloKeyFields"]
}

public override var debugDescription: String {
Expand Down
Loading