Merge pull request #1262 from cerebris/linkbuilder_track_routes_0_9

lgebhardt · web-flow · commit 0a344a0d196c · 2019-06-26T07:46:38.000-04:00
Build Links with strings, track route setup on resource and relationships
diff --git a/lib/jsonapi/acts_as_resource_controller.rb b/lib/jsonapi/acts_as_resource_controller.rb
@@ -125,7 +125,8 @@ def resource_serializer
         base_url: base_url,
         key_formatter: key_formatter,
         route_formatter: route_formatter,
-        serialization_options: serialization_options
+        serialization_options: serialization_options,
+        controller: self
       )
       @resource_serializer
     end
diff --git a/lib/jsonapi/link_builder.rb b/lib/jsonapi/link_builder.rb
@@ -2,74 +2,85 @@ module JSONAPI
   class LinkBuilder
     attr_reader :base_url,
                 :primary_resource_klass,
+                :route_formatter,
                 :engine,
-                :routes
+                :engine_mount_point,
+                :url_helpers
+
+    @@url_helper_methods = {}
 
     def initialize(config = {})
-      @base_url               = config[:base_url]
+      @base_url = config[:base_url]
       @primary_resource_klass = config[:primary_resource_klass]
-      @engine                 = build_engine
-
-      if engine?
-        @routes = @engine.routes
-      else
-        @routes = Rails.application.routes
-      end
+      @route_formatter = config[:route_formatter]
+      @engine = build_engine
+      @engine_mount_point = @engine ? @engine.routes.find_script_name({}) : ""
 
-      # ToDo: Use NaiveCache for values. For this we need to not return nils and create composite keys which work
-      # as efficient cache lookups. This could be an array of the [source.identifier, relationship] since the
-      # ResourceIdentity will compare equality correctly
+      # url_helpers may be either a controller which has the route helper methods, or the application router's
+      # url helpers module, `Rails.application.routes.url_helpers`. Because the method no longer behaves as a
+      # singleton, and it's expensive to generate the module, the controller is preferred.
+      @url_helpers = config[:url_helpers]
     end
 
     def engine?
       !!@engine
     end
 
     def primary_resources_url
-      @primary_resources_url_cached ||= "#{ base_url }#{ primary_resources_path }"
-    rescue NoMethodError
-      warn "primary_resources_url for #{@primary_resource_klass} could not be generated" if JSONAPI.configuration.warn_on_missing_routes
+      if @primary_resource_klass._routed
+        primary_resources_path = resources_path(primary_resource_klass)
+        @primary_resources_url_cached ||= "#{ base_url }#{ engine_mount_point }#{ primary_resources_path }"
+      else
+        if JSONAPI.configuration.warn_on_missing_routes && !@primary_resource_klass._warned_missing_route
+          warn "primary_resources_url for #{@primary_resource_klass} could not be generated"
+          @primary_resource_klass._warned_missing_route = true
+        end
+        nil
+      end
     end
 
     def query_link(query_params)
-      "#{ primary_resources_url }?#{ query_params.to_query }"
+      url = primary_resources_url
+      return url if url.nil?
+      "#{ url }?#{ query_params.to_query }"
     end
 
     def relationships_related_link(source, relationship, query_params = {})
-      if relationship.parent_resource.singleton?
-        url_helper_name = singleton_related_url_helper_name(relationship)
-        url = call_url_helper(url_helper_name)
+      if relationship._routed
+        url = "#{ self_link(source) }/#{ route_for_relationship(relationship) }"
+        url = "#{ url }?#{ query_params.to_query }" if query_params.present?
+        url
       else
-        url_helper_name = related_url_helper_name(relationship)
-        url = call_url_helper(url_helper_name, source.id)
+        if JSONAPI.configuration.warn_on_missing_routes && !relationship._warned_missing_route
+          warn "related_link for #{relationship} could not be generated"
+          relationship._warned_missing_route = true
+        end
+        nil
       end
-
-      url = "#{ base_url }#{ url }"
-      url = "#{ url }?#{ query_params.to_query }" if query_params.present?
-      url
-    rescue NoMethodError
-      warn "related_link for #{relationship} could not be generated" if JSONAPI.configuration.warn_on_missing_routes
     end
 
     def relationships_self_link(source, relationship)
-      if relationship.parent_resource.singleton?
-        url_helper_name = singleton_relationship_self_url_helper_name(relationship)
-        url = call_url_helper(url_helper_name)
+      if relationship._routed
+        "#{ self_link(source) }/relationships/#{ route_for_relationship(relationship) }"
       else
-        url_helper_name = relationship_self_url_helper_name(relationship)
-        url = call_url_helper(url_helper_name, source.id)
+        if JSONAPI.configuration.warn_on_missing_routes && !relationship._warned_missing_route
+          warn "self_link for #{relationship} could not be generated"
+          relationship._warned_missing_route = true
+        end
+        nil
       end
-
-      url = "#{ base_url }#{ url }"
-      url
-    rescue NoMethodError
-      warn "self_link for #{relationship} could not be generated" if JSONAPI.configuration.warn_on_missing_routes
     end
 
     def self_link(source)
-      "#{ base_url }#{ resource_path(source) }"
-    rescue NoMethodError
-      warn "self_link for #{source.class} could not be generated" if JSONAPI.configuration.warn_on_missing_routes
+      if source.class._routed
+        resource_url(source)
+      else
+        if JSONAPI.configuration.warn_on_missing_routes && !source.class._warned_missing_route
+          warn "self_link for #{source.class} could not be generated"
+          source.class._warned_missing_route = true
+        end
+        nil
+      end
     end
 
     private
@@ -81,105 +92,55 @@ def build_engine
         unless scopes.empty?
           "#{ scopes.first.to_s.camelize }::Engine".safe_constantize
         end
+
           # :nocov:
       rescue LoadError => _e
         nil
         # :nocov:
       end
     end
 
-    def call_url_helper(method, *args)
-      routes.url_helpers.public_send(method, args)
-    rescue NoMethodError => e
-      raise e
+    def format_route(route)
+      route_formatter.format(route)
     end
 
-    def path_from_resource_class(klass)
-      url_helper_name = resources_url_helper_name_from_class(klass)
-      call_url_helper(url_helper_name)
-    end
+    def formatted_module_path_from_class(klass)
+      scopes = if @engine
+                 module_scopes_from_class(klass)[1..-1]
+               else
+                 module_scopes_from_class(klass)
+               end
 
-    def resource_path(source)
-      url_helper_name = resource_url_helper_name_from_source(source)
-      if source.class.singleton?
-        call_url_helper(url_helper_name)
+      unless scopes.empty?
+        "/#{ scopes.map {|scope| format_route(scope.to_s.underscore)}.compact.join('/') }/"
       else
-        call_url_helper(url_helper_name, source.id)
+        "/"
       end
     end
 
-    def primary_resources_path
-      path_from_resource_class(primary_resource_klass)
+    def module_scopes_from_class(klass)
+      klass.name.to_s.split("::")[0...-1]
     end
 
-    def url_helper_name_from_parts(parts)
-      (parts << "path").reject(&:blank?).join("_")
+    def resources_path(source_klass)
+      formatted_module_path_from_class(source_klass) + format_route(source_klass._type.to_s)
     end
 
-    def resources_path_parts_from_class(klass)
-      if engine?
-        scopes = module_scopes_from_class(klass)[1..-1]
-      else
-        scopes = module_scopes_from_class(klass)
-      end
-
-      base_path_name = scopes.map { |scope| scope.underscore }.join("_")
-      end_path_name  = klass._type.to_s
-      [base_path_name, end_path_name]
-    end
-
-    def resources_url_helper_name_from_class(klass)
-      url_helper_name_from_parts(resources_path_parts_from_class(klass))
-    end
+    def resource_path(source)
+      url = "#{resources_path(source.class)}"
 
-    def resource_path_parts_from_class(klass)
-      if engine?
-        scopes = module_scopes_from_class(klass)[1..-1]
-      else
-        scopes = module_scopes_from_class(klass)
+      unless source.class.singleton?
+        url = "#{url}/#{source.id}"
       end
-
-      base_path_name = scopes.map { |scope| scope.underscore }.join("_")
-      end_path_name  = klass._type.to_s.singularize
-      [base_path_name, end_path_name]
-    end
-
-    def resource_url_helper_name_from_source(source)
-      url_helper_name_from_parts(resource_path_parts_from_class(source.class))
-    end
-
-    def related_url_helper_name(relationship)
-      relationship_parts = resource_path_parts_from_class(relationship.parent_resource)
-      relationship_parts << "related"
-      relationship_parts << relationship.name
-      url_helper_name_from_parts(relationship_parts)
-    end
-
-    def singleton_related_url_helper_name(relationship)
-      relationship_parts = []
-      relationship_parts << "related"
-      relationship_parts << relationship.name
-      relationship_parts += resource_path_parts_from_class(relationship.parent_resource)
-      url_helper_name_from_parts(relationship_parts)
-    end
-
-    def relationship_self_url_helper_name(relationship)
-      relationship_parts = resource_path_parts_from_class(relationship.parent_resource)
-      relationship_parts << "relationships"
-      relationship_parts << relationship.name
-      url_helper_name_from_parts(relationship_parts)
+      url
     end
 
-    def singleton_relationship_self_url_helper_name(relationship)
-      relationship_parts = []
-      relationship_parts << "relationships"
-      relationship_parts << relationship.name
-      relationship_parts += resource_path_parts_from_class(relationship.parent_resource)
-      url_helper_name_from_parts(relationship_parts)
+    def resource_url(source)
+      "#{ base_url }#{ engine_mount_point }#{ resource_path(source) }"
     end
 
-    def module_scopes_from_class(klass)
-      klass.name.to_s.split("::")[0...-1]
+    def route_for_relationship(relationship)
+      format_route(relationship.name)
     end
   end
 end
diff --git a/lib/jsonapi/relationship.rb b/lib/jsonapi/relationship.rb
@@ -4,6 +4,8 @@ class Relationship
                 :class_name, :polymorphic, :always_include_linkage_data,
                 :parent_resource, :eager_load_on_include
 
+    attr_accessor :_routed, :_warned_missing_route
+
     def initialize(name, options = {})
       @name = name.to_s
       @options = options
@@ -15,6 +17,9 @@ def initialize(name, options = {})
       @always_include_linkage_data = options.fetch(:always_include_linkage_data, false) == true
       @eager_load_on_include = options.fetch(:eager_load_on_include, true) == true
 
+      @_routed = false
+      @_warned_missing_route = false
+
       exclude_links(options.fetch(:exclude_links, :none))
     end
 
diff --git a/lib/jsonapi/resource.rb b/lib/jsonapi/resource.rb
@@ -457,6 +457,9 @@ def inherited(subclass)
         end
 
         check_reserved_resource_name(subclass._type, subclass.name)
+
+        subclass._routed = false
+        subclass._warned_missing_route = false
       end
 
       def rebuild_relationships(relationships)
@@ -502,7 +505,7 @@ def resource_type_for(model)
         end
       end
 
-      attr_accessor :_attributes, :_relationships, :_type, :_model_hints
+      attr_accessor :_attributes, :_relationships, :_type, :_model_hints, :_routed, :_warned_missing_route
       attr_writer :_allowed_filters, :_paginator
 
       def create(context)
diff --git a/lib/jsonapi/resource_serializer.rb b/lib/jsonapi/resource_serializer.rb
@@ -539,6 +539,8 @@ def generate_link_builder(primary_resource_klass, options)
       LinkBuilder.new(
         base_url: options.fetch(:base_url, ''),
         primary_resource_klass: primary_resource_klass,
+        route_formatter: options.fetch(:route_formatter, JSONAPI.configuration.route_formatter),
+        url_helpers: options.fetch(:url_helpers, options[:controller]),
       )
     end
   end
diff --git a/lib/jsonapi/routing_ext.rb b/lib/jsonapi/routing_ext.rb
@@ -20,6 +20,8 @@ def jsonapi_resource(*resources, &_block)
           @resource_type = resources.first
           res = JSONAPI::Resource.resource_for(resource_type_with_module_prefix(@resource_type))
 
+          res._routed = true
+
           unless res.singleton?
             warn "Singleton routes created for non singleton resource #{res}. Links may not be generated correctly."
           end
@@ -84,6 +86,8 @@ def jsonapi_resources(*resources, &_block)
           @resource_type = resources.first
           res = JSONAPI::Resource.resource_for(resource_type_with_module_prefix(@resource_type))
 
+          res._routed = true
+
           if res.singleton?
             warn "Singleton resource #{res} should use `jsonapi_resource` instead."
           end
@@ -223,6 +227,8 @@ def jsonapi_related_resource(*relationship)
           relationship_name = relationship.first
           relationship = source._relationships[relationship_name]
 
+          relationship._routed = true
+
           formatted_relationship_name = format_route(relationship.name)
 
           if relationship.polymorphic?
@@ -245,6 +251,8 @@ def jsonapi_related_resources(*relationship)
           relationship_name = relationship.first
           relationship = source._relationships[relationship_name]
 
+          relationship._routed = true
+
           formatted_relationship_name = format_route(relationship.name)
           related_resource = JSONAPI::Resource.resource_for(resource_type_with_module_prefix(relationship.class_name.underscore))
           options[:controller] ||= related_resource._type.to_s
diff --git a/test/fixtures/active_record.rb b/test/fixtures/active_record.rb
@@ -1681,7 +1681,9 @@ module V4
     class PostResource < PostResource; end
     class PersonResource < PersonResource; end
     class ExpenseEntryResource < ExpenseEntryResource; end
-    class IsoCurrencyResource < IsoCurrencyResource; end
+    class IsoCurrencyResource < IsoCurrencyResource
+      has_many :expense_entries, exclude_links: :default
+    end
 
     class BookResource < Api::V2::BookResource
       paginator :paged
@@ -1987,6 +1989,7 @@ class PersonResource < JSONAPI::Resource
 
 module ApiV2Engine
   class PostResource < PostResource
+    has_one :person
   end
 
   class PersonResource < JSONAPI::Resource
diff --git a/test/integration/requests/request_test.rb b/test/integration/requests/request_test.rb
@@ -92,6 +92,7 @@ def test_get_camelized_route_and_key_filtered
   def test_get_camelized_route_and_links
     original_config = JSONAPI.configuration.dup
     JSONAPI.configuration.json_key_format = :camelized_key
+    JSONAPI.configuration.route_format = :camelized_route
     assert_cacheable_jsonapi_get '/api/v4/expenseEntries/1/relationships/isoCurrency'
     assert_hash_equals({'links' => {
                          'self' => 'http://www.example.com/api/v4/expenseEntries/1/relationships/isoCurrency',
diff --git a/test/unit/serializer/link_builder_test.rb b/test/unit/serializer/link_builder_test.rb
diff --git a/test/unit/serializer/polymorphic_serializer_test.rb b/test/unit/serializer/polymorphic_serializer_test.rb
diff --git a/test/unit/serializer/response_document_test.rb b/test/unit/serializer/response_document_test.rb
diff --git a/test/unit/serializer/serializer_test.rb b/test/unit/serializer/serializer_test.rb

Original file line number	Diff line number	Diff line change
`@@ -539,6 +539,8 @@ def generate_link_builder(primary_resource_klass, options)`
`539`	`539`	`LinkBuilder.new(`
`540`	`540`	`base_url: options.fetch(:base_url, ''),`
`541`	`541`	`primary_resource_klass: primary_resource_klass,`
	`542`	`+ route_formatter: options.fetch(:route_formatter, JSONAPI.configuration.route_formatter),`
	`543`	`+ url_helpers: options.fetch(:url_helpers, options[:controller]),`
`542`	`544`	`)`
`543`	`545`	`end`
`544`	`546`	`end`