From c9c43906191511bce8ef6f4dc8b84600eebaebf2 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Fri, 23 Jun 2017 17:40:06 +1200 Subject: [PATCH] NEW Ensure polymorphic has_one fields are indexed * Add tests for config based indexing on composite DBFields * Allow fields to have "indexed" option passed via field spec --- src/ORM/DataObjectSchema.php | 65 ++++++++++--- src/ORM/FieldType/DBClassName.php | 9 +- src/ORM/FieldType/DBComposite.php | 7 ++ src/ORM/FieldType/DBEnum.php | 5 +- src/ORM/FieldType/DBField.php | 91 ++++++++++++++++++- src/ORM/FieldType/DBForeignKey.php | 2 + src/ORM/FieldType/DBPolymorphicForeignKey.php | 3 +- src/ORM/FieldType/DBString.php | 45 ++++----- tests/php/ORM/DataObjectSchemaTest.php | 58 +++++++++++- .../DataObjectSchemaTest/HasComposites.php | 18 ++++ .../HasIndexesInFieldSpecs.php | 16 ++++ 11 files changed, 269 insertions(+), 50 deletions(-) create mode 100644 tests/php/ORM/DataObjectSchemaTest/HasComposites.php create mode 100644 tests/php/ORM/DataObjectSchemaTest/HasIndexesInFieldSpecs.php diff --git a/src/ORM/DataObjectSchema.php b/src/ORM/DataObjectSchema.php index 236630fae..a71856b66 100644 --- a/src/ORM/DataObjectSchema.php +++ b/src/ORM/DataObjectSchema.php @@ -43,6 +43,13 @@ class DataObjectSchema */ protected $databaseIndexes = []; + /** + * Fields that should be indexed, by class name + * + * @var array + */ + protected $defaultDatabaseIndexes = []; + /** * Cache of composite database field * @@ -65,6 +72,7 @@ class DataObjectSchema $this->tableNames = []; $this->databaseFields = []; $this->databaseIndexes = []; + $this->defaultDatabaseIndexes = []; $this->compositeFields = []; } @@ -193,7 +201,6 @@ class DataObjectSchema foreach ($classes as $tableClass) { // Find all fields on this class $fields = $this->databaseFields($tableClass, false); - // Merge with composite fields if (!$dbOnly) { $compositeFields = $this->compositeFields($tableClass, false); @@ -486,30 +493,59 @@ class DataObjectSchema } /** - * Cache all indexes for the given class. - * Will do nothing if already cached + * Cache all indexes for the given class. Will do nothing if already cached. * * @param $class */ protected function cacheDatabaseIndexes($class) { - if (array_key_exists($class, $this->databaseIndexes)) { - return; + if (!array_key_exists($class, $this->databaseIndexes)) { + $this->databaseIndexes[$class] = array_merge( + $this->cacheDefaultDatabaseIndexes($class), + $this->buildCustomDatabaseIndexes($class) + ); } - $indexes = []; + } - // look for indexable field types - foreach ($this->databaseFields($class, false) as $field => $type) { - /** @skipUpgrade */ - if ($type === 'ForeignKey' || $type === 'DBClassName') { - $indexes[$field] = [ + /** + * Get "default" database indexable field types + * + * @param string $class + * @return array + */ + protected function cacheDefaultDatabaseIndexes($class) + { + $indexes = []; + if (array_key_exists($class, $this->defaultDatabaseIndexes)) { + return $this->defaultDatabaseIndexes[$class]; + } + $this->defaultDatabaseIndexes[$class] = []; + + $fieldSpecs = $this->fieldSpecs($class, self::UNINHERITED); + foreach ($fieldSpecs as $field => $spec) { + /** @var DBField $fieldObj */ + $fieldObj = Injector::inst()->create($spec, $field); + if ($fieldObj->getIndexed()) { + $this->defaultDatabaseIndexes[$class][$field] = [ 'type' => 'index', - 'columns' => [$field], + 'columns' => $fieldObj->getIndexSpecs(), ]; } } + return $this->defaultDatabaseIndexes[$class]; + } - // look for custom indexes declared on the class + /** + * Look for custom indexes declared on the class + * + * @param string $class + * @return array + * @throws InvalidArgumentException If an index already exists on the class + * @throws InvalidArgumentException If a custom index format is not valid + */ + protected function buildCustomDatabaseIndexes($class) + { + $indexes = []; $classIndexes = Config::inst()->get($class, 'indexes', Config::UNINHERITED) ?: []; foreach ($classIndexes as $indexName => $indexSpec) { if (array_key_exists($indexName, $indexes)) { @@ -546,8 +582,7 @@ class DataObjectSchema } $indexes[$indexName] = $indexSpec; } - - $this->databaseIndexes[$class] = $indexes; + return $indexes; } /** diff --git a/src/ORM/FieldType/DBClassName.php b/src/ORM/FieldType/DBClassName.php index 276a856f2..09dcb4d10 100644 --- a/src/ORM/FieldType/DBClassName.php +++ b/src/ORM/FieldType/DBClassName.php @@ -37,6 +37,8 @@ class DBClassName extends DBEnum */ protected static $classname_cache = array(); + private static $indexed = true; + /** * Clear all cached classname specs. It's necessary to clear all cached subclassed names * for any classes if a new class manifest is generated. @@ -49,13 +51,14 @@ class DBClassName extends DBEnum /** * Create a new DBClassName field * - * @param string $name Name of field + * @param string $name Name of field * @param string|null $baseClass Optional base class to limit selections + * @param array $options Optional parameters for this DBField instance */ - public function __construct($name = null, $baseClass = null) + public function __construct($name = null, $baseClass = null, $options = []) { $this->setBaseClass($baseClass); - parent::__construct($name); + parent::__construct($name, null, null, $options); } /** diff --git a/src/ORM/FieldType/DBComposite.php b/src/ORM/FieldType/DBComposite.php index 8ad958611..a7ba4ae0c 100644 --- a/src/ORM/FieldType/DBComposite.php +++ b/src/ORM/FieldType/DBComposite.php @@ -293,4 +293,11 @@ abstract class DBComposite extends DBField return parent::castingHelper($field); } + + public function getIndexSpecs() + { + return array_map(function ($name) { + return $this->getName() . $name; + }, array_keys((array) static::config()->get('composite_db'))); + } } diff --git a/src/ORM/FieldType/DBEnum.php b/src/ORM/FieldType/DBEnum.php index 59f85fc42..fc7802de7 100644 --- a/src/ORM/FieldType/DBEnum.php +++ b/src/ORM/FieldType/DBEnum.php @@ -51,8 +51,9 @@ class DBEnum extends DBString * @param string $name * @param string|array $enum A string containing a comma separated list of options or an array of Vals. * @param string $default The default option, which is either NULL or one of the items in the enumeration. + * @param array $options Optional parameters for this DB field */ - public function __construct($name = null, $enum = null, $default = null) + public function __construct($name = null, $enum = null, $default = null, $options = []) { if ($enum) { $this->setEnum($enum); @@ -73,7 +74,7 @@ class DBEnum extends DBString } } - parent::__construct($name); + parent::__construct($name, $options); } /** diff --git a/src/ORM/FieldType/DBField.php b/src/ORM/FieldType/DBField.php index c43253b61..4abc05d5e 100644 --- a/src/ORM/FieldType/DBField.php +++ b/src/ORM/FieldType/DBField.php @@ -74,6 +74,13 @@ abstract class DBField extends ViewableData */ protected $arrayValue; + /** + * Optional parameters for this field + * + * @var array + */ + protected $options = []; + /** * The escape type for this field when inserted into a template - either "xml" or "raw". * @@ -90,6 +97,16 @@ abstract class DBField extends ViewableData */ private static $default_search_filter_class = 'PartialMatchFilter'; + /** + * Whether this field type should be indexed by default. Can be overridden by {@link setOptions()} + * with "indexed" => true or via a DBField constructor, e.g. "MyField" => "Varchar(255, ['indexed' => false])". + * Note: if doing this, be aware of the existing field constructor arguments, like the length above. + * + * @var bool + * @config + */ + private static $indexed = false; + private static $casting = array( 'ATT' => 'HTMLFragment', 'CDATA' => 'HTMLFragment', @@ -110,10 +127,24 @@ abstract class DBField extends ViewableData */ protected $defaultVal; - public function __construct($name = null) + /** + * Provide the DBField name and an array of options, e.g. ['indexed' => true], or ['nullifyEmpty' => false] + * + * @param string $name + * @param array $options + * @throws InvalidArgumentException If $options was passed by not an array + */ + public function __construct($name = null, $options = []) { $this->name = $name; + if ($options) { + if (!is_array($options)) { + throw new \InvalidArgumentException("Invalid options $options"); + } + $this->setOptions($options); + } + parent::__construct(); } @@ -224,6 +255,54 @@ abstract class DBField extends ViewableData return $this; } + /** + * Update the optional parameters for this field + * + * @param array $options Array of options + * @return $this + */ + public function setOptions(array $options = []) + { + $this->options = $options; + return $this; + } + + /** + * Get optional parameters for this field + * + * @return array + */ + public function getOptions() + { + return $this->options; + } + + /** + * Set whether this DBField instance should be indexed + * + * @param bool $indexed + * @return $this + */ + public function setIndexed($indexed) + { + $this->options['indexed'] = (bool) $indexed; + return $this; + } + + /** + * Get whether this DBField should be indexed + * + * @return bool + */ + public function getIndexed() + { + if (array_key_exists('indexed', $this->options)) { + return (bool) $this->options['indexed']; + } + + // Default to global configuration per DBField instance + return (bool) static::config()->get('indexed'); + } /** * Determines if the field has a value which is not considered to be 'null' @@ -549,4 +628,14 @@ DBG; { return $this->RAW(); } + + /** + * Get the field(s) that should be used if this field is indexed + * + * @return array + */ + public function getIndexSpecs() + { + return [$this->getName()]; + } } diff --git a/src/ORM/FieldType/DBForeignKey.php b/src/ORM/FieldType/DBForeignKey.php index 7c3d6beb5..622db5f03 100644 --- a/src/ORM/FieldType/DBForeignKey.php +++ b/src/ORM/FieldType/DBForeignKey.php @@ -29,6 +29,8 @@ class DBForeignKey extends DBInt */ protected $object; + private static $indexed = true; + private static $default_search_filter_class = 'ExactMatchFilter'; public function __construct($name, $object = null) diff --git a/src/ORM/FieldType/DBPolymorphicForeignKey.php b/src/ORM/FieldType/DBPolymorphicForeignKey.php index aea9344b4..85bdd8f1c 100644 --- a/src/ORM/FieldType/DBPolymorphicForeignKey.php +++ b/src/ORM/FieldType/DBPolymorphicForeignKey.php @@ -9,10 +9,11 @@ use SilverStripe\ORM\DataObject; */ class DBPolymorphicForeignKey extends DBComposite { + private static $indexed = true; private static $composite_db = array( 'ID' => 'Int', - 'Class' => "DBClassName('SilverStripe\\ORM\\DataObject')" + 'Class' => "DBClassName('" . DataObject::class . "', ['indexed' => false])" ); public function scaffoldFormField($title = null, $params = null) diff --git a/src/ORM/FieldType/DBString.php b/src/ORM/FieldType/DBString.php index de5e4730b..3732c0952 100644 --- a/src/ORM/FieldType/DBString.php +++ b/src/ORM/FieldType/DBString.php @@ -7,12 +7,6 @@ namespace SilverStripe\ORM\FieldType; */ abstract class DBString extends DBField { - - /** - * @var boolean - */ - protected $nullifyEmpty = true; - /** * @var array */ @@ -26,22 +20,14 @@ abstract class DBString extends DBField ); /** - * Construct a string type field with a set of optional parameters. + * Set the default value for "nullify empty" * - * @param string $name string The name of the field - * @param array $options array An array of options e.g. array('nullifyEmpty'=>false). See - * {@link StringField::setOptions()} for information on the available options + * {@inheritDoc} */ - public function __construct($name = null, $options = array()) + public function __construct($name = null, $options = []) { - if ($options) { - if (!is_array($options)) { - throw new \InvalidArgumentException("Invalid options $options"); - } - $this->setOptions($options); - } - - parent::__construct($name); + $this->options['nullifyEmpty'] = true; + parent::__construct($name, $options); } /** @@ -56,14 +42,17 @@ abstract class DBString extends DBField * * @return $this */ - public function setOptions(array $options = array()) + public function setOptions(array $options = []) { - if (array_key_exists("nullifyEmpty", $options)) { - $this->nullifyEmpty = $options["nullifyEmpty"] ? true : false; + parent::setOptions($options); + + if (array_key_exists('nullifyEmpty', $options)) { + $this->options['nullifyEmpty'] = (bool) $options['nullifyEmpty']; } - if (array_key_exists("default", $options)) { - $this->setDefaultValue($options["default"]); + if (array_key_exists('default', $options)) { + $this->setDefaultValue($options['default']); } + return $this; } @@ -72,10 +61,12 @@ abstract class DBString extends DBField * them to null. * * @param $value boolean True if empty strings are to be converted to null + * @return $this */ public function setNullifyEmpty($value) { - $this->nullifyEmpty = ($value ? true : false); + $this->options['nullifyEmpty'] = (bool) $value; + return $this; } /** @@ -86,7 +77,7 @@ abstract class DBString extends DBField */ public function getNullifyEmpty() { - return $this->nullifyEmpty; + return $this->options['nullifyEmpty']; } /** @@ -108,7 +99,7 @@ abstract class DBString extends DBField } // Return "empty" value - if ($this->nullifyEmpty || $value === null) { + if ($this->options['nullifyEmpty'] || $value === null) { return null; } return ''; diff --git a/tests/php/ORM/DataObjectSchemaTest.php b/tests/php/ORM/DataObjectSchemaTest.php index 242094bcc..02529dfa9 100644 --- a/tests/php/ORM/DataObjectSchemaTest.php +++ b/tests/php/ORM/DataObjectSchemaTest.php @@ -3,8 +3,10 @@ namespace SilverStripe\ORM\Tests; use SilverStripe\Core\ClassInfo; -use SilverStripe\ORM\DataObject; +use SilverStripe\Core\Config\Config; +use SilverStripe\ORM\FieldType\DBMoney; use SilverStripe\Dev\SapphireTest; +use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataObjectSchema; use SilverStripe\ORM\Tests\DataObjectSchemaTest\AllIndexes; use SilverStripe\ORM\Tests\DataObjectSchemaTest\BaseClass; @@ -12,7 +14,9 @@ use SilverStripe\ORM\Tests\DataObjectSchemaTest\BaseDataClass; use SilverStripe\ORM\Tests\DataObjectSchemaTest\ChildClass; use SilverStripe\ORM\Tests\DataObjectSchemaTest\DefaultTableName; use SilverStripe\ORM\Tests\DataObjectSchemaTest\GrandChildClass; +use SilverStripe\ORM\Tests\DataObjectSchemaTest\HasComposites; use SilverStripe\ORM\Tests\DataObjectSchemaTest\HasFields; +use SilverStripe\ORM\Tests\DataObjectSchemaTest\HasIndexesInFieldSpecs; use SilverStripe\ORM\Tests\DataObjectSchemaTest\NoFields; use SilverStripe\ORM\Tests\DataObjectSchemaTest\WithCustomTable; use SilverStripe\ORM\Tests\DataObjectSchemaTest\WithRelation; @@ -271,4 +275,56 @@ class DataObjectSchemaTest extends SapphireTest 'columns' => ['Title'], ], $indexes['IndexNormal']); } + + public function testCompositeDatabaseFieldIndexes() + { + $indexes = DataObject::getSchema()->databaseIndexes(HasComposites::class); + $this->assertCount(3, $indexes); + $this->assertArrayHasKey('RegularHasOneID', $indexes); + $this->assertEquals([ + 'type' => 'index', + 'columns' => ['RegularHasOneID'] + ], $indexes['RegularHasOneID']); + + $this->assertArrayHasKey('Polymorpheus', $indexes); + $this->assertEquals([ + 'type' => 'index', + 'columns' => ['PolymorpheusID', 'PolymorpheusClass'] + ], $indexes['Polymorpheus']); + + // Check that DBPolymorphicForeignKey's "Class" is not indexed on its own + $this->assertArrayNotHasKey('PolymorpheusClass', $indexes); + } + + public function testCompositeFieldsCanBeIndexedByDefaultConfiguration() + { + Config::modify()->set(DBMoney::class, 'indexed', true); + $indexes = DataObject::getSchema()->databaseIndexes(HasComposites::class); + $this->assertCount(4, $indexes); + $this->assertArrayHasKey('Amount', $indexes); + $this->assertEquals([ + 'type' => 'index', + 'columns' => ['AmountCurrency', 'AmountAmount'] + ], $indexes['Amount']); + } + + public function testFieldsCanBeIndexedFromFieldSpecs() + { + $indexes = DataObject::getSchema()->databaseIndexes(HasIndexesInFieldSpecs::class); + + $this->assertCount(3, $indexes); + $this->assertArrayHasKey('ClassName', $indexes); + + $this->assertArrayHasKey('IndexedTitle', $indexes); + $this->assertEquals([ + 'type' => 'index', + 'columns' => ['IndexedTitle'] + ], $indexes['IndexedTitle']); + + $this->assertArrayHasKey('IndexedMoney', $indexes); + $this->assertEquals([ + 'type' => 'index', + 'columns' => ['IndexedMoneyCurrency', 'IndexedMoneyAmount'] + ], $indexes['IndexedMoney']); + } } diff --git a/tests/php/ORM/DataObjectSchemaTest/HasComposites.php b/tests/php/ORM/DataObjectSchemaTest/HasComposites.php new file mode 100644 index 000000000..06a030acf --- /dev/null +++ b/tests/php/ORM/DataObjectSchemaTest/HasComposites.php @@ -0,0 +1,18 @@ + 'Money' + ]; + + private static $has_one = [ + 'RegularHasOne' => ChildClass::class, + 'Polymorpheus' => DataObject::class + ]; +} diff --git a/tests/php/ORM/DataObjectSchemaTest/HasIndexesInFieldSpecs.php b/tests/php/ORM/DataObjectSchemaTest/HasIndexesInFieldSpecs.php new file mode 100644 index 000000000..914faa009 --- /dev/null +++ b/tests/php/ORM/DataObjectSchemaTest/HasIndexesInFieldSpecs.php @@ -0,0 +1,16 @@ + 'Varchar', + 'IndexedTitle' => 'Varchar(255, ["indexed" => true])', + 'NormalMoney' => 'Money', + 'IndexedMoney' => 'Money(["indexed" => true])' + ]; +}