From 2cd1dc85576937dd55de007a1be2a70ddb70a752 Mon Sep 17 00:00:00 2001 From: Larry Gebhardt Date: Wed, 17 Apr 2024 10:42:55 -0400 Subject: [PATCH] Warn on missing inverse relationships (#1451) --- lib/jsonapi/active_relation_retrieval.rb | 25 +++++++++--------- lib/jsonapi/active_relation_retrieval_v09.rb | 4 ++- lib/jsonapi/active_relation_retrieval_v10.rb | 12 +++------ lib/jsonapi/configuration.rb | 11 ++++++++ lib/jsonapi/link_builder.rb | 8 +++--- lib/jsonapi/relationship.rb | 27 ++++++++++++++++---- lib/jsonapi/resource_identity.rb | 2 +- test/unit/serializer/link_builder_test.rb | 2 +- 8 files changed, 59 insertions(+), 32 deletions(-) diff --git a/lib/jsonapi/active_relation_retrieval.rb b/lib/jsonapi/active_relation_retrieval.rb index 2ee2e026..88f886c8 100644 --- a/lib/jsonapi/active_relation_retrieval.rb +++ b/lib/jsonapi/active_relation_retrieval.rb @@ -304,10 +304,10 @@ def find_included_fragments(source_fragments, relationship, options) end def find_related_fragments_from_inverse(source, source_relationship, options, connect_source_identity) - relationship = source_relationship.resource_klass._relationship(source_relationship.inverse_relationship) - raise "missing inverse relationship" unless relationship.present? + inverse_relationship = source_relationship._inverse_relationship + return {} if inverse_relationship.blank? - parent_resource_klass = relationship.resource_klass + parent_resource_klass = inverse_relationship.resource_klass include_directives = options.fetch(:include_directives, {}) @@ -332,7 +332,7 @@ def find_related_fragments_from_inverse(source, source_relationship, options, co end join_manager = ActiveRelation::JoinManager.new(resource_klass: self, - source_relationship: relationship, + source_relationship: inverse_relationship, relationships: linkage_relationships.collect(&:name), sort_criteria: sort_criteria, filters: filters) @@ -352,7 +352,7 @@ def find_related_fragments_from_inverse(source, source_relationship, options, co if options[:cache] # This alias is going to be resolve down to the model's table name and will not actually be an alias resource_table_alias = self._table_name - parent_table_alias = join_manager.join_details_by_relationship(relationship)[:alias] + parent_table_alias = join_manager.join_details_by_relationship(inverse_relationship)[:alias] pluck_fields = [ sql_field_with_alias(resource_table_alias, self._primary_key), @@ -400,7 +400,7 @@ def find_related_fragments_from_inverse(source, source_relationship, options, co fragments[rid].add_related_from(parent_rid) if connect_source_identity - fragments[rid].add_related_identity(relationship.name, parent_rid) + fragments[rid].add_related_identity(inverse_relationship.name, parent_rid) end attributes_offset = 2 @@ -452,7 +452,7 @@ def find_related_fragments_from_inverse(source, source_relationship, options, co end end - parent_table_alias = join_manager.join_details_by_relationship(relationship)[:alias] + parent_table_alias = join_manager.join_details_by_relationship(inverse_relationship)[:alias] source_field = sql_field_with_fixed_alias(parent_table_alias, parent_resource_klass._primary_key, "jr_source_id") records = records.select(concat_table_field(_table_name, Arel.star), source_field) @@ -471,7 +471,7 @@ def find_related_fragments_from_inverse(source, source_relationship, options, co parent_rid = JSONAPI::ResourceIdentity.new(parent_resource_klass, resource._model.attributes['jr_source_id']) if connect_source_identity - fragments[rid].add_related_identity(relationship.name, parent_rid) + fragments[rid].add_related_identity(inverse_relationship.name, parent_rid) end fragments[rid].add_related_from(parent_rid) @@ -503,15 +503,16 @@ def count_related(source, relationship, options = {}) end def count_related_from_inverse(source_resource, source_relationship, options = {}) - relationship = source_relationship.resource_klass._relationship(source_relationship.inverse_relationship) + inverse_relationship = source_relationship._inverse_relationship + return -1 if inverse_relationship.blank? - related_klass = relationship.resource_klass + related_klass = inverse_relationship.resource_klass filters = options.fetch(:filters, {}) # Joins in this case are related to the related_klass join_manager = ActiveRelation::JoinManager.new(resource_klass: self, - source_relationship: relationship, + source_relationship: inverse_relationship, filters: filters) records = apply_request_settings_to_records(records: records(options), @@ -521,7 +522,7 @@ def count_related_from_inverse(source_resource, source_relationship, options = { filters: filters, options: options) - related_alias = join_manager.join_details_by_relationship(relationship)[:alias] + related_alias = join_manager.join_details_by_relationship(inverse_relationship)[:alias] records = records.select(Arel.sql("#{concat_table_field(related_alias, related_klass._primary_key)}")) diff --git a/lib/jsonapi/active_relation_retrieval_v09.rb b/lib/jsonapi/active_relation_retrieval_v09.rb index 2c526f0d..ca72a7b3 100644 --- a/lib/jsonapi/active_relation_retrieval_v09.rb +++ b/lib/jsonapi/active_relation_retrieval_v09.rb @@ -287,7 +287,9 @@ def load_resources_to_fragments(fragments, related_resources, source_resource, s if source_resource source_resource.add_related_identity(source_relationship.name, related_resource.identity) fragment.add_related_from(source_resource.identity) - fragment.add_related_identity(source_relationship.inverse_relationship, source_resource.identity) + + inverse_relationship = source_relationship._inverse_relationship + fragment.add_related_identity(inverse_relationship.name, source_resource.identity) if inverse_relationship.present? end end end diff --git a/lib/jsonapi/active_relation_retrieval_v10.rb b/lib/jsonapi/active_relation_retrieval_v10.rb index f48c54c6..0e3a7760 100644 --- a/lib/jsonapi/active_relation_retrieval_v10.rb +++ b/lib/jsonapi/active_relation_retrieval_v10.rb @@ -464,10 +464,8 @@ def find_related_monomorphic_fragments(source_fragments, relationship, options, end if connect_source_identity - related_relationship = resource_klass._relationship(relationship.inverse_relationship) - if related_relationship - fragments[rid].add_related_identity(related_relationship.name, source_rid) - end + inverse_relationship = relationship._inverse_relationship + fragments[rid].add_related_identity(inverse_relationship.name, source_rid) if inverse_relationship``.present? end end @@ -598,10 +596,8 @@ def find_related_polymorphic_fragments(source_fragments, relationship, options, related_fragments[rid].add_related_from(source_rid) if connect_source_identity - related_relationship = related_klass._relationship(relationship.inverse_relationship) - if related_relationship - related_fragments[rid].add_related_identity(related_relationship.name, source_rid) - end + inverse_relationship = relationship._inverse_relationship + related_fragments[rid].add_related_identity(inverse_relationship.name, source_rid) if inverse_relationship.present? end relation_position = relation_positions[row[2].underscore.pluralize] diff --git a/lib/jsonapi/configuration.rb b/lib/jsonapi/configuration.rb index dce8b9e3..bfaeb5fd 100644 --- a/lib/jsonapi/configuration.rb +++ b/lib/jsonapi/configuration.rb @@ -14,6 +14,8 @@ class Configuration :warn_on_missing_routes, :warn_on_performance_issues, :warn_on_eager_loading_disabled, + :warn_on_missing_relationships, + :raise_on_missing_relationships, :default_allow_include_to_one, :default_allow_include_to_many, :allow_sort, @@ -69,6 +71,11 @@ def initialize self.warn_on_missing_routes = true self.warn_on_performance_issues = true self.warn_on_eager_loading_disabled = true + self.warn_on_missing_relationships = true + # If this is set to true an error will be raised if a resource is found to be missing a relationship + # If this is set to false a warning will be logged (see warn_on_missing_relationships) and related resouces for + # this relationship will not be included in the response. + self.raise_on_missing_relationships = false # :none, :offset, :paged, or a custom paginator name self.default_paginator = :none @@ -330,6 +337,10 @@ def allow_include=(allow_include) attr_writer :warn_on_eager_loading_disabled + attr_writer :warn_on_missing_relationships + + attr_writer :raise_on_missing_relationships + attr_writer :use_relationship_reflection attr_writer :resource_cache diff --git a/lib/jsonapi/link_builder.rb b/lib/jsonapi/link_builder.rb index 63b160fc..dd12c7a4 100644 --- a/lib/jsonapi/link_builder.rb +++ b/lib/jsonapi/link_builder.rb @@ -34,7 +34,7 @@ def primary_resources_url @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" + warn "primary_resources_url for #{@primary_resource_klass.name} could not be generated" @primary_resource_klass._warned_missing_route = true end nil @@ -54,7 +54,7 @@ def relationships_related_link(source, relationship, query_params = {}) url else if JSONAPI.configuration.warn_on_missing_routes && !relationship._warned_missing_route - warn "related_link for #{relationship} could not be generated" + warn "related_link for #{relationship.display_name} could not be generated" relationship._warned_missing_route = true end nil @@ -66,7 +66,7 @@ def relationships_self_link(source, relationship) "#{ self_link(source) }/relationships/#{ route_for_relationship(relationship) }" else if JSONAPI.configuration.warn_on_missing_routes && !relationship._warned_missing_route - warn "self_link for #{relationship} could not be generated" + warn "self_link for #{relationship.display_name} could not be generated" relationship._warned_missing_route = true end nil @@ -78,7 +78,7 @@ def self_link(source) 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" + warn "self_link for #{source.class.name} could not be generated" source.class._warned_missing_route = true end nil diff --git a/lib/jsonapi/relationship.rb b/lib/jsonapi/relationship.rb index 51be3719..8f7d0fb1 100644 --- a/lib/jsonapi/relationship.rb +++ b/lib/jsonapi/relationship.rb @@ -9,7 +9,7 @@ class Relationship attr_writer :allow_include - attr_accessor :_routed, :_warned_missing_route + attr_accessor :_routed, :_warned_missing_route, :_warned_missing_inverse_relationship def initialize(name, options = {}) @name = name.to_s @@ -47,6 +47,7 @@ def initialize(name, options = {}) @_routed = false @_warned_missing_route = false + @_warned_missing_inverse_relationship = false exclude_links(options.fetch(:exclude_links, JSONAPI.configuration.default_exclude_links)) @@ -162,6 +163,22 @@ def _relation_name @relation_name || @name end + def to_s + display_name + end + + def _inverse_relationship + @inverse_relationship_klass ||= self.resource_klass._relationship(self.inverse_relationship) + if @inverse_relationship_klass.blank? + message = "Missing inverse relationship detected for: #{self}" + warn message if JSONAPI.configuration.warn_on_missing_relationships && !@_warned_missing_inverse_relationship + @_warned_missing_inverse_relationship = true + + raise message if JSONAPI.configuration.raise_on_missing_relationships + end + @inverse_relationship_klass + end + class ToOne < Relationship attr_reader :foreign_key_on @@ -182,9 +199,9 @@ def initialize(name, options = {}) @polymorphic_type_relationship_for = options[:polymorphic_type_relationship_for] end - def to_s + def display_name # :nocov: useful for debugging - "#{parent_resource}.#{name}(#{belongs_to? ? 'BelongsToOne' : 'ToOne'})" + "#{parent_resource.name}.#{name}(#{belongs_to? ? 'BelongsToOne' : 'ToOne'})" # :nocov: end @@ -247,9 +264,9 @@ def initialize(name, options = {}) # end end - def to_s + def display_name # :nocov: useful for debugging - "#{parent_resource_klass}.#{name}(ToMany)" + "#{parent_resource.name}.#{name}(ToMany)" # :nocov: end diff --git a/lib/jsonapi/resource_identity.rb b/lib/jsonapi/resource_identity.rb index 58936d95..f55d151c 100644 --- a/lib/jsonapi/resource_identity.rb +++ b/lib/jsonapi/resource_identity.rb @@ -58,7 +58,7 @@ def <=>(other_identity) # Creates a string representation of the identifier. def to_s # :nocov: - "#{resource_klass}:#{id}" + "#{resource_klass.name}:#{id}" # :nocov: end end diff --git a/test/unit/serializer/link_builder_test.rb b/test/unit/serializer/link_builder_test.rb index fd502400..f01efb4a 100644 --- a/test/unit/serializer/link_builder_test.rb +++ b/test/unit/serializer/link_builder_test.rb @@ -248,7 +248,7 @@ def test_relationships_related_link_not_routed link = builder.relationships_related_link(source, relationship) assert_nil link end - assert_equal(err, "related_link for Api::Secret::PostResource.author(BelongsToOne) could not be generated\n") + assert_equal("related_link for Api::Secret::PostResource.author(BelongsToOne) could not be generated\n", err) # should only warn once builder = JSONAPI::LinkBuilder.new(config)