From a625749f9b31b5c879f6e3ff2a9b0cc0d8dc27e4 Mon Sep 17 00:00:00 2001 From: Alex Oliveira Date: Sat, 24 Oct 2026 21:10:47 -0300 Subject: [PATCH 1/2] Order included linkage rows by source key When a resource is included through a has_many, its inverse linkage (for example the book's book_comments when comments include their book) is built from a SELECT DISTINCT ordered only by the related key. Every row shares that key, so the linkage order depends on the query plan. On Postgres the plan switches from Sort/Unique to HashAggregate once autovacuum analyzes the freshly created test tables, and the cache warmup request (which adds updated_at to the DISTINCT) then returns a different order than the normal request. This made test_pagination_related_resources_data_includes fail at random in Postgres CI jobs, and gave API clients an unstable linkage order. This change adds the source primary key as a final ORDER BY in find_related_monomorphic_fragments. The column is already in the select list as source_id, so it is valid with DISTINCT. It also adds a test that analyzes the tables first, which makes the failure reproduce on every run. find_related_polymorphic_fragments builds a similar query and may need the same tiebreaker. test_sort_included_attribute has a separate flake: it sorts on a nullable column and asserts which of several NULL rows comes first. --- lib/jsonapi/active_relation_resource.rb | 3 +++ test/integration/requests/request_test.rb | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/lib/jsonapi/active_relation_resource.rb b/lib/jsonapi/active_relation_resource.rb index 581ed1e02..c963557ad 100644 --- a/lib/jsonapi/active_relation_resource.rb +++ b/lib/jsonapi/active_relation_resource.rb @@ -422,6 +422,9 @@ def find_related_monomorphic_fragments(source_fragments, relationship, options, join_manager: join_manager, options: options) + # Break ties on the source key so the inverse linkage order doesn't depend on the query plan + records = records.order(Arel.sql("#{_table_name}.#{_primary_key} asc")) + resource_table_alias = join_manager.join_details_by_relationship(relationship)[:alias] pluck_fields = [ diff --git a/test/integration/requests/request_test.rb b/test/integration/requests/request_test.rb index b7895608c..5742843cf 100644 --- a/test/integration/requests/request_test.rb +++ b/test/integration/requests/request_test.rb @@ -884,6 +884,19 @@ def test_pagination_related_resources_data_includes assert_equal 'This is comment 18 on book 1.', json_response['data'][9]['attributes']['body'] end + def test_included_resource_inverse_linkage_order_with_table_statistics + # Once tables have statistics (autovacuum analyzes them mid-suite in CI), Postgres can + # plan SELECT DISTINCT as a HashAggregate, which scrambles rows the query leaves unordered. + ActiveRecord::Base.connection.execute('ANALYZE books') + ActiveRecord::Base.connection.execute('ANALYZE book_comments') + Api::V2::BookResource.paginator :offset + Api::V2::BookCommentResource.paginator :offset + + assert_cacheable_jsonapi_get '/api/v2/books/1/book_comments?page[limit]=10&include=book' + linkage_ids = json_response['included'].first['relationships']['book_comments']['data'].map { |r| r['id'] } + assert_equal linkage_ids.sort_by(&:to_i), linkage_ids + end + def test_pagination_empty_results Api::V2::BookResource.paginator :offset Api::V2::BookCommentResource.paginator :offset From d709508065b8c93568929843e8d997b49926f4f6 Mon Sep 17 00:00:00 2001 From: Alex Oliveira Date: Sat, 24 Oct 2026 21:37:00 -0300 Subject: [PATCH 2/2] Compare to-many linkage as unordered in cache tests The cacheable GET assertions compared the cached and uncached responses as whole strings, so they also required the same order inside to-many linkage arrays. JSON:API gives that order no meaning, and the library does not guarantee it: on Postgres the plan for the included linkage query changes once autovacuum analyzes the test tables, and the two requests then return the linkage in different orders. The previous commit made the order fixed with an extra ORDER BY, which puts a production cost on every included has_many only to satisfy a test expectation stricter than the spec. This change reverts that ORDER BY. It adds normalize_linkage_order to the functional helpers, which sorts relationship data arrays in data and included, and both cacheable assertions compare normalized responses. Primary data order is untouched, since the sort parameter controls it. The regression test now checks which comment ids are in the linkage, not their order. The included array has no defined order either, and could vary the same way if a future plan change reorders it. It is left strict until a test shows that happening. --- lib/jsonapi/active_relation_resource.rb | 3 -- test/helpers/functional_helpers.rb | 18 ++++++++++ test/helpers/functional_helpers_test.rb | 43 +++++++++++++++++++++++ test/integration/requests/request_test.rb | 8 ++--- test/test_helper.rb | 12 +++---- 5 files changed, 71 insertions(+), 13 deletions(-) create mode 100644 test/helpers/functional_helpers_test.rb diff --git a/lib/jsonapi/active_relation_resource.rb b/lib/jsonapi/active_relation_resource.rb index c963557ad..581ed1e02 100644 --- a/lib/jsonapi/active_relation_resource.rb +++ b/lib/jsonapi/active_relation_resource.rb @@ -422,9 +422,6 @@ def find_related_monomorphic_fragments(source_fragments, relationship, options, join_manager: join_manager, options: options) - # Break ties on the source key so the inverse linkage order doesn't depend on the query plan - records = records.order(Arel.sql("#{_table_name}.#{_primary_key} asc")) - resource_table_alias = join_manager.join_details_by_relationship(relationship)[:alias] pluck_fields = [ diff --git a/test/helpers/functional_helpers.rb b/test/helpers/functional_helpers.rb index 3d6dc9d34..d8df4e203 100644 --- a/test/helpers/functional_helpers.rb +++ b/test/helpers/functional_helpers.rb @@ -55,5 +55,23 @@ module FunctionalHelpers def json_response JSON.parse(@response.body) end + + # JSON:API gives no meaning to the order of to-many linkage, so sort it before comparing responses + def normalize_linkage_order(json) + return json unless json.is_a?(Hash) + + json.deep_dup.tap do |normalized| + resources = Array.wrap(normalized['data']) + Array.wrap(normalized['included']) + resources.each { |resource| sort_to_many_linkage(resource) } + end + end + + private + + def sort_to_many_linkage(resource) + (resource['relationships'] || {}).each_value do |relationship| + relationship['data'].sort_by! { |r| [r['type'], r['id']] } if relationship['data'].is_a?(Array) + end + end end end \ No newline at end of file diff --git a/test/helpers/functional_helpers_test.rb b/test/helpers/functional_helpers_test.rb new file mode 100644 index 000000000..f892d6863 --- /dev/null +++ b/test/helpers/functional_helpers_test.rb @@ -0,0 +1,43 @@ +require File.expand_path('../../test_helper', __FILE__) + +class FunctionalHelpersTest < ActionController::TestCase + + def test_normalize_linkage_order_sorts_to_many_linkage_in_data_and_included + json = { + 'data' => [ + { 'type' => 'books', 'id' => '1', 'relationships' => { + 'book_comments' => { 'data' => [{ 'type' => 'book_comments', 'id' => '9' }, + { 'type' => 'book_comments', 'id' => '2' }] } } } + ], + 'included' => [ + { 'type' => 'people', 'id' => '3', 'relationships' => { + 'comments' => { 'data' => [{ 'type' => 'comments', 'id' => '5' }, + { 'type' => 'comments', 'id' => '4' }] } } } + ] + } + + normalized = normalize_linkage_order(json) + + assert_equal %w[2 9], normalized['data'][0]['relationships']['book_comments']['data'].map { |r| r['id'] } + assert_equal %w[4 5], normalized['included'][0]['relationships']['comments']['data'].map { |r| r['id'] } + end + + def test_normalize_linkage_order_leaves_everything_else_unchanged + assert_nil normalize_linkage_order(nil) + + json = { + 'data' => [ + { 'type' => 'books', 'id' => '2', 'relationships' => { + 'author' => { 'data' => { 'type' => 'people', 'id' => '7' } }, + 'tags' => { 'links' => { 'related' => '/books/2/tags' } } } }, + { 'type' => 'books', 'id' => '1' } + ] + } + + normalized = normalize_linkage_order(json) + + assert_equal %w[2 1], normalized['data'].map { |r| r['id'] } + assert_equal({ 'type' => 'people', 'id' => '7' }, normalized['data'][0]['relationships']['author']['data']) + assert_equal({ 'links' => { 'related' => '/books/2/tags' } }, normalized['data'][0]['relationships']['tags']) + end +end diff --git a/test/integration/requests/request_test.rb b/test/integration/requests/request_test.rb index 5742843cf..bae5b9eaa 100644 --- a/test/integration/requests/request_test.rb +++ b/test/integration/requests/request_test.rb @@ -884,9 +884,9 @@ def test_pagination_related_resources_data_includes assert_equal 'This is comment 18 on book 1.', json_response['data'][9]['attributes']['body'] end - def test_included_resource_inverse_linkage_order_with_table_statistics - # Once tables have statistics (autovacuum analyzes them mid-suite in CI), Postgres can - # plan SELECT DISTINCT as a HashAggregate, which scrambles rows the query leaves unordered. + def test_included_resource_inverse_linkage_with_table_statistics + # Once tables have statistics (autovacuum analyzes them mid-suite in CI), Postgres can plan + # SELECT DISTINCT as a HashAggregate, so the cached and uncached linkage come back in different orders. ActiveRecord::Base.connection.execute('ANALYZE books') ActiveRecord::Base.connection.execute('ANALYZE book_comments') Api::V2::BookResource.paginator :offset @@ -894,7 +894,7 @@ def test_included_resource_inverse_linkage_order_with_table_statistics assert_cacheable_jsonapi_get '/api/v2/books/1/book_comments?page[limit]=10&include=book' linkage_ids = json_response['included'].first['relationships']['book_comments']['data'].map { |r| r['id'] } - assert_equal linkage_ids.sort_by(&:to_i), linkage_ids + assert_equal %w[51 53 55 57 59 61 63 65 67 69], linkage_ids.sort_by(&:to_i) end def test_pagination_empty_results diff --git a/test/test_helper.rb b/test/test_helper.rb index a02fff9e7..6108362a6 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -505,8 +505,8 @@ def assert_cacheable_jsonapi_get(url, cached_classes = :all) end assert_equal( - non_caching_response.pretty_inspect, - json_response.pretty_inspect, + normalize_linkage_order(non_caching_response).pretty_inspect, + normalize_linkage_order(json_response).pretty_inspect, "Cache warmup response must match normal response" ) @@ -515,8 +515,8 @@ def assert_cacheable_jsonapi_get(url, cached_classes = :all) end assert_equal( - non_caching_response.pretty_inspect, - json_response.pretty_inspect, + normalize_linkage_order(non_caching_response).pretty_inspect, + normalize_linkage_order(json_response).pretty_inspect, "Cached response must match normal response" ) assert_equal 0, cached[:total][:misses], "Cached response must not cause any cache misses" @@ -584,8 +584,8 @@ def assert_cacheable_get(action, **args) "Cache (mode: #{mode}) #{phase} response status must match normal response" ) assert_equal( - non_caching_response.pretty_inspect, - json_response_sans_all_backtraces.pretty_inspect, + normalize_linkage_order(non_caching_response).pretty_inspect, + normalize_linkage_order(json_response_sans_all_backtraces).pretty_inspect, "Cache (mode: #{mode}) #{phase} response body must match normal response" ) assert_operator(