json-api icon indicating copy to clipboard operation
json-api copied to clipboard

`include` without query string

Open snellingio opened this issue 3 years ago • 1 comments

Currently, there is not a way to include relationships without explicitly asking for them. While this makes sense in almost all cases, I have a unique case where on a specific resource I want to include some morphTo's in the include by default.

Where includes happen: https://github.com/timacdonald/json-api/blob/c3f89eef3e6c3e6b4f46321211543acc99f5134f/src/Support/Includes.php#L38

Options could be adding a default to the $request->query('include'), which would work for my use case, but probably wouldn't be flexible enough for most people where they would want to ALWAYS have a default include even when passing in more.

I'll think a little bit about it, but I imagine that @timacdonald will have a better / specific implementation that he wants. If you can describe the best way to do it, I could take a shot at it.

snellingio avatar May 31 '22 13:05 snellingio

I should note that Laravel as far as I can tell doesn't allow for modifying the query string, unlike $request->merge([]). That's also an easy way to do this. We could just look in the $request->input for merged query variables.

snellingio avatar May 31 '22 14:05 snellingio

+1 on this idea. I like using JsonAPI resources for things like Inertia apps as well, just to keep the number of needed resource classes to a minimum. The work around I've implements is to just extends the base class and add in a method to force some includes, but it would be nice to have this as a core feature.

class AppResource extends JsonApiResource
{

    protected array $forcedIncludes = [];

    public function withIncludes($includes): static {
        $this->forcedIncludes = collect(explode(',', $includes))->filter()->unique()->toArray();
        return $this;
    }

    public function toResponse($request): JsonResponse
    {
        $includes = '';
        if ($request->include) {
            $includes = collect(array_merge(explode(',', $request->include), $this->forcedIncludes))
                ->filter()
                ->unique()
                ->join(',');
        }
        return parent::toResponse($request->merge(['include'=> $includes]));
    }
}

HollyIT avatar Dec 18 '22 17:12 HollyIT

Although I'm not 100% against this, my main concern is that is against the spec and leads to weird results for users.

In my eyes the beauty of not including anything by default is that the client gets full control and responses are deterministic. I don't any unexpected data.

Would it not be possible to the client to just include the relationships in the query parameter? What makes these relationships special? Just trying to understand better.

timacdonald avatar Jan 26 '23 22:01 timacdonald

Take an Inertia app as an example. Instead of having to create generic Laravel JSON Resources for the App, then JSON-API resources for the API endpoints, you could utilize a single JSON API resource for both.

This is kind of a gray area of the spec:

https://jsonapi.org/format/#fetching-includes

If an endpoint supports the include parameter and a client supplies it:

The server MUST NOT include unrequested resource objects in the included section of the compound document.

Since I'm talking about an app endpoint (Inertia in this case), which puts the actual response inside the HTML body (as an attribute), it technically wouldn't be an API endpoint, so it wouldn't actually violate this.

Honestly, a better solution would be the ability to make send a custom request to the Resource class. Not sure if that's entirely possible given the underlying usage of Laravel JSON Resources.

HollyIT avatar Jan 26 '23 23:01 HollyIT

Passing a custom request is already possible.

return UserResource::make($user)->response($customRequest);

// or

return UserResource::make($user)->toResponse($customRequest);

timacdonald avatar Jan 27 '23 19:01 timacdonald

@timacdonald It seems like I tried that before and couldn't get it to work, but if it's handling it, then that's a perfect solution. Thanks!

HollyIT avatar Jan 27 '23 20:01 HollyIT

No worries. I'm gonna leave this ticket open, just to see what the vibe is on this idea from more people.

timacdonald avatar Jan 28 '23 00:01 timacdonald

Before, when we used league/fractal (through spatie/laravel-fractal), they have a feature where you could set the Default Includes for a resource. This plays well with the spec as it also mentions:

An endpoint MAY return resources related to the primary data by default.

This could also resolve https://github.com/timacdonald/json-api/issues/23 as we could just say that a Node will default include its children.

juliomotol avatar Feb 07 '23 02:02 juliomotol

@timacdonald - just tried the custom request object and it works great. Thanks!

HollyIT avatar Feb 22 '23 21:02 HollyIT

No problem.

I've had an idea on this that I will implement soon. Will give more flexibility in general.

timacdonald avatar Feb 22 '23 23:02 timacdonald