From b6512f2150d8b6079965501b1632a2417e06121e Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Tue, 31 Jan 2023 14:13:57 -0600 Subject: [PATCH 01/12] refactor: easily quote table/column --- lib/jsonapi/active_relation_resource.rb | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/jsonapi/active_relation_resource.rb b/lib/jsonapi/active_relation_resource.rb index c8631172..673544fc 100644 --- a/lib/jsonapi/active_relation_resource.rb +++ b/lib/jsonapi/active_relation_resource.rb @@ -804,14 +804,14 @@ def concat_table_field(table, field, quoted = false) if table.blank? || field.to_s.include?('.') # :nocov: if quoted - quote(field) + quote(field) # split on '.'? 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}" @@ -828,7 +828,7 @@ 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 @@ -836,7 +836,7 @@ def alias_table_field(table, field, quoted = false) 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}" @@ -844,6 +844,14 @@ def alias_table_field(table, field, quoted = false) end end + def quote_table_name(table_name) + quote(table_name) + end + + def quote_column_name(column_name) + quote(column_name) + end + def quote(field) "\"#{field.to_s}\"" end From 64520caa41776ead5f5b062473f3d2da5293a5c3 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Tue, 31 Jan 2023 14:35:35 -0600 Subject: [PATCH 02/12] refactor: extract table name when missing --- lib/jsonapi/active_relation_resource.rb | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/jsonapi/active_relation_resource.rb b/lib/jsonapi/active_relation_resource.rb index 673544fc..2606f162 100644 --- a/lib/jsonapi/active_relation_resource.rb +++ b/lib/jsonapi/active_relation_resource.rb @@ -801,10 +801,17 @@ def sort_records(records, order_options, options) 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) # split on '.'? + quote_column_name(field) else field.to_s end From eb7912436dadc15b67aa4de7184651c45e1ac68f Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Tue, 31 Jan 2023 14:46:14 -0600 Subject: [PATCH 03/12] fix: Reliably quote columns/tables --- lib/jsonapi/active_relation_resource.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/jsonapi/active_relation_resource.rb b/lib/jsonapi/active_relation_resource.rb index 2606f162..101de405 100644 --- a/lib/jsonapi/active_relation_resource.rb +++ b/lib/jsonapi/active_relation_resource.rb @@ -852,11 +852,19 @@ def alias_table_field(table, field, quoted = false) end def quote_table_name(table_name) - quote(table_name) + if _model_class.try(:connection) + _model_class.connection.quote(table_name) + else + quote(table_name) + end end def quote_column_name(column_name) - quote(column_name) + if _model_class.try(:connection) + _model_class.connection.quote(column_name) + else + quote(column_name) + end end def quote(field) From d332579a14301300fc9b641771ff5d785bde7b24 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Tue, 31 Jan 2023 15:15:24 -0600 Subject: [PATCH 04/12] refactor: putting quoting methods together --- lib/jsonapi/active_relation_resource.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/jsonapi/active_relation_resource.rb b/lib/jsonapi/active_relation_resource.rb index 101de405..bdaab21c 100644 --- a/lib/jsonapi/active_relation_resource.rb +++ b/lib/jsonapi/active_relation_resource.rb @@ -800,6 +800,10 @@ 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? split_table, split_field = field.to_s.split('.') @@ -827,10 +831,6 @@ 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: From f72b46a89d56a82bbe56e9b26071558861b1cec2 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Tue, 31 Jan 2023 16:27:54 -0600 Subject: [PATCH 05/12] fix: typo-ish --- lib/jsonapi/active_relation_resource.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/jsonapi/active_relation_resource.rb b/lib/jsonapi/active_relation_resource.rb index bdaab21c..ae82f208 100644 --- a/lib/jsonapi/active_relation_resource.rb +++ b/lib/jsonapi/active_relation_resource.rb @@ -853,7 +853,7 @@ def alias_table_field(table, field, quoted = false) def quote_table_name(table_name) if _model_class.try(:connection) - _model_class.connection.quote(table_name) + _model_class.connection.quote_table_name(table_name) else quote(table_name) end @@ -861,14 +861,14 @@ def quote_table_name(table_name) def quote_column_name(column_name) if _model_class.try(:connection) - _model_class.connection.quote(column_name) + _model_class.connection.quote_column_name(column_name) else quote(column_name) end end def quote(field) - "\"#{field.to_s}\"" + %{"#{field.to_s}"} end def apply_filters(records, filters, options = {}) From 6e446a93198081e904c3cf149922cc08898c0247 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Tue, 31 Jan 2023 17:00:59 -0600 Subject: [PATCH 06/12] test: where is the bad mysql expectation coming from? --- lib/jsonapi/active_relation_resource.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/jsonapi/active_relation_resource.rb b/lib/jsonapi/active_relation_resource.rb index ae82f208..a57f25ca 100644 --- a/lib/jsonapi/active_relation_resource.rb +++ b/lib/jsonapi/active_relation_resource.rb @@ -868,6 +868,7 @@ def quote_column_name(column_name) end def quote(field) + raise "zomg, we are using the fallback for #{field}" %{"#{field.to_s}"} end From 68b00bafe1be2f40e0f56fac071ed1068060943b Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Tue, 31 Jan 2023 17:04:17 -0600 Subject: [PATCH 07/12] fix: hack mysql test query comparison --- test/test_helper.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/test_helper.rb b/test/test_helper.rb index 9850a49c..6316e917 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -501,8 +501,8 @@ def assert_cacheable_jsonapi_get(url, cached_classes = :all) end assert_equal( - non_caching_response.pretty_inspect, - json_response.pretty_inspect, + non_caching_response.pretty_inspect.tr("`", %{"}), + json_response.pretty_inspect.tr("`", %{"}), "Cache warmup response must match normal response" ) @@ -511,8 +511,8 @@ def assert_cacheable_jsonapi_get(url, cached_classes = :all) end assert_equal( - non_caching_response.pretty_inspect, - json_response.pretty_inspect, + non_caching_response.pretty_inspect.tr("`", %{"}), + json_response.pretty_inspect.tr("`", %{"}), "Cached response must match normal response" ) assert_equal 0, cached[:total][:misses], "Cached response must not cause any cache misses" @@ -580,8 +580,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, + non_caching_response.pretty_inspect.tr("`", %{"}), + json_response_sans_all_backtraces.pretty_inspect.tr("`", %{"}), "Cache (mode: #{mode}) #{phase} response body must match normal response" ) assert_operator( From 6bf350fc62325c242753af3a6e2ff7e4faaf8ca1 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Tue, 31 Jan 2023 17:19:42 -0600 Subject: [PATCH 08/12] fix: hack mysql query comparison --- .../join_manager_test.rb | 43 +++++++++++-------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/test/unit/active_relation_resource_finder/join_manager_test.rb b/test/unit/active_relation_resource_finder/join_manager_test.rb index 840c90ee..cf0ca14c 100644 --- a/test/unit/active_relation_resource_finder/join_manager_test.rb +++ b/test/unit/active_relation_resource_finder/join_manager_test.rb @@ -3,25 +3,32 @@ 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' + def adapter_name + ActiveRecord::Base.connection.adapter_name + end + + def db_quote_identifier + case adapter_name + when 'SQLite', 'PostgreSQL' + %{"} + when 'MySQL' + %{`} + else + fail ArgumentError, "Unhandled adapter #{adapter_name} in #{__callee__}" + end end end + def db_true + ActiveRecord::Base.connection.quote(true) + 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"', records.to_sql.tr(db_quote_identifier, %{"}) assert_hash_equals({alias: 'posts', join_type: :root}, join_manager.source_join_details) end @@ -42,7 +49,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"', records.to_sql.tr(db_quote_identifier, %{"}) 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 @@ -53,7 +60,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"', records.to_sql.tr(db_quote_identifier, %{"}) 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 @@ -68,7 +75,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"', records.to_sql.tr(db_quote_identifier, %{"}) 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))) @@ -81,7 +88,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"', records.to_sql.tr(db_quote_identifier, %{"}) assert_hash_equals({alias: 'comments', join_type: :inner}, join_manager.source_join_details) end @@ -94,7 +101,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, records.to_sql.tr(db_quote_identifier, %{"}) assert_hash_equals({alias: 'comments', join_type: :inner}, join_manager.source_join_details) end @@ -195,7 +202,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\'', records.to_sql.tr(db_quote_identifier, %{"}) 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')) @@ -209,7 +216,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\'', records.to_sql.tr(db_quote_identifier, %{"}) 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')) From 765e68d728f692b617cd186423124c04f4b495a6 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Tue, 31 Jan 2023 17:25:31 -0600 Subject: [PATCH 09/12] test: always sql for compare --- test/test_helper.rb | 35 ++++++++++++++--- .../join_manager_test.rb | 38 +++++-------------- 2 files changed, 38 insertions(+), 35 deletions(-) diff --git a/test/test_helper.rb b/test/test_helper.rb index 6316e917..19cfdc01 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -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 'MySQL' + %{`} + 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 @@ -501,8 +524,8 @@ def assert_cacheable_jsonapi_get(url, cached_classes = :all) end assert_equal( - non_caching_response.pretty_inspect.tr("`", %{"}), - json_response.pretty_inspect.tr("`", %{"}), + sql_for_compare(non_caching_response.pretty_inspect), + sql_for_compare(json_response.pretty_inspect), "Cache warmup response must match normal response" ) @@ -511,8 +534,8 @@ def assert_cacheable_jsonapi_get(url, cached_classes = :all) end assert_equal( - non_caching_response.pretty_inspect.tr("`", %{"}), - json_response.pretty_inspect.tr("`", %{"}), + 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" @@ -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.tr("`", %{"}), - json_response_sans_all_backtraces.pretty_inspect.tr("`", %{"}), + 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( diff --git a/test/unit/active_relation_resource_finder/join_manager_test.rb b/test/unit/active_relation_resource_finder/join_manager_test.rb index cf0ca14c..43387f38 100644 --- a/test/unit/active_relation_resource_finder/join_manager_test.rb +++ b/test/unit/active_relation_resource_finder/join_manager_test.rb @@ -3,32 +3,12 @@ class JoinTreeTest < ActiveSupport::TestCase - def adapter_name - ActiveRecord::Base.connection.adapter_name - end - - def db_quote_identifier - case adapter_name - when 'SQLite', 'PostgreSQL' - %{"} - when 'MySQL' - %{`} - else - fail ArgumentError, "Unhandled adapter #{adapter_name} in #{__callee__}" - end - end - end - - def db_true - ActiveRecord::Base.connection.quote(true) - 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.tr(db_quote_identifier, %{"}) + 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 @@ -38,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 @@ -49,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.tr(db_quote_identifier, %{"}) + 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 @@ -60,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.tr(db_quote_identifier, %{"}) + 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 @@ -75,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.tr(db_quote_identifier, %{"}) + 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))) @@ -88,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.tr(db_quote_identifier, %{"}) + 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 @@ -101,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.tr(db_quote_identifier, %{"}) + assert_equal sql, sql_for_compare(records.to_sql) assert_hash_equals({alias: 'comments', join_type: :inner}, join_manager.source_join_details) end @@ -202,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.tr(db_quote_identifier, %{"}) + # 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')) @@ -216,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.tr(db_quote_identifier, %{"}) + # 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')) From 55c3f23cf179a858f6b40c818dad31ed0ebde790 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Tue, 31 Jan 2023 17:30:13 -0600 Subject: [PATCH 10/12] fix: correct adapter name --- test/test_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_helper.rb b/test/test_helper.rb index 19cfdc01..ce8b18d8 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -468,7 +468,7 @@ def db_quote_identifier case adapter_name when 'SQLite', 'PostgreSQL' %{"} - when 'MySQL' + when 'Mysql2' %{`} else fail ArgumentError, "Unhandled adapter #{adapter_name} in #{__callee__}" From c21667aa5ba42e5d220998435e8f4092875a7024 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Tue, 31 Jan 2023 17:45:02 -0600 Subject: [PATCH 11/12] Handle special case of * --- lib/jsonapi/active_relation_resource.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/jsonapi/active_relation_resource.rb b/lib/jsonapi/active_relation_resource.rb index a57f25ca..5832c09f 100644 --- a/lib/jsonapi/active_relation_resource.rb +++ b/lib/jsonapi/active_relation_resource.rb @@ -860,6 +860,7 @@ def quote_table_name(table_name) end def quote_column_name(column_name) + return column_name if column_name == "*" if _model_class.try(:connection) _model_class.connection.quote_column_name(column_name) else @@ -868,7 +869,6 @@ def quote_column_name(column_name) end def quote(field) - raise "zomg, we are using the fallback for #{field}" %{"#{field.to_s}"} end From 15d0ec643adaf023a6586071cc7ad0124fe4d0af Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Thu, 2 Feb 2023 10:17:47 -0600 Subject: [PATCH 12/12] chore: update per review --- lib/jsonapi/active_relation_resource.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/jsonapi/active_relation_resource.rb b/lib/jsonapi/active_relation_resource.rb index 5832c09f..a03c076f 100644 --- a/lib/jsonapi/active_relation_resource.rb +++ b/lib/jsonapi/active_relation_resource.rb @@ -852,7 +852,7 @@ def alias_table_field(table, field, quoted = false) end def quote_table_name(table_name) - if _model_class.try(:connection) + if _model_class&.connection _model_class.connection.quote_table_name(table_name) else quote(table_name) @@ -861,13 +861,14 @@ def quote_table_name(table_name) def quote_column_name(column_name) return column_name if column_name == "*" - if _model_class.try(:connection) + 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}"} end