Disallow reading entire StoreObject from EntityStore by ID. #5828
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Because we were computing a fresh fragment query document each time readFragment was called, readFragment was effectively never benefitting from result caching.
One of the most important internal changes in Apollo Client 3.0 is that the result caching system now tracks its dependencies at the field level, rather than at the level of whole entity objects: #5617 As proof that this transformation is complete, we can finally remove the ability to read entire entity objects from the EntityStore by ID, so that the only way to read from the store is by providing both an ID and the name of a field. The logic I improved in #5772 is now even simpler, without the need for overloading multiple EntityStore#get signatures. In the process, I noticed that the EntityStore#has(ID) method was accidentally depending on any changes to the contents of the identified entity, even though store.has only cares about the existence of the ID in the store. This was only a problem if we called store.has during a larger read operation, which currently only happens when InMemoryCache#read is called with options.rootId. The symptoms of this oversight went unnoticed because the caching of readFragment results was just a little more sensitive to changes to the given entity. The results themselves were still logically correct, but their caching was not as effective as it could be. That's the beauty and the curse of caching: you might not notice when it isn't working, because all your tests still pass, and nobody complains that their application is broken. Fortunately, we can model this dependency with an ID plus a fake field name called "__exists", which is only dirtied when the ID is newly added to or removed from the store.
benjamn
changed the title
hwillson
approved these changes
Jan 23, 2020
2 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One of the most important internal changes in Apollo Client 3.0 is that the result caching system (introduced in #3394) now tracks its dependencies at the field level, rather than at the level of whole entity objects, thanks to #5617.
As proof that this transformation is complete, we can finally remove the ability to read entire entity objects from the
EntityStoreby ID, so that the only way to read from the store is by providing both an ID and the name of a field. The logic I improved in #5772 is now even simpler, without needing multiple overloadedEntityStore#getsignatures.In the process, I noticed that the
EntityStore#has(ID)method was accidentally depending on any changes to the contents of the identified entity, even thoughstore.hasonly "cares" about the existence of the ID in the store. This was a problem only if we calledstore.hasduring a larger read operation, which currently only happens duringreadFragment, which callsInMemoryCache#readwithoptions.rootId.The symptoms of this oversight went unnoticed for two reasons:
ApolloCache#readFragmentmethod was computing a fresh fragment query document every time it was called, even if it had seen the given fragment document before (see f039d40). In other words, fragments were not benefitting from result caching at all, which masked the problem of result caching being too sensitive for fragments.readFragmentresults was still unnecessarily sensitive to changes to unrelated fields within the given entity, due to the problem withstore.has. The results themselves were still logically correct, but their caching was not as effective as it could be. Fortunately, I was able to model this dependency with an ID plus a fake field name called__exists, which is only dirtied when the ID is newly added to (or removed from) the store.That's the beauty and the curse of caching: you might not notice when it isn't working, because all your tests still pass, and nobody complains that their application is broken. Now we have more aggressive tests of fragment result caching behavior, so this kind of oversight won't happen again.