Merge pull request #1 from DFE-Digital/caching-bugfix

Change how renderer output is converted to JSON

Author: malcolmbaig

.gitignore

@@ -13,6 +13,7 @@
 ## Dummy app temp files:
 /spec/dummy/db
 /spec/dummy/log
+/spec/dummy/tmp
 
 ## Specific to RubyMotion:
 .dat*

jsonapi-rails.gemspec

@@ -23,4 +23,5 @@ Gem::Specification.new do |spec|
   spec.add_development_dependency 'rspec-rails', '~> 3.5'
   spec.add_development_dependency 'with_model',  '~> 2.0'
   spec.add_development_dependency 'simplecov'
+  spec.add_development_dependency 'pry-byebug'
 end

lib/jsonapi/rails/railtie.rb

@@ -54,10 +54,76 @@ def register_renderers
               # Renderer proc is evaluated in the controller context.
               headers['Content-Type'] = Mime[:jsonapi].to_s
 
-              ActiveSupport::Notifications.instrument('render.jsonapi-rails',
-                                                      resources: resources,
-                                                      options: options) do
-                JSON.generate(renderer.render(resources, options, self))
+              ActiveSupport::Notifications.instrument(
+                'render.jsonapi-rails',
+                resources: resources,
+                options: options
+              ) do
+                # Depending on whether or not a valid cache object is present
+                # in the options, the #render call below will return two
+                # slightly different kinds of hash.
+                #
+                # Both hashes have broadly the following structure, where r is
+                # some representation of a JSON::API resource:
+                #
+                # {
+                #   data: [ r1, r2, r3 ],
+                #   meta: { count: 12345 },
+                #   jsonapi: { version: "1.0" }
+                # }
+                #
+                # For non-cached calls to this method, the `data` field in the
+                # return value will contain an array of Ruby hashes.
+                #
+                # For cached calls, the `data` field will contain an array of
+                # JSON strings corresponding to the same data. This happens
+                # because jsonapi-renderer caches both the JSON serialization
+                # step as well as the assembly of the relevant attributes into
+                # a JSON::API-compliant structure. Those JSON strings are
+                # created via calls to `to_json`. They are then wrapped in
+                # CachedResourcesProcessor::JSONString. This defines a
+                # `to_json` method which simply returns self, ie - it attempts
+                # to ensure that any further `to_json` calls result in no
+                # changes.
+                #
+                # That isn't what happens in a Rails context, however. Below,
+                # the last step is to convert the entire output hash of the
+                # renderer into a JSON string to send to the client. If we
+                # call `to_json` on the cached output, the already-made JSON
+                # strings in the `data` field will be converted again,
+                # resulting in malformed data reaching the client. This happens
+                # because the ActiveSupport `to_json` takes precedent, meaning
+                # the "no-op" `to_json` definition on JSONString never gets
+                # executed.
+                #
+                # We can get around this by using JSON.generate instead, which
+                # will use the `to_json` defined on JSONString rather than the
+                # ActiveSupport one.
+                #
+                # However, we can't use JSON.generate on the non-cached output.
+                # Doing so means that its `data` field contents are converted
+                # with a non-ActiveSupport `to_json`. This means cached and
+                # non-cached responses have subtle differences in how their
+                # resources are serialized. For example:
+                #
+                # x = Time.new(2021,1,1)
+                #
+                # x.to_json
+                # => "\"2021-01-01T00:00:00.000+00:00\""
+                #
+                # JSON.generate x
+                # => "\"2021-01-01 00:00:00 +0000\""
+                #
+                # The different outputs mean we need to take different
+                # approaches when converting the entire payload into JSON,
+                # hence the check below.
+                jsonapi_hash = renderer.render(resources, options, self)
+
+                if jsonapi_hash[:data]&.first&.class == JSONAPI::Renderer::CachedResourcesProcessor::JSONString
+                  JSON.generate jsonapi_hash
+                else
+                  jsonapi_hash.to_json
+                end
               end
             end
           end

spec/render_jsonapi_spec.rb

@@ -23,32 +23,67 @@ def index
       def serializer
         Class.new(JSONAPI::Serializable::Resource) do
           type 'users'
-          attribute :name
+          attributes :id, :name, :dob
 
           def jsonapi_cache_key(*)
-            'foo'
+            'cache_key'
           end
         end
       end
 
+      def user
+        OpenStruct.new(id: 1, name: 'Johnny Cache', dob: Time.new(2021,1,1))
+      end
+
       def index
-        user = OpenStruct.new(id: 1, name: 'Lucas')
+        render jsonapi: [user],
+               class: { OpenStruct: serializer }
+      end
 
-        render jsonapi: user,
+      def index_with_caching
+        render jsonapi: [user],
                class: { OpenStruct: serializer },
                cache: Rails.cache
       end
     end
 
-    subject { JSON.parse(response.body) }
+    before do
+      routes.draw do
+        get "index_with_caching" => "anonymous#index_with_caching"
+        get "index" => "anonymous#index"
+      end
+    end
+
+    let(:rendered_json) { JSON.parse(response.body) }
 
     it 'renders a JSON API success document' do
-      get :index
-      expect(Rails.cache.exist?('foo')).to be true
-      get :index
+      get :index_with_caching
 
       expect(response.content_type).to eq('application/vnd.api+json')
-      expect(subject.key?('data')).to be true
+      expect(rendered_json.key?('data')).to be true
+    end
+
+    it 'caches resources' do
+      get :index_with_caching
+
+      expect(Rails.cache.exist?('cache_key')).to be true
+      expect(JSON.parse(Rails.cache.read('cache_key'))).to eq rendered_json['data'].first
+    end
+
+    it 'renders equivalent JSON whether caching or not' do
+      expected_response = {
+        "data"=>[{"id"=>"1", "type"=>"users", "attributes"=>{"id"=>1, "name"=>"Johnny Cache", "dob"=>"2021-01-01T00:00:00.000+00:00"}}],
+        "jsonapi"=>{"version"=>"1.0"}
+      }
+
+      get :index
+      response_with_no_caching = rendered_json.deep_dup
+
+      get :index_with_caching
+      response_with_caching = rendered_json
+
+      expect(response_with_no_caching).to eq expected_response
+      expect(response_with_caching).to eq expected_response
     end
   end
 end