Skip to content

Commit

Permalink
fix: Reliably quote columns/tables (#1400)
Browse files Browse the repository at this point in the history 
* refactor: easily quote table/column
* refactor: extract table name when missing
* fix: Reliably quote columns/tables
* refactor: putting quoting methods together
* Handle special case of *
- tests
  * fix: hack mysql test query comparison
  • Loading branch information
@bf4
bf4 authored Feb 2, 2023
1 parent 9156734 commit 5c24399
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 38 deletions.
45 changes: 35 additions & 10 deletions lib/jsonapi/active_relation_resource.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -800,18 +800,29 @@ def sort_records(records, order_options, options)
apply_sort(records, order_options, options)
end

def sql_field_with_alias(table, field, quoted = true)
Arel.sql("#{concat_table_field(table, field, quoted)} AS #{alias_table_field(table, field, quoted)}")
end

def concat_table_field(table, field, quoted = false)
if table.blank? || field.to_s.include?('.')
if table.blank?
split_table, split_field = field.to_s.split('.')
if split_table && split_field
table = split_table
field = split_field
end
end
if table.blank?
# :nocov:
if quoted
quote(field)
quote_column_name(field)
else
field.to_s
end
# :nocov:
else
if quoted
"#{quote(table)}.#{quote(field)}"
"#{quote_table_name(table)}.#{quote_column_name(field)}"
else
# :nocov:
"#{table.to_s}.#{field.to_s}"
Expand All @@ -820,32 +831,46 @@ def concat_table_field(table, field, quoted = false)
end
end

def sql_field_with_alias(table, field, quoted = true)
Arel.sql("#{concat_table_field(table, field, quoted)} AS #{alias_table_field(table, field, quoted)}")
end

def alias_table_field(table, field, quoted = false)
if table.blank? || field.to_s.include?('.')
# :nocov:
if quoted
quote(field)
quote_column_name(field)
else
field.to_s
end
# :nocov:
else
if quoted
# :nocov:
quote("#{table.to_s}_#{field.to_s}")
quote_column_name("#{table.to_s}_#{field.to_s}")
# :nocov:
else
"#{table.to_s}_#{field.to_s}"
end
end
end

def quote_table_name(table_name)
if _model_class&.connection
_model_class.connection.quote_table_name(table_name)
else
quote(table_name)
end
end

def quote_column_name(column_name)
return column_name if column_name == "*"
if _model_class&.connection
_model_class.connection.quote_column_name(column_name)
else
quote(column_name)
end
end

# fallback quote identifier when database adapter not available
def quote(field)
"\"#{field.to_s}\""
%{"#{field.to_s}"}
end

def apply_filters(records, filters, options = {})
Expand Down
35 changes: 29 additions & 6 deletions test/test_helper.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,29 @@ def run_in_transaction?

self.fixture_path = "#{Rails.root}/fixtures"
fixtures :all

def adapter_name
ActiveRecord::Base.connection.adapter_name
end

def db_quote_identifier
case adapter_name
when 'SQLite', 'PostgreSQL'
%{"}
when 'Mysql2'
%{`}
else
fail ArgumentError, "Unhandled adapter #{adapter_name} in #{__callee__}"
end
end

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

def sql_for_compare(sql)
sql.tr(db_quote_identifier, %{"})
end
end

class ActiveSupport::TestCase
Expand Down Expand Up @@ -501,8 +524,8 @@ def assert_cacheable_jsonapi_get(url, cached_classes = :all)
end

assert_equal(
non_caching_response.pretty_inspect,
json_response.pretty_inspect,
sql_for_compare(non_caching_response.pretty_inspect),
sql_for_compare(json_response.pretty_inspect),
"Cache warmup response must match normal response"
)

Expand All @@ -511,8 +534,8 @@ def assert_cacheable_jsonapi_get(url, cached_classes = :all)
end

assert_equal(
non_caching_response.pretty_inspect,
json_response.pretty_inspect,
sql_for_compare(non_caching_response.pretty_inspect),
sql_for_compare(json_response.pretty_inspect),
"Cached response must match normal response"
)
assert_equal 0, cached[:total][:misses], "Cached response must not cause any cache misses"
Expand Down Expand Up @@ -580,8 +603,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,
sql_for_compare(non_caching_response.pretty_inspect),
sql_for_compare(json_response_sans_all_backtraces.pretty_inspect),
"Cache (mode: #{mode}) #{phase} response body must match normal response"
)
assert_operator(
Expand Down
31 changes: 9 additions & 22 deletions test/unit/active_relation_resource_finder/join_manager_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,12 @@

class JoinTreeTest < ActiveSupport::TestCase

def db_true
case ActiveRecord::Base.connection.adapter_name
when 'SQLite'
if Rails::VERSION::MAJOR >= 6 || (Rails::VERSION::MAJOR >= 5 && ActiveRecord::VERSION::MINOR >= 2)
"1"
else
"'t'"
end
when 'PostgreSQL'
'TRUE'
end
end

def test_no_added_joins
join_manager = JSONAPI::ActiveRelation::JoinManager.new(resource_klass: PostResource)

records = PostResource.records({})
records = join_manager.join(records, {})
assert_equal 'SELECT "posts".* FROM "posts"', records.to_sql
assert_equal 'SELECT "posts".* FROM "posts"', sql_for_compare(records.to_sql)

assert_hash_equals({alias: 'posts', join_type: :root}, join_manager.source_join_details)
end
Expand All @@ -31,7 +18,7 @@ def test_add_single_join
join_manager = JSONAPI::ActiveRelation::JoinManager.new(resource_klass: PostResource, filters: filters)
records = PostResource.records({})
records = join_manager.join(records, {})
assert_equal 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "posts_tags" ON "posts_tags"."post_id" = "posts"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "posts_tags"."tag_id"', records.to_sql
assert_equal 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "posts_tags" ON "posts_tags"."post_id" = "posts"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "posts_tags"."tag_id"', sql_for_compare(records.to_sql)
assert_hash_equals({alias: 'posts', join_type: :root}, join_manager.source_join_details)
assert_hash_equals({alias: 'tags', join_type: :left}, join_manager.join_details_by_relationship(PostResource._relationship(:tags)))
end
Expand All @@ -42,7 +29,7 @@ def test_add_single_sort_join
records = PostResource.records({})
records = join_manager.join(records, {})

assert_equal 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "posts_tags" ON "posts_tags"."post_id" = "posts"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "posts_tags"."tag_id"', records.to_sql
assert_equal 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "posts_tags" ON "posts_tags"."post_id" = "posts"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "posts_tags"."tag_id"', sql_for_compare(records.to_sql)
assert_hash_equals({alias: 'posts', join_type: :root}, join_manager.source_join_details)
assert_hash_equals({alias: 'tags', join_type: :left}, join_manager.join_details_by_relationship(PostResource._relationship(:tags)))
end
Expand All @@ -53,7 +40,7 @@ def test_add_single_sort_and_filter_join
join_manager = JSONAPI::ActiveRelation::JoinManager.new(resource_klass: PostResource, sort_criteria: sort_criteria, filters: filters)
records = PostResource.records({})
records = join_manager.join(records, {})
assert_equal 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "posts_tags" ON "posts_tags"."post_id" = "posts"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "posts_tags"."tag_id"', records.to_sql
assert_equal 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "posts_tags" ON "posts_tags"."post_id" = "posts"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "posts_tags"."tag_id"', sql_for_compare(records.to_sql)
assert_hash_equals({alias: 'posts', join_type: :root}, join_manager.source_join_details)
assert_hash_equals({alias: 'tags', join_type: :left}, join_manager.join_details_by_relationship(PostResource._relationship(:tags)))
end
Expand All @@ -68,7 +55,7 @@ def test_add_sibling_joins
records = PostResource.records({})
records = join_manager.join(records, {})

assert_equal 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "posts_tags" ON "posts_tags"."post_id" = "posts"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "posts_tags"."tag_id" LEFT OUTER JOIN "people" ON "people"."id" = "posts"."author_id"', records.to_sql
assert_equal 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "posts_tags" ON "posts_tags"."post_id" = "posts"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "posts_tags"."tag_id" LEFT OUTER JOIN "people" ON "people"."id" = "posts"."author_id"', sql_for_compare(records.to_sql)
assert_hash_equals({alias: 'posts', join_type: :root}, join_manager.source_join_details)
assert_hash_equals({alias: 'tags', join_type: :left}, join_manager.join_details_by_relationship(PostResource._relationship(:tags)))
assert_hash_equals({alias: 'people', join_type: :left}, join_manager.join_details_by_relationship(PostResource._relationship(:author)))
Expand All @@ -81,7 +68,7 @@ def test_add_joins_source_relationship
records = PostResource.records({})
records = join_manager.join(records, {})

assert_equal 'SELECT "posts".* FROM "posts" INNER JOIN "comments" ON "comments"."post_id" = "posts"."id"', records.to_sql
assert_equal 'SELECT "posts".* FROM "posts" INNER JOIN "comments" ON "comments"."post_id" = "posts"."id"', sql_for_compare(records.to_sql)
assert_hash_equals({alias: 'comments', join_type: :inner}, join_manager.source_join_details)
end

Expand All @@ -94,7 +81,7 @@ def test_add_joins_source_relationship_with_custom_apply

sql = 'SELECT "posts".* FROM "posts" INNER JOIN "comments" ON "comments"."post_id" = "posts"."id" WHERE "comments"."approved" = ' + db_true

assert_equal sql, records.to_sql
assert_equal sql, sql_for_compare(records.to_sql)

assert_hash_equals({alias: 'comments', join_type: :inner}, join_manager.source_join_details)
end
Expand Down Expand Up @@ -195,7 +182,7 @@ def test_polymorphic_join_belongs_to_just_source
records = PictureResource.records({})
records = join_manager.join(records, {})

# assert_equal 'SELECT "pictures".* FROM "pictures" LEFT OUTER JOIN "products" ON "products"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Product\' LEFT OUTER JOIN "documents" ON "documents"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Document\'', records.to_sql
# assert_equal 'SELECT "pictures".* FROM "pictures" LEFT OUTER JOIN "products" ON "products"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Product\' LEFT OUTER JOIN "documents" ON "documents"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Document\'', sql_for_compare(records.to_sql)
assert_hash_equals({alias: 'products', join_type: :left}, join_manager.source_join_details('products'))
assert_hash_equals({alias: 'documents', join_type: :left}, join_manager.source_join_details('documents'))
assert_hash_equals({alias: 'products', join_type: :left}, join_manager.join_details_by_polymorphic_relationship(PictureResource._relationship(:imageable), 'products'))
Expand All @@ -209,7 +196,7 @@ def test_polymorphic_join_belongs_to_filter
records = PictureResource.records({})
records = join_manager.join(records, {})

# assert_equal 'SELECT "pictures".* FROM "pictures" LEFT OUTER JOIN "products" ON "products"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Product\' LEFT OUTER JOIN "documents" ON "documents"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Document\'', records.to_sql
# assert_equal 'SELECT "pictures".* FROM "pictures" LEFT OUTER JOIN "products" ON "products"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Product\' LEFT OUTER JOIN "documents" ON "documents"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Document\'', sql_for_compare(records.to_sql)
assert_hash_equals({alias: 'pictures', join_type: :root}, join_manager.source_join_details)
assert_hash_equals({alias: 'products', join_type: :left}, join_manager.join_details_by_polymorphic_relationship(PictureResource._relationship(:imageable), 'products'))
assert_hash_equals({alias: 'documents', join_type: :left}, join_manager.join_details_by_polymorphic_relationship(PictureResource._relationship(:imageable), 'documents'))
Expand Down

0 comments on commit 5c24399

Please sign in to comment.