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

include without query string #15

Closed
snellingio opened this issue May 31, 2022 · 10 comments
Closed

include without query string #15

snellingio opened this issue May 31, 2022 · 10 comments
Labels
enhancement New feature or request

Comments

@snellingio
Copy link
Contributor

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:

public function parse(Request $request, string $prefix): Collection

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
Copy link
Contributor Author

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.

@timacdonald timacdonald moved this to Todo in JSON:API Aug 13, 2022
@HollyIT
Copy link

HollyIT commented Dec 18, 2022

+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]));
    }
}

@timacdonald
Copy link
Owner

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 timacdonald added the enhancement New feature or request label Jan 26, 2023
@HollyIT
Copy link

HollyIT commented Jan 26, 2023

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.

@timacdonald
Copy link
Owner

timacdonald commented Jan 27, 2023

Passing a custom request is already possible.

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

// or

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

@HollyIT
Copy link

HollyIT commented Jan 27, 2023

@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!

@timacdonald
Copy link
Owner

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

@juliomotol
Copy link

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 #23 as we could just say that a Node will default include its children.

@HollyIT
Copy link

HollyIT commented Feb 22, 2023

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

@timacdonald
Copy link
Owner

No problem.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

4 participants