fix: Reliably quote columns/tables (#1400)

* 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

lib/jsonapi/active_relation_resource.rb

@@ -802,18 +802,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}"
@@ -822,32 +833,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 = {})

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 '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
@@ -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"
     )
 
@@ -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"
@@ -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(

test/unit/active_relation_resource_finder/join_manager_test.rb

@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)))
@@ -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
 
@@ -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
@@ -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'))
@@ -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'))