salsify / goldiloader

Just the right amount of Rails eager loading
MIT License
1.61k stars 53 forks source link

Fix #88 - Infinite recursion in `ActiveStorage::Attachment#blob` association #112

Closed jturkel closed 3 years ago

jturkel commented 3 years ago

This PR fixes #88 which caused an infinite recursion when accessing ActiveStorage::Attachment#blob. The infinite loop was caused by the following:

  1. When calling ActiveStorage::Attachment#blob, Goldiloader calls Goldiloader::AutoIncludeContext.register_models with the ActiveStorage::Attachment to setup the automatic eager loading context.
  2. Goldiloader::AutoIncludeContext.register_models calls Array.wrap with the ActiveStorage::Attachment instance so it can handle either a model or an array of models with a unified code path.
  3. Array.wrap checks ActiveStorage::Attachment#respond_to?(:to_ary) to see if it should invoke an array conversion method.
  4. ActiveStorage::Attachment does not implement the to_ary method but it is configured to delegate missing methods to its associated ActiveStorage::Blob which triggers loading the ActiveStorage::Blob association and we're back to step 1.

Fortunately Goldiloader::AutoIncludeContext.register_models is an internal method and we can avoid the Array.wrap call by doing a simple is_a?(Array) check.

The test setup for this is a bit messy since lots of ActiveStorage code is loaded asynchronously during Rails initialization but it's good enough for now. I may look into using something like combustion in the future to simplify test setup.