Fix some testing issues including flappy tests, simplify quoting generated SQL strings (#1363)

* Refine when config.active_record.sqlite3.represent_boolean_as_integer is set

Tests with rails > 6.1.1 breaks with this option

* Update rails 6.1 tested version

* Add quoting for fields in built up sql

* Remove tests for generated sql statements

These are changing with new rails versions and it's creating a lot of false failures

* Remove tests for generated sql statements using joins_left

* Fix default sort for when empty array is provided

Fixes issue with relationship requests not getting a default sort

* Add helper methods for generating sql fields with aliases and quotes

Author: lgebhardt

.github/workflows/ruby.yml

@@ -30,7 +30,7 @@ jobs:
           - 2.7.2
           - 3.0.0
         rails:
-          - 6.1.1
+          - 6.1.3.1
           - 6.0.3.4
           - 5.2.4.4
           - 5.1.7

lib/jsonapi/active_relation_resource.rb

@@ -121,11 +121,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 = []
@@ -141,10 +141,10 @@ def find_fragments(filters, options = {})
 
               linkage_fields << {relationship_name: name,
                                  resource_klass: klass,
-                                 field: "#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}",
-                                 alias: "#{linkage_table_alias}_#{primary_key}"}
+                                 field: sql_field_with_alias(linkage_table_alias, primary_key),
+                                 alias: alias_table_field(linkage_table_alias, 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
@@ -153,10 +153,10 @@ def find_fragments(filters, options = {})
 
             linkage_fields << {relationship_name: name,
                                resource_klass: klass,
-                               field: "#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}",
-                               alias: "#{linkage_table_alias}_#{primary_key}"}
+                               field: sql_field_with_alias(linkage_table_alias, primary_key),
+                               alias: alias_table_field(linkage_table_alias, 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
 
@@ -165,7 +165,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)
@@ -423,13 +423,13 @@ def find_related_monomorphic_fragments(source_fragments, relationship, options,
         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 = []
@@ -444,15 +444,15 @@ def find_related_monomorphic_fragments(source_fragments, relationship, 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
 
@@ -461,7 +461,7 @@ def find_related_monomorphic_fragments(source_fragments, relationship, 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)
@@ -557,9 +557,9 @@ def find_related_polymorphic_fragments(source_fragments, relationship, options,
         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
@@ -584,7 +584,7 @@ def find_related_polymorphic_fragments(source_fragments, relationship, options,
 
             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
 
@@ -593,7 +593,7 @@ def find_related_polymorphic_fragments(source_fragments, relationship, options,
             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
 
@@ -630,15 +630,15 @@ def find_related_polymorphic_fragments(source_fragments, relationship, 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: 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
 
@@ -804,22 +804,50 @@ 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}"
+            # :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
             field.to_s
           end
           # :nocov:
         else
           if quoted
             # :nocov:
-            "\"#{table.to_s}\".\"#{field.to_s}\""
+            quote("#{table.to_s}_#{field.to_s}")
             # :nocov:
           else
-            "#{table.to_s}.#{field.to_s}"
+            "#{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|

lib/jsonapi/basic_resource.rb

@@ -1057,7 +1057,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
 

test/test_helper.rb

@@ -62,7 +62,7 @@ class TestApp < Rails::Application
   config.active_support.halt_callback_chains_on_return_false = false
   config.active_record.time_zone_aware_types = [:time, :datetime]
   config.active_record.belongs_to_required_by_default = false
-  unless Rails::VERSION::MAJOR == 5 && Rails::VERSION::MINOR < 2
+  unless Rails::VERSION::MAJOR == 5 && Rails::VERSION::MINOR < 2 || Rails::VERSION::MAJOR == 6 && Rails::VERSION::MINOR >= 1
     config.active_record.sqlite3.represent_boolean_as_integer = true
   end
 end