From 66d241099984810e26ae809629b23e7dcade116f Mon Sep 17 00:00:00 2001 From: Tim Nagel Date: Mon, 16 Jun 2014 15:57:27 +1000 Subject: [PATCH 1/4] Move Indexable callback calculations to a new service --- DependencyInjection/Configuration.php | 17 +- DependencyInjection/FOSElasticaExtension.php | 24 ++- Doctrine/Listener.php | 172 +++++------------- Persister/ObjectPersister.php | 21 +++ Provider/Indexable.php | 174 +++++++++++++++++++ Provider/IndexableInterface.php | 25 +++ Resources/config/config.xml | 5 + Resources/config/mongodb.xml | 3 +- Resources/config/orm.xml | 6 +- Tests/Doctrine/AbstractListenerTest.php | 142 +++++++-------- Tests/Functional/IndexableCallbackTest.php | 51 ++++++ Tests/Functional/TypeObj.php | 25 +++ Tests/Functional/app/Basic/config.yml | 2 +- Tests/Functional/app/ORM/bundles.php | 11 ++ Tests/Functional/app/ORM/config.yml | 44 +++++ Tests/Provider/IndexableTest.php | 91 ++++++++++ composer.json | 1 + 17 files changed, 584 insertions(+), 230 deletions(-) create mode 100644 Provider/Indexable.php create mode 100644 Provider/IndexableInterface.php create mode 100644 Tests/Functional/IndexableCallbackTest.php create mode 100644 Tests/Functional/TypeObj.php create mode 100644 Tests/Functional/app/ORM/bundles.php create mode 100644 Tests/Functional/app/ORM/config.yml create mode 100644 Tests/Provider/IndexableTest.php diff --git a/DependencyInjection/Configuration.php b/DependencyInjection/Configuration.php index ce1c982..f32f295 100644 --- a/DependencyInjection/Configuration.php +++ b/DependencyInjection/Configuration.php @@ -187,9 +187,23 @@ class Configuration implements ConfigurationInterface return $v; }) ->end() + ->beforeNormalization() + ->ifTrue(function ($v) { + return isset($v['persistence']) && + isset($v['persistence']['listener']) && + isset($v['persistence']['listener']['is_indexable_callback']); + }) + ->then(function ($v) { + $v['indexable_callback'] = $v['persistence']['listener']['is_indexable_callback']; + unset($v['persistence']['listener']['is_indexable_callback']); + + return $v; + }) + ->end() ->children() ->scalarNode('index_analyzer')->end() ->scalarNode('search_analyzer')->end() + ->scalarNode('indexable_callback')->end() ->append($this->getPersistenceNode()) ->append($this->getSerializerNode()) ->end() @@ -230,7 +244,7 @@ class Configuration implements ConfigurationInterface unset($v[$prop]); } } - + return $v; }) ->end() @@ -674,7 +688,6 @@ class Configuration implements ConfigurationInterface ->treatTrueLike('fos_elastica.logger') ->end() ->scalarNode('service')->end() - ->variableNode('is_indexable_callback')->defaultNull()->end() ->end() ->end() ->arrayNode('finder') diff --git a/DependencyInjection/FOSElasticaExtension.php b/DependencyInjection/FOSElasticaExtension.php index 1529544..9e50c0b 100644 --- a/DependencyInjection/FOSElasticaExtension.php +++ b/DependencyInjection/FOSElasticaExtension.php @@ -186,8 +186,11 @@ class FOSElasticaExtension extends Extension */ protected function loadTypes(array $types, ContainerBuilder $container, $indexName, $indexId, array $typePrototypeConfig) { + $indexableCallbacks = array(); + foreach ($types as $name => $type) { $type = self::deepArrayUnion($typePrototypeConfig, $type); + $typeName = sprintf('%s/%s', $indexName, $name); $typeId = sprintf('%s.%s', $indexId, $name); $typeDefArgs = array($name); $typeDef = new Definition('%fos_elastica.type.class%', $typeDefArgs); @@ -240,7 +243,6 @@ class FOSElasticaExtension extends Extension } if (isset($type['_parent'])) { $this->indexConfigs[$indexName]['config']['properties'][$name]['_parent'] = array('type' => $type['_parent']['type']); - $typeName = sprintf('%s/%s', $indexName, $name); $this->typeFields[$typeName]['_parent'] = $type['_parent']; } if (isset($type['persistence'])) { @@ -252,6 +254,9 @@ class FOSElasticaExtension extends Extension if (isset($type['search_analyzer'])) { $this->indexConfigs[$indexName]['config']['properties'][$name]['search_analyzer'] = $type['search_analyzer']; } + if (isset($type['indexable_callback'])) { + $indexableCallbacks[$typeName] = $type['indexable_callback']; + } if (isset($type['index'])) { $this->indexConfigs[$indexName]['config']['properties'][$name]['index'] = $type['index']; } @@ -271,6 +276,9 @@ class FOSElasticaExtension extends Extension } } } + + $indexable = $container->getDefinition('fos_elastica.indexable'); + $indexable->replaceArgument(0, $indexableCallbacks); } /** @@ -431,8 +439,7 @@ class FOSElasticaExtension extends Extension $listenerId = sprintf('fos_elastica.listener.%s.%s', $indexName, $typeName); $listenerDef = new DefinitionDecorator($abstractListenerId); $listenerDef->replaceArgument(0, new Reference($objectPersisterId)); - $listenerDef->replaceArgument(1, $typeConfig['model']); - $listenerDef->replaceArgument(2, $this->getDoctrineEvents($typeConfig)); + $listenerDef->replaceArgument(1, $this->getDoctrineEvents($typeConfig)); $listenerDef->replaceArgument(3, $typeConfig['identifier']); if ($typeConfig['listener']['logger']) { $listenerDef->replaceArgument(4, new Reference($typeConfig['listener']['logger'])); @@ -442,18 +449,7 @@ class FOSElasticaExtension extends Extension case 'orm': $listenerDef->addTag('doctrine.event_subscriber'); break; case 'mongodb': $listenerDef->addTag('doctrine_mongodb.odm.event_subscriber'); break; } - if (isset($typeConfig['listener']['is_indexable_callback'])) { - $callback = $typeConfig['listener']['is_indexable_callback']; - if (is_array($callback)) { - list($class) = $callback + array(null); - if (is_string($class) && !class_exists($class)) { - $callback[0] = new Reference($class); - } - } - - $listenerDef->addMethodCall('setIsIndexableCallback', array($callback)); - } $container->setDefinition($listenerId, $listenerDef); return $listenerId; diff --git a/Doctrine/Listener.php b/Doctrine/Listener.php index ff9fc60..4a01aa1 100644 --- a/Doctrine/Listener.php +++ b/Doctrine/Listener.php @@ -2,15 +2,13 @@ namespace FOS\ElasticaBundle\Doctrine; -use Psr\Log\LoggerInterface; use Doctrine\Common\EventArgs; use Doctrine\Common\EventSubscriber; use FOS\ElasticaBundle\Persister\ObjectPersisterInterface; use FOS\ElasticaBundle\Persister\ObjectPersister; -use Symfony\Component\ExpressionLanguage\Expression; -use Symfony\Component\ExpressionLanguage\ExpressionLanguage; -use Symfony\Component\ExpressionLanguage\SyntaxError; +use FOS\ElasticaBundle\Provider\IndexableInterface; use Symfony\Component\PropertyAccess\PropertyAccess; +use Symfony\Component\PropertyAccess\PropertyAccessorInterface; /** * Automatically update ElasticSearch based on changes to the Doctrine source @@ -25,13 +23,6 @@ class Listener implements EventSubscriber */ protected $objectPersister; - /** - * Class of the domain model - * - * @var string - */ - protected $objectClass; - /** * List of subscribed events * @@ -46,13 +37,6 @@ class Listener implements EventSubscriber */ protected $esIdentifierField; - /** - * Callback for determining if an object should be indexed - * - * @var mixed - */ - protected $isIndexableCallback; - /** * Objects scheduled for insertion and replacement */ @@ -64,13 +48,6 @@ class Listener implements EventSubscriber */ public $scheduledForDeletion = array(); - /** - * An instance of ExpressionLanguage - * - * @var ExpressionLanguage - */ - protected $expressionLanguage; - /** * PropertyAccessor instance * @@ -78,26 +55,36 @@ class Listener implements EventSubscriber */ protected $propertyAccessor; + /** + * @var \FOS\ElasticaBundle\Provider\IndexableInterface + */ + private $indexable; + /** * Constructor. * * @param ObjectPersisterInterface $objectPersister - * @param string $objectClass - * @param array $events - * @param string $esIdentifierField + * @param array $events + * @param IndexableInterface $indexable + * @param string $esIdentifierField + * @param null $logger */ - public function __construct(ObjectPersisterInterface $objectPersister, $objectClass, array $events, $esIdentifierField = 'id', $logger = null) - { - $this->objectPersister = $objectPersister; - $this->objectClass = $objectClass; - $this->events = $events; - $this->esIdentifierField = $esIdentifierField; + public function __construct( + ObjectPersisterInterface $objectPersister, + array $events, + IndexableInterface $indexable, + $esIdentifierField = 'id', + $logger = null + ) { + $this->esIdentifierField = $esIdentifierField; + $this->events = $events; + $this->indexable = $indexable; + $this->objectPersister = $objectPersister; + $this->propertyAccessor = PropertyAccess::createPropertyAccessor(); if ($logger) { $this->objectPersister->setLogger($logger); } - - $this->propertyAccessor = PropertyAccess::createPropertyAccessor(); } /** @@ -108,82 +95,6 @@ class Listener implements EventSubscriber return $this->events; } - /** - * Set the callback for determining object index eligibility. - * - * If callback is a string, it must be public method on the object class - * that expects no arguments and returns a boolean. Otherwise, the callback - * should expect the object for consideration as its only argument and - * return a boolean. - * - * @param callback $callback - * @throws \RuntimeException if the callback is not callable - */ - public function setIsIndexableCallback($callback) - { - if (is_string($callback)) { - if (!is_callable(array($this->objectClass, $callback))) { - if (false !== ($expression = $this->getExpressionLanguage())) { - $callback = new Expression($callback); - try { - $expression->compile($callback, array($this->getExpressionVar())); - } catch (SyntaxError $e) { - throw new \RuntimeException(sprintf('Indexable callback %s::%s() is not callable or a valid expression.', $this->objectClass, $callback), 0, $e); - } - } else { - throw new \RuntimeException(sprintf('Indexable callback %s::%s() is not callable.', $this->objectClass, $callback)); - } - } - } elseif (!is_callable($callback)) { - if (is_array($callback)) { - list($class, $method) = $callback + array(null, null); - if (is_object($class)) { - $class = get_class($class); - } - - if ($class && $method) { - throw new \RuntimeException(sprintf('Indexable callback %s::%s() is not callable.', $class, $method)); - } - } - throw new \RuntimeException('Indexable callback is not callable.'); - } - - $this->isIndexableCallback = $callback; - } - - /** - * Return whether the object is indexable with respect to the callback. - * - * @param object $object - * @return boolean - */ - protected function isObjectIndexable($object) - { - if (!$this->isIndexableCallback) { - return true; - } - - if ($this->isIndexableCallback instanceof Expression) { - return $this->getExpressionLanguage()->evaluate($this->isIndexableCallback, array($this->getExpressionVar($object) => $object)); - } - - return is_string($this->isIndexableCallback) - ? call_user_func(array($object, $this->isIndexableCallback)) - : call_user_func($this->isIndexableCallback, $object); - } - - /** - * @param mixed $object - * @return string - */ - private function getExpressionVar($object = null) - { - $class = $object ?: $this->objectClass; - $ref = new \ReflectionClass($class); - - return strtolower($ref->getShortName()); - } - /** * Provides unified method for retrieving a doctrine object from an EventArgs instance * @@ -204,27 +115,11 @@ class Listener implements EventSubscriber throw new \RuntimeException('Unable to retrieve object from EventArgs.'); } - /** - * @return bool|ExpressionLanguage - */ - private function getExpressionLanguage() - { - if (null === $this->expressionLanguage) { - if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) { - return false; - } - - $this->expressionLanguage = new ExpressionLanguage(); - } - - return $this->expressionLanguage; - } - public function postPersist(EventArgs $eventArgs) { $entity = $this->getDoctrineObject($eventArgs); - if ($entity instanceof $this->objectClass && $this->isObjectIndexable($entity)) { + if ($this->objectPersister->handlesObject($entity) && $this->isObjectIndexable($entity)) { $this->scheduledForInsertion[] = $entity; } } @@ -233,7 +128,7 @@ class Listener implements EventSubscriber { $entity = $this->getDoctrineObject($eventArgs); - if ($entity instanceof $this->objectClass) { + if ($this->objectPersister->handlesObject($entity)) { if ($this->isObjectIndexable($entity)) { $this->scheduledForUpdate[] = $entity; } else { @@ -251,7 +146,7 @@ class Listener implements EventSubscriber { $entity = $this->getDoctrineObject($eventArgs); - if ($entity instanceof $this->objectClass) { + if ($this->objectPersister->handlesObject($entity)) { $this->scheduleForDeletion($entity); } } @@ -305,4 +200,19 @@ class Listener implements EventSubscriber $this->scheduledForDeletion[] = $identifierValue; } } + + /** + * Checks if the object is indexable or not. + * + * @param object $object + * @return bool + */ + private function isObjectIndexable($object) + { + return $this->indexable->isObjectIndexable( + $this->objectPersister->getType()->getIndex()->getName(), + $this->objectPersister->getType()->getName(), + $object + ); + } } diff --git a/Persister/ObjectPersister.php b/Persister/ObjectPersister.php index c279ec7..2b6a8af 100644 --- a/Persister/ObjectPersister.php +++ b/Persister/ObjectPersister.php @@ -31,6 +31,27 @@ class ObjectPersister implements ObjectPersisterInterface $this->fields = $fields; } + /** + * @internal Temporary method that will be removed. + * + * @return Type + */ + public function getType() + { + return $this->type; + } + + /** + * If the ObjectPersister handles a given object. + * + * @param object $object + * @return bool + */ + public function handlesObject($object) + { + return $object instanceof $this->objectClass; + } + public function setLogger(LoggerInterface $logger) { $this->logger = $logger; diff --git a/Provider/Indexable.php b/Provider/Indexable.php new file mode 100644 index 0000000..b388c58 --- /dev/null +++ b/Provider/Indexable.php @@ -0,0 +1,174 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FOS\ElasticaBundle\Provider; + +use Symfony\Component\ExpressionLanguage\Expression; +use Symfony\Component\ExpressionLanguage\ExpressionLanguage; +use Symfony\Component\ExpressionLanguage\SyntaxError; +use Symfony\Component\PropertyAccess\PropertyAccess; +use Symfony\Component\PropertyAccess\PropertyAccessorInterface; + +class Indexable implements IndexableInterface +{ + /** + * An array of raw configured callbacks for all types. + * + * @var array + */ + private $callbacks = array(); + + /** + * An instance of ExpressionLanguage + * + * @var ExpressionLanguage + */ + private $expressionLanguage; + + /** + * An array of initialised callbacks. + * + * @var array + */ + private $initialisedCallbacks = array(); + + /** + * PropertyAccessor instance + * + * @var PropertyAccessorInterface + */ + private $propertyAccessor; + + /** + * @param array $callbacks + */ + public function __construct(array $callbacks) + { + $this->callbacks = $callbacks; + $this->propertyAccessor = PropertyAccess::createPropertyAccessor(); + } + + /** + * Return whether the object is indexable with respect to the callback. + * + * @param string $indexName + * @param string $typeName + * @param mixed $object + * @return bool + */ + public function isObjectIndexable($indexName, $typeName, $object) + { + $type = sprintf('%s/%s', $indexName, $typeName); + $callback = $this->getCallback($type, $object); + if (!$callback) { + return true; + } + + if ($callback instanceof Expression) { + return $this->getExpressionLanguage()->evaluate($callback, array( + 'object' => $object, + $this->getExpressionVar($object) => $object + )); + } + + return is_string($callback) + ? call_user_func(array($object, $callback)) + : call_user_func($callback, $object); + } + + /** + * Builds and initialises a callback. + * + * @param string $type + * @param object $object + * @return mixed + */ + private function buildCallback($type, $object) + { + if (!array_key_exists($type, $this->callbacks)) { + throw new \InvalidArgumentException(sprintf('Callback for type "%s" is not configured', $type)); + } + + $callback = $this->callbacks[$type]; + + if (is_callable($callback) or is_callable(array($object, $callback))) { + return $callback; + } + + if (is_array($callback)) { + list($class, $method) = $callback + array(null, null); + if (is_object($class)) { + $class = get_class($class); + } + + if ($class && $method) { + throw new \InvalidArgumentException(sprintf('Callback for type "%s", "%s::%s()", is not callable.', $type, $class, $method)); + } + } + + if (is_string($callback) && $expression = $this->getExpressionLanguage()) { + $callback = new Expression($callback); + + try { + $expression->compile($callback, array('object', $this->getExpressionVar($object))); + + return $callback; + } catch (SyntaxError $e) { + throw new \InvalidArgumentException(sprintf('Callback for type "%s" is an invalid expression', $type), $e->getCode(), $e); + } + } + + throw new \InvalidArgumentException(sprintf('Callback for type "%s" is not a valid callback.', $type)); + } + + /** + * Retreives a cached callback, or creates a new callback if one is not found. + * + * @param string $type + * @param object $object + * @return mixed + */ + private function getCallback($type, $object) + { + if (!array_key_exists($type, $this->initialisedCallbacks)) { + $this->initialisedCallbacks[$type] = $this->buildCallback($type, $object); + } + + return $this->initialisedCallbacks[$type]; + } + + /** + * @return bool|ExpressionLanguage + */ + private function getExpressionLanguage() + { + if (null === $this->expressionLanguage) { + if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) { + return false; + } + + $this->expressionLanguage = new ExpressionLanguage(); + } + + return $this->expressionLanguage; + } + + /** + * @param mixed $object + * @return string + */ + private function getExpressionVar($object = null) + { + $ref = new \ReflectionClass($object); + + return strtolower($ref->getShortName()); + } +} diff --git a/Provider/IndexableInterface.php b/Provider/IndexableInterface.php new file mode 100644 index 0000000..4871b58 --- /dev/null +++ b/Provider/IndexableInterface.php @@ -0,0 +1,25 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FOS\ElasticaBundle\Provider; + +interface IndexableInterface +{ + /** + * Checks if an object passed should be indexable or not. + * + * @param string $indexName + * @param string $typeName + * @param mixed $object + * @return bool + */ + public function isObjectIndexable($indexName, $typeName, $object); +} diff --git a/Resources/config/config.xml b/Resources/config/config.xml index 7687250..f4b2606 100644 --- a/Resources/config/config.xml +++ b/Resources/config/config.xml @@ -9,6 +9,7 @@ FOS\ElasticaBundle\DynamicIndex Elastica\Type FOS\ElasticaBundle\IndexManager + FOS\ElasticaBundle\Provider\Indexable FOS\ElasticaBundle\Resetter FOS\ElasticaBundle\Finder\TransformedFinder FOS\ElasticaBundle\Logger\ElasticaLogger @@ -44,6 +45,10 @@ + + + + diff --git a/Resources/config/mongodb.xml b/Resources/config/mongodb.xml index 0af7aa1..e575e5d 100644 --- a/Resources/config/mongodb.xml +++ b/Resources/config/mongodb.xml @@ -15,9 +15,10 @@ - + + diff --git a/Resources/config/orm.xml b/Resources/config/orm.xml index 5bd16e5..43d1670 100644 --- a/Resources/config/orm.xml +++ b/Resources/config/orm.xml @@ -15,10 +15,10 @@ - - - + + + diff --git a/Tests/Doctrine/AbstractListenerTest.php b/Tests/Doctrine/AbstractListenerTest.php index ee657f1..de5ba0c 100644 --- a/Tests/Doctrine/AbstractListenerTest.php +++ b/Tests/Doctrine/AbstractListenerTest.php @@ -11,12 +11,12 @@ abstract class ListenerTest extends \PHPUnit_Framework_TestCase { public function testObjectInsertedOnPersist() { - $persister = $this->getMockPersister(); - $entity = new Listener\Entity(1); + $persister = $this->getMockPersister($entity, 'index', 'type'); $eventArgs = $this->createLifecycleEventArgs($entity, $this->getMockObjectManager()); + $indexable = $this->getMockIndexable('index', 'type', $entity, true); - $listener = $this->createListener($persister, get_class($entity), array()); + $listener = $this->createListener($persister, array(), $indexable); $listener->postPersist($eventArgs); $this->assertEquals($entity, current($listener->scheduledForInsertion)); @@ -28,18 +28,14 @@ abstract class ListenerTest extends \PHPUnit_Framework_TestCase $listener->postFlush($eventArgs); } - /** - * @dataProvider provideIsIndexableCallbacks - */ - public function testNonIndexableObjectNotInsertedOnPersist($isIndexableCallback) + public function testNonIndexableObjectNotInsertedOnPersist() { - $persister = $this->getMockPersister(); - - $entity = new Listener\Entity(1, false); + $entity = new Listener\Entity(1); + $persister = $this->getMockPersister($entity, 'index', 'type'); $eventArgs = $this->createLifecycleEventArgs($entity, $this->getMockObjectManager()); + $indexable = $this->getMockIndexable('index', 'type', $entity, false); - $listener = $this->createListener($persister, get_class($entity), array()); - $listener->setIsIndexableCallback($isIndexableCallback); + $listener = $this->createListener($persister, array(), $indexable); $listener->postPersist($eventArgs); $this->assertEmpty($listener->scheduledForInsertion); @@ -54,12 +50,12 @@ abstract class ListenerTest extends \PHPUnit_Framework_TestCase public function testObjectReplacedOnUpdate() { - $persister = $this->getMockPersister(); - $entity = new Listener\Entity(1); + $persister = $this->getMockPersister($entity, 'index', 'type'); $eventArgs = $this->createLifecycleEventArgs($entity, $this->getMockObjectManager()); + $indexable = $this->getMockIndexable('index', 'type', $entity, true); - $listener = $this->createListener($persister, get_class($entity), array()); + $listener = $this->createListener($persister, array(), $indexable); $listener->postUpdate($eventArgs); $this->assertEquals($entity, current($listener->scheduledForUpdate)); @@ -73,17 +69,15 @@ abstract class ListenerTest extends \PHPUnit_Framework_TestCase $listener->postFlush($eventArgs); } - /** - * @dataProvider provideIsIndexableCallbacks - */ - public function testNonIndexableObjectRemovedOnUpdate($isIndexableCallback) + public function testNonIndexableObjectRemovedOnUpdate() { $classMetadata = $this->getMockClassMetadata(); $objectManager = $this->getMockObjectManager(); - $persister = $this->getMockPersister(); - $entity = new Listener\Entity(1, false); + $entity = new Listener\Entity(1); + $persister = $this->getMockPersister($entity, 'index', 'type'); $eventArgs = $this->createLifecycleEventArgs($entity, $objectManager); + $indexable = $this->getMockIndexable('index', 'type', $entity, false); $objectManager->expects($this->any()) ->method('getClassMetadata') @@ -95,8 +89,7 @@ abstract class ListenerTest extends \PHPUnit_Framework_TestCase ->with($entity, 'id') ->will($this->returnValue($entity->getId())); - $listener = $this->createListener($persister, get_class($entity), array()); - $listener->setIsIndexableCallback($isIndexableCallback); + $listener = $this->createListener($persister, array(), $indexable); $listener->postUpdate($eventArgs); $this->assertEmpty($listener->scheduledForUpdate); @@ -115,10 +108,11 @@ abstract class ListenerTest extends \PHPUnit_Framework_TestCase { $classMetadata = $this->getMockClassMetadata(); $objectManager = $this->getMockObjectManager(); - $persister = $this->getMockPersister(); $entity = new Listener\Entity(1); + $persister = $this->getMockPersister($entity, 'index', 'type'); $eventArgs = $this->createLifecycleEventArgs($entity, $objectManager); + $indexable = $this->getMockIndexable('index', 'type', $entity); $objectManager->expects($this->any()) ->method('getClassMetadata') @@ -130,7 +124,7 @@ abstract class ListenerTest extends \PHPUnit_Framework_TestCase ->with($entity, 'id') ->will($this->returnValue($entity->getId())); - $listener = $this->createListener($persister, get_class($entity), array()); + $listener = $this->createListener($persister, array(), $indexable); $listener->preRemove($eventArgs); $this->assertEquals($entity->getId(), current($listener->scheduledForDeletion)); @@ -146,11 +140,12 @@ abstract class ListenerTest extends \PHPUnit_Framework_TestCase { $classMetadata = $this->getMockClassMetadata(); $objectManager = $this->getMockObjectManager(); - $persister = $this->getMockPersister(); $entity = new Listener\Entity(1); $entity->identifier = 'foo'; + $persister = $this->getMockPersister($entity, 'index', 'type'); $eventArgs = $this->createLifecycleEventArgs($entity, $objectManager); + $indexable = $this->getMockIndexable('index', 'type', $entity); $objectManager->expects($this->any()) ->method('getClassMetadata') @@ -162,7 +157,7 @@ abstract class ListenerTest extends \PHPUnit_Framework_TestCase ->with($entity, 'identifier') ->will($this->returnValue($entity->getId())); - $listener = $this->createListener($persister, get_class($entity), array(), 'identifier'); + $listener = $this->createListener($persister, array(), $indexable, 'identifier'); $listener->preRemove($eventArgs); $this->assertEquals($entity->identifier, current($listener->scheduledForDeletion)); @@ -174,36 +169,6 @@ abstract class ListenerTest extends \PHPUnit_Framework_TestCase $listener->postFlush($eventArgs); } - /** - * @dataProvider provideInvalidIsIndexableCallbacks - * @expectedException \RuntimeException - */ - public function testInvalidIsIndexableCallbacks($isIndexableCallback) - { - $listener = $this->createListener($this->getMockPersister(), 'FOS\ElasticaBundle\Tests\Doctrine\Listener\Entity', array()); - $listener->setIsIndexableCallback($isIndexableCallback); - } - - public function provideInvalidIsIndexableCallbacks() - { - return array( - array('nonexistentEntityMethod'), - array(array(new Listener\IndexableDecider(), 'internalMethod')), - array(42), - array('entity.getIsIndexable() && nonexistentEntityFunction()'), - ); - } - - public function provideIsIndexableCallbacks() - { - return array( - array('getIsIndexable'), - array(array(new Listener\IndexableDecider(), 'isIndexable')), - array(function(Listener\Entity $entity) { return $entity->getIsIndexable(); }), - array('entity.getIsIndexable()') - ); - } - abstract protected function getLifecycleEventArgsClass(); abstract protected function getListenerClass(); @@ -240,9 +205,48 @@ abstract class ListenerTest extends \PHPUnit_Framework_TestCase ->getMock(); } - private function getMockPersister() + private function getMockPersister($object, $indexName, $typeName) { - return $this->getMock('FOS\ElasticaBundle\Persister\ObjectPersisterInterface'); + $mock = $this->getMockBuilder('FOS\ElasticaBundle\Persister\ObjectPersister') + ->disableOriginalConstructor() + ->getMock(); + + $mock->expects($this->any()) + ->method('handlesObject') + ->with($object) + ->will($this->returnValue(true)); + + $index = $this->getMockBuilder('Elastica\Index')->disableOriginalConstructor()->getMock(); + $index->expects($this->any()) + ->method('getName') + ->will($this->returnValue($indexName)); + $type = $this->getMockBuilder('Elastica\Type')->disableOriginalConstructor()->getMock(); + $type->expects($this->any()) + ->method('getName') + ->will($this->returnValue($typeName)); + $type->expects($this->any()) + ->method('getIndex') + ->will($this->returnValue($index)); + + $mock->expects($this->any()) + ->method('getType') + ->will($this->returnValue($type)); + + return $mock; + } + + private function getMockIndexable($indexName, $typeName, $object, $return = null) + { + $mock = $this->getMock('FOS\ElasticaBundle\Provider\IndexableInterface'); + + if (null !== $return) { + $mock->expects($this->once()) + ->method('isObjectIndexable') + ->with($indexName, $typeName, $object) + ->will($this->returnValue($return)); + } + + return $mock; } } @@ -251,33 +255,15 @@ namespace FOS\ElasticaBundle\Tests\Doctrine\Listener; class Entity { private $id; - private $isIndexable; - public function __construct($id, $isIndexable = true) + public function __construct($id) { $this->id = $id; - $this->isIndexable = $isIndexable; } public function getId() { return $this->id; } - - public function getIsIndexable() - { - return $this->isIndexable; - } } -class IndexableDecider -{ - public function isIndexable(Entity $entity) - { - return $entity->getIsIndexable(); - } - - protected function internalMethod() - { - } -} diff --git a/Tests/Functional/IndexableCallbackTest.php b/Tests/Functional/IndexableCallbackTest.php new file mode 100644 index 0000000..89fca1d --- /dev/null +++ b/Tests/Functional/IndexableCallbackTest.php @@ -0,0 +1,51 @@ + + * + * This source file is subject to the MIT license that is bundled + * with this source code in the file LICENSE. + */ + +namespace FOS\ElasticaBundle\Tests\Functional; + +/** + * @group functional + */ +class IndexableCallbackTest extends WebTestCase +{ + /** + * 2 reasons for this test: + * + * 1) To test that the configuration rename from is_indexable_callback under the listener + * key is respected, and + * 2) To test the Extension's set up of the Indexable service. + */ + public function testIndexableCallback() + { + $client = $this->createClient(array('test_case' => 'ORM')); + + /** @var \FOS\ElasticaBundle\Provider\Indexable $in */ + $in = $client->getContainer()->get('fos_elastica.indexable'); + + $this->assertTrue($in->isObjectIndexable('index', 'type', new TypeObj())); + $this->assertFalse($in->isObjectIndexable('index', 'type2', new TypeObj())); + $this->assertFalse($in->isObjectIndexable('index', 'type3', new TypeObj())); + } + + protected function setUp() + { + parent::setUp(); + + $this->deleteTmpDir('ORM'); + } + + protected function tearDown() + { + parent::tearDown(); + + $this->deleteTmpDir('ORM'); + } +} diff --git a/Tests/Functional/TypeObj.php b/Tests/Functional/TypeObj.php new file mode 100644 index 0000000..c264e7b --- /dev/null +++ b/Tests/Functional/TypeObj.php @@ -0,0 +1,25 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FOS\ElasticaBundle\Tests\Functional; + +class TypeObj +{ + public function isIndexable() + { + return true; + } + + public function isntIndexable() + { + return false; + } +} diff --git a/Tests/Functional/app/Basic/config.yml b/Tests/Functional/app/Basic/config.yml index 7025532..09e5aec 100644 --- a/Tests/Functional/app/Basic/config.yml +++ b/Tests/Functional/app/Basic/config.yml @@ -50,4 +50,4 @@ fos_elastica: _parent: type: "parent" property: "parent" - identifier: "id" \ No newline at end of file + identifier: "id" diff --git a/Tests/Functional/app/ORM/bundles.php b/Tests/Functional/app/ORM/bundles.php new file mode 100644 index 0000000..d0b6efb --- /dev/null +++ b/Tests/Functional/app/ORM/bundles.php @@ -0,0 +1,11 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FOS\ElasticaBundle\Tests\Provider; + +use FOS\ElasticaBundle\Provider\Indexable; + +class IndexableTest extends \PHPUnit_Framework_TestCase +{ + /** + * @dataProvider provideIsIndexableCallbacks + */ + public function testValidIndexableCallbacks($callback, $return) + { + $indexable = new Indexable(array( + 'index/type' => $callback + )); + $index = $indexable->isObjectIndexable('index', 'type', new Entity); + + $this->assertEquals($return, $index); + } + + /** + * @dataProvider provideInvalidIsIndexableCallbacks + * @expectedException \InvalidArgumentException + */ + public function testInvalidIsIndexableCallbacks($callback) + { + $indexable = new Indexable(array( + 'index/type' => $callback + )); + $indexable->isObjectIndexable('index', 'type', new Entity); + } + + public function provideInvalidIsIndexableCallbacks() + { + return array( + array('nonexistentEntityMethod'), + array(array(new IndexableDecider(), 'internalMethod')), + array(42), + array('entity.getIsIndexable() && nonexistentEntityFunction()'), + ); + } + + public function provideIsIndexableCallbacks() + { + return array( + array('isIndexable', false), + array(array(new IndexableDecider(), 'isIndexable'), true), + array(function(Entity $entity) { return $entity->maybeIndex(); }, true), + array('entity.maybeIndex()', true), + array('!object.isIndexable() && entity.property == "abc"', true), + array('entity.property != "abc"', false), + ); + } +} + +class Entity +{ + public $property = 'abc'; + + public function isIndexable() + { + return false; + } + + public function maybeIndex() + { + return true; + } +} + +class IndexableDecider +{ + public function isIndexable(Entity $entity) + { + return !$entity->isIndexable(); + } + + protected function internalMethod() + { + } +} diff --git a/composer.json b/composer.json index 8783822..d67e329 100644 --- a/composer.json +++ b/composer.json @@ -21,6 +21,7 @@ }, "require-dev":{ "doctrine/orm": "~2.2", + "doctrine/doctrine-bundle": "~1.2@beta", "doctrine/mongodb-odm": "1.0.*@dev", "propel/propel1": "1.6.*", "pagerfanta/pagerfanta": "1.0.*@dev", From e54cc3c2434cebc341f06d96596f11f14052094c Mon Sep 17 00:00:00 2001 From: Tim Nagel Date: Mon, 16 Jun 2014 16:17:04 +1000 Subject: [PATCH 2/4] Implement callback checking in the provider --- Doctrine/AbstractProvider.php | 24 +++++-- Persister/ObjectPersisterInterface.php | 7 ++ Provider/AbstractProvider.php | 35 ++++++++-- Tests/Doctrine/AbstractProviderTest.php | 88 ++++++++++++++++++++----- 4 files changed, 126 insertions(+), 28 deletions(-) diff --git a/Doctrine/AbstractProvider.php b/Doctrine/AbstractProvider.php index b9ffda5..0c43b2d 100644 --- a/Doctrine/AbstractProvider.php +++ b/Doctrine/AbstractProvider.php @@ -6,6 +6,7 @@ use Doctrine\Common\Persistence\ManagerRegistry; use Elastica\Exception\Bulk\ResponseException as BulkResponseException; use FOS\ElasticaBundle\Persister\ObjectPersisterInterface; use FOS\ElasticaBundle\Provider\AbstractProvider as BaseAbstractProvider; +use FOS\ElasticaBundle\Provider\IndexableInterface; abstract class AbstractProvider extends BaseAbstractProvider { @@ -15,13 +16,19 @@ abstract class AbstractProvider extends BaseAbstractProvider * Constructor. * * @param ObjectPersisterInterface $objectPersister - * @param string $objectClass - * @param array $options - * @param ManagerRegistry $managerRegistry + * @param IndexableInterface $indexable + * @param string $objectClass + * @param array $options + * @param ManagerRegistry $managerRegistry */ - public function __construct(ObjectPersisterInterface $objectPersister, $objectClass, array $options, $managerRegistry) - { - parent::__construct($objectPersister, $objectClass, array_merge(array( + public function __construct( + ObjectPersisterInterface $objectPersister, + IndexableInterface $indexable, + $objectClass, + array $options, + ManagerRegistry $managerRegistry + ) { + parent::__construct($objectPersister, $indexable, $objectClass, array_merge(array( 'clear_object_manager' => true, 'debug_logging' => false, 'ignore_errors' => false, @@ -53,6 +60,10 @@ abstract class AbstractProvider extends BaseAbstractProvider $stepStartTime = microtime(true); } $objects = $this->fetchSlice($queryBuilder, $batchSize, $offset); + if ($loggerClosure) { + $stepNbObjects = count($objects); + } + $objects = array_filter($objects, array($this, 'isObjectIndexable')); if (!$ignoreErrors) { $this->objectPersister->insertMany($objects); @@ -73,7 +84,6 @@ abstract class AbstractProvider extends BaseAbstractProvider usleep($sleep); if ($loggerClosure) { - $stepNbObjects = count($objects); $stepCount = $stepNbObjects + $offset; $percentComplete = 100 * $stepCount / $nbObjects; $timeDifference = microtime(true) - $stepStartTime; diff --git a/Persister/ObjectPersisterInterface.php b/Persister/ObjectPersisterInterface.php index 2b4c8ee..5953d5a 100644 --- a/Persister/ObjectPersisterInterface.php +++ b/Persister/ObjectPersisterInterface.php @@ -68,4 +68,11 @@ interface ObjectPersisterInterface * @param array $identifiers array of domain model object identifiers */ public function deleteManyByIdentifiers(array $identifiers); + + /** + * Returns the elastica type used by this persister + * + * @return \Elastica\Type + */ + public function getType(); } diff --git a/Provider/AbstractProvider.php b/Provider/AbstractProvider.php index 2761a25..8642be8 100644 --- a/Provider/AbstractProvider.php +++ b/Provider/AbstractProvider.php @@ -24,23 +24,48 @@ abstract class AbstractProvider implements ProviderInterface */ protected $options; + /** + * @var Indexable + */ + private $indexable; + /** * Constructor. * * @param ObjectPersisterInterface $objectPersister - * @param string $objectClass - * @param array $options + * @param IndexableInterface $indexable + * @param string $objectClass + * @param array $options */ - public function __construct(ObjectPersisterInterface $objectPersister, $objectClass, array $options = array()) - { - $this->objectPersister = $objectPersister; + public function __construct( + ObjectPersisterInterface $objectPersister, + IndexableInterface $indexable, + $objectClass, + array $options = array() + ) { + $this->indexable = $indexable; $this->objectClass = $objectClass; + $this->objectPersister = $objectPersister; $this->options = array_merge(array( 'batch_size' => 100, ), $options); } + /** + * Checks if a given object should be indexed or not. + * + * @param object $object + * @return bool + */ + protected function isObjectIndexable($object) + { + $typeName = $this->objectPersister->getType()->getName(); + $indexName = $this->objectPersister->getType()->getIndex()->getName(); + + return $this->indexable->isObjectIndexable($indexName, $typeName, $object); + } + /** * Get string with RAM usage information (current and peak) * diff --git a/Tests/Doctrine/AbstractProviderTest.php b/Tests/Doctrine/AbstractProviderTest.php index dcceccf..3640d16 100644 --- a/Tests/Doctrine/AbstractProviderTest.php +++ b/Tests/Doctrine/AbstractProviderTest.php @@ -9,24 +9,42 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase private $objectPersister; private $options; private $managerRegistry; + private $indexable; public function setUp() { - if (!interface_exists('Doctrine\Common\Persistence\ManagerRegistry')) { - $this->markTestSkipped('Doctrine Common is not available.'); - } + if (!interface_exists('Doctrine\Common\Persistence\ManagerRegistry')) { + $this->markTestSkipped('Doctrine Common is not available.'); + } - $this->objectClass = 'objectClass'; - $this->options = array('debug_logging' => true); + $this->objectClass = 'objectClass'; + $this->options = array('debug_logging' => true); - $this->objectPersister = $this->getMockObjectPersister(); - $this->managerRegistry = $this->getMockManagerRegistry(); - $this->objectManager = $this->getMockObjectManager(); + $this->objectPersister = $this->getMockObjectPersister(); + $this->managerRegistry = $this->getMockManagerRegistry(); + $this->objectManager = $this->getMockObjectManager(); + $this->indexable = $this->getMockIndexable(); - $this->managerRegistry->expects($this->any()) - ->method('getManagerForClass') - ->with($this->objectClass) - ->will($this->returnValue($this->objectManager)); + $index = $this->getMockBuilder('Elastica\Index')->disableOriginalConstructor()->getMock(); + $index->expects($this->any()) + ->method('getName') + ->will($this->returnValue('index')); + $type = $this->getMockBuilder('Elastica\Type')->disableOriginalConstructor()->getMock(); + $type->expects($this->any()) + ->method('getName') + ->will($this->returnValue('type')); + $type->expects($this->any()) + ->method('getIndex') + ->will($this->returnValue($index)); + + $this->objectPersister->expects($this->any()) + ->method('getType') + ->will($this->returnValue($type)); + + $this->managerRegistry->expects($this->any()) + ->method('getManagerForClass') + ->with($this->objectClass) + ->will($this->returnValue($this->objectManager)); } /** @@ -59,14 +77,13 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase ->with($queryBuilder, $batchSize, $offset) ->will($this->returnValue($objects)); - $this->objectPersister->expects($this->at($i)) - ->method('insertMany') - ->with($objects); - $this->objectManager->expects($this->at($i)) ->method('clear'); } + $this->objectPersister->expects($this->exactly(count($objectsByIteration))) + ->method('insertMany'); + $provider->populate(); } @@ -159,6 +176,36 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase $provider->populate(null, array('ignore-errors' => false)); } + public function testPopulateRunsIndexCallable() + { + $nbObjects = 2; + $objects = array(1, 2); + + $provider = $this->getMockAbstractProvider(); + $provider->expects($this->any()) + ->method('countObjects') + ->will($this->returnValue($nbObjects)); + $provider->expects($this->any()) + ->method('fetchSlice') + ->will($this->returnValue($objects)); + + $this->indexable->expects($this->at(0)) + ->method('isObjectIndexable') + ->with('index', 'type', 1) + ->will($this->returnValue(false)); + $this->indexable->expects($this->at(1)) + ->method('isObjectIndexable') + ->with('index', 'type', 2) + ->will($this->returnValue(true)); + + + $this->objectPersister->expects($this->once()) + ->method('insertMany') + ->with(array(1 => 2)); + + $provider->populate(); + } + /** * @return \FOS\ElasticaBundle\Doctrine\AbstractProvider|\PHPUnit_Framework_MockObject_MockObject */ @@ -166,6 +213,7 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase { return $this->getMockForAbstractClass('FOS\ElasticaBundle\Doctrine\AbstractProvider', array( $this->objectPersister, + $this->indexable, $this->objectClass, $this->options, $this->managerRegistry, @@ -205,6 +253,14 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase { return $this->getMock('FOS\ElasticaBundle\Persister\ObjectPersisterInterface'); } + + /** + * @return \FOS\ElasticaBundle\Provider\IndexableInterface|\PHPUnit_Framework_MockObject_MockObject + */ + private function getMockIndexable() + { + return $this->getMock('FOS\ElasticaBundle\Provider\IndexableInterface'); + } } /** From 391e18dcbf9e165a4881119b3e321806ed1b0b0f Mon Sep 17 00:00:00 2001 From: Tim Nagel Date: Mon, 16 Jun 2014 16:20:17 +1000 Subject: [PATCH 3/4] Update changelog --- CHANGELOG-3.0.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/CHANGELOG-3.0.md b/CHANGELOG-3.0.md index 9d74640..f93832d 100644 --- a/CHANGELOG-3.0.md +++ b/CHANGELOG-3.0.md @@ -12,7 +12,17 @@ https://github.com/FriendsOfSymfony/FOSElasticaBundle/compare/v3.0.0...v3.0.1 To generate a changelog summary since the last version, run `git log --no-merges --oneline v3.0.0...3.0.x` -* 3.0.0-ALPHA3 (xxxx-xx-xx) +* 3.0.0-ALPHA6 (xxxx-xx-xx) + + * Moved `is_indexable_callback` from the listener properties to a type property called + `indexable_callback` which is run when both populating and listening for object + changes. + * BC BREAK `ObjectPersisterInterface` added method getType() (To be removed during + ObjectPersister refactoring before 3.0.0 stable when ObjectPersister will change) + * AbstractProvider constructor change: Second argument is now an `IndexableInterface` + instance. + +* 3.0.0-ALPHA3 (2014-04-01) * a9c4c93: Logger is now only enabled in debug mode by default * #463: allowing hot swappable reindexing From 2e23f4a1bf31eb4e2775880c2ecd319717fac984 Mon Sep 17 00:00:00 2001 From: Tim Nagel Date: Tue, 17 Jun 2014 10:07:10 +1000 Subject: [PATCH 4/4] Added documentation changes for indexable_callback --- Resources/doc/setup.md | 2 +- Resources/doc/types.md | 81 ++++++++++++++++++++---------------------- 2 files changed, 39 insertions(+), 44 deletions(-) diff --git a/Resources/doc/setup.md b/Resources/doc/setup.md index 9a39b95..485f290 100644 --- a/Resources/doc/setup.md +++ b/Resources/doc/setup.md @@ -7,7 +7,7 @@ A) Install FOSElasticaBundle FOSElasticaBundle is installed using [Composer](https://getcomposer.org). ```bash -$ php composer.phar require friendsofsymfony/elastica-bundle "3.0.*" +$ php composer.phar require friendsofsymfony/elastica-bundle "3.0.*@alpha" ``` ### Elasticsearch diff --git a/Resources/doc/types.md b/Resources/doc/types.md index aacb09e..be687d7 100644 --- a/Resources/doc/types.md +++ b/Resources/doc/types.md @@ -149,6 +149,44 @@ analyzer, you could write: title: { boost: 8, analyzer: my_analyzer } ``` +Testing if an object should be indexed +-------------------------------------- + +FOSElasticaBundle can be configured to automatically index changes made for +different kinds of objects if your persistence backend supports these methods, +but in some cases you might want to run an external service or call a property +on the object to see if it should be indexed. + +A property, `indexable_callback` is provided under the type configuration that +lets you configure this behaviour which will apply for any automated watching +for changes and for a repopulation of an index. + +In the example below, we're checking the enabled property on the user to only +index enabled users. + +```yaml + types: + users: + indexable_callback: 'enabled' +``` + +The callback option supports multiple approaches: + +* A method on the object itself provided as a string. `enabled` will call + `Object->enabled()` +* An array of a service id and a method which will be called with the object as the first + and only argument. `[ @my_custom_service, 'userIndexable' ]` will call the userIndexable + method on a service defined as my_custom_service. +* If you have the ExpressionLanguage component installed, A valid ExpressionLanguage + expression provided as a string. The object being indexed will be supplied as `object` + in the expression. `object.isEnabled() or object.shouldBeIndexedAnyway()`. For more + information on the ExpressionLanguage component and its capabilities see its + [documentation](http://symfony.com/doc/current/components/expression_language/index.html) + +In all cases, the callback should return a true or false, with true indicating it will be +indexed, and a false indicating the object should not be indexed, or should be removed +from the index if we are running an update. + Provider Configuration ---------------------- @@ -234,49 +272,6 @@ You can also choose to only listen for some of the events: > **Propel** doesn't support this feature yet. -### Checking an entity method for listener - -If you use listeners to update your index, you may need to validate your -entities before you index them (e.g. only index "public" entities). Typically, -you'll want the listener to be consistent with the provider's query criteria. -This may be achieved by using the `is_indexable_callback` config parameter: - -```yaml - persistence: - listener: - is_indexable_callback: "isPublic" -``` - -If `is_indexable_callback` is a string and the entity has a method with the -specified name, the listener will only index entities for which the method -returns `true`. Additionally, you may provide a service and method name pair: - -```yaml - persistence: - listener: - is_indexable_callback: [ "%custom_service_id%", "isIndexable" ] -``` - -In this case, the callback_class will be the `isIndexable()` method on the specified -service and the object being considered for indexing will be passed as the only -argument. This allows you to do more complex validation (e.g. ACL checks). - -If you have the [Symfony ExpressionLanguage](https://github.com/symfony/expression-language) -component installed, you can use expressions to evaluate the callback: - -```yaml - persistence: - listener: - is_indexable_callback: "user.isActive() && user.hasRole('ROLE_USER')" -``` - -As you might expect, new entities will only be indexed if the callback_class returns -`true`. Additionally, modified entities will be updated or removed from the -index depending on whether the callback_class returns `true` or `false`, respectively. -The delete listener disregards the callback_class. - -> **Propel** doesn't support this feature yet. - Flushing Method ---------------