[feature] Allow using an ActiveRecord::Relation as a dependency

[katyho]

Summary

Enable using an ActiveRecord::Relation object as a dependency of a memoized method. Do this by:

• Adding a new case for ActiveRecord::Relation while extracting dependencies in RedisMemo::Dependency.depends_on
• Re-use the logic to extract bind params while executing a query. (Is it safe to also hijack the thread local variables that are shared between activerecord/redismemo in this case?)

Follow up:

• Do we want to be able to specify queries from finder methods? e.g. .find(), .pluck(), which immediately execute the query instead of lazy loading it like ActiveRecord::Relation?

Testing

Added specs

[donaldong]

Do we want to be able to specify queries from finder methods? e.g. .find(), .pluck(), which immediately execute the query instead of lazy loading it like ActiveRecord::Relation?

Nope, this would not make sense IMO, it's the same as depends on an array of fixed values. Though I think we would expect people to call where.to_a / find / pluck to set depends_on dynamically sometimes. Those queries in the depends_on block might be cached elsewhere separately.

[donaldong]

I think this is the right method to use here! we need to thoroughly test the rails versions we support before remove the "beta" tag anyway 😄

Note this method returns a tuple of 3: https://github.com/rails/rails/blob/291a3d2ef29a3842d1156ada7526f4ee60dd2b59/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L42

sql, binds, preparable

[donaldong]

Is it safe to also hijack the thread local variables that are shared between activerecord/redismemo in this case?

I think there might be an issue: this assumes people won't run actual queries (exec_query) in the depends_on which might not be true. If there is an exec_query while extracting dependencies, it might continue to use whatever thread locals we have set since we have not cleared them yet. We can probably just reset everything before calling dependency_of tho

[donaldong]

I think in this dependency_of, we will can still pick up the thread locals from the previous extract_bind_params.

I do like the with_new_query_context idea tho! Perhaps we should wrap dependency_of in that block instead.

[katyho]

Perhaps we should wrap dependency_of in that block instead.

Hm I think an issue is the memoized exec_query dependency block relies on the bind parameters being extracted previously in the overridden method here. I explicitly set the current_query thread local variable, and expect extract_bind_params to set the bind params thread local variable (retrieved in CachedSelect.current_query_bind_params).

I think we can get around this but would have to change the logic in the exec_query dependency block to be able to extract bind params on the fly.

I think in this dependency_of, we will can still pick up the thread locals from the previous extract_bind_params.

Hm not sure I understand this. Unless another db query occurs within the block that with_new_query_context is wrapped around, we should be okay right?

[donaldong]

Oh I guess

Unless another db query occurs within the block that with_new_query_context is wrapped around, we should be okay right?

Oh yeah I guess that's true. We don't call exec_query in exec_query.

The case I was thinking about is to have

memoize_method :site_names do |teacher|
  teacher.sites.each do |site|
    depends_on Site.where(id: site.id)
  end
end

but the inner exaction would not be affected by the outer query anyway.

[donaldong]

Oh I guess

Unless another db query occurs within the block that with_new_query_context is wrapped around, we should be okay right?

Oh yeah I guess that's true. We don't call exec_query in exec_query.

The case I was thinking about is to have

memoize_method :site_names do |teacher|
  teacher.sites.each do |site|
    depends_on Site.where(id: site.id)
  end
end

but the inner exaction would not be affected by the outer query anyway.