-
Notifications
You must be signed in to change notification settings - Fork 59
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
rfc: Conformance to Identifiable protocol #549
base: main
Are you sure you want to change the base?
Changes from all commits
b3cd87b
4656a47
6430f37
fcb15f0
addfcfa
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 |
---|---|---|
@@ -0,0 +1,124 @@ | ||
# Summary | ||
|
||
This document proposes a new GraphQL directive named `@identity`, to be used to mark a field which can uniquely identify an object. | ||
|
||
# Introduction | ||
|
||
SwiftUI makes heavy use of the [Identifiable](https://developer.apple.com/documentation/swift/identifiable) protocol, which is used to track the identity of entities across data changes. If an object with a List is replaced with a different object with the same identity, SwiftUI will animate the item changing instead of animating an insertion. Objects that do not conform to the Identifiable protocol require additional boilerplate to be usable inside SwiftUI. | ||
|
||
Apollo Client iOS could assist the developer by adding conformance to the Identifiable protocol to its generated models. Selecting the field to be used as an identity is done by adding a new GraphQL directive. | ||
|
||
A concept of identity is required to allow response objects to be cached. The various Apollo Client projects have mechanisms that allow for identifiers to be selected through additional code. The new directive could allow schema authors to assist client authors with caching. | ||
|
||
# Definition | ||
|
||
The directive is defined as: | ||
```graphql | ||
directive @identity(scope: IdentityScope = TYPE) on FIELD | FIELD_DEFINITION | ||
|
||
enum IdentityScope { | ||
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. This is a novel idea but you can probably derive the scope from the context in which the directive is used; interface field vs. type field vs. selection field. This may also introduce a bunch of validation required to make sure it isn't used with excessive scope, i.e.: should a selection field be able to define global scope? Probably not. 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. I changed my mind regarding the SELECTION scope. Its application is too limited (protocol conformance only), so I took it out. I think removing this case also removes the reason for clients to widen the scope themselves. |
||
TYPE | ||
SERVICE | ||
GLOBAL | ||
} | ||
``` | ||
|
||
This directive MUST be used on a field with a non-nullable scalar type. Other types are not supported. | ||
|
||
## Scope parameter | ||
|
||
The directive can have a parameter indicating the scope of the identity. They are ordered from _narrowest_ to _widest_: | ||
|
||
1. `TYPE`: The identifier is unique for the type, e.g. an auto-incrementing column from a database table. | ||
2. `SERVICE`: The identifier is unique across the current GraphQL Service. | ||
3. `GLOBAL`: The identifier is unique across all GraphQL services. This can be used by identifiers generated according to [RFC 4122](https://datatracker.ietf.org/doc/html/rfc4122) or [RFC 9562](https://datatracker.ietf.org/doc/html/rfc9562) (also known as Universally Unique IDentifiers or UUIDs). | ||
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. How can you guarantee that this is unique across all GraphQL services globally? Are we just assuming you won't likely ever hit a conflicting ID if you are using UUIDs? In the future of a global graph using 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. There's no guarantee beyond what the RFC provides (where the chance of collision is near-zero). I've specifically worded it as "generated according to the spec" instead of only looking like 128 bits. It's still allowed for a GraphQL server to append more text to an identifier of this type (as long as the UUID portion itself doesn't have any data removed) to make the risk of collisions even smaller. |
||
|
||
## Usage in types | ||
|
||
A type MAY have a field annotated with the directive. | ||
|
||
```graphql | ||
type Animal { | ||
id: ID! @identity(scope: SERVICE) | ||
name: String | ||
} | ||
``` | ||
|
||
Within a type, the directive MUST NOT be used on more than one field. | ||
|
||
## Usage in interfaces | ||
|
||
It's possible for interfaces to use the directive on a field. | ||
|
||
```graphql | ||
interface Identifiable { | ||
id: ID! @identity(scope: TYPE) | ||
} | ||
``` | ||
|
||
Implementations of this interface MUST copy the directive to the same field. The scope argument in the implementation MUST NOT be narrower than the scope in the interface, but it may be wider. | ||
Comment on lines
+54
to
+59
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. I'm not sure of how else to handle this, but I'm concerned this may be too constraining for many real life applications. What if you have a field of an interface type and you want to conform to that interface by multiple different types that have different fields used for identity? Maybe that isn't too much of a concern? But it seems to me that what we really want is some way of saying "all types that implement this interface must have a field marked I'm also not clear how this would work with fields of a union type. There was once an RFC to allow unions to require an interface implementation, but it's stalled out as of now. |
||
|
||
## Usage in operations | ||
|
||
It's likely that an external schema will not use this directive. In this case, an identity MAY be chosen when writing a query. The directive SHOULD NOT be included in the query sent to the GraphQL server. | ||
|
||
```graphql | ||
query GetAllAnimals { | ||
allAnimals { | ||
id @identity | ||
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. In operation usage, do you not need to provide a 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. Omitting the parameter defaults to the narrowest scope. |
||
string | ||
} | ||
} | ||
``` | ||
|
||
It is allowed for a query and a type to both use the directive to describe the same field. If the client and schema use different scopes for the same field, the scope defined by the schema is used. Clients SHOULD NOT define a wider scope than declared by the schema. Codegen should emit a warning in this case. | ||
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. I'm unclear why the client would ever need to define any scope here. When using |
||
|
||
If the server defines a field as an identity, a query SHOULD NOT choose another field. A query MUST NOT use the directive on more than one field in the same selection (unless they are in differently nested objects). | ||
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. While a directive provided by schema authors is likely to be helpful in many cases. I also have a concern that it won't be 100% correct for all client use cases. There would need to be a mechanism for client developers to easily override that behavior. |
||
|
||
## Protocol conformance | ||
|
||
If an operation contains a field marked with the `@identity` directive (by either the schema or the operation itself), the generated SelectionSet will have a conformance to the Identifiable protocol. Since the [Swift documentation](https://developer.apple.com/documentation/swift/identifiable) states that the scope and duration of an identity is unspecified, the scope parameter is ignored when deciding to add a conformance. | ||
|
||
```swift | ||
// Inline selection | ||
public struct Data: AnimalKingdomAPI.SelectionSet, Identifiable { /* ... */ } | ||
|
||
// Fragment selection | ||
public struct PetDetails: AnimalKingdomAPI.SelectionSet, Fragment, Identifiable { /* ... */ } | ||
``` | ||
|
||
The protocol requires that the identity is accessible through a public field named exactly `id`. If the annotated field has a different name, an additional getter will be generated: | ||
|
||
```swift | ||
public var id: String { self.uuid } | ||
``` | ||
|
||
### Naming conflicts | ||
|
||
If the identity field is not called `id`, but another field called `id` is present in the selection, a custom getter cannot be added. Swift does not support using another field to handle the conformance. | ||
|
||
In this case, a conformance to Identifiable SHOULD NOT be generated. Codegen should emit a warning without stopping the generation process. | ||
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. This is a definite, unavoidable edge case. It's not ideal, but I suppose just emitting a warning and not generating the conformance is the proper behavior here. |
||
|
||
## Caching behavior | ||
|
||
If no `@identity` directive is present, clients MAY use other mechanisms to determine if and how to cache the object. | ||
|
||
An identifier with scope `TYPE` can be combined with the `__typename` field to generate a caching key that's unique for the GraphQL Service. | ||
|
||
Identifiers with a scope of `SERVICE` or `GLOBAL` can be directly used as a caching key. | ||
Comment on lines
+106
to
+108
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. This makes sense, but honestly, we may continue to combine with the |
||
|
||
# Alternatives | ||
|
||
## Automatic conformance to Identifiable | ||
|
||
The code generator could be updated to always emit a conformance to Identifiable if a scalar `id` field is present in the selection set, removing the need for a custom directive. This was suggested in Pull Request [#548](https://github.com/apollographql/apollo-ios-dev/pull/548). | ||
|
||
## Apollo Kotlin's @typePolicy directive | ||
|
||
Apollo Kotlin has [custom directives](https://www.apollographql.com/docs/kotlin/caching/declarative-ids) that allows for client authors to specify the caching key through pure GraphQL: | ||
|
||
```graphql | ||
extend type Book @typePolicy(keyFields: "id") | ||
``` | ||
|
||
This could be used for Identifiable conformance by generating a getter which returns a tuple of all listed key fields, and caching behavior could also be matched with Apollo Kotlin. A disadvantage is that this directive is only used inside type extensions, and can't be used to mark fields directly in a query. | ||
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. I actually think that is a more principled approach, and leads to far less awkward and confusing interactions. In a GraphQL client's normalized cache, you really want identity to be consistent across your operations for proper normalization. I think the use case where you want to use a field for identity in only a single operation is pretty narrow and probably an anti-pattern. |
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.
This is a nice ideal but not likely, as you noted below in Usage in operations.
@key
directive but I don't think those are passed through in the API schema SDL, nor introspection.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.
I agree this document is coming close to a xkcd 927 situation. I wasn't aware of the
@key
directive, but it not being revealed to the clients is a deal-breaker for my use case.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 have not previously defined any directives that are exposed as part of the public schema and are intended for consumption by the Apollo client's specifically. While this is certainly possible, we need to consider all of the issues this can cause. These directives would need to be namespaced accordingly. It is very possible that some schema's out there already have an
@identity
directive that they are using for some other use case. We can't just assume that directive is what we are looking for and consume it on the client for this usage.Namespacing of directives doesn't really have great support in the GraphQL spec currently. We'd need to use some ugly directive name like
@apollo_client_identity
. I know that an@core
directive was discussed by us at some point to help with this, but it's not an accepted and used feature currently. @martinbonnin knows more about that than I do.