[12.x] Fix circular references support on JSON:API component

[mateusjatenee]

Right now, JsonApiResource enters an infinite loop when serializing models with bidirectional Eloquent relationships that use chaperone().

For example, imagine you have:

class User extends Model
{
    public function posts()
    {
        return $this->hasMany(Post::class)->chaperone();
    }
}

class Post extends Model
{
    public function author()
    {
        return $this->belongsTo(User::class);
    }
}


class UserResource extends JsonApiResource
{
    protected array $relationships = [
        'posts' => PostResource::class,
    ];
}

class PostResource extends JsonApiResource
{
    protected array $relationships = [
        'author' => UserResource::class,
    ];
}

The following hangs:

return User::with('posts')->find(1)->toResource();

This happens because on resolveIncludedResourceObjects we call includePreviouslyLoadedRelationships, which includes all available relations for a model on compileResourceRelationships.

So, essentially:

1. User resource adds Post to the map
2. Post resource calls includePreviouslyLoadedRelationships -> sees author is loaded (via chaperone) → adds the same User instance back to the map
3. That User is processed again -> adds the same Post back -> infinite loop

We can't remove includePreviouslyLoadedRelationships, as it'd break nested relationships (I think, pretty tired, might need to revisit this), so my fix involves tracking which objects we have visited on resolveIncludedResourceObjects, which is only invoked for the "root" resource/model.
My first approach tracked {type}-{id}, but it breaks some use cases (and tests), e.g. BelongsToMany where you have e.g. the same model attached to another model but with different pivot data, for example testItCanGenerateJsonApiResponseWithRelationshipsUsingSparseIncluded where it expects two teams, but it'd only give one back. Tracking the object itself with spl_object_id handles that.

There are some edge cases I can think of that this solution does not support:

• If you want the same object legitimately included twice: e.g. /users/1?include=posts.user — I don't see when this would be useful tbh but it'd not work.
• Wanting the chaperoned relation to show up as a different resource type in included: e.g. Post author uses AuthorResource (type: authors) while the root uses UserResource (type: users). The Post relationship identifier would reference authors/1, but there'd be no matching entry in included since we skip the same object. Bit of a weird setup though.

2 is a bit hard to understand, here's some Claude art:

User (root, type: users)
  └── Post
        └── author (chaperone) → AuthorResource (type: authors)

I'd prefer to avoid tracking objects tbh. I can revisit this tomorrow and go through the code more carefully to see if there's a better option, but wanted to at least get a failing test in place.

[mateusjatenee]

@timacdonald brought up some interesting edge cases; already got failing tests for them. Will fix in the morning, drafting the PR until then

[dxnter]

I'd prefer to avoid tracking objects tbh

I don't think there's a way to avoid object tracking here, it's the only way to distinguish between BelongsToMany with different pivot data vs chaperone circular references.

For the second case (same object, different type through chaperone): I think something like this could work?

$visitedResources = [
    spl_object_id($this->resource).':'.$this->resolveResourceType($request) => true,
];

// in the while loop
$resourceKey = spl_object_id($resourceInstance->resource).':'.$type;

That way, it allows the same object to appear with different resource types.

---

For the first case (/users/1?include=posts.user with chaperone), I don't think that needs to be supported. Per https://jsonapi.org/format/#document-compound-documents

A compound document MUST NOT include more than one resource object for each type and id pair.

This applies across the entire document, duplicating a resource in both data and included violates the spec. As a separate issue, the existing testItCanResolveRelationshipWithRecursiveNestedRelationship test actually expects this violation (User appears in both data and included) which may be worth addressing separately.

[mateusjatenee]

@dxnter good stuff! Appreciate it.

I wonder if we should support case 2. Maybe it's not as uncommon as I think.
@timacdonald suggested using a WeakMap if i were uncomfortable with tracking object IDs — guess we'll need to switch back to that tuple if we wish to support that case.

Regarding #1 - agreed on tackling that separately.

[dxnter]

@mateusjatenee Perfect use for WeakMap! You could still support the second case with a minor adjustment:

$visitedObjects[$this->resource] = [$this->resolveResourceType($request) => true];

// in the loop
if (isset($visitedObjects[$underlyingResource][$type])) { ... }
$visitedObjects[$underlyingResource] ??= [];
$visitedObjects[$underlyingResource][$type] = true;

Without it, something like this would fail:

$user = User::factory()->create();
$post = Post::factory()->create(['user_id' => $user->getKey()]);

// Post's author uses AuthorResource ("authors"), root uses UserResource ("users")
$response = $this->getJson("/users/{$user->getKey()}/with-chaperone-posts?".http_build_query([
    'include' => 'chaperonePosts.author',
]));

// author is blocked because it's the same object
$response->assertJsonPath('included.1.type', 'authors');

imo it does seem a bit unusual to do, but since there's support for mapping relationships to different resource classes it should probably work when combined with chaperone? 🤷‍♂️

[donnysim]

Object tracking is definitely unavoidable for JSON:API resolution. Things like:

User::with(['posts.author:id,name'])->get(['id', 'name', 'email']);

must return

{
    "data": [
        {
            "type": "users",
            "id": "1",
            "attributes": {
                "name": "Alice Johnson",
                "email": "alice@example.com"
            },
            "relationships": {
                "posts": {
                    "data": [
                        { "type": "posts", "id": "11" }
                    ]
                }
            }
        }
    ],
    "included": [
        {
            "type": "posts",
            "id": "11",
            "attributes": {
                "title": "Second Post"
            },
            "relationships": {
                "author": {
                    "data": { "type": "users", "id": "1" }
                }
            }
        }
    ]
}

the User resource merged at the top level, because by the spec only one instance of the resource can exist in the document. Same goes if we reverse and the user is included - it still needs to be merged from different User models into a single one.
I haven't kept track of the implementation and not sure if it does that, but it would require differentiating if a specific Model was already parsed or not instead of type+id to avoid circular references and proper merging.

[mateusjatenee]

@donnysim @dxnter appreciate the help here!

I decided to stop being lazy and actually read the spec in full (and your comments). Correct me if I'm wrong, but this is what I got from it:

Resource identity = type + id

A compound document MUST NOT include more than one resource object for each type and id pair.
https://jsonapi.org/format/#document-compound-documents

So:
users/1 and authors/1 -> different resources, both should appear in included
/users/1 appearing twice -> should be deduplicated
tl;dr:
We should deduplicate on (model + resource type), not just model.

GET /users/1?include=posts.author should include author as a separate resource, even if they’re backed by the same model
GET /users/1?include=posts.user should deduplicate user

Additionally, there's the scenario where a user might inadvertently have multiple instances of the same model (ID) on the object graph, but not necessarily the same instance. However that's already addressed separately — I added a test still.

Edit:
I forgot to mention: additionally, I think our impl for BelongsToMany violates the spec - e.g. testItCanGenerateJsonApiResponseWithRelationshipsUsingSparseIncluded should not pass as it includes the same team twice. But we should probably tackle that separately. But we should probably tackle that separately.

[donnysim]

Yeah, I think this should just focus on circular references as intended, and the whole duplication and merging should be tackled and raised separately as maybe it would cause a breaking change, though hopefully not as I can't imagine how you're supposed to find the correct entry if multiple are present.

[timacdonald]

@mateusjatenee, we should deduplicate on the resolved resource ID and type, not the model itself.

Imagine I have a user resource (type: "user") and an author resource (type: "author"). Both are powered by a user model.

Both user 1 and author 1 are allowed to appear in the response.

• Each resource has a unique key based on the type and ID (https://github.com/timacdonald/json-api/blob/main/src/Concerns/Identification.php#L117)
• Perform a uniqueStrict on included resources https://github.com/timacdonald/json-api/blob/cf3b6d2339306d93212ea95105642910c6da8e75/src/JsonApiResource.php#L143

I know the framework implementation strayed a bit from mine, so I'm not entirely sure where to do this kinda thing, but that is what I would expect to happen.

[mateusjatenee]

@timacdonald right — the current implementation does not deduplicate on the model instance, but on {model_instance, resource_type}.

The reason it isn't {resource_id, resource_type} is because that would breaktestItCanGenerateJsonApiResponseWithRelationshipsUsingSparseIncluded — technically, that is a spec violation, as it expects two entries for the same teams/1 resource — one with role => Admin and one with role => Member. Note that this is intentional, see:

framework/src/Illuminate/Http/Resources/JsonApi/Concerns/ResolvesJsonApiElements.php

Line 251 in 98376cd

$isUnique = ! $relationship instanceof BelongsToMany;

[mateusjatenee]

OK - just pushed a commit that dedupes on { resource_id, resource_type }, but only for included. That means if you do /users/1?include=profile.user, it'll return both profile and user on included, even though user is a duplicate.
The reason for that is that otherwise, the relationship profile.user would still exist, but a consumer would not find user within included, as it would've been deduplicated since it is the root resource. So I think that makes sense.

Note that I did have to update the BelongsToMany test. if we truly want to support that, I believe we'll need to track { object_id, resource_type }.

That solves the initial circular reference problem for chaperone and also handles deduplication (well, the two are linked).

[mateusjatenee]

Closing in favor of #58329 - we can discuss dedup separately