jsonapi-rb / jsonapi-renderer

Efficiently render JSON API documents.
http://jsonapi-rb.org
MIT License
27 stars 11 forks source link

Inconsistence between CachedResourcesProcessor#process_resources vs SimpleResourcesProcessor#process_resources #31

Open samnang opened 6 years ago

samnang commented 6 years ago

When I start implementing caching in my app, I noticed CachedResourcesProcessor#process_resources returns an array of strings, but SimpleResourcesProcessor#process_resources returns an array of hashes.

I assume the api should expect to return an array of hashes, so that when Rails' controllers calls render in jsonapi-rails, it converts that data hash into json correctly, but if it's a string, it will to_json again on string which I don't think we want that behaviour.

Option 1

Change CachedResourcesProcessor#process_resources to return array of hash:

def process_resources
  [@primary, @included].each do |resources|
    cache_hash = cache_key_map(resources)
    processed_resources = @cache.fetch_multi(*cache_hash.keys) do |key|
      res, include, fields = cache_hash[key]
      json = res.as_jsonapi(include: include, fields: fields).to_json

      JSONString.new(json)
    end

    values = processed_resources.values.map { |json| JSON.parse(json) }
    resources.replace(values)
  end
end

Pros: Consistency API. Cons: It's a bit slow if we do with big data because it has to convert json data back and forth.

Option 2

Change jsonapi-rails to not calling to_json again if it's already in string.

::ActionController::Renderers.add(name) do |resources, options|
  # Renderer proc is evaluated in the controller context.
  self.content_type ||= Mime[:jsonapi]

  ActiveSupport::Notifications.instrument('render.jsonapi-rails',
                                           resources: resources,
                                           options: options) do
    data = renderer.render(resources, options, self)
    data.is_a?(String) ? data : data.to_json
  end
end

Pros: It's fast because we don't need to do double works on converting json data. Cons: Inconsistency API between CachedResourcesProcessor#process_resources vs SimpleResourcesProcessor#process_resources.

Edit: Option 2 is not working because renderer.render(resources, options, self) return jsonapi respone data hash not just data attribute.

samnang commented 6 years ago

After I investigate the issue, it seems JSONAPI::Renderer::CachedResourcesProcessor::JSONString#to_json is never run.

(byebug) { data: JSONAPI::Renderer::CachedResourcesProcessor::JSONString.new('{"id": "1"}') }.to_json
"{\"data\":\"{\\\"id\\\": \\\"1\\\"}\"}"

What we expect the return value should be "{\"data\":{\"id\": \"1\"}}"

I am on Rails 5.1.4.

beauby commented 6 years ago

Hi @samnang – you are right. There is a fix for Rails 5 in https://github.com/jsonapi-rb/jsonapi-rails/pull/70, and I'll try to cut new releases of all gems next week. In the meantime, in order to use caching, you should add the following to your Gemfile:

gem 'jsonapi-renderer', github: 'jsonapi-rb/jsonapi-renderer'
gem 'jsonapi-rails', github: 'jsonapi-rb/jsonapi-rails', branch: 'json-caching'
samnang commented 6 years ago

Thank @beauby, you saved my day. I have been tried different options to get it to work, but none of them are good.

samnang commented 6 years ago

@beauby I have few questions that try to understand how the caching work here:

  1. When I render a resource that includes relationships, are nested relationships required to override cache key, or we just only define in top/root resource only?
  2. When I use cache for a top resource and include relationship resources, so if the cache hasn't expired yet, will fetch just a key for top resource and put the entire data from cache store in the response? Or it needs to go each resource in included relationship to fetch one by one?
  3. When I use cacheoption in my index action for rendering collection, does it cache individual resource within collection separately or cache the entire collection?
beauby commented 6 years ago

Hi @samnang

When I render a resource that includes relationships, are nested relationships required to override cache key, or we just only define in top/root resource only?

Not sure I understand, but what is cached is a fragment of JSON representing a resource (along with its relationships' linkage data).

When I use cache for a top resource and include relationship resources, so if the cache hasn't expired yet, will fetch just a key for top resource and put the entire data from cache store in the response? Or it needs to go each resource in included relationship to fetch one by one?

Each resource will be fetched (though using fetch_multi).

When I use cacheoption in my index action for rendering collection, does it cache individual resource within collection separately or cache the entire collection?

It caches individual resources.