rfc: Conformance to Identifiable protocol #549
Conversation
👷 Deploy request for apollo-ios-docc pending review.Visit the deploys page to approve it
|
👷 Deploy request for eclectic-pie-88a2ba pending review.Visit the deploys page to approve it
|
🚫 Docs Preview DeniedYou must have approval from an Apollo team member to request a docs preview. If you are a team member, please comment File Changes 0 new, 1 changed, 0 removed |
|
|
||
| ## 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 does not need to be included in the query sent to the GraphQL server. |
There was a problem hiding this comment.
Are we thinking that applying the directive at the query level would bubble up so that all selections of that type across all queries would receive the Identifiable? Wondering if it may result in a confusing result for users if one selection of a given type is Identifiable, while another use of the same type in a different selection set is not?
There was a problem hiding this comment.
It doesn't seem like a good idea to let queries change the behavior of other queries. If the external schema doesn't set a field, a query should only add Identifiable if the directive is added in the query itself, or in one of the fragments it uses.
There was a problem hiding this comment.
Yea that would be my general sentiment as well, figured this scenario was at least worth discussion because of the behavior being provided. Although since this directive would really leave it up to the user to define how and when to use Identifiable then they have the flexibility to apply it to the type itself if they want.
There was a problem hiding this comment.
It doesn't seem like a good idea to let queries change the behavior of other queries.
This sentiment might be true if the only application of the directive is Identifable conformance; I believe it's larger than that.
There was a problem hiding this comment.
Can you give an example of how it would be beneficial to look at only the queries, instead of using existing GraphQL constructs like fragments or type extensions? I think it would be surprising behavior if the existence of certain queries had any effect how other queries are handled (not just regarding the Swift protocol)
There was a problem hiding this comment.
I'm not advocating for operation-based directives to supersede type-based directives - I agree that's bad behaviour.
This RFC should focus on object identity, not Identifiable conformance. Once we've expanded it to include all desired applications of the directive there may be a case where its use does influence other selection sets within the same operation. I don't know if those exist, none of us do until we expand the scope of the RFC.
This directive is about giving objects a declarative identity. This focus is not |
|
This is a good start, thanks @x-sheep. As I said above, this RFC should focus on uncovering all possible uses of object identity. |
The final section regarding 'Scope' briefly touches on this. This document could be expanded to mention caching explicitly. This would also make the directive relevant to all Apollo Client libraries (not just Swift) so a rename could be in order. |
|
On the topic of caching, Apollo Kotlin uses its own mechanism that Apollo iOS currently doesn't. It adds a directive that should be used in a type extension, instead of as part of the query.
|
|
I've added an idea of a scope parameter, and made mention of how it can be used for caching purposes. WDYT? |
|
|
||
| # 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. |
There was a problem hiding this comment.
The new directive could allow schema authors to assist client authors with caching.
This is a nice ideal but not likely, as you noted below in Usage in operations.
- Federated schema authors already have the
@keydirective but I don't think those are passed through in the API schema SDL, nor introspection. - You'll end up with a string of different directives if there isn't uniformity across all the clients.
There was a problem hiding this comment.
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.
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.
| directive @apollo_client_ios_identity on FIELD | FIELD_DEFINITION | ||
| directive @identity(scope: IdentityScope = SELECTION) on FIELD | FIELD_DEFINITION | ||
|
|
||
| enum IdentityScope { |
There was a problem hiding this comment.
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.
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.
|
|
||
| The directive could be expanded to declare the scope of an identity (e.g. unique across the entire API or only one specific Database table). This makes the directive beneficial outside of Swift code generation, but it doesn't change the behavior of the Identifiable protocol. The [Swift documentation](https://developer.apple.com/documentation/swift/identifiable) states that the scope and duration of an identity is unspecified, so modifying the scope would not lead to a difference in the generated code. | ||
| This could be used for Identifiable conformance if a single field is provided, 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.
If @typePolicy was used could the Identifiable id value be a hash value of all specified key fields? I don't have an answer for not being able to use @typePolicy in operations though.
There was a problem hiding this comment.
If this were possible then maybe Identifiable conformance could just be a codegen configuration option (generateIdentifiableConformance: Bool) and the hashed type policy key values are used for id value?
Not great that there would be two places for configuration but it does highlight that Identifiable is a side-effect of object identity. Maybe that's not a bad requirement, plus it wouldn't need to be used in operations then?
There was a problem hiding this comment.
could the Identifiable id value be a hash value of all specified key fields?
You're right, I forgot that a tuple of Hashable objects is also Hashable itself, which makes it valid for use with Identifiable.
There was a problem hiding this comment.
If this were possible then maybe Identifiable conformance could just be a codegen configuration option (generateIdentifiableConformance: Bool)
When the identity is established as a single field named id, there is no drawback to always adding Identifiable conformance.
If a custom getter for id has to be added for conformance, then following a configuration option seems like a good idea. This would also make it less surprising when Codegen suddenly emits a warning about a getter being unable to be generated because of naming conflicts.
There was a problem hiding this comment.
When the identity is established as a single field named id, there is no drawback to always adding Identifiable conformance.
Even if it is a single field named id I think we'd still prefer configuration over convention. That configuration option can have a default of true if not supplied explicitly = less auto-magic behaviour.
|
|
@AnthonyMDev - it would be a good idea for you to catch up on this conversation. Would appreciate your opinion here as I know you have thoughts on type policy, caching, etc. |
There was a problem hiding this comment.
Thank you so much @x-sheep for putting together this detailed and well-formed RFC. I definitely have a lot of questions and concerns about this approach, but I appreciate the time and thought that has gone into it thus far!
I do think that the @typePolicy alternative from Apollo Kotlin is a better approach. It allows for multiple fields to be used together to create identity. And as I commented inline, I think that it's better to not be able to configure identity on a per-selection-set basis, but leave it to types only.
@typePolicy implementation has been on our Roadmap for quite some time, but other things have taken priority. However, using the @typePolicy to conform to Identifiable in Swift was not on our radar. You've convinced me that this should be part of that work.
|
|
||
| # 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. |
There was a problem hiding this comment.
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.
|
|
||
| 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.
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 @link directive (which is only a concept for one possible future right now), there will likely need to be a way to ensure global uniqueness, but I have a suspicion that it will end up being much more complex than this.
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
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 @identity. Not "all types that implement this interface must have a field with this specific name and type marked @identity.
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.
| ```graphql | ||
| query GetAllAnimals { | ||
| allAnimals { | ||
| id @identity |
There was a problem hiding this comment.
In operation usage, do you not need to provide a scope? If so, then I think there needs to be two different directives defined for FIELD and FIELD_DEFINITION.
There was a problem hiding this comment.
Omitting the parameter defaults to the narrowest scope.
| } | ||
| ``` | ||
|
|
||
| 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.
I'm unclear why the client would ever need to define any scope here. When using @identity in a operation, you aren't going to be using these keys for cache normalization, since other selections sets may not provide an @identity field, or could even provide a different field.
|
|
||
| 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. | ||
|
|
||
| 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.
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.
|
|
||
| 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.
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.
| 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.
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.
| 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. |
There was a problem hiding this comment.
This makes sense, but honestly, we may continue to combine with the __typename even when the scope is SERVICE or GLOBAL, just for consistency sake.
|
Thanks for the feedback, everyone. I think the document as I wrote it is somewhere between "high level directive that should maybe be part of the GraphQL spec itself" and "client-specific details". If this were to become an RFC for GraphQL itself, I would remove the entire portion that allows the directive to be used on queries or fragments and focus only on schema authoring. This would make it a replacement for Apollo Federation's If schemas were not changed, I now think that My current idea is to emit an |
If you look at the wider GraphQL ecosystem these things are already in-play to make them part of 'GraphQL':
I certainly think this is the correct way forward. |
Here is a document for how to add Identifiable conformance to generated models using a new GraphQL directive.
This is a followup to #548 and apollographql/apollo-ios#710 (comment).
I'm currently unsure on which cases should lead to a validation error, and what should be handled silently by the codegen library. I'd love to hear your feedback.