cerebris / jsonapi-resources

A resource-focused Rails library for developing JSON:API compliant servers.
http://jsonapi-resources.com
MIT License
2.32k stars 530 forks source link

Test for missing inverse relationships #1422

Open lgebhardt opened 9 months ago

lgebhardt commented 9 months ago

This issue is a (choose one):

Checklist before submitting:

Description

A way to detect missing inverse relationships is needed. I would like to be able to add a test to a project to ensure no new missing relationship conditions have been added. Should also account for intentionally having a missing inverse relationship, probably with a flag on the primary relationship.

There are probably other relationship tests we could add, so we should consider how to best handle these in a consistent manner.

SingleShot commented 9 months ago

Hi! I just hit a "missing inverse relationship" error. v10.x does not require inverse relationships but I see v11.x does.

Just one person's feedback, but I suggest this is overly invasive. For example, we have a very large code base and purposely avoid bi-directional relationships so we can more easily manage dependencies between code/modules (e.g. avoid dependency cycles). This works fine with jsonapi-resources v10.x. However if we wish to continue to stay up to date and use v11.x, we now have to add a bunch of undesired relationships to our models, creating tighter coupling in our codebase. I know ther are certain requirements for using jsonapi-resources like using ActiveRecord but this goes further, requiring us to define our models in a very specific way.

Perhaps this is something that can be reconsidered?

EDIT: I see you may consider a flag to allow the rule to be disabled - that would satisfy our use case I think

lgebhardt commented 9 months ago

@SingleShot Thanks for the feedback. I will revisit this v11 change with your concern in mind and try to provide some guidance soon.

SingleShot commented 8 months ago

Hi. Just checking back here. We're wondering if you've given further thought to this topic. We're of course using v11 at our own risk but do find requiring bidirectional relationships to be very undesirable. The spec implies relationships are optional and there is no requirement to define two-way relationships so I think loosening this back up to the behavior in v10 would be more compliant.

I can imagine some users of the library would always want bidirectional relationships and others (like us) wouldn't so maybe having a config setting of require_bidirectional_relationships would be useful. It seems the relationship fetching logic would need to be rewritten (or reverted to v10) regardless to support the case where this is set to false.

Anyway, just checking back.

zion commented 5 months ago

I'm running into this as well on 0.11.

It seems that it only occurs when the API request is asking for an included association, especially for associations that use through:.

For example, a Part/Tag architecture. Where a Part has many Tag model associations and relations are recorded in a part_tags table that has part_id and tag_id columns.

In the Part model you could associate to tags like:

has_many :part_tags
has_many :tags, through: :part_tags

If I wanted to include all tags for a PartResource using `/parts/100?include=tags I could add:

has_many :tags

However, this is where the issue occurs and it's because JR cant determine the inverse relationship, and in v11 it needs to. (I agree this should be a configurable opt-in feature). But it can be worked around fairly easy.

I resolved the error by adding the following:

# resources/tag_resource.rb
class TagResource < JSONAPI::Resource
  has_many :parts
end

# models/tag.rb
class Tag < ApplicationRecord
  has_and_belongs_to_many :parts, join_table: 'part_tags'
end

Essentially you need to tell ActiveRecord this relation is bi-directional (I think thats the correct term? LOL). Then you need to specify the has_many on the TagResource class.