Merge pull request #7055 from robbieaverill/pulls/4.0/polymorphic-has-one-indexes

NEW Ensure polymorphic has_one fields are indexed

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,56 @@ class DataObjectSchema
     }
 
     /**
-     * Cache all indexes for the given class.
+     * Cache all indexes for the given class. Will do nothing if already cached.
-     * Will do nothing if already cached
      *
      * @param $class
      */
     protected function cacheDatabaseIndexes($class)
     {
-        if (array_key_exists($class, $this->databaseIndexes)) {
+        if (!array_key_exists($class, $this->databaseIndexes)) {
-            return;
+            $this->databaseIndexes[$class] = array_merge(
+                $this->cacheDefaultDatabaseIndexes($class),
+                $this->buildCustomDatabaseIndexes($class)
+            );
         }
+    }
+
+    /**
+     * 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] = [];
 
-        // look for indexable field types
+        $fieldSpecs = $this->fieldSpecs($class, self::UNINHERITED);
-        foreach ($this->databaseFields($class, false) as $field => $type) {
+        foreach ($fieldSpecs as $field => $spec) {
-            /** @skipUpgrade */
+            /** @var DBField $fieldObj */
-            if ($type === 'ForeignKey' || $type === 'DBClassName') {
+            $fieldObj = Injector::inst()->create($spec, $field);
-                $indexes[$field] = [
+            if ($indexSpecs = $fieldObj->getIndexSpecs()) {
-                    'type' => 'index',
+                $this->defaultDatabaseIndexes[$class][$field] = $indexSpecs;
-                    'columns' => [$field],
-                ];
             }
         }
+        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 +579,7 @@ class DataObjectSchema
             }
             $indexes[$indexName] = $indexSpec;
         }
-
+        return $indexes;
-        $this->databaseIndexes[$class] = $indexes;
     }
 
     /**

src/ORM/FieldType/DBClassName.php

@@ -37,6 +37,8 @@ class DBClassName extends DBEnum
      */
     protected static $classname_cache = array();
 
+    private static $index = 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.
@@ -51,11 +53,12 @@ class DBClassName extends DBEnum
      *
      * @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);
     }
 
     /**

src/ORM/FieldType/DBComposite.php

@@ -293,4 +293,18 @@ abstract class DBComposite extends DBField
 
         return parent::castingHelper($field);
     }
+
+    public function getIndexSpecs()
+    {
+        if ($type = $this->getIndexType()) {
+            $columns = array_map(function ($name) {
+                return $this->getName() . $name;
+            }, array_keys((array) static::config()->get('composite_db')));
+
+            return [
+                'type' => $type,
+                'columns' => $columns,
+            ];
+        }
+    }
 }

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);
     }
 
     /**

src/ORM/FieldType/DBField.php

@@ -43,7 +43,7 @@ use SilverStripe\View\ViewableData;
  *
  * @todo remove MySQL specific code from subclasses
  */
-abstract class DBField extends ViewableData
+abstract class DBField extends ViewableData implements DBIndexable
 {
 
     /**
@@ -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,15 @@ abstract class DBField extends ViewableData
      */
     private static $default_search_filter_class = 'PartialMatchFilter';
 
+    /**
+     * The type of index to use for this field. Can either be a string (one of the DBIndexable type options) or a
+     * boolean. When a boolean is given, false will not index the field, and true will use the default index type.
+     *
+     * @var string|bool
+     * @config
+     */
+    private static $index = false;
+
     private static $casting = array(
         'ATT' => 'HTMLFragment',
         'CDATA' => 'HTMLFragment',
@@ -110,10 +126,24 @@ abstract class DBField extends ViewableData
      */
     protected $defaultVal;
 
-    public function __construct($name = null)
+    /**
+     * Provide the DBField name and an array of options, e.g. ['index' => 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 +254,59 @@ 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;
+    }
+
+    public function setIndexType($type)
+    {
+        if (!is_bool($type)
+            && !in_array($type, [DBIndexable::TYPE_INDEX, DBIndexable::TYPE_UNIQUE, DBIndexable::TYPE_FULLTEXT])
+        ) {
+            throw new \InvalidArgumentException(
+                "{$type} is not a valid index type or boolean. Please see DBIndexable."
+            );
+        }
+
+        $this->options['index'] = $type;
+        return $this;
+    }
+
+    public function getIndexType()
+    {
+        if (array_key_exists('index', $this->options)) {
+            $type = $this->options['index'];
+        } else {
+            $type = static::config()->get('index');
+        }
+
+        if (is_bool($type)) {
+            if (!$type) {
+                return false;
+            }
+            $type = DBIndexable::TYPE_DEFAULT;
+        }
+
+        return $type;
+    }
 
     /**
      * Determines if the field has a value which is not considered to be 'null'
@@ -549,4 +632,14 @@ DBG;
     {
         return $this->RAW();
     }
+
+    public function getIndexSpecs()
+    {
+        if ($type = $this->getIndexType()) {
+            return [
+                'type' => $type,
+                'columns' => [$this->getName()],
+            ];
+        }
+    }
 }

src/ORM/FieldType/DBForeignKey.php

@@ -29,6 +29,8 @@ class DBForeignKey extends DBInt
      */
     protected $object;
 
+    private static $index = true;
+
     private static $default_search_filter_class = 'ExactMatchFilter';
 
     public function __construct($name, $object = null)

src/ORM/FieldType/DBIndexable.php

@@ -0,0 +1,57 @@
+<?php
+
+namespace SilverStripe\ORM\FieldType;
+
+/**
+ * Classes that implement the DBIndexable interface will provide options to set various index types and index
+ * contents, which will be processed by {@link \SilverStripe\ORM\DataObjectSchema}
+ */
+interface DBIndexable
+{
+    /**
+     * Index types that can be used. Please check your database driver to ensure choices are supported.
+     *
+     * @var string
+     */
+    const TYPE_INDEX = 'index';
+    const TYPE_UNIQUE = 'unique';
+    const TYPE_FULLTEXT = 'fulltext';
+
+    /**
+     * If "true" is provided to setIndexType, this default index type will be returned
+     *
+     * @var string
+     */
+    const TYPE_DEFAULT = 'index';
+
+    /**
+     * Set the desired index type to use
+     *
+     * @param string|bool $indexType Either of the types listed in {@link SilverStripe\ORM\FieldType\DBIndexable}, or
+     *                               boolean true to indicate that the default index type should be used.
+     * @return $this
+     * @throws InvalidArgumentException If $type is not one of TYPE_INDEX, TYPE_UNIQUE or TYPE_FULLTEXT
+     */
+    public function setIndexType($type);
+
+    /**
+     * Return the desired index type to use. Will return false if the field instance should not be indexed.
+     *
+     * @return string|bool
+     */
+    public function getIndexType();
+
+    /**
+     * Returns the index specifications for the field instance, for example:
+     *
+     * <code>
+     * [
+     *     'type' => 'unique',
+     *     'columns' => ['FieldName']
+     * ]
+     * </code>
+     *
+     * @return array
+     */
+    public function getIndexSpecs();
+}

src/ORM/FieldType/DBPolymorphicForeignKey.php

@@ -9,10 +9,11 @@ use SilverStripe\ORM\DataObject;
  */
 class DBPolymorphicForeignKey extends DBComposite
 {
+    private static $index = true;
 
     private static $composite_db = array(
         'ID' => 'Int',
-        'Class' => "DBClassName('SilverStripe\\ORM\\DataObject')"
+        'Class' => "DBClassName('" . DataObject::class . "', ['index' => false])"
     );
 
     public function scaffoldFormField($title = null, $params = null)

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
+     * {@inheritDoc}
-     * @param array $options array An array of options e.g. array('nullifyEmpty'=>false).  See
-     *                       {@link StringField::setOptions()} for information on the available options
      */
-    public function __construct($name = null, $options = array())
+    public function __construct($name = null, $options = [])
     {
-        if ($options) {
+        $this->options['nullifyEmpty'] = true;
-            if (!is_array($options)) {
+        parent::__construct($name, $options);
-                throw new \InvalidArgumentException("Invalid options $options");
-            }
-            $this->setOptions($options);
-        }
-
-        parent::__construct($name);
     }
 
     /**
@@ -56,14 +42,17 @@ abstract class DBString extends DBField
      *   </li></ul>
      * @return $this
      */
-    public function setOptions(array $options = array())
+    public function setOptions(array $options = [])
     {
-        if (array_key_exists("nullifyEmpty", $options)) {
+        parent::setOptions($options);
-            $this->nullifyEmpty = $options["nullifyEmpty"] ? true : false;
+
+        if (array_key_exists('nullifyEmpty', $options)) {
+            $this->options['nullifyEmpty'] = (bool) $options['nullifyEmpty'];
         }
-        if (array_key_exists("default", $options)) {
+        if (array_key_exists('default', $options)) {
-            $this->setDefaultValue($options["default"]);
+            $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 '';

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\Dev\SapphireTest;
+use SilverStripe\ORM\FieldType\DBMoney;
+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,70 @@ 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, 'index', 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 testIndexTypeIsConfigurable()
+    {
+        Config::modify()->set(DBMoney::class, 'index', 'unique');
+
+        $indexes = DataObject::getSchema()->databaseIndexes(HasComposites::class);
+        $this->assertCount(4, $indexes);
+        $this->assertArrayHasKey('Amount', $indexes);
+        $this->assertEquals([
+            'type' => 'unique',
+            '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' => 'fulltext',
+            'columns' => ['IndexedTitle']
+        ], $indexes['IndexedTitle']);
+
+        $this->assertArrayHasKey('IndexedMoney', $indexes);
+        $this->assertEquals([
+            'type' => 'index',
+            'columns' => ['IndexedMoneyCurrency', 'IndexedMoneyAmount']
+        ], $indexes['IndexedMoney']);
+    }
 }

tests/php/ORM/DataObjectSchemaTest/AllIndexes.php

@@ -3,6 +3,7 @@ namespace SilverStripe\ORM\Tests\DataObjectSchemaTest;
 
 use SilverStripe\Dev\TestOnly;
 use SilverStripe\ORM\DataObject;
+use SilverStripe\ORM\FieldType\DBIndexable;
 
 class AllIndexes extends DataObject implements TestOnly
 {
@@ -18,7 +19,7 @@ class AllIndexes extends DataObject implements TestOnly
         'Content' => true,
         'IndexCols' => ['Title', 'Content'],
         'IndexUnique' => [
-            'type' => 'unique',
+            'type' => DBIndexable::TYPE_UNIQUE,
             'columns' => ['Number'],
         ],
         'IndexNormal' => [

tests/php/ORM/DataObjectSchemaTest/HasComposites.php

@@ -0,0 +1,18 @@
+<?php
+
+namespace SilverStripe\ORM\Tests\DataObjectSchemaTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+
+class HasComposites extends DataObject implements TestOnly
+{
+    private static $db = [
+        'Amount' => 'Money'
+    ];
+
+    private static $has_one = [
+        'RegularHasOne' => ChildClass::class,
+        'Polymorpheus' => DataObject::class
+    ];
+}

tests/php/ORM/DataObjectSchemaTest/HasIndexesInFieldSpecs.php

@@ -0,0 +1,16 @@
+<?php
+
+namespace SilverStripe\ORM\Tests\DataObjectSchemaTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+
+class HasIndexesInFieldSpecs extends DataObject implements TestOnly
+{
+    private static $db = [
+        'Normal' => 'Varchar',
+        'IndexedTitle' => 'Varchar(255, ["index" => "fulltext"])',
+        'NormalMoney' => 'Money',
+        'IndexedMoney' => 'Money(["index" => true])'
+    ];
+}