neomerx / json-api

Framework agnostic JSON API (jsonapi.org) implementation
Apache License 2.0
743 stars 66 forks source link

Mapping now can handle base classes and interfaces as resource types #215

Closed lencse closed 6 years ago

lencse commented 6 years ago

Sometimes it can be useful if we can map base classes or interfaces to a schema, not only the leaf classes.

One example:

interface AuthorInterface {}

class Author implements AuthorInterface {
    // ...
}

class AnonymousAuthor implements AuthorInterface {
    // ...
}

$encoder = Encoder::instance([
    AuthorInterface::class => AuthorSchema::class,
], new EncoderOptions(JSON_PRETTY_PRINT, 'http://example.com/api/v1'));

echo $encoder->encodeData(new Author('John Doe'));
echo $encoder->encodeData(new AnonymousAuthor());
neomerx commented 6 years ago

Hi,

With existing implementation, you can get the same result with

interface AuthorInterface {}

class Author implements AuthorInterface {
    // ...
}

class AnonymousAuthor implements AuthorInterface {
    // ...
}

$encoder = Encoder::instance([
    Author::class          => AuthorSchema::class,
    AnonymousAuthor::class => AuthorSchema::class,
], new EncoderOptions(JSON_PRETTY_PRINT, 'http://example.com/api/v1'));

echo $encoder->encodeData(new Author('John Doe'));
echo $encoder->encodeData(new AnonymousAuthor());

You can also use your own custom container which would accept

[
    AuthorInterface::class => AuthorSchema::class,
]

convert it into

[
    Author::class          => AuthorSchema::class,
    AnonymousAuthor::class => AuthorSchema::class,
]

and pass it to defaul container if you need syntax with interfaces.

lencse commented 6 years ago

Thanks for your feedback

  1. The first solution definitely works. My concern is that it somehow breaks the dependency inversion principle because the json encoder service has to know all implementations of the business objects. I can bypass it and inject the list of subclasses from a different service, but I thought that it would be better if the library can solve this on its own.

  2. I'm not 100% what you thought in the custom container approach. Should I replace the Container with the one I implemented in the PR? (Thanks for the link, I didn't know about the extending option)

neomerx commented 6 years ago

The first solution definitely works. My concern is that it somehow breaks the dependency inversion principle because the json encoder service has to know all implementations of the business objects. I can bypass it and inject the list of subclasses from a different service, but I thought that it would be better if the library can solve this on its own.

I've got a few thoughts on this. Firsltly, if we do

        foreach (array_keys($this->getProviderMappings()) as $schema) {
            if (is_subclass_of($resource, $schema)) {
                return $schema;
            }
        }

then result depend on the random order elements are stored and it will very likely have unintended consequenses for existing users when suddenly the library start making assumptions about model inheritance or interfaces. For this reason it's much safer to keep it as simple as possible and have ability to add support for inheritance, proxies, etc.

I'm not 100% what you thought in the custom container approach.

You can have your own encoder that would understand model relationships the way you think is best for your application. You can replace default behaviour with your own. There are a few samples in tests how to do that. That's not a hack and officially supportted.

Should I replace the Container with the one I implemented in the PR?

If I were you I would just add a method `convertInterfaceMappingToClassMapping(array $mapping): array and used it with the library (or more likely had factory method that did all I wanted and returned EncoderInterface instance). But if you're a real purist then, of course, you should go further and have custom container :smiley:

Anyway, if you have any further questions please don't hesitate to ask. I'm happy to help.

neomerx commented 6 years ago

I'm closing it, but again, if you need any help don't hesitate to ask.