fix(response caching): respect interface objects as entities#8582
fix(response caching): respect interface objects as entities#8582aaronArinder merged 6 commits intodevfrom
Conversation
This comment has been minimized.
This comment has been minimized.
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 10 changed, 0 removedBuild ID: 4ea92bf93ba65e6e189352a9 URL: https://www.apollographql.com/docs/deploy-preview/4ea92bf93ba65e6e189352a9 |
| } | ||
| }) | ||
| }); | ||
| // supports both Object types and Interface types (for interface objects with isInterfaceObject: true) |
There was a problem hiding this comment.
Where do you check that isInterfaceObject is set to true ? Do you assume it's checked at composition ?
There was a problem hiding this comment.
I'm assuming in composition; the second test below, where we have a concrete object and a normal interface (ie, not an interface object) shows that (I think, at least insofar as I understand how parsing/validating works, which is very little), where the first test (with just an interface object and no concrete object) gets us back the __typename we'd expect (in that case, Item)
There was a problem hiding this comment.
Cc @duckki I don't have enough knowledge on this maybe you could help me
There was a problem hiding this comment.
Let me double check the status of validation rules.
Update: We allow @cacheTag on interface objects in composition. But, I need to check how it's represented in supergraph schemas. Unfortunately, I'm away from office today. I'll think more about this when I have a chance.
There was a problem hiding this comment.
I reviewed the supergraph composition of cache-tag directives applied on interface object entity types.
- In supergraph, those directives are translated using
@directivedirective applied on the interface type, instead of its implementation types. - Since we don't normally allow
@cacheTagon interface, such directive applications only happen with interface object in specified subgraphs.
Thus, in order to check if a type has @cacheTag, we must
- Check the type (interface or object) has one or more
@cacheTagdirectives applied on it. - (For object types) check all of its interface types if one or more
@cacheTagdirectives applied on them.
There was a problem hiding this comment.
I thought @cacheTag is per graph? so in a graph that there is an @interfaceObject we won't have any of its implementations - hence why do we need to look up the interface types?
edit: nvm, found it -> filtering per graph is done afterwards
Co-authored-by: Renée <renee.kooi@apollographql.com>
There was a problem hiding this comment.
It looked good.
I pushed a commit handling another case.
Cache tags can be on an interface object or regular objects. Also, we should consider the combinations of the source subgraph and the target subgraph when we jump from one to another.
- From non-interface object to non-interface object: Originally handled
- From interface object to interface object: Handled in the previous commit of this PR
- From non-interface object to interface object:Handled in my new commit pushed to this PR
- From interface object to non-interface object: Not handled (= No cache keys since it can't be determined)
I also added two more tests demonstrating those cases.
| } | ||
| }) | ||
| }); | ||
| // supports both Object types and Interface types (for interface objects with isInterfaceObject: true) |
There was a problem hiding this comment.
I reviewed the supergraph composition of cache-tag directives applied on interface object entity types.
- In supergraph, those directives are translated using
@directivedirective applied on the interface type, instead of its implementation types. - Since we don't normally allow
@cacheTagon interface, such directive applications only happen with interface object in specified subgraphs.
Thus, in order to check if a type has @cacheTag, we must
- Check the type (interface or object) has one or more
@cacheTagdirectives applied on it. - (For object types) check all of its interface types if one or more
@cacheTagdirectives applied on them.
ea07517 to
a7b0d4a
Compare
a7b0d4a to
fa9d801
Compare
|
Over the weekend, I realized it could be improved:
|
|
My attempt to improve it didn't really work. Let me explain:
I wanted to derive the interface type name from the query plan, but it's not readily available (we would have to parse the fetch query to get that). However, I think the situation is not too bad. Even if we enumerate all interfaces, the result will be at most one anyways. That's because we can't really have multiple interface objects covering the same object type. Instead, I just moved the directive filtering before collecting them for efficiency.
I misspoke. Actually, object types CAN have different sets of cache tags. So, in this case, we can't determine cache tag after all. 😢 This is a fundamental issue with interface objects. I just added some more comments in the code explaining it. I'll leave end-to-end testing to you. Otherwise, this PR looks good. |
|
ack on e2e testing, @duckki; I'll probably not get to it until next week, but have it on my list of todos thank you so much for running with this pr and fleshing it out into something useable! |
for cache invalidation keys, we only supported objects and not interface entities
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩