Skip to content
This repository has been archived by the owner on May 29, 2024. It is now read-only.

Multiple hydrator strategy per field #18

Open
santiph opened this issue Jul 29, 2015 · 11 comments
Open

Multiple hydrator strategy per field #18

santiph opened this issue Jul 29, 2015 · 11 comments

Comments

@santiph
Copy link

santiph commented Jul 29, 2015

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 zfcampus/zf-apigility-doctrine#215

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

@veewee
Copy link
Contributor

veewee commented Jul 29, 2015

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
Copy link
Author

santiph commented Jul 29, 2015

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: zfcampus/zf-apigility-doctrine#215 (comment)

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
Copy link
Author

santiph commented Jul 29, 2015

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
Copy link
Contributor

veewee commented Jul 30, 2015

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
Copy link

akomm commented Aug 31, 2015

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
Copy link
Contributor

veewee commented Sep 13, 2015

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
Copy link

akomm commented Sep 14, 2015

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
Copy link
Contributor

veewee commented Sep 14, 2015

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
Copy link

akomm commented Sep 14, 2015

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
Copy link
Contributor

veewee commented Sep 14, 2015

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
Copy link

akomm commented Sep 15, 2015

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.

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

No branches or pull requests

3 participants