flugg / laravel-responder

A Laravel Fractal package for building API responses, giving you the power of Fractal with Laravel's elegancy.
MIT License
865 stars 86 forks source link

Lazy loading relations via query string #121

Open kmichailg opened 6 years ago

kmichailg commented 6 years ago

Hi! I was looking alternatives for API development and came across this gem.

Unfortunately, one feature I am trying to find is loading relationships for models via query string. I have accomplished this using this package and Fractal, but is becoming cumbersome to maintain in between projects.

This package has documentation regarding it but it doesn't work as per my setup. I might be doing something wrong. Please see my use case below.

Request

/api/v1/users/{id}?with=profile

Controller

        return responder()->success(
            \App\Modules\User\Models\User::find($id),
            \App\Transformers\DefaultTransformer::class
        )->serializer(\League\Fractal\Serializer\JsonApiSerializer::class);

Transformer

namespace App\Transformers;

use Flugg\Responder\Transformers\Transformer;

class DefaultTransformer extends Transformer
{
    /**
     * List of available relations.
     *
     * @var string[]
     */
    protected $relations = ['*']; //should allow loading any/all relation/s on the model

    /**
     * A list of autoloaded default relations.
     *
     * @var array
     */
    protected $load = [];

    /**
     * A Fractal transformer.
     *
     * @return array
     */
    public function transform($data)
    {
        if (is_array($data)) {
            return $data;
        }

        $data = $data->toArray();

        return $data;
    }
}

Is there anything I missed? Or do I still need to define all relations on the Transformer class?

kmichailg commented 6 years ago

Additional Info

I have tried adding the relationship into the $relations array as a string and confirmed that the relation was working as intended. Also tried using ->with() function.

flugg commented 6 years ago

Hey @kmichailg - in v3.0.0 the relationship code was change quite a bit, to make it more secure. Previously, when using wildcards in the transformer and allowing inclusion of relations through a query string, allowed API consumers to execute any method on the model as Laravel doesn't differentiate between a relationship method and regular method.

Going forward you should indeed define all relations on the transformer. I've simply forgotten to update the documentation about this, but will get the section on the wildcard removed. Sorry about that!

kmichailg commented 6 years ago

Would it be possible to support something like that again? In the package I've linked here I could do the same thing, however I wanted to remove the custom ResponseTrait I've created and just rely on another third-party package(e.g. this). It throws an error (Illuminate\Database\Eloquent\RelationNotFoundException) if you try to include a relationship that is non-existent.

flugg commented 6 years ago

Sorry for the late reply! Wanted to re-dig into the matter before giving an answer. The RelationNotFoundException is only thrown if a BadMethodCallException was thrown, which means it doesn't protect you from calling unwanted methods on models. So, considering how Laravel models work and the fact you can't dynamically know what relations exist I'm afraid there's no way to support a wildcard without also providing the users with a potential security hole.

I would rather advocate being explicit in this case. Besides being a slight performance boost, it also allows for auto loading relations from nested transformers using the $load property. Which was an issue that was corrected with the patch removing wildcard relations. Hope you understand!

kmichailg commented 6 years ago

But one could argue that defining relationships twice just to format and load data is quite repetitive. I'm trying to avoid defining relationships twice to promote brevity when reading code, making sure that a developer would only need to look at the models to ascertain what relationships are available, and not have to look at the Transformers for a missing relationship. I think for now, I'd stick to what I have, since I've worked around the requirement I'm having problems with. But I am interested if we could make this work in this package.

flugg commented 6 years ago

That was my initial hope for the package, but unfortunately, as I've learned while developing this package, we can't get the best of both worlds due to how Laravel's relationship system work.

I'm looking for ways to change the transformation driver from Fractal to Laravel Resources and will definitely keep it in mind for the process to see if that opens up new possibilities.

However, feel free to hack at the source code and let me know if you find some solution I haven't thought of :)

kmichailg commented 6 years ago

Will do. Thanks. Keeping this open for future use.

gdespiritorflex commented 5 years ago

Great conversation. As this is not an issue I think it could be closed? :) Hope to see an implmentation with Laravel Resources 👍