Skip to content

Fix sql quoting 0.10 #1367

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

Merged
lgebhardt merged 1 commit into from
May 19, 2021
Merged

Fix sql quoting 0.10 #1367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
71 changes: 50 additions & 21 deletions lib/jsonapi/active_relation_resource.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,11 @@ def find_fragments(filters, options = {})
# This alias is going to be resolve down to the model's table name and will not actually be an alias
resource_table_alias = resource_klass._table_name

pluck_fields = [Arel.sql("#{concat_table_field(resource_table_alias, resource_klass._primary_key)} AS #{resource_table_alias}_#{resource_klass._primary_key}")]
pluck_fields = [sql_field_with_alias(resource_table_alias, resource_klass._primary_key)]

cache_field = attribute_to_model_field(:_cache_field) if options[:cache]
if cache_field
pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, cache_field[:name])} AS #{resource_table_alias}_#{cache_field[:name]}")
pluck_fields << sql_field_with_alias(resource_table_alias, cache_field[:name])
end

linkage_fields = []
Expand All @@ -133,15 +133,15 @@ def find_fragments(filters, options = {})

linkage_table_alias = join_manager.join_details_by_polymorphic_relationship(linkage_relationship, resource_type)[:alias]
primary_key = klass._primary_key
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}")
pluck_fields << sql_field_with_alias(linkage_table_alias, primary_key)
end
else
klass = linkage_relationship.resource_klass
linkage_fields << {relationship_name: name, resource_klass: klass}

linkage_table_alias = join_manager.join_details_by_relationship(linkage_relationship)[:alias]
primary_key = klass._primary_key
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}")
pluck_fields << sql_field_with_alias(linkage_table_alias, primary_key)
end
end

Expand All @@ -150,7 +150,7 @@ def find_fragments(filters, options = {})
attributes.try(:each) do |attribute|
model_field = resource_klass.attribute_to_model_field(attribute)
model_fields[attribute] = model_field
pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, model_field[:name])} AS #{resource_table_alias}_#{model_field[:name]}")
pluck_fields << sql_field_with_alias(resource_table_alias, model_field[:name])
end

sort_fields = options.dig(:_relation_helper_options, :sort_fields)
Expand Down Expand Up @@ -409,13 +409,13 @@ def find_related_monomorphic_fragments(source_rids, relationship, options, conne
resource_table_alias = join_manager.join_details_by_relationship(relationship)[:alias]

pluck_fields = [
Arel.sql("#{_table_name}.#{_primary_key} AS source_id"),
Arel.sql("#{concat_table_field(resource_table_alias, resource_klass._primary_key)} AS #{resource_table_alias}_#{resource_klass._primary_key}")
Arel.sql("#{_table_name}.#{_primary_key} AS \"source_id\""),
sql_field_with_alias(resource_table_alias, resource_klass._primary_key)
]

cache_field = resource_klass.attribute_to_model_field(:_cache_field) if options[:cache]
if cache_field
pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, cache_field[:name])} AS #{resource_table_alias}_#{cache_field[:name]}")
pluck_fields << sql_field_with_alias(resource_table_alias, cache_field[:name])
end

linkage_fields = []
Expand All @@ -430,15 +430,15 @@ def find_related_monomorphic_fragments(source_rids, relationship, options, conne

linkage_table_alias = join_manager.join_details_by_polymorphic_relationship(linkage_relationship, resource_type)[:alias]
primary_key = klass._primary_key
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}")
pluck_fields << sql_field_with_alias(linkage_table_alias, primary_key)
end
else
klass = linkage_relationship.resource_klass
linkage_fields << {relationship_name: name, resource_klass: klass}

linkage_table_alias = join_manager.join_details_by_relationship(linkage_relationship)[:alias]
primary_key = klass._primary_key
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}")
pluck_fields << sql_field_with_alias(linkage_table_alias, primary_key)
end
end

Expand All @@ -447,7 +447,7 @@ def find_related_monomorphic_fragments(source_rids, relationship, options, conne
attributes.try(:each) do |attribute|
model_field = resource_klass.attribute_to_model_field(attribute)
model_fields[attribute] = model_field
pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, model_field[:name])} AS #{resource_table_alias}_#{model_field[:name]}")
pluck_fields << sql_field_with_alias(resource_table_alias, model_field[:name])
end

sort_fields = options.dig(:_relation_helper_options, :sort_fields)
Expand Down Expand Up @@ -543,9 +543,9 @@ def find_related_polymorphic_fragments(source_rids, relationship, options, conne
related_type = concat_table_field(_table_name, relationship.polymorphic_type)

pluck_fields = [
Arel.sql("#{primary_key} AS #{_table_name}_#{_primary_key}"),
Arel.sql("#{related_key} AS #{_table_name}_#{relationship.foreign_key}"),
Arel.sql("#{related_type} AS #{_table_name}_#{relationship.polymorphic_type}")
Arel.sql("#{primary_key} AS #{alias_table_field(_table_name, _primary_key)}"),
Arel.sql("#{related_key} AS #{alias_table_field(_table_name, relationship.foreign_key)}"),
Arel.sql("#{related_type} AS #{alias_table_field(_table_name, relationship.polymorphic_type)}")
]

# Get the additional fields from each relation. There's a limitation that the fields must exist in each relation
Expand All @@ -570,7 +570,7 @@ def find_related_polymorphic_fragments(source_rids, relationship, options, conne

cache_offset = relation_index
if cache_field
pluck_fields << Arel.sql("#{concat_table_field(table_alias, cache_field[:name])} AS cache_#{type}_#{cache_field[:name]}")
pluck_fields << sql_field_with_alias(table_alias, cache_field[:name])
relation_index+= 1
end

Expand All @@ -579,7 +579,7 @@ def find_related_polymorphic_fragments(source_rids, relationship, options, conne
attributes.try(:each) do |attribute|
model_field = related_klass.attribute_to_model_field(attribute)
model_fields[attribute] = model_field
pluck_fields << Arel.sql("#{concat_table_field(table_alias, model_field[:name])} AS #{table_alias}_#{model_field[:name]}")
pluck_fields << sql_field_with_alias(table_alias, model_field[:name])
relation_index+= 1
end

Expand Down Expand Up @@ -616,15 +616,15 @@ def find_related_polymorphic_fragments(source_rids, relationship, options, conne

linkage_table_alias = join_manager.join_details_by_polymorphic_relationship(linkage_relationship, resource_type)[:alias]
primary_key = klass._primary_key
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}")
pluck_fields << sql_field_with_alias(linkage_table_alias, primary_key)
end
else
klass = linkage_relationship.resource_klass
linkage_fields << {relationship: linkage_relationship, resource_klass: klass}

linkage_table_alias = join_manager.join_details_by_relationship(linkage_relationship)[:alias]
primary_key = klass._primary_key
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}")
pluck_fields << sql_field_with_alias(linkage_table_alias, primary_key)
end
end

Expand Down Expand Up @@ -790,22 +790,51 @@ def concat_table_field(table, field, quoted = false)
if table.blank? || field.to_s.include?('.')
# :nocov:
if quoted
"\"#{field.to_s}\""
quote(field)
else
field.to_s
end
# :nocov:
else
if quoted
"#{quote(table)}.#{quote(field)}"
else
# :nocov:
"\"#{table.to_s}\".\"#{field.to_s}\""
"#{table.to_s}.#{field.to_s}"
# :nocov:
end
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)
else
"#{table.to_s}.#{field.to_s}"
field.to_s
end
# :nocov:
else
if quoted
# :nocov:
quote("#{table.to_s}_#{field.to_s}")
# :nocov:
else
"#{table.to_s}_#{field.to_s}"
end
end
end

def quote(field)
"\"#{field.to_s}\""
end


def apply_filters(records, filters, options = {})
if filters
filters.each do |filter, value|
Expand Down
2 changes: 1 addition & 1 deletion lib/jsonapi/basic_resource.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,7 +1058,7 @@ def default_sort
end

def construct_order_options(sort_params)
sort_params ||= default_sort
sort_params = default_sort if sort_params.blank?

return {} unless sort_params

Expand Down
57 changes: 0 additions & 57 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 @@ -110,14 +110,6 @@ def test_add_nested_scoped_joins
records = Api::V10::PostResource.records({})
records = join_manager.join(records, {})

if (Rails::VERSION::MAJOR == 6 && Rails::VERSION::MINOR >= 1) || Rails::VERSION::MAJOR > 6
sql = 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" author ON author."id" = "posts"."author_id" LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
else
sql = 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" ON "people"."id" = "posts"."author_id" LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
end

assert_equal sql, records.to_sql

assert_hash_equals({alias: 'posts', join_type: :root}, join_manager.source_join_details)
assert_hash_equals({alias: 'comments', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::PostResource._relationship(:comments)))
assert_hash_equals({alias: 'authors_comments', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:author)))
Expand All @@ -135,37 +127,11 @@ def test_add_nested_scoped_joins
records = Api::V10::PostResource.records({})
records = join_manager.join(records, {})

# Note sql is in different order, but aliases should still be right
if (Rails::VERSION::MAJOR == 6 && Rails::VERSION::MINOR >= 1) || Rails::VERSION::MAJOR > 6
sql = 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "people" author ON author."id" = "posts"."author_id" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
else
sql = 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "people" ON "people"."id" = "posts"."author_id" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
end

assert_equal sql, records.to_sql

assert_hash_equals({alias: 'posts', join_type: :root}, join_manager.source_join_details)
assert_hash_equals({alias: 'comments', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::PostResource._relationship(:comments)))
assert_hash_equals({alias: 'authors_comments', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:author)))
assert_hash_equals({alias: 'tags', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:tags)))
assert_hash_equals({alias: 'people', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::PostResource._relationship(:author)))

# Easier to read SQL to show joins are the same, but in different order
# Pass 1
# SELECT "posts".* FROM "posts"
# LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id"
# LEFT OUTER JOIN "people" ON "people"."id" = "posts"."author_id"
# LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id"
# LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id"
# LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = 1 AND "author"."special" = 1
#
# Pass 2
# SELECT "posts".* FROM "posts"
# LEFT OUTER JOIN "people" ON "people"."id" = "posts"."author_id"
# LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id"
# LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id"
# LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id"
# LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = 1 AND "author"."special" = 1
end

def test_add_nested_joins_with_fields
Expand All @@ -179,14 +145,6 @@ def test_add_nested_joins_with_fields
records = Api::V10::PostResource.records({})
records = join_manager.join(records, {})

if (Rails::VERSION::MAJOR == 6 && Rails::VERSION::MINOR >= 1) || Rails::VERSION::MAJOR > 6
sql = 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" author ON author."id" = "posts"."author_id" LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
else
sql = 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" ON "people"."id" = "posts"."author_id" LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
end

assert_equal sql, records.to_sql

assert_hash_equals({alias: 'posts', join_type: :root}, join_manager.source_join_details)
assert_hash_equals({alias: 'comments', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::PostResource._relationship(:comments)))
assert_hash_equals({alias: 'authors_comments', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:author)))
Expand All @@ -202,16 +160,6 @@ def test_add_joins_with_sub_relationship
records = Api::V10::PostResource.records({})
records = join_manager.join(records, {})

if (Rails::VERSION::MAJOR == 6 && Rails::VERSION::MINOR >= 1) || Rails::VERSION::MAJOR > 6
sql = 'SELECT "posts".* FROM "posts" INNER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" author ON author."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" LEFT OUTER JOIN "comments" "comments_people" ON "comments_people"."author_id" = "people"."id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
assert_hash_equals({alias: 'author', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:author)))
else
sql = 'SELECT "posts".* FROM "posts" INNER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" ON "people"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" LEFT OUTER JOIN "comments" "comments_people" ON "comments_people"."author_id" = "people"."id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
assert_hash_equals({alias: 'people', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:author)))
end

assert_equal sql, records.to_sql

assert_hash_equals({alias: 'comments', join_type: :inner}, join_manager.source_join_details)
assert_hash_equals({alias: 'comments', join_type: :inner}, join_manager.join_details_by_relationship(Api::V10::PostResource._relationship(:comments)))
assert_hash_equals({alias: 'tags', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:tags)))
Expand Down Expand Up @@ -280,11 +228,6 @@ def test_polymorphic_join_belongs_to_filter_on_resource
records = PictureResource.records({})
records = join_manager.join(records, {})

#TODO: Fix this with a better test
sql_v1 = 'SELECT "pictures".* FROM "pictures" LEFT OUTER JOIN "documents" ON "documents"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Document\' LEFT OUTER JOIN "products" ON "products"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Product\' LEFT OUTER JOIN "file_properties" ON "file_properties"."fileable_id" = "pictures"."id" AND "file_properties"."fileable_type" = \'Picture\''
sql_v2 = 'SELECT "pictures".* FROM "pictures" LEFT OUTER JOIN "documents" ON "documents"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Document\' LEFT OUTER JOIN "products" ON "products"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Product\' LEFT OUTER JOIN "file_properties" ON "file_properties"."fileable_type" = \'Picture\' AND "file_properties"."fileable_id" = "pictures"."id"'
assert records.to_sql == sql_v1 || records.to_sql == sql_v2, 'did not generate an expected sql statement'

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