Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Included morph to many data is null #280

Open
maartenpaauw opened this issue May 15, 2024 · 5 comments
Open

Included morph to many data is null #280

maartenpaauw opened this issue May 15, 2024 · 5 comments

Comments

@maartenpaauw
Copy link

maartenpaauw commented May 15, 2024

The use case is, I've a Round that has many TextOverrides and belongs to many DefaultTexts. Within the RoundSchema I've added the following field:

MorphToMany::make('texts', [
    BelongsToMany::make('defaultTexts'),
    HasMany::make('textOverrides'),
])->readOnly(),

When I navigate to http://localhost/api/v1/rounds/1/texts, I see both default-texts and text-overrides nicely listed. But when I try to include texts, using http://localhost/api/v1/rounds/1?include=texts, I get the following exception message:

The attribute [texts] either does not exist or was not retrieved for model [App\Models\Round].

This exception message is displayed because we prevent accessing missing attributes using; Model::preventAccessingMissingAttributes();. When I remove this line the view round API endpoint loads, but has the following outcome. The data is null which is incorrect because there are default-texts and text-overrides.

{
    "jsonapi": {
        "version": "1.0"
    },
    "links": {
        "self": "http://localhost/api/v1/rounds/1"
    },
    "data": {
        "type": "rounds",
        "id": "1",
        "attributes": {
            "name": "Round 1"
        },
        "relationships": {
            "texts": {
                "links": {
                    "related": "http://localhost/api/v1/rounds/1/texts",
                    "self": "http://localhost/api/v1/rounds/1/relationships/texts"
                },
                "data": null
            }
        },
        "links": {
            "self": "http://localhost/api/v1/rounds/1"
        },
        "meta": {
            "createdAt": "2024-05-15T07:48:27.000000Z",
            "updatedAt": "2024-05-15T07:48:34.000000Z"
        }
    }
}

I do have added a TextCollectionQuery, which extends the ResourceQuery, and registered in the serving method of the Server class, as described on the documentation page; https://laraveljsonapi.io/docs/3.0/digging-deeper/polymorphic-to-many.html#query-parameters.

At this point I've no idea what's wrong with the implementation. What am I missing?

@maartenpaauw
Copy link
Author

Could be related to #164

@maartenpaauw
Copy link
Author

Additional research; I did try to refactor the HasMany relationship to an BelongsToMany relationship. Unfortunately the same outcome.

@maartenpaauw
Copy link
Author

maartenpaauw commented May 15, 2024

I did a deep-dive in the package code. Here I try to address the point where I think it goes wrong.

Within the encoder-neomerx package, the willShowData() methods returns true as expected at; https://github.com/laravel-json-api/encoder-neomerx/blob/v4.0.0/src/Schema/Relation.php#L107. This because the MorphToMany relationship named texts is included. As far so good.

Because the willShowData() method returns true, the data() method is executed. The data() method tries to receive the data using the Relation instance at https://github.com/laravel-json-api/encoder-neomerx/blob/v4.0.0/src/Schema/Relation.php#L85.

At this point we enter the data() method from the Relation class inside the core package. Somehow $hasData is false at this point (I don't know why). Because of this, the method value() gets executed at: https://github.com/laravel-json-api/core/blob/v4.0.0/src/Core/Resources/Relation.php#L147-L149.

And in the end, the value() method (at: https://github.com/laravel-json-api/core/blob/v4.0.0/src/Core/Resources/Relation.php#L333-L336) tries to retrieve the relation data by directly reading the (in the example above) texts attribute, which returns null. This because there is no texts relationship, but a separate BelongsToMany defaultTexts and a HasMany textOverrides which are morphed to many within the RoundSchema (code in issue) and not in the model itself.

I don't know how to solve this issue, but I hope this deep-dive helps debugging the issue @lindyhopchris.

@maartenpaauw
Copy link
Author

maartenpaauw commented May 15, 2024

Alright! I've found the actual problem!

Because there is no equivalent relationship type for JSON:API MorphToMany in Eloquent, this package cannot directly retrieve the relationship's value by reading the property on the model. That is the reason why the laravel-json-api/eloquent package has an extended Relation class. Within this Relation class, the value() method groups all field values together; https://github.com/laravel-json-api/eloquent/blob/v4.0.0/src/Resources/Relation.php#L97-L106

Only this extended Relation class is not instantiated when defining the relations within the relationships method inside the JSON:API resource class, as described in the documentation: Defining Relationships.

As a workaround, you could directly instantiate an extended Relation instead of calling the relation() method. For example:

public function relationships($request): iterable
{
    return [
        $this->relation('owner'),
        $this->relation('tags'),
        new \LaravelJsonApi\Eloquent\Resources\Relation(
            $this->resource,
            $this->selfUrl(),
            $this->schema->relationship('texts'),
        ),
    ];
}

In my opinion, the relation() method should instantiate the extended Relation class when a JSON:API MorphToMany relationship field is used, instead of always using the core Relation class. The only downside is when adding an if-statement inside the relation() method, that we are using the Eloquent namespace inside the Core namespace. I'm not sure if that is appreciated.

Let me know what you think! At least I have a temporary workaround for now.

@lindyhopchris
Copy link
Contributor

Thanks for the explanation with this.

I think this is another example why actually I think it's a bad idea to allow people to use the Resource class directly. A lot of the implementation relies on what we've implemented. Over time I want to get rid of the need to do that, and ensure that everything a developer needs to do can be done via the schema.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants