SerializerInterface violates LSP

[f3ath]

Hi @tobscure First of all, thanks for your efforts making this library. I was looking at the php libraries implementing jsonapi in order to pick one for one of my projects. There are a few of them worth trying, including yours. Unfortunately, it looks like that all of them suffer from the same issue.

Issue

They all have the similar idea of serializers - the logic which converts a business entity (model) into a json data object. And these serializers look pretty much similarly: they have a number of methods like getId($model), getAttributes($model) which take the entity as a parameter. These methods know about their $models internal structure and they use this knowledge to extract the id, attributes or whatever is needed to build the json. Consider the example from readme:

use Tobscure\JsonApi\AbstractSerializer;

class PostSerializer extends AbstractSerializer
{
    protected $type = 'posts';

    public function getAttributes($post, array $fields = null)
    {
        return [
            'title' => $post->title,
            'body'  => $post->body,
            'date'  => $post->date
        ];
    }
}

PostSerializer can only deal with $post models. The getAttributes() call will fail on everything else except a properly structured post object. But SerializerInterface does not restrict the model to anything particular. What SerializerInterface says is basically: you give me anything (@param mixed $model) and I tell you its attributes. Therefore, PostSerializer breaks the contract given by SerializerInterface. It means violation of Liskov Substitution Principle.

Solution (well, a kind of)

Instead of these pseudo-universal serializers, we should have something like ResourceInterface:

interface ResourceInterface
{
    public function getType(): string;
    public function getId(): string;
    public function getAttributes(array $fields = null): array;
    public function getLinks(): array;
    public function getMeta(): array;
    public function getRelationship($name):  ?\Tobscure\JsonApi\Relationship;
}

And a use-case would be like

class PostResource implements ResourceInterface
{
    protected $type = 'posts';
    public function __construct($post)
    {
       // validate $post
    }

    public function getAttributes(array $fields = null)
    {
        return [
            'title' => $this->post->title,
            'body'  => $this->post->body,
            'date'  => $this->post->date
        ];
    }
}
$document->setData(new PostResource($post));

Since your library version is still 0.*, it is not too late to make backward-incompatible changes to fix this issue. Please let me know what you think.

[tobyzerner]

Interesting, thanks for pointing this out. I'm open to exploring this. I'll play around with it when I have some time.

[tobyzerner]

One issue (inconvenience?) I can see, at least in the context of Laravel and other environments with a similar dependency injection container: What if I want to inject dependencies into a serializer to aid with the serialization?

Where previously something like this would be possible:

class GroupSerializer extends AbstractSerializer {
    protected $type = 'groups';
    public function __construct(TranslatorInterface $translator) {
        $this->translator = $translator;
    }
    public function getAttributes($model) {
        return ['name' => $this->translator->trans($model->name)];
    }
}
$resource = new Resource($group, $container->make('GroupSerializer'));

With a ResourceInterface I guess you'd have to either inject deps alongside the model:

class GroupResource implements ResourceInterface {
    protected $type = 'groups';
    public function __construct(Group $group, TranslatorInterface $translator) {
        $this->group = $group;
        $this->translator = $translator;
    }
    public function getAttributes() {
        return ['name' => $this->translator->trans($this->group->name)];
    }
}
$resource = $container->make('GroupResource', ['group' => $group]);

or inject them statically (this is what Laravel's Eloquent ORM does for its models):

class GroupResource implements ResourceInterface {
    protected $type = 'groups';
    public function __construct(Group $group) {
        $this->group = $group;
    }
    public function getAttributes() {
        return ['name' => static::$translator->trans($this->group->name)];
    }
}
GroupResource::setTranslator($translator);
$resource = new GroupResource($group);

[f3ath]

I imagine a factory might mitigate the incovenience:

class TranslatingGroupResourceFactory
{
    private $translator;

    public function __construct(TranslatorInterface $translator)
    {
        $this->translator = $translator;
    }

    public function createResourceFor(Group $group): ResourceInterface
    {
        return new TranslatingGroupResource($group, $this->translator);
    }
}

The factory itself is created in the DI, but the GroupResource instances are created at runtime.

[tobyzerner]

🤦‍♂️ I always forget about factories... that's a good solution.

Well, I think I'm all for this then. Feel free to send a PR if you're up for it? Otherwise, I'll try and tackle it sometime in the next few weeks.

[tobyzerner]

@f3ath and let me know if you are, so we don't double up :)

[f3ath]

@tobscure i don't think i have enough free resources to do anything meaningful (at least on this week). So please go ahead. I would be happy to watch the progress though ;)

[tobyzerner]

Alright! I've got something working. I had to rewrite most of the document-construction logic — which was a bit of a mess anyway — and now it's a lot simpler (and hopefully more performant). Still want to spend a bit more time thinking to make sure I've got it right... but otherwise I'll clean up and push to a new branch soon so we can start testing.

[tobyzerner]

Aaaand just squeezed out an extra drop of performance and made the algorithm even simpler.

Did some basic benchmarks. New version is 2x faster for a simple document, 3x faster for a more complex one.

[franzliedke]

@tobscure Is any of this online? I'm quite interested - I have built a similar tool in Ruby at work in the last few months. ;)

[tobyzerner]

@franzliedke tomorrow!

[tobyzerner]

@franzliedke #119