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

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

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.
@@ -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,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')));
+    }
 }

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

@@ -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()];
+    }
 }

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)

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)

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
      *   </li></ul>
      * @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 '';

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']);
+    }
 }

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, ["indexed" => true])',
+        'NormalMoney' => 'Money',
+        'IndexedMoney' => 'Money(["indexed" => true])'
+    ];
+}