Merge pull request #1112 from senid231/1094-fix-nested-relationship-sorting

Fixes #1094 #1097 fix sorting by nested relationships

Author: lgebhardt

lib/jsonapi/active_relation_resource_finder.rb

@@ -3,7 +3,7 @@ module ActiveRelationResourceFinder
     def self.included(base)
       base.extend ClassMethods
     end
-    
+
     module ClassMethods
 
       # Finds Resources using the `filters`. Pagination and sort options are used when provided
@@ -387,7 +387,7 @@ def apply_pagination(records, paginator, order_options)
         records
       end
 
-      def apply_sort(records, order_options, context = {})
+      def apply_sort(records, order_options, _context = {})
         if order_options.any?
           order_options.each_pair do |field, direction|
             if field.to_s.include?(".")
@@ -396,8 +396,7 @@ def apply_sort(records, order_options, context = {})
               associations = _lookup_association_chain([records.model.to_s, *model_names])
               joins_query = _build_joins([records.model, *associations])
 
-              # _sorting is appended to avoid name clashes with manual joins eg. overridden filters
-              order_by_query = "#{associations.last.name}_sorting.#{column_name} #{direction}"
+              order_by_query = "#{_join_table_name(associations.last)}.#{column_name} #{direction}"
               records = records.joins(joins_query).order(order_by_query)
             else
               field = _attribute_delegated_name(field)
@@ -423,17 +422,28 @@ def _build_joins(associations)
         joins = []
 
         associations.inject do |prev, current|
-          if current.has_one?
-            joins << "LEFT JOIN #{current.table_name} AS #{current.name}_sorting ON #{current.name}_sorting.#{current.foreign_key} = #{prev.table_name}.id"
+          prev_table_name = _join_table_name(prev)
+          curr_table_name = _join_table_name(current)
+          if current.belongs_to?
+            joins << "LEFT JOIN #{current.table_name} AS #{curr_table_name} ON #{curr_table_name}.id = #{prev_table_name}.#{current.foreign_key}"
           else
-            joins << "LEFT JOIN #{current.table_name} AS #{current.name}_sorting ON #{current.name}_sorting.id = #{prev.table_name}.#{current.foreign_key}"
+            joins << "LEFT JOIN #{current.table_name} AS #{curr_table_name} ON #{curr_table_name}.#{current.foreign_key} = #{prev_table_name}.id"
           end
 
           current
         end
         joins.join("\n")
       end
 
+      # _sorting is appended to avoid name clashes with manual joins eg. overridden filters
+      def _join_table_name(association)
+        if association.is_a?(ActiveRecord::Reflection::AssociationReflection)
+          "#{association.name}_sorting"
+        else
+          association.table_name
+        end
+      end
+
       # Assumes ActiveRecord's counting. Override if you need a different counting method
       def count_records(records)
         records.count(:all)

test/controllers/controller_test.rb

@@ -3911,3 +3911,45 @@ def test_fields_with_delegated_attribute
     JSONAPI.configuration = original_config
   end
 end
+
+class WidgetsControllerTest < ActionController::TestCase
+  def teardown
+    Widget.delete_all
+    Indicator.delete_all
+    Agency.delete_all
+  end
+
+  def test_fetch_widgets_sort_by_agency_name
+    agency_1 = Agency.create! name: 'beta'
+    agency_2 = Agency.create! name: 'alpha'
+    indicator_1 = Indicator.create! name: 'bar', agency: agency_1
+    indicator_2 = Indicator.create! name: 'foo', agency: agency_2
+    Widget.create! name: 'bar', indicator: indicator_1
+    widget = Widget.create! name: 'foo', indicator: indicator_2
+    assert_cacheable_get :index, params: {sort: 'indicator.agency.name'}
+    assert_response :success
+    assert_equal widget.id.to_s, json_response['data'].first['id']
+  end
+end
+
+class IndicatorsControllerTest < ActionController::TestCase
+  def teardown
+    Widget.delete_all
+    Indicator.delete_all
+    Agency.delete_all
+  end
+
+  def test_fetch_indicators_sort_by_widgets_name
+    agency = Agency.create! name: 'test'
+    indicator_1 = Indicator.create! name: 'bar', agency: agency
+    indicator_2 = Indicator.create! name: 'foo', agency: agency
+    Widget.create! name: 'omega', indicator: indicator_1
+    Widget.create! name: 'beta', indicator: indicator_1
+    Widget.create! name: 'alpha', indicator: indicator_2
+    Widget.create! name: 'zeta', indicator: indicator_2
+    assert_cacheable_get :index, params: {sort: 'widgets.name'}
+    assert_response :success
+    assert_equal indicator_2.id.to_s, json_response['data'].first['id']
+    assert_equal 2, json_response['data'].size
+  end
+end

test/fixtures/active_record.rb

@@ -333,6 +333,23 @@
     t.integer :access_card_id, null: false
     t.timestamps null: false
   end
+
+  create_table :agencies, force: true do |t|
+    t.string :name
+    t.timestamps null: false
+  end
+
+  create_table :indicators, force: true do |t|
+    t.string :name
+    t.integer :agency_id, null: false
+    t.timestamps null: false
+  end
+
+  create_table :widgets, force: true do |t|
+    t.string :name
+    t.integer :indicator_id, null: false
+    t.timestamps null: false
+  end
 end
 
 ### MODELS
@@ -368,7 +385,7 @@ class Post < ActiveRecord::Base
   has_many :special_post_tags, source: :tag
   has_many :special_tags, through: :special_post_tags, source: :tag
   belongs_to :section
-  has_one :parent_post, class_name: 'Post', foreign_key: 'parent_post_id'
+  belongs_to :parent_post, class_name: 'Post', foreign_key: 'parent_post_id'
 
   validates :author, presence: true
   validates :title, length: { maximum: 35 }
@@ -696,6 +713,18 @@ class Worker < ActiveRecord::Base
   belongs_to :access_card
 end
 
+class Agency < ActiveRecord::Base
+end
+
+class Indicator < ActiveRecord::Base
+  belongs_to :agency
+  has_many :widgets
+end
+
+class Widget < ActiveRecord::Base
+  belongs_to :indicator
+end
+
 ### CONTROLLERS
 class AuthorsController < JSONAPI::ResourceControllerMetal
 end
@@ -986,6 +1015,12 @@ class AccessCardsController < BaseController
 class WorkersController < BaseController
 end
 
+class WidgetsController < JSONAPI::ResourceController
+end
+
+class IndicatorsController < JSONAPI::ResourceController
+end
+
 ### RESOURCES
 class BaseResource < JSONAPI::Resource
   abstract
@@ -1992,6 +2027,29 @@ class BlogPostResource < JSONAPI::Resource
   filter :name
 end
 
+class AgencyResource < JSONAPI::Resource
+  attributes :name
+end
+
+class IndicatorResource < JSONAPI::Resource
+  attributes :name
+  has_one :agency
+  has_many :widgets
+
+  def self.sortable_fields(_context = nil)
+    super + [:'widgets.name']
+  end
+end
+
+class WidgetResource < JSONAPI::Resource
+  attributes :name
+  has_one :indicator
+
+  def self.sortable_fields(_context = nil)
+    super + [:'indicator.agency.name']
+  end
+end
+
 # CustomProcessors
 class Api::V4::BookProcessor < JSONAPI::Processor
   after_find do

test/test_helper.rb

@@ -398,6 +398,8 @@ class CatResource < JSONAPI::Resource
   jsonapi_resources :keepers, only: [:show]
   jsonapi_resources :storages
   jsonapi_resources :workers, only: [:show]
+  jsonapi_resources :widgets, only: [:index]
+  jsonapi_resources :indicators, only: [:index]
 
   mount MyEngine::Engine => "/boomshaka", as: :my_engine
   mount ApiV2Engine::Engine => "/api_v2", as: :api_v2_engine

test/unit/resource/resource_test.rb

@@ -345,7 +345,7 @@ def apply_sort(records, order_options, context = {})
 
   def test_lookup_association_chain
     model_names = %w(person posts parent_post)
-    result = PostResource._lookup_association_chain(model_names)
+    result = PersonResource._lookup_association_chain(model_names)
     assert_equal 2, result.length
 
     posts_reflection, parent_post_reflection = result
@@ -361,12 +361,15 @@ def test_lookup_association_chain
 
   def test_build_joins
     model_names = %w(person posts parent_post author author_detail)
-    associations = PostResource._lookup_association_chain(model_names)
-    result = PostResource.send(:_build_joins, associations)
-
-    sql = "LEFT JOIN posts AS parent_post_sorting ON parent_post_sorting.parent_post_id = posts.id
-LEFT JOIN people AS author_sorting ON author_sorting.id = posts.author_id
-LEFT JOIN author_details AS author_detail_sorting ON author_detail_sorting.person_id = people.id"
+    associations = PersonResource._lookup_association_chain(model_names)
+    result = PersonResource.send(:_build_joins, [Person, *associations])
+
+    sql = [
+        'LEFT JOIN posts AS posts_sorting ON posts_sorting.author_id = people.id',
+        'LEFT JOIN posts AS parent_post_sorting ON parent_post_sorting.id = posts_sorting.parent_post_id',
+        'LEFT JOIN people AS author_sorting ON author_sorting.id = parent_post_sorting.author_id',
+        'LEFT JOIN author_details AS author_detail_sorting ON author_detail_sorting.person_id = author_sorting.id'
+    ].join("\n")
 
     assert_equal sql, result
   end