Merge pull request #1287 from cerebris/test_on_pg

Fix for Postgres SQL generation error

Author: lgebhardt

.travis.yml

@@ -1,26 +1,31 @@
 language: ruby
 sudo: false
+services:
+  - postgresql
 env:
-  - "RAILS_VERSION=4.2.11"
-  - "RAILS_VERSION=5.0.7.2"
-  - "RAILS_VERSION=5.1.7"
-  - "RAILS_VERSION=5.2.3"
-  - "RAILS_VERSION=6.0.0"
-#  - "RAILS_VERSION=master"
+  - RAILS_VERSION=6.0.0 DATABASE_URL=postgres://postgres@localhost/jr_test
+  - RAILS_VERSION=6.0.0
+  - RAILS_VERSION=5.2.3 DATABASE_URL=postgres://postgres@localhost/jr_test
+  - RAILS_VERSION=5.2.3
+  - RAILS_VERSION=5.1.7
+  - RAILS_VERSION=5.0.7.2
+  - RAILS_VERSION=4.2.11
 rvm:
-  - 2.3.8
-  - 2.4.7
-  - 2.5.6
-  - 2.6.4
+  - 2.4.9
+  - 2.5.7
+  - 2.6.5
 matrix:
-  allow_failures:
-    - env: "RAILS_VERSION=master"
   exclude:
-    - rvm: 2.6.4
+    - rvm: 2.6.5
       env: "RAILS_VERSION=4.2.11"
-    - rvm: 2.3.8
-      env: "RAILS_VERSION=6.0.0"
-    - rvm: 2.4.7
+    - rvm: 2.4.9
       env: "RAILS_VERSION=6.0.0"
+    - rvm: 2.4.9
+      env: "RAILS_VERSION=6.0.0 DATABASE_URL=postgres://postgres@localhost/jr_test"
+    - rvm: 2.4.9
+      env: "RAILS_VERSION=5.2.3 DATABASE_URL=postgres://postgres@localhost/jr_test"
 before_install:
-  - gem install bundler --version 1.17.3
+  - gem install bundler --version 1.17.3
+before_script:
+  - sh -c "if [ '$DATABASE_URL' = 'postgres://postgres@localhost/jr_test' ]; then psql -c 'DROP DATABASE IF EXISTS jr_test;' -U postgres; fi"
+  - sh -c "if [ '$DATABASE_URL' = 'postgres://postgres@localhost/jr_test' ]; then psql -c 'CREATE DATABASE jr_test;' -U postgres; fi"

Gemfile

@@ -9,6 +9,8 @@ end
 version = ENV['RAILS_VERSION'] || 'default'
 
 platforms :ruby do
+  gem 'pg'
+
   if version.start_with?('4.2', '5.0')
     gem 'sqlite3', '~> 1.3.13'
   else

lib/jsonapi/active_relation_resource.rb

@@ -153,6 +153,11 @@ def find_fragments(filters, options = {})
           pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, model_field[:name])} AS #{resource_table_alias}_#{model_field[:name]}")
         end
 
+        sort_fields = options.dig(:_relation_helper_options, :sort_fields)
+        sort_fields.try(:each) do |field|
+          pluck_fields << Arel.sql(field)
+        end
+
         fragments = {}
         rows = records.pluck(*pluck_fields)
         rows.each do |row|
@@ -445,6 +450,11 @@ def find_related_monomorphic_fragments(source_rids, relationship, options, conne
           pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, model_field[:name])} AS #{resource_table_alias}_#{model_field[:name]}")
         end
 
+        sort_fields = options.dig(:_relation_helper_options, :sort_fields)
+        sort_fields.try(:each) do |field|
+          pluck_fields << Arel.sql(field)
+        end
+
         fragments = {}
         rows = records.distinct.pluck(*pluck_fields)
         rows.each do |row|
@@ -680,24 +690,23 @@ def apply_request_settings_to_records(records:,
                                             paginator: nil,
                                             options: {})
 
-        opts = options.dup
-        records = resource_klass.apply_joins(records, join_manager, opts)
+        options[:_relation_helper_options] = { join_manager: join_manager, sort_fields: [] }
+
+        records = resource_klass.apply_joins(records, join_manager, options)
 
         if primary_keys
           records = records.where(_primary_key => primary_keys)
         end
 
-        opts[:join_manager] = join_manager
-
         unless filters.empty?
-          records = resource_klass.filter_records(records, filters, opts)
+          records = resource_klass.filter_records(records, filters, options)
         end
 
         if sort_primary
           records = records.order(_primary_key => :asc)
         else
           order_options = resource_klass.construct_order_options(sort_criteria)
-          records = resource_klass.sort_records(records, order_options, opts)
+          records = resource_klass.sort_records(records, order_options, options)
         end
 
         if paginator
@@ -731,12 +740,16 @@ def apply_single_sort(records, field, direction, options)
 
         strategy = _allowed_sort.fetch(field.to_sym, {})[:apply]
 
+        options[:_relation_helper_options] ||= {}
+        options[:_relation_helper_options][:sort_fields] ||= []
+
         if strategy
           records = call_method_or_proc(strategy, records, direction, context)
         else
-          join_manager = options[:join_manager]
-
-          records = records.order(Arel.sql("#{get_aliased_field(field, join_manager)} #{direction}"))
+          join_manager = options.dig(:_relation_helper_options, :join_manager)
+          sort_field = join_manager ? get_aliased_field(field, join_manager) : field
+          options[:_relation_helper_options][:sort_fields].push("#{sort_field}")
+          records = records.order(Arel.sql("#{sort_field} #{direction}"))
         end
         records
       end
@@ -825,8 +838,9 @@ def apply_filter(records, filter, value, options = {})
         if strategy
           records = call_method_or_proc(strategy, records, value, options)
         else
-          join_manager = options[:join_manager]
-          records = records.where(Arel.sql(get_aliased_field(filter, join_manager)) => value)
+          join_manager = options.dig(:_relation_helper_options, :join_manager)
+          field = join_manager ? get_aliased_field(filter, join_manager) : filter
+          records = records.where(Arel.sql(field) => value)
         end
 
         records

test/config/database.yml

@@ -6,6 +6,7 @@ def set_content_type_header!
 
 class PostsControllerTest < ActionController::TestCase
   def setup
+    super
     JSONAPI.configuration.raise_if_parameters_not_allowed = true
     JSONAPI.configuration.always_include_to_one_linkage_data = false
   end
@@ -445,7 +446,7 @@ def test_sorting_asc
     assert_cacheable_get :index, params: {sort: 'title'}
 
     assert_response :success
-    assert_equal "A First Post", json_response['data'][0]['attributes']['title']
+    assert_equal "A 1ST Post", json_response['data'][0]['attributes']['title']
   end
 
   def test_sorting_desc
@@ -459,7 +460,7 @@ def test_sorting_by_multiple_fields
     assert_cacheable_get :index, params: {sort: 'title,body'}
 
     assert_response :success
-    assert_equal '14', json_response['data'][0]['id']
+    assert_equal '15', json_response['data'][0]['id']
   end
 
   def create_alphabetically_first_user_and_post
@@ -473,8 +474,15 @@ def test_sorting_by_relationship_field
 
     assert_response :success
     assert json_response['data'].length > 10, 'there are enough records to show sort'
-    assert_equal '17', json_response['data'][0]['id'], 'nil is at the top'
-    assert_equal post.id.to_s, json_response['data'][1]['id'], 'alphabetically first user is second'
+
+    # Postgres sorts nulls last, whereas sqlite and mysql sort nulls first
+    if ENV['DATABASE_URL'].starts_with?('postgres')
+      assert_equal '17', json_response['data'][-1]['id'], 'nil is at the start'
+      assert_equal post.id.to_s, json_response['data'][0]['id'], 'alphabetically first user is not first'
+    else
+      assert_equal '17', json_response['data'][0]['id'], 'nil is at the end'
+      assert_equal post.id.to_s, json_response['data'][1]['id'], 'alphabetically first user is second'
+    end
   end
 
   def test_desc_sorting_by_relationship_field
@@ -483,8 +491,15 @@ def test_desc_sorting_by_relationship_field
 
     assert_response :success
     assert json_response['data'].length > 10, 'there are enough records to show sort'
-    assert_equal '17', json_response['data'][-1]['id'], 'nil is at the bottom'
-    assert_equal post.id.to_s, json_response['data'][-2]['id'], 'alphabetically first user is second last'
+
+    # Postgres sorts nulls last, whereas sqlite and mysql sort nulls first
+    if ENV['DATABASE_URL'].starts_with?('postgres')
+      assert_equal '17', json_response['data'][0]['id'], 'nil is at the start'
+      assert_equal post.id.to_s, json_response['data'][-1]['id']
+    else
+      assert_equal '17', json_response['data'][-1]['id'], 'nil is at the end'
+      assert_equal post.id.to_s, json_response['data'][-2]['id'], 'alphabetically first user is second last'
+    end
   end
 
   def test_sorting_by_relationship_field_include
@@ -493,8 +508,14 @@ def test_sorting_by_relationship_field_include
 
     assert_response :success
     assert json_response['data'].length > 10, 'there are enough records to show sort'
-    assert_equal '17', json_response['data'][0]['id'], 'nil is at the top'
-    assert_equal post.id.to_s, json_response['data'][1]['id'], 'alphabetically first user is second'
+
+    if ENV['DATABASE_URL'].starts_with?('postgres')
+      assert_equal '17', json_response['data'][-1]['id'], 'nil is at the top'
+      assert_equal post.id.to_s, json_response['data'][0]['id']
+    else
+      assert_equal '17', json_response['data'][0]['id'], 'nil is at the top'
+      assert_equal post.id.to_s, json_response['data'][1]['id'], 'alphabetically first user is second'
+    end
   end
 
   def test_invalid_sort_param
@@ -3107,7 +3128,7 @@ def test_type_formatting
     assert json_response['data'].is_a?(Hash)
     assert_equal 'Jane Author', json_response['data']['attributes']['spouseName']
     assert_equal 'First man to run across Antartica.', json_response['data']['attributes']['bio']
-    assert_equal 23.89/45.6, json_response['data']['attributes']['qualityRating']
+    assert_equal (23.89/45.6).round(5), json_response['data']['attributes']['qualityRating'].round(5)
     assert_equal '47000.56', json_response['data']['attributes']['salary']
     assert_equal '2013-08-07T20:25:00.000Z', json_response['data']['attributes']['dateTimeJoined']
     assert_equal '1965-06-30', json_response['data']['attributes']['birthday']
@@ -4707,7 +4728,12 @@ def test_fetch_robots_with_sort_by_name
     Robot.create! name: 'jane', version: 1
     assert_cacheable_get :index, params: {sort: 'name'}
     assert_response :success
-    assert_equal 'John', json_response['data'].first['attributes']['name']
+
+    if ENV['DATABASE_URL'].starts_with?('postgres')
+      assert_equal 'jane', json_response['data'].first['attributes']['name']
+    else
+      assert_equal 'John', json_response['data'].first['attributes']['name']
+    end
   end
 
   def test_fetch_robots_with_sort_by_lower_name

test/controllers/controller_test.rb

@@ -1650,7 +1650,7 @@ class CraterResource < JSONAPI::Resource
 
   filter :description, apply: -> (records, value, options) {
     fail "context not set" unless options[:context][:current_user] != nil && options[:context][:current_user] == $test_user
-    records.where(concat_table_field(options[:join_manager].source_join_details[:alias], :description) => value)
+    records.where(concat_table_field(options.dig(:_relation_helper_options, :join_manager).source_join_details[:alias], :description) => value)
   }
 
   def self.verify_key(key, context = nil)
@@ -1694,7 +1694,7 @@ class PictureResource < JSONAPI::Resource
   has_one :file_properties, inverse_relationship: :fileable, :foreign_key_on => :related, polymorphic: true
 
   filter 'imageable.name', perform_joins: true, apply: -> (records, value, options) {
-    join_manager = options[:join_manager]
+    join_manager = options.dig(:_relation_helper_options, :join_manager)
     relationship = _relationship(:imageable)
     or_parts = relationship.resource_types.collect do |type|
       table_alias = join_manager.join_details_by_polymorphic_relationship(relationship, type)[:alias]
@@ -2038,7 +2038,7 @@ class AuthorResource < JSONAPI::Resource
       relationship :author_detail, to: :one, foreign_key_on: :related
 
       filter :name, apply: lambda { |records, value, options|
-        table_alias = options[:join_manager].source_join_details[:alias]
+        table_alias = options.dig(:_relation_helper_options, :join_manager).source_join_details[:alias]
         t = Arel::Table.new(:people, as: table_alias)
         records.where(t[:name].matches("%#{value[0]}%"))
       }

test/fixtures/active_record.rb

@@ -85,7 +85,7 @@ post_14:
 
 post_15:
   id: 15
-  title: AAAA First Post
+  title: A 1ST Post
   body: First!!!!!!!!!
   author_id: 1003
 

test/fixtures/posts.yml

@@ -1438,13 +1438,16 @@ def test_sort_primary_attribute
   end
 
   def test_sort_included_attribute
+    # Postgres sorts nulls last, whereas sqlite and mysql sort nulls first
+    pg = ENV['DATABASE_URL'].starts_with?('postgres')
+
     get '/api/v6/authors?sort=author_detail.author_stuff', headers: { 'Accept' => JSONAPI::MEDIA_TYPE }
     assert_jsonapi_response 200
-    assert_equal '1000', json_response['data'][0]['id']
+    assert_equal pg ? '1001' : '1000', json_response['data'][0]['id']
 
     get '/api/v6/authors?sort=-author_detail.author_stuff', headers: { 'Accept' => JSONAPI::MEDIA_TYPE }
     assert_jsonapi_response 200
-    assert_equal '1002', json_response['data'][0]['id']
+    assert_equal pg ? '1000' : '1002', json_response['data'][0]['id']
   end
 
   def test_include_parameter_quoted

test/integration/requests/request_test.rb

@@ -21,6 +21,8 @@
   end
 end
 
+ENV['DATABASE_URL'] ||= "sqlite3:test_db"
+
 require 'active_record/railtie'
 require 'rails/test_help'
 require 'minitest/mock'
@@ -68,6 +70,9 @@ class TestApp < Rails::Application
   end
 end
 
+DatabaseCleaner.allow_remote_database_url = true
+DatabaseCleaner.strategy = :transaction
+
 module MyEngine
   class Engine < ::Rails::Engine
     isolate_namespace MyEngine
@@ -477,8 +482,6 @@ class CatResource < JSONAPI::Resource
   jsonapi_resources :people
 end
 
-DatabaseCleaner.strategy = :transaction
-
 # Ensure backward compatibility with Minitest 4
 Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)