Fix for has_one relationships where related record does not exist

lib/jsonapi/resource.rb

@@ -1213,16 +1213,20 @@ def preload_included_fragments(resources, records, serializer, options)
               .map{|row| row.last(2) }
               .reject{|row| target_resources[klass.name].has_key?(row.first) }
               .uniq
-            target_resources[klass.name].merge! CachedResourceFragment.fetch_fragments(
-              klass, serializer, context, sub_cache_ids
-            )
+            unless sub_cache_ids.nil? || sub_cache_ids.empty?
+              target_resources[klass.name].merge! CachedResourceFragment.fetch_fragments(
+                klass, serializer, context, sub_cache_ids
+              )
+            end
           else
             sub_res_ids = id_rows
               .map(&:last)
               .reject{|id| target_resources[klass.name].has_key?(id) }
               .uniq
-            found = klass.find({klass._primary_key => sub_res_ids}, context: options[:context])
-            target_resources[klass.name].merge! found.map{|r| [r.id, r] }.to_h
+            unless sub_res_ids.nil? || sub_res_ids.empty?
+              found = klass.find({klass._primary_key => sub_res_ids}, context: options[:context])
+              target_resources[klass.name].merge! found.map{|r| [r.id, r] }.to_h
+            end
           end
 
           id_rows.each do |row|

lib/jsonapi/resource_serializer.rb

@@ -343,6 +343,7 @@ def cached_relationships_hash(source, include_directives)
             relation_resources = [real_res.public_send(rel_name)].flatten(1).compact
             fragments = relation_resources.map{|r| [r.id, r]}.to_h
           end
+
           fragments.each do |id, f|
             add_resource(f, ia)
 

test/controllers/controller_test.rb

@@ -254,6 +254,22 @@ def test_index_filter_not_allowed
     JSONAPI.configuration.allow_filter = true
   end
 
+  def test_include_belongs_to_exists
+    assert_cacheable_get :index, params: {filter: {id: '2'}, include: 'section'}
+    assert_response :success
+    assert_equal 1, json_response['data'].size
+    assert_equal '2', json_response['data'][0]['relationships']['section']['data']['id']
+    assert_equal 1, json_response['included'].size
+  end
+
+  def test_include_belongs_to_nonexistent
+    assert_cacheable_get :index, params: {filter: {id: '1'}, include: 'section'}
+    assert_response :success
+    assert_equal 1, json_response['data'].size
+    assert_nil json_response['data'][0]['relationships']['section']['data']
+    assert_nil json_response['included']
+  end
+
   def test_index_include_one_level_query_count
     assert_query_count(2) do
       assert_cacheable_get :index, params: {include: 'author'}
@@ -3564,6 +3580,47 @@ def test_immutable_update_not_supported
   end
 end
 
+class AuthorsControllerTest < ActionController::TestCase
+  def setup
+    JSONAPI.configuration.json_key_format = :dasherized_key
+  end
+
+  def test_include_has_one_exists
+    assert_cacheable_get :index, params: {filter: {id: '2'}, include: 'author-detail'}
+    assert_response :success
+    assert_equal 1, json_response['data'].size
+    assert_equal '2', json_response['data'][0]['relationships']['author-detail']['data']['id']
+    assert_equal 1, json_response['included'].size
+  end
+
+  def test_include_has_one_nonexistent
+    assert_cacheable_get :index, params: {filter: {id: '3'}, include: 'author-detail'}
+    assert_response :success
+    assert_equal 1, json_response['data'].size
+    assert_nil json_response['data'][0]['relationships']['author-detail']['data']
+    assert_nil json_response['included']
+  end
+end
+
+class AuthorDetailsControllerTest < ActionController::TestCase
+  def test_author_details_index_include_has_one_does_exist
+    assert_cacheable_get :index, params: {filter: {id: '1'}, include: 'author'}
+    assert_response :success
+    assert_equal 1, json_response['data'].size
+    assert_equal '1', json_response['data'][0]['relationships']['author']['data']['id']
+    assert_equal 1, json_response['included'].size
+  end
+
+  def test_author_details_index_include_has_one_does_not_exist
+    assert_cacheable_get :index, params: {filter: {id: '3'}, include: 'author'}, test_options: {allowed_extra_queries: 1}
+    assert_response :success
+    assert_equal 1, json_response['data'].size
+    assert_nil json_response['data'][0]['relationships']['author']['data']
+    assert_nil json_response['included']
+  end
+
+end
+
 class Api::V7::ClientsControllerTest < ActionController::TestCase
   def test_get_namespaced_model_not_matching_resource_using_model_hint
     assert_cacheable_get :index

test/fixtures/active_record.rb

@@ -22,6 +22,7 @@
   create_table :author_details, force: true do |t|
     t.integer :person_id
     t.string  :author_stuff
+    t.timestamps null: false
   end
 
   create_table :posts, force: true do |t|
@@ -658,6 +659,9 @@ class Customer < Customer
 class AuthorsController < JSONAPI::ResourceControllerMetal
 end
 
+class AuthorDetailsController < JSONAPI::ResourceControllerMetal
+end
+
 class PeopleController < JSONAPI::ResourceController
 end
 
@@ -1299,6 +1303,7 @@ class AuthorResource < JSONAPI::Resource
   model_name 'Person'
   attributes :name
 
+  has_one :author_detail, :foreign_key_on => :related
   has_many :books, inverse_relationship: :authors
 end
 
@@ -1314,6 +1319,7 @@ def title
 
 class AuthorDetailResource < JSONAPI::Resource
   attributes :author_stuff
+  has_one :author, foreign_key: 'person_id', class_name: 'Person'
 end
 
 class SimpleCustomLinkResource < JSONAPI::Resource

test/fixtures/author_details.yml

@@ -6,4 +6,8 @@ a:
 b:
   id: 2
   person_id: 2
-  author_stuff: blah blah blah
+  author_stuff: blah blah blah
+
+c:
+  id: 3
+  author_stuff: orphaned author_detail

test/test_helper.rb

@@ -197,8 +197,8 @@ def assert_query_count(expected, msg = nil, &block)
   @queries = nil
 end
 
-def show_queries
-  @queries.each_with_index do |query, index|
+def show_queries(queries = @queries)
+  queries.each_with_index do |query, index|
     puts "sql[#{index}]: #{query}"
   end
 end
@@ -254,6 +254,7 @@ class CatResource < JSONAPI::Resource
 
   jsonapi_resources :books
   jsonapi_resources :authors
+  jsonapi_resources :author_details
 
   jsonapi_resources :questions
   jsonapi_resources :answers
@@ -484,6 +485,9 @@ class ActionController::TestCase
   def assert_cacheable_get(action, *args)
     assert_nil JSONAPI.configuration.resource_cache
 
+    test_options = args[0] && args[0][:test_options]
+    allowed_extra_queries = test_options.nil? ? 0 : test_options.fetch(:allowed_extra_queries, 0)
+
     normal_queries = []
     normal_query_callback = lambda {|_, _, _, _, payload| normal_queries.push payload[:sql] }
     ActiveSupport::Notifications.subscribed(normal_query_callback, 'sql.active_record') do
@@ -515,7 +519,9 @@ def assert_cacheable_get(action, *args)
       [:warmup, :lookup].each do |phase|
         begin
           cache_queries = []
-          cache_query_callback = lambda {|_, _, _, _, payload| cache_queries.push payload[:sql] }
+          cache_query_callback = lambda {|_, _, _, _, payload|
+            cache_queries.push payload[:sql]
+          }
           cache_activity[phase] = with_resource_caching(cache, cached_resources) do
             ActiveSupport::Notifications.subscribed(cache_query_callback, 'sql.active_record') do
               @controller = nil
@@ -532,6 +538,7 @@ def assert_cacheable_get(action, *args)
         if response.status != non_caching_status
           pp json_response rescue nil
         end
+
         assert_equal(
           non_caching_status,
           response.status,
@@ -542,10 +549,16 @@ def assert_cacheable_get(action, *args)
           json_response_sans_backtraces.pretty_inspect,
           "Cache (mode: #{mode}) #{phase} response body must match normal response"
         )
+
+        # ToDo: Determine if there is a way to return to allowing normal_queries.size*2
+        # instead of normal_queries.size*2 +allowed_extra_queries
+        # +allowed_extra_queries is to account for the has_one where the foreign key is on self and is nil
+        # in the not cached version this is only one query, however in the cached version
+        # cached_resources_for handles this all in SQL and isn't skipping the query
         assert_operator(
           cache_queries.size,
           :<=,
-          normal_queries.size*2, # Allow up to double the number of queries as the uncached action
+          normal_queries.size*2 + allowed_extra_queries, # Allow up to double the number of queries + 1 as the uncached action
           "Cache (mode: #{mode}) #{phase} action made too many queries:\n#{cache_queries.pretty_inspect}"
         )
       end