fast_jsonapi icon indicating copy to clipboard operation
fast_jsonapi copied to clipboard

caching with conditional associations

Open ChrisHampel opened this issue 6 years ago • 7 comments

if you use caching with condition associations, (possible conditional attributes, I haven't checked), what is cache is the state that conditions are in, when the first request comes in, as that is what is cached.

ie:

class LocationSerializer
  include FastJsonapi::ObjectSerializer

  has_many :owners,serializer: UserSerializer, if: Proc.new {|obj, params | params[:condition]}

  cache_options enabled: true
end
class OwnerSerializer
  include FastJsonapi::ObjectSerializer

  has_many :homes,serialized: LocationSerializer, if: Proc.new {|obj, params | params[:condition2]}

  cache_options enabled: true
end

if you serialize locations of them and tell the gem to include the owners, it will result in a large amount of data being loading in 3+ queries

  • load the locations
  • load the owners
  • load the locations of those owners in order to get their IDs

this could load a lot of data.

as such we would like to eliminate the third query as if a user wants that information they can request through the endpoint starting at users.

this all works fine with caching off, but if you turn it on. the returned hash for any cached object is in the state it was when it was first cached, despite that the condition has changed.

Is this intended behavior?

ChrisHampel avatar Jul 19 '18 16:07 ChrisHampel

@ChrisHampel that definitely seems like a bug. I don't think we have tested caching with all the new features we have added so it would be worth it to test it. Also caching has very little performance benefit in fact I was thinking of taking it out. It doesn't cache the records that are passed in. If the same record is passed it uses the cached json which wont work with all the conditionals that can change with params.

shishirmk avatar Jul 21 '18 02:07 shishirmk

what about, adding the params to the hash key somehow so that when the params change the hash key changes. this could be extended to allow the user to specify what params to take into account, giving the user more control. over cache invalidation

ChrisHampel avatar Jul 21 '18 03:07 ChrisHampel

I'm having the same issue with sparse field sets. What gets stored in the cache is whatever the first request got and that's it. After that no matter what fields and includes you specify you get the same cached result. @ChrisHampel , it does make sense to add params to cache_key, so that we can invalidate the cache.

everlast240 avatar Aug 07 '19 09:08 everlast240

@shishirmk Is this still something that's worth thinking about? You mentioned you were going to remove caching anyway; curious if I should look into this more at all.

andrewhaines avatar Sep 04 '19 18:09 andrewhaines

@andrewhaines , it's worth it IMHO. Personally , I've made a monkey-patch like that in serialization_core.rb - to pass the fieldset (array of symbols, which we get as param to the record_hash method anyways (and I merge params, just in case I need it in the future. In essence the patch is:

67c11
<           record_hash = Rails.cache.fetch(record.cache_key, expires_in: cache_length, race_condition_ttl: race_condition_ttl) do
---
>           record_hash = Rails.cache.fetch(record.cache_key(params.merge(fs: fieldset)), expires_in: cache_length, race_condition_ttl: race_condition_ttl) do

With this, I can implement the cache_key method on my model to get params and make it flexible. E.g.:

    def cache_key(params = {})
      updated_at_ts = (updated_at.utc || 0).to_i

      "#{I18n.locale}-model_name-#{id}-#{updated_at_ts}-#{params.to_query}"
    end

PS: I would remove the Rails part and make the cache configuration flexible & configurable. This gem has the chance of becoming the 1st choice for everyone who doesn't want to use the full RoR stack and this single constant reference is... unnecessary IMHO.

everlast240 avatar Sep 05 '19 13:09 everlast240

we have an app that returns records based on the current_user requesting; it would make quite a lot of sense for us to cache based on this given user

looking at the method : https://github.com/Netflix/fast_jsonapi/blob/master/lib/fast_jsonapi/serialization_core.rb#L65

params is clearly available and could be easily used

@shishirmk would that makes to extend the current gem's state ? I might not see all the downsides to change it

benbonnet avatar Oct 21 '19 19:10 benbonnet

I've monkey patched record_hash myself, so record.cache_key is called with fieldset props as @everlast240 suggested 👍

Edit: I'm using a different fork for this gem as this one apparently is 'not maintained anymore', however it also does not contain a fix for this issue: https://github.com/fast-jsonapi/fast_jsonapi

dblommesteijn avatar Jan 14 '20 08:01 dblommesteijn