From 4eac76cccb2d2428501714d611d63f5499580114 Mon Sep 17 00:00:00 2001 From: Larry Gebhardt Date: Thu, 16 Feb 2017 17:24:29 -0500 Subject: [PATCH] Fix for has_one relationships where related record does not exist --- lib/jsonapi/resource.rb | 14 ++++--- lib/jsonapi/resource_serializer.rb | 1 + test/controllers/controller_test.rb | 57 +++++++++++++++++++++++++++++ test/fixtures/active_record.rb | 6 +++ test/fixtures/author_details.yml | 6 ++- test/test_helper.rb | 21 +++++++++-- 6 files changed, 95 insertions(+), 10 deletions(-) diff --git a/lib/jsonapi/resource.rb b/lib/jsonapi/resource.rb index efaae4635..a4faf276d 100644 --- a/lib/jsonapi/resource.rb +++ b/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| diff --git a/lib/jsonapi/resource_serializer.rb b/lib/jsonapi/resource_serializer.rb index 62a23c1d7..b39ba5e13 100644 --- a/lib/jsonapi/resource_serializer.rb +++ b/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) diff --git a/test/controllers/controller_test.rb b/test/controllers/controller_test.rb index 7bdf19c2d..f32de2771 100644 --- a/test/controllers/controller_test.rb +++ b/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 diff --git a/test/fixtures/active_record.rb b/test/fixtures/active_record.rb index b5c11d72d..a3a65be46 100644 --- a/test/fixtures/active_record.rb +++ b/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 diff --git a/test/fixtures/author_details.yml b/test/fixtures/author_details.yml index 5711265c8..22ec802fe 100644 --- a/test/fixtures/author_details.yml +++ b/test/fixtures/author_details.yml @@ -6,4 +6,8 @@ a: b: id: 2 person_id: 2 - author_stuff: blah blah blah \ No newline at end of file + author_stuff: blah blah blah + +c: + id: 3 + author_stuff: orphaned author_detail \ No newline at end of file diff --git a/test/test_helper.rb b/test/test_helper.rb index 51ba0459d..bfd5a9291 100644 --- a/test/test_helper.rb +++ b/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