phpro / zf-doctrine-hydration-module

Configurable Doctrine hydrators for ZF2
18 stars 33 forks source link

Multiple hydrator strategy per field #18

Open santiph opened 9 years ago

santiph commented 9 years ago

I'm, working with three entities: Albums, Artists and Tracks. They have a many-to-many relationship with doctrine. And I'm trying to update the Artists and Tracks list for a particular venue. The way I'm trying to do so is by using PATCH.

//Adding Artists and Tracks to a specific Album entity
PATCH /Album/:album_id
{
  "artist": [
    {"id":123},
    {"id":456},
    {"id":789}
  ],
  "tracks": [
    {"id":111},
    {"id":222},
    {"id":333}
  ]
}

//Removing a particular Artist and two tracks from this Album.
PATCH /Album/:album_id?collection_action=remove
{
  "artist": [
    {"id":123}
  ]
  "tracks": [
    {"id":111},
    {"id":222}
  ]
}

//Removing a particular Artist and adding one track back to this Album.
PATCH /Album/:album_id?collection_action=remove&collection_remove=artist
{
  "artist": [
    {"id":123}
  ]
  "tracks": [
    {"id":222}
  ]
}

//Removing all artists and tracks from the Album.
//PATCH /Album/:album_id?collection_action=remove&collection_remove=artists,tracks
PATCH /Album/:album_id?collection_action=remove
{
  "artist": [],
  "tracks: []
}

A detailed example of what I'm proposing could be found https://github.com/zfcampus/zf-apigility-doctrine/issues/215

Problem is, where in the modules could I specify this multiple strategies and make them dependent on the query being sent?

veewee commented 9 years ago

You could use one of the default strategies in DoctrineModule. These make it possible to call the add* and remove* methods for collections. The entity will contain the logic to remove and add related entities from your entity. If you want the hydrators to be dependent on custom paramters, like e.g. the URL paramter, you will probably want to create your own application-specific hydrator. You could use an existing hydrator with some custom logic for this.

santiph commented 9 years ago

I don't think it's a good idea to let hydrators be aware of URLs parameters.

Also, how could I use the default strategies from DoctrineModule and specify whether I want to add or remove elements from the collection... or even remove all the elements from the collection? Entities handles the add/remove operationsm but who calls them? in here, there is an example of Tom H Anderson and how he solved it for a project of his: https://github.com/zfcampus/zf-apigility-doctrine/issues/215#issuecomment-123156146

Given that hydrate() only expects one parameter to be a the value (or collection of values) to hydrate the entity with, I think it could be improved by building Add and Remove strategies with the required logic inside.

Do you still think default strategies in DoctrineModule are the answer? Could you please help me understand?

santiph commented 9 years ago

An alternative with a custom hydrator could be:

PATCH /Album/:album_id
{
   "artist":[
      {
         "add":[
            {
               "id":123
            },
            {
               "id":456
            }
         ]
      },
      {
         "remove":[
            {
               "id":789
            }
         ]
      }
   ],
   "tracks":[
      {
         "add":[
            {
               "id":111
            },
            {
               "id":222
            }
         ]
      },
      {
         "remove":[
            {
               "id":333
            }
         ]
      }
   ]
}

Then, a custom hydrator could be set to identify "add" and "remove" parts, and process them accordingly.

veewee commented 9 years ago

Hello @santiph,

Assume following simplified json to start with:

 GET /Albums/:album_id
{
  "artist": [
    {"id":123},
    {"id":456},
    {"id":789}
  ],
  "tracks": [
    {"id":111},
    {"id":222},
    {"id":333}
  ]
}

As you can see the album contains 3 artists and 3 tracks. Next you want to remove an artist and a track, you could patch it like this:

PATCH /Album/:album_id
{
  "artist": [
    {"id":123},
    {"id":456},
  ],
  "tracks": [
    {"id":111},
    {"id":222},
  ]
}

This means that the artist and track property will be replaced with the new data. The ORM hydrators will call the removeArtist() and removeTrack() method on the entity. Not sure if the IDs are also translated to the corresponding entities, but normally they should do this.

If you want to clear a property, you just send an empty array to the server.

With other words: if you PATCH the json, you will always have to specify the full NEW state of the object. If you want the hydrators to work in another way, you will have to write your own custom hydrator.

akomm commented 8 years ago

If you apply a PATCH on a resource, its field- and not value-driven. A collection of associated resources is not a field, but value.

If you want to PATCH the association collection it must become a resource itself. Then you can POST/DELETE (evtl. +list) it.

Regarding multiple strategies: There are cases in which you might need multiple strategies, but independend of any URL. DoctrineHydratorFactory should simply create a "StrategyChain", if the value of the strategy is an array.

Something like that:

namespace module\Stdlib\Hydrator;

use DoctrineModule\Stdlib\Hydrator\Strategy\AbstractCollectionStrategy;
use Zend\Stdlib\Hydrator\Strategy\StrategyChain;

/**
 * Class DoctrineCollectionStrategy
 * @package module\Stdlib\Hydrator
 */
class DoctrineCollectionStrategy extends AbstractCollectionStrategy
{
    /**
     * Strategy chain for extraction
     *
     * @var StrategyInterface[]
     */
    private $extractionStrategies;

    /**
     * Strategy chain for hydration
     *
     * @var StrategyInterface[]
     */
    private $hydrationStrategies;

    /**
     * Constructor
     *
     * @param array|\Traversable $extractionStrategies
     */
    public function __construct($extractionStrategies)
    {
        $extractionStrategies = ArrayUtils::iteratorToArray($extractionStrategies);

        $this->extractionStrategies = array_map(
            function (StrategyInterface $strategy) {
                // this callback is here only to ensure type-safety
                return $strategy;
            },
            $extractionStrategies
        );

        $this->hydrationStrategies = array_reverse($extractionStrategies);
    }

    /**
     * @param ClassMetadata $classMetadata
     * @return AbstractCollectionStrategy
     */
    public function setClassMetadata(ClassMetadata $classMetadata)
    {
        foreach ($this->hydrationStrategies as $hydrationStrategy) {
            if ($hydrationStrategy instanceof AbstractCollectionStrategy) {
                $hydrationStrategy->setClassMetadata($classMetadata);
            }
        }

        return parent::setClassMetadata($classMetadata);
    }

    /**
     * @param string $collectionName
     * @return AbstractCollectionStrategy
     */
    public function setCollectionName($collectionName)
    {
        foreach ($this->hydrationStrategies as $hydrationStrategy) {
            if ($hydrationStrategy instanceof AbstractCollectionStrategy) {
                $hydrationStrategy->setCollectionName($collectionName);
            }
        }

        return parent::setCollectionName($collectionName);
    }

    /**
     * @param object $object
     * @return AbstractCollectionStrategy
     */
    public function setObject($object)
    {
        foreach ($this->hydrationStrategies as $hydrationStrategy) {
            if ($hydrationStrategy instanceof AbstractCollectionStrategy) {
                $hydrationStrategy->setObject($object);
            }
        }

        return parent::setObject($object);
    }

    /**
     * {@inheritDoc}
     */
    public function extract($value)
    {
        foreach ($this->extractionStrategies as $strategy) {
            $value = $strategy->extract($value);
        }

        return $value;
    }

    /**
     * {@inheritDoc}
     */
    public function hydrate($value)
    {
        foreach ($this->hydrationStrategies as $strategy) {
            $value = $strategy->hydrate($value) ?: $value;
        }

        return $value;
    }
}

If you think that makes sense, I would implement it the "proper" way (more generic) and make a PR.

Note: i'm not that happy with that part:

$value = $strategy->hydrate($value) ?: $value;

But otherwise some implementations, which depend on doctrine-hydration-module would break. For example the CollectionExtraft of zf-apigility-doctrine does not return the value (hydrate method), which evaluates to NULL:

https://github.com/zfcampus/zf-apigility-doctrine/blob/master/src/Server/Hydrator/Strategy/CollectionExtract.php#L26-L32

I wanted a working example.

veewee commented 8 years ago

Sorry for the late response, I was on a 2 weeks vacation. Can you please explain to me why you need this chain of strategies? If you want to hydrate or extract a field, you should normally do this in one specific way. It does not make sense to me to extract or hydrate in steps.

For example: If you have a datetime field that you want to extract, you don't want to convert it first to int and next to some custom Y-m-d format. You just want to extract it directly to the Y-m-d format. The same rules apply for entities: you don't want to convert an entity to a numeric ID and next to a GUID or something.

I am looking forward to your look on this.

akomm commented 8 years ago

For the same reason, why HydratorInterface, which you use, implements both ExtractionInterface and HydrationInterface:

https://github.com/zendframework/zend-stdlib/blob/master/src/Hydrator/HydratorInterface.php

Example: zf-apigility-doctrine specifies one hydrator for entities, which is used for both extraction and hydration, which is the whole point of using an hydrator interface, which implements both.

Otherwise, you would have to create an hydrator and extractor and possibly make same configuration for two different objects with slight differences, which would create a lot overhead and boilerplate.

The extraction is used on GET, the hydration is used on POST and PUT.

veewee commented 8 years ago

Hello @akomm,

It looks like I did not phrase the question very wel. Let me try again: Why do you want to use a chain? Can you give me a valid use case for this? Normally I would expect a strategy to do one thing only, not multiple things.

Thanks!

akomm commented 8 years ago

Example, if you want to use both:

https://github.com/doctrine/DoctrineModule/blob/master/src/DoctrineModule/Stdlib/Hydrator/Strategy/AllowRemoveByValue.php

and:

https://github.com/soliantconsulting/zf-apigility-doctrine-server/blob/master/src/ZF/Apigility/Doctrine/Server/Hydrator/Strategy/CollectionExtract.php

And you don't want to copy & paste code into a custom class which has both implementations. Assume the code is changed in new version, my copy & pasted would be broken or contain issues, which were fixed.

It is also a problem if you create a class, which internaly invokes the extract from the one strategy and hydrate from another, because they have lot of initialization currently and you would have to proxy them from the "custom" strategy, which aggregates them (all the mapping and collection/object related data).

veewee commented 8 years ago

Hi @akomm, That seems like a valid use-case. If you want, you can work it out and create a PR for this strategy chain.

Thanks!

akomm commented 8 years ago

Ok. I'm on it, but the example is not a good enough so I will have to make some adjustments and test to see if it actually works out.

I see a prob using chains with some strategy implementations currently around. Example: the already mentioned CollectionExtract, which returns nothing in extract, which would evaluate as null and "break" the result, depending on the order in the chain. But this is rather an issue of the implementation itself - I have to check if this is true too, but I assume it should simply return the value unchanged instead of returning nothing.