Netflix / fast_jsonapi

No Longer Maintained - A lightning fast JSON:API serializer for Ruby Objects.
Apache License 2.0
5.07k stars 425 forks source link

caching with conditional associations #279

Open ChrisHampel opened 6 years ago

ChrisHampel commented 6 years ago

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

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?

shishirmk commented 6 years ago

@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.

ChrisHampel commented 6 years ago

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

everlast240 commented 5 years ago

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.

andrewhaines commented 5 years ago

@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.

everlast240 commented 5 years ago

@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.

benbonnet commented 5 years ago

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

dblommesteijn commented 4 years ago

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