Skip to content

Commit

Permalink
Warn on missing inverse relationships (#1451)
Browse files Browse the repository at this point in the history
  • Loading branch information
lgebhardt committed Apr 18, 2024
1 parent fa3e059 commit 3509c4a
Show file tree
Hide file tree
Showing 8 changed files with 59 additions and 32 deletions.
25 changes: 13 additions & 12 deletions lib/jsonapi/active_relation_retrieval.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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, {})

Expand All @@ -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)
Expand All @@ -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),
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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),
Expand All @@ -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)}"))

Expand Down
4 changes: 3 additions & 1 deletion lib/jsonapi/active_relation_retrieval_v09.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 4 additions & 8 deletions lib/jsonapi/active_relation_retrieval_v10.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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]
Expand Down
11 changes: 11 additions & 0 deletions lib/jsonapi/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions lib/jsonapi/link_builder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
27 changes: 22 additions & 5 deletions lib/jsonapi/relationship.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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))

Expand Down Expand Up @@ -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

Expand All @@ -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

Expand Down Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion lib/jsonapi/resource_identity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/unit/serializer/link_builder_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 3509c4a

Please sign in to comment.