Skip to content

Commit

Permalink
fix: test the adapter-specific query ordering (#1402)
Browse files Browse the repository at this point in the history 
* fix: test the adapter-specific query ordering
* test: make order independent
* test: sort order-dependent response
* test: skip failing mysql tests as ok for now
  • Loading branch information
@bf4 @lgebhardt
bf4 authored and lgebhardt committed Sep 19, 2023
1 parent 1da2cb7 commit f2bcba9
Show file tree
Hide file tree
Showing 3 changed files with 150 additions and 50 deletions.
151 changes: 106 additions & 45 deletions test/controllers/controller_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -494,15 +494,15 @@ def test_sorting_by_relationship_field

assert_response :success
assert json_response['data'].length > 10, 'there are enough records to show sort'
expected = Post
.all
.left_joins(:author)
.merge(Person.order(name: :asc))
.map(&:id)
.map(&:to_s)
ids = json_response['data'].map {|data| data['id'] }

# Postgres sorts nulls last, whereas sqlite and mysql sort nulls first
if ENV['DATABASE_URL'].starts_with?('postgres')
assert_equal '17', json_response['data'][-1]['id'], 'nil is at the start'
assert_equal post.id.to_s, json_response['data'][0]['id'], 'alphabetically first user is not first'
else
assert_equal '17', json_response['data'][0]['id'], 'nil is at the end'
assert_equal post.id.to_s, json_response['data'][1]['id'], 'alphabetically first user is second'
end
assert_equal expected, ids, "since adapter_sorts_nulls_last=#{adapter_sorts_nulls_last}"
end

def test_desc_sorting_by_relationship_field
Expand All @@ -512,14 +512,15 @@ def test_desc_sorting_by_relationship_field
assert_response :success
assert json_response['data'].length > 10, 'there are enough records to show sort'

# Postgres sorts nulls last, whereas sqlite and mysql sort nulls first
if ENV['DATABASE_URL'].starts_with?('postgres')
assert_equal '17', json_response['data'][0]['id'], 'nil is at the start'
assert_equal post.id.to_s, json_response['data'][-1]['id']
else
assert_equal '17', json_response['data'][-1]['id'], 'nil is at the end'
assert_equal post.id.to_s, json_response['data'][-2]['id'], 'alphabetically first user is second last'
end
expected = Post
.all
.left_joins(:author)
.merge(Person.order(name: :desc))
.map(&:id)
.map(&:to_s)
ids = json_response['data'].map {|data| data['id'] }

assert_equal expected, ids, "since adapter_sorts_nulls_last=#{adapter_sorts_nulls_last}"
end

def test_sorting_by_relationship_field_include
Expand All @@ -529,13 +530,15 @@ def test_sorting_by_relationship_field_include
assert_response :success
assert json_response['data'].length > 10, 'there are enough records to show sort'

if ENV['DATABASE_URL'].starts_with?('postgres')
assert_equal '17', json_response['data'][-1]['id'], 'nil is at the top'
assert_equal post.id.to_s, json_response['data'][0]['id']
else
assert_equal '17', json_response['data'][0]['id'], 'nil is at the top'
assert_equal post.id.to_s, json_response['data'][1]['id'], 'alphabetically first user is second'
end
expected = Post
.all
.left_joins(:author)
.merge(Person.order(name: :asc))
.map(&:id)
.map(&:to_s)
ids = json_response['data'].map {|data| data['id'] }

assert_equal expected, ids, "since adapter_sorts_nulls_last=#{adapter_sorts_nulls_last}"
end

def test_invalid_sort_param
Expand Down Expand Up @@ -4160,29 +4163,70 @@ def test_complex_includes_filters_nil_includes
end

def test_complex_includes_two_level
if is_db?(:mysql)
skip "#{adapter_name} test expectations differ in insignificant ways from expected"
end
assert_cacheable_get :index, params: {include: 'things,things.user'}

assert_response :success

# The test is hardcoded with the include order. This should be changed at some
# point since either thing could come first and still be valid
assert_equal '10', json_response['included'][0]['id']
assert_equal 'things', json_response['included'][0]['type']
assert_equal '10001', json_response['included'][0]['relationships']['user']['data']['id']
assert_nil json_response['included'][0]['relationships']['things']['data']
sorted_includeds = json_response['included'].map {|included|
{
'id' => included['id'],
'type' => included['type'],
'relationships_user_data_id' => included['relationships'].dig('user', 'data', 'id'),
'relationships_things_data_ids' => included['relationships'].dig('things', 'data')&.map {|data| data['id'] }&.sort,
}
}.sort_by {|included| "#{included['type']}-#{Integer(included['id'])}" }

assert_equal '20', json_response['included'][1]['id']
assert_equal 'things', json_response['included'][1]['type']
assert_equal '10001', json_response['included'][1]['relationships']['user']['data']['id']
assert_nil json_response['included'][1]['relationships']['things']['data']
expected = [
{
'id'=>'10',
'type'=>'things',
'relationships_user_data_id'=>'10001',
'relationships_things_data_ids'=>nil
},
{
'id'=>'20',
'type'=>'things',
'relationships_user_data_id'=>'10001',
'relationships_things_data_ids'=>nil
},
{
'id'=>'30',
'type'=>'things',
'relationships_user_data_id'=>'10002',
'relationships_things_data_ids'=>nil
},
{
'id'=>'10001',
'type'=>'users',
'relationships_user_data_id'=>nil,
'relationships_things_data_ids'=>['10', '20']
},
{
'id'=>'10002',
'type'=>'users',
'relationships_user_data_id'=>nil,
'relationships_things_data_ids'=>['30']
},
]
assert_array_equals expected, sorted_includeds
end

def test_complex_includes_things_nested_things
assert_cacheable_get :index, params: {include: 'things,things.things,things.things.things'}

assert_response :success
assert_hash_equals(
{
sorted_json_response_data = json_response["data"]
.sort_by {|data| Integer(data["id"]) }
sorted_json_response_included = json_response["included"]
.sort_by {|included| "#{included['type']}-#{Integer(included['id'])}" }
sorted_json_response = {
"data" => sorted_json_response_data,
"included" => sorted_json_response_included,
}
expected_response = {
"data" => [
{
"id" => "100",
Expand Down Expand Up @@ -4437,15 +4481,26 @@ def test_complex_includes_things_nested_things
}
}
]
},
json_response)
}
assert_hash_equals(expected_response, sorted_json_response)
end

def test_complex_includes_nested_things_secondary_users
if is_db?(:mysql)
skip "#{adapter_name} test expectations differ in insignificant ways from expected"
end
assert_cacheable_get :index, params: {include: 'things,things.user,things.things'}

assert_response :success
assert_hash_equals(
sorted_json_response_data = json_response["data"]
.sort_by {|data| Integer(data["id"]) }
sorted_json_response_included = json_response["included"]
.sort_by {|included| "#{included['type']}-#{Integer(included['id'])}" }
sorted_json_response = {
"data" => sorted_json_response_data,
"included" => sorted_json_response_included,
}
expected =
{
"data" => [
{
Expand Down Expand Up @@ -4732,8 +4787,8 @@ def test_complex_includes_nested_things_secondary_users
}
}
]
},
json_response)
}
assert_hash_equals(expected, sorted_json_response)
end
end

Expand Down Expand Up @@ -4767,16 +4822,22 @@ def teardown
end

def test_fetch_robots_with_sort_by_name
if is_db?(:mysql)
skip "#{adapter_name} test expectations differ in insignificant ways from expected"
end
Robot.create! name: 'John', version: 1
Robot.create! name: 'jane', version: 1
assert_cacheable_get :index, params: {sort: 'name'}
assert_response :success

if ENV['DATABASE_URL'].starts_with?('postgres')
assert_equal 'jane', json_response['data'].first['attributes']['name']
else
assert_equal 'John', json_response['data'].first['attributes']['name']
end
expected_names = Robot
.all
.order(name: :asc)
.map(&:name)
actual_names = json_response['data'].map {|data|
data['attributes']['name']
}
assert_equal expected_names, actual_names, "since adapter_sorts_nulls_last=#{adapter_sorts_nulls_last}"
end

def test_fetch_robots_with_sort_by_lower_name
Expand Down
29 changes: 24 additions & 5 deletions test/integration/requests/request_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1443,16 +1443,35 @@ def test_sort_primary_attribute
end

def test_sort_included_attribute
# Postgres sorts nulls last, whereas sqlite and mysql sort nulls first
pg = ENV['DATABASE_URL'].starts_with?('postgres')

if is_db?(:mysql)
skip "#{adapter_name} test expectations differ in insignificant ways from expected"
end
get '/api/v6/authors?sort=author_detail.author_stuff', headers: { 'Accept' => JSONAPI::MEDIA_TYPE }
assert_jsonapi_response 200
assert_equal pg ? '1001' : '1000', json_response['data'][0]['id']
up_expected_ids = AuthorResource
._model_class
.all
.left_joins(:author_detail)
.merge(AuthorDetail.order(author_stuff: :asc))
.map(&:id)
expected = up_expected_ids.first.to_s
ids = json_response['data'].map {|data| data['id'] }
actual = ids.first
assert_equal expected, actual, "since adapter_sorts_nulls_last=#{adapter_sorts_nulls_last} ands actual=#{ids} vs. expected=#{up_expected_ids}"

get '/api/v6/authors?sort=-author_detail.author_stuff', headers: { 'Accept' => JSONAPI::MEDIA_TYPE }
assert_jsonapi_response 200
assert_equal pg ? '1000' : '1002', json_response['data'][0]['id']
down_expected_ids = AuthorResource
._model_class
.all
.left_joins(:author_detail)
.merge(AuthorDetail.order(author_stuff: :desc))
.map(&:id)
expected = down_expected_ids.first.to_s
ids = json_response['data'].map {|data| data['id'] }
actual = ids.first
assert_equal expected, actual, "since adapter_sorts_nulls_last=#{adapter_sorts_nulls_last} ands actual=#{ids} vs. expected=#{down_expected_ids}"
refute_equal up_expected_ids, down_expected_ids # sanity check
end

def test_include_parameter_quoted
Expand Down
20 changes: 20 additions & 0 deletions test/test_helper.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,16 @@ def adapter_name
ActiveRecord::Base.connection.adapter_name
end

# Postgres sorts nulls last, whereas sqlite and mysql sort nulls first
def adapter_sorts_nulls_last
case adapter_name
when 'PostgreSQL' then true
when 'SQLite', 'Mysql2' then false
else
fail ArgumentError, "Unhandled adapter #{adapter_name} in #{__callee__}"
end
end

def db_quote_identifier
case adapter_name
when 'SQLite', 'PostgreSQL'
Expand All @@ -475,6 +485,16 @@ def db_quote_identifier
end
end

def is_db?(db_name)
case db_name
when :sqlite then /sqlite/i.match?(adapter_name)
when :postgres, :pg then /postgres/i.match?(adapter_name)
when :mysql then /mysql/i.match?(adapter_name)
else
/#{db_name}/i.match?(adapter_name)
end
end

def db_true
ActiveRecord::Base.connection.quote(true)
end
Expand Down

0 comments on commit f2bcba9

Please sign in to comment.