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

Problem with the resource transformer design pattern and cross relations #67

Open
gfemorris opened this issue Dec 18, 2017 · 9 comments

Comments

@gfemorris
Copy link

When using cross relations between 2 resources the current design pattern used by the yin resource transformers has the issue of circular dependencies. I think it is necessary to cross relate objects. For example:

  • You have a book object and want to know the author(s)
  • You have an author object and want two know books written by the author

I solved this in my case outside of the lib with injecting just a container into the transformers which can require the transformers on demand. But in my opinion this should be solved by the lib itself.

The neomerx lib solves this by mapping the transformers "hard" with the models.. but i liked your modular "loose" approach here.. so maybe a factory patter where one needs to register all transformers and a factory where you can get them whenever you need them could solve the problem.

@gfemorris
Copy link
Author

Hi, sorry to bother you again...
it would be very nice if you could check this, because i think it is a bigger issue which everyone will face sooner or later. It would be best if it would be solved inside the lib, everything else feels like a dirty workaround.

Thanks anyway for this great lib and your hard work!!!

@kocsismate
Copy link
Member

kocsismate commented Mar 9, 2018

Hey,

Sorry for leaving your issue unanswered so long, and thanks for bringing this up again. In the current months I am overwhelmed with other tasks and really busy at work. :/ So I didn't have possibility to think about the solution and respond you.

I'll answer you in detail in some days (and definitely not in many days)!

@gfemorris
Copy link
Author

Hey,
thanks for your reply. You just answer whenever you have time..
I just wanted to bring it up again so that it is not forgotten.

@kocsismate
Copy link
Member

kocsismate commented Mar 19, 2018

I have thought about the problem since our last messages, and I believe, it should be solved outside of the library indeed. Here are the solutions which might work for you in my opinion:

  • The quick and dirty solution might be using property or setter injection if you have a DI Container. These methods effectively mitigate the problem of circular dependencies. Usually, I don't recommend these dependency injection types because they make a class not easy to unit test, but in this case, it's not a serious issue as JSON:API transformation is not usually unit tested.

  • A child class of an AbstractTransformer can have any dependency so you can implement your own BookTransfomerFactory which can create an AuthorTransformer or BookTransformer class. I admit that a Factory like that might not be easy to implement.

What do you think about these ideas? To be honest, I don't like the idea to register transformers because that would require an own Service Locator to be implemented and a plain old DI Container is just perfect for this purpose.

Sorry for the late response again.

@gfemorris
Copy link
Author

Sorry didn't have the time to think it through.. but i will reply in detail soon... thanks for your ideas.

@guilhermeaiolfi
Copy link

I don't think creating the graph of how transformers are related is the best alternative. I think you should specify the Transformer class when you create relationships. So it would looks like:

public function getRelationships($book): array
    {
        return [
            "authors" => function (array $book) {
                return ToManyRelationship::create()
                    ->setLinks(
                        Links::createWithoutBaseUri()->setSelf(new Link("/books/relationships/authors"))
                    )
                    ->setData($book["authors"], \AuthorTransformer)
                ;
            },
            "publisher" => function ($book) {
                return ToOneRelationship::create()
                    ->setLinks(
                        Links::createWithoutBaseUri()->setSelf(new Link("/books/relationships/publisher"))
                    )
                    ->setData($book["publisher"], \PublisherTransformer)
                ;
            }
        ];
    }

You could instantiate them as needed and remove the need for this:

// Instantiating a book document
    $document = new BookDocument(
        new BookResourceTransformer(
            new AuthorResourceTransformer(),
            new PublisherResourceTransformer()
        )
    );

@gfemorris
Copy link
Author

@guilhermeaiolfi Yes i came up with that idea as well and actually solved it like that in another project. Sometimes the easy solutions are the hardest to find.. :D
And i totally agree, that this is the best approach here. The only thing considering would be, that we will end up with a lot of transformer instances. Transformers are stateless so they could be singletons without any issues.
Maybe @kocsismate should just change the examples to that easy approach because i'm sure a lot of people are running into the same issues here.

@jakagacic
Copy link
Contributor

I know it's been a while, but I came across the same problem yesterday. I have an Offer entity that has many Images and each Image entity is related back to an Offer. I wanted to be able to include both Images with Offer and Offer with Image. After trying and failing for a while, I got it to work using Symfony's setter injection. The main trick is not to inject anything in the constructor because this is what makes the reference circle.

In OfferResourceTransformer add the setter for ImageResourceTransformer:

    private $imageResourceTransformer;

    public function setImageResourceTransformer(ImageResourceTransformer $imageResourceTransformer): void
    {
        $this->imageResourceTransformer = $imageResourceTransformer;
    }

In ImageResourceTransformer add the setter for OfferResourceTransformer:

    private $offerResourceTransformer;

    public function setOfferResourceTransformer(OfferResourceTransformer $offerResourceTransformer): void
    {
        $this->offerResourceTransformer = $offerResourceTransformer;
    }

In services.yaml add:

    App\JsonApi\Transformer\OfferResourceTransformer:
      calls:
        - method: setImageResourceTransformer

    App\JsonApi\Transformer\ImageResourceTransformer:
      calls:
        - method: setOfferResourceTransformer

And now it works like a charm!

@kocsismate
Copy link
Member

kocsismate commented Mar 29, 2019

Hi @jakagacic,

Thanks for sharing your example!

To give an answer to the prior discussion: as @guilhermeaiolfi suggested, transformers are not at all needed to be instantiated ahead of time, a DI container can easily do the job on-the-fly.

I don't plan to add better support for service location of transformers in Yin as I believe it can be still conveniently done in user-land code. However, I agree with @gfemorris , it could be noted in the examples and also in the read me that you have a choice when to instantiate the transformers (ahead-of-time via Dependency Injection or just-in-time via Service Location).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants