Skip to content
This repository was archived by the owner on Oct 26, 2022. It is now read-only.

Fix Connection Cache for ActiveRecord #68

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ssnickolay
Copy link

Problem

Connection cache does not work with ActiveRecord.

Cause (for defects/bugs)

For ActiveRecord this returns GraphQL::Relay::RelationConnection where nodes is instance of ActiveRecord_Associations_CollectionProxy. This is because ActiveRecord is lazy by design and when we call field.resolve_proc.call(obj, args, ctx) there is not a single SQL query in the database. SQL query is executed when we call to_a on RelationConnection level.
So the problem is that currently we put into the cache not loaded ActiveRecord_Associations_CollectionProxy and whenever we have cache hit we still have an SQL query:

DEBUG -- : Cache hit: (gql:::Playground:roles:Playground:playgrounds/726-20191216110227000000)
Role Load (3.6ms)  SELECT `roles`.* FROM `roles` WHERE `roles`.`playground_id` = 726 LIMIT 50

DEBUG -- : Cache hit: (gql:::Playground:roles:Playground:playgrounds/726-20191216110227000000)
Role Load (3.6ms)  SELECT `roles`.* FROM `roles` WHERE `roles`.`playground_id` = 726 LIMIT 50

...

Solution

  1. Add ActiveRecord test suite and reproduce the bug.
  2. Handle RelationConnection separately and cache paged_nodes when they are loaded.

Notes

This is still not an ideal solution because we cache only paged_nodes, but RelationConnection has other methods (e.g. sliced_nodes_count). But we don't know which exactly will be executed and I don't see how we can handle it without extra cache write.

@ssnickolay ssnickolay force-pushed the fix/relay-connection-cache branch from d948a42 to dfc6701 Compare December 20, 2019 11:28
@ssnickolay ssnickolay changed the title Fix/relay connection cache Fix Connection Cache for ActiveRecord Dec 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants