Fix include_optional_linkage_data with joins (#1450)

When the user adds a join in `records`, it confuses `get_join_arel_node`
because it finds the same number of joins before & after adding a join
for `include_optional_linkage_data` (or equivalently for
`always_include_*_linkage_data`).

This commit falls back to searching the existing arel nodes for a
compatible join and uses that if found.

Author: pjungwir

lib/jsonapi/active_relation/join_manager.rb

@@ -70,7 +70,7 @@ def join_details_by_relationship(relationship)
         @join_details[segment]
       end
 
-      def self.get_join_arel_node(records, options = {})
+      def self.get_join_arel_node(records, relationship, join_type, options = {})
         init_join_sources = records.arel.join_sources
         init_join_sources_length = init_join_sources.length
 
@@ -80,9 +80,27 @@ def self.get_join_arel_node(records, options = {})
         if join_sources.length > init_join_sources_length
           last_join = (join_sources - init_join_sources).last
         else
+          # Try to find a pre-existing join for this table.
+          # We can get here if include_optional_linkage_data is true
+          # (or always_include_to_xxx_linkage_data),
+          # and the user's custom `records` method has already added that join.
+          #
+          # If we want a left join and there is already an inner/left join,
+          # then we can use that.
+          # If we want an inner join and there is alrady an inner join,
+          # then we can use that (but not a left join, since that doesn't filter things out).
+          valid_join_types = [Arel::Nodes::InnerJoin]
+          valid_join_types << Arel::Nodes::OuterJoin if join_type == :left
+          table_name = relationship.resource_klass._table_name
+
+          last_join = join_sources.find { |j|
+            valid_join_types.any? { |t| j.is_a?(t) } && j.left.name == table_name
+          }
+        end
+
+        if last_join.nil?
           # :nocov:
           warn "get_join_arel_node: No join added"
-          last_join = nil
           # :nocov:
         end
 
@@ -154,7 +172,7 @@ def perform_joins(records, options)
               next
             end
 
-            records, join_node = self.class.get_join_arel_node(records, options) {|records, options|
+            records, join_node = self.class.get_join_arel_node(records, relationship, join_type, options) {|records, options|
               related_resource_klass.join_relationship(
                 records: records,
                 resource_type: related_resource_klass._type,
@@ -294,4 +312,4 @@ def add_relationships(relationships)
       end
     end
   end
-end
+end

test/fixtures/active_record.rb

@@ -1059,6 +1059,8 @@ def context
   module V3
     class PostsController < JSONAPI::ResourceController
     end
+    class MoonsController < JSONAPI::ResourceController
+    end
   end
 
   module V4
@@ -2042,6 +2044,15 @@ module Api
   module V3
     class PostResource < PostResource; end
     class PreferencesResource < PreferencesResource; end
+    class PlanetResource < JSONAPI::Resource
+    end
+    class MoonResource < JSONAPI::Resource
+      has_one :planet, always_include_optional_linkage_data: true
+
+      def self.records(options = {})
+        Moon.joins(:planet).merge(Planet.where(name: 'Satern')) # sic
+      end
+    end
   end
 end
 

test/integration/requests/request_test.rb

@@ -1855,4 +1855,10 @@ def test_destroy_singleton_not_found
     JSONAPI.configuration = original_config
     $test_user = $original_test_user
   end
+
+  def test_include_optional_linkage_data_with_join
+    get "/api/v3/moons", headers: { 'Accept' => JSONAPI::MEDIA_TYPE }
+    assert_jsonapi_response 200
+    refute_nil json_response['data'][0]['relationships']['planet']
+  end
 end

test/test_helper.rb

@@ -312,6 +312,9 @@ class CatResource < JSONAPI::Resource
         jsonapi_link :author, except: [:destroy]
         jsonapi_links :tags, only: [:show, :create]
       end
+
+      jsonapi_resources :planets
+      jsonapi_resources :moons
     end
 
     JSONAPI.configuration.route_format = :camelized_route