Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Reliably quote columns/tables #1400

Merged
merged 12 commits into from
Feb 2, 2023
Merged

fix: Reliably quote columns/tables #1400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b6512f2
refactor: easily quote table/column
bf4 Jan 31, 2023
64520ca
refactor: extract table name when missing
bf4 Jan 31, 2023
eb79124
fix: Reliably quote columns/tables
bf4 Jan 31, 2023
d332579
refactor: putting quoting methods together
bf4 Jan 31, 2023
f72b46a
fix: typo-ish
bf4 Jan 31, 2023
6e446a9
test: where is the bad mysql expectation coming from?
bf4 Jan 31, 2023
68b00ba
fix: hack mysql test query comparison
bf4 Jan 31, 2023
6bf350f
fix: hack mysql query comparison
bf4 Jan 31, 2023
765e68d
test: always sql for compare
bf4 Jan 31, 2023
55c3f23
fix: correct adapter name
bf4 Jan 31, 2023
c21667a
Handle special case of *
bf4 Jan 31, 2023
15d0ec6
chore: update per review
bf4 Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 35 additions & 10 deletions lib/jsonapi/active_relation_resource.rb
View file
Open in desktop
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}\""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in pg, is basically quote_ident. for values, should be single-quotes like quote_literal

%{"#{field.to_s}"}
end

def apply_filters(records, filters, options = {})
Expand Down
35 changes: 29 additions & 6 deletions test/test_helper.rb
View file
Open in desktop
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
Open in desktop
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