From a27808cd1b14d8c5d5e46705b60a2424be33313c Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 7 Sep 2018 15:30:50 +0100 Subject: [PATCH 1/2] MDL-63020 xmldb: Improve PHPdoc comments for better IDE autocomplete --- lib/ddl/sql_generator.php | 2 +- lib/ddl/tests/ddl_test.php | 4 +++- lib/dml/moodle_database.php | 2 +- lib/xmldb/xmldb_index.php | 2 +- lib/xmldb/xmldb_table.php | 36 ++++++++++++++++++------------------ 5 files changed, 24 insertions(+), 22 deletions(-) diff --git a/lib/ddl/sql_generator.php b/lib/ddl/sql_generator.php index 721fce7ea37..64fa5d73177 100644 --- a/lib/ddl/sql_generator.php +++ b/lib/ddl/sql_generator.php @@ -1129,7 +1129,7 @@ abstract class sql_generator { * if it's a reserved word * * @param string|array $input String to quote. - * @return string Quoted string. + * @return string|array Quoted string. */ public function getEncQuoted($input) { diff --git a/lib/ddl/tests/ddl_test.php b/lib/ddl/tests/ddl_test.php index 734e604fdc8..fb01903e1aa 100644 --- a/lib/ddl/tests/ddl_test.php +++ b/lib/ddl/tests/ddl_test.php @@ -26,8 +26,10 @@ defined('MOODLE_INTERNAL') || die(); class core_ddl_testcase extends database_driver_testcase { + /** @var xmldb_table[] keys are table name. Created in setUp. */ private $tables = array(); - private $records= array(); + /** @var array table name => array of stdClass test records loaded into that table. Created in setUp. */ + private $records = array(); protected function setUp() { parent::setUp(); diff --git a/lib/dml/moodle_database.php b/lib/dml/moodle_database.php index a16b9336399..e5839d30fb0 100644 --- a/lib/dml/moodle_database.php +++ b/lib/dml/moodle_database.php @@ -1080,7 +1080,7 @@ abstract class moodle_database { * Returns detailed information about columns in table. This information is cached internally. * @param string $table The table's name. * @param bool $usecache Flag to use internal cacheing. The default is true. - * @return array of database_column_info objects indexed with column names + * @return database_column_info[] of database_column_info objects indexed with column names */ public abstract function get_columns($table, $usecache=true); diff --git a/lib/xmldb/xmldb_index.php b/lib/xmldb/xmldb_index.php index e8c82d8f733..7c07b680f27 100644 --- a/lib/xmldb/xmldb_index.php +++ b/lib/xmldb/xmldb_index.php @@ -338,7 +338,7 @@ class xmldb_index extends xmldb_object { */ public function validateDefinition(xmldb_table $xmldb_table=null) { if (!$xmldb_table) { - return 'Invalid xmldb_index->validateDefinition() call, $xmldb_table si required.'; + return 'Invalid xmldb_index->validateDefinition() call, $xmldb_table is required.'; } $total = 0; diff --git a/lib/xmldb/xmldb_table.php b/lib/xmldb/xmldb_table.php index b380fa53808..b68350a93fe 100644 --- a/lib/xmldb/xmldb_table.php +++ b/lib/xmldb/xmldb_table.php @@ -28,13 +28,13 @@ defined('MOODLE_INTERNAL') || die(); class xmldb_table extends xmldb_object { - /** @var array table columns */ + /** @var xmldb_field[] table columns */ protected $fields; - /** @var array keys */ + /** @var xmldb_key[] keys */ protected $keys; - /** @var array indexes */ + /** @var xmldb_index[] indexes */ protected $indexes; /** @@ -239,7 +239,7 @@ class xmldb_table extends xmldb_object { /** * This function will return the array of fields in the table - * @return array + * @return xmldb_field[] */ public function getFields() { return $this->fields; @@ -247,7 +247,7 @@ class xmldb_table extends xmldb_object { /** * This function will return the array of keys in the table - * @return array + * @return xmldb_key[] */ public function getKeys() { return $this->keys; @@ -255,7 +255,7 @@ class xmldb_table extends xmldb_object { /** * This function will return the array of indexes in the table - * @return array + * @return xmldb_index[] */ public function getIndexes() { return $this->indexes; @@ -264,7 +264,7 @@ class xmldb_table extends xmldb_object { /** * Returns one xmldb_field * @param string $fieldname - * @return mixed + * @return xmldb_field|null */ public function getField($fieldname) { $i = $this->findFieldInArray($fieldname); @@ -277,7 +277,7 @@ class xmldb_table extends xmldb_object { /** * Returns the position of one field in the array. * @param string $fieldname - * @return mixed + * @return int|null index of the field, or null if not found. */ public function findFieldInArray($fieldname) { foreach ($this->fields as $i => $field) { @@ -290,7 +290,7 @@ class xmldb_table extends xmldb_object { /** * This function will reorder the array of fields - * @return bool + * @return bool whether the reordering succeeded. */ public function orderFields() { $result = $this->orderElements($this->fields); @@ -305,7 +305,7 @@ class xmldb_table extends xmldb_object { /** * Returns one xmldb_key * @param string $keyname - * @return mixed + * @return xmldb_key|null */ public function getKey($keyname) { $i = $this->findKeyInArray($keyname); @@ -318,7 +318,7 @@ class xmldb_table extends xmldb_object { /** * Returns the position of one key in the array. * @param string $keyname - * @return mixed + * @return int|null index of the key, or null if not found. */ public function findKeyInArray($keyname) { foreach ($this->keys as $i => $key) { @@ -331,7 +331,7 @@ class xmldb_table extends xmldb_object { /** * This function will reorder the array of keys - * @return bool + * @return bool whether the reordering succeeded. */ public function orderKeys() { $result = $this->orderElements($this->keys); @@ -346,7 +346,7 @@ class xmldb_table extends xmldb_object { /** * Returns one xmldb_index * @param string $indexname - * @return mixed + * @return xmldb_index|null */ public function getIndex($indexname) { $i = $this->findIndexInArray($indexname); @@ -359,7 +359,7 @@ class xmldb_table extends xmldb_object { /** * Returns the position of one index in the array. * @param string $indexname - * @return mixed + * @return int|null index of the index, or null if not found. */ public function findIndexInArray($indexname) { foreach ($this->indexes as $i => $index) { @@ -372,7 +372,7 @@ class xmldb_table extends xmldb_object { /** * This function will reorder the array of indexes - * @return bool + * @return bool whether the reordering succeeded. */ public function orderIndexes() { $result = $this->orderElements($this->indexes); @@ -386,7 +386,7 @@ class xmldb_table extends xmldb_object { /** * This function will set the array of fields in the table - * @param array $fields + * @param xmldb_field[] $fields */ public function setFields($fields) { $this->fields = $fields; @@ -394,7 +394,7 @@ class xmldb_table extends xmldb_object { /** * This function will set the array of keys in the table - * @param array $keys + * @param xmldb_key[] $keys */ public function setKeys($keys) { $this->keys = $keys; @@ -402,7 +402,7 @@ class xmldb_table extends xmldb_object { /** * This function will set the array of indexes in the table - * @param array $indexes + * @param xmldb_index[] $indexes */ public function setIndexes($indexes) { $this->indexes = $indexes; From eb43bdbbb022c9e37ca9b604be4f1e174ae12ad7 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 7 Sep 2018 15:32:33 +0100 Subject: [PATCH 2/2] MDL-63020 ddl: fix nullable unique indexes in OCI and MS SQL This works-around the default non-standard behaviour of these DB engines. --- lib/ddl/mssql_sql_generator.php | 30 +++++++++++++++++ lib/ddl/oracle_sql_generator.php | 55 ++++++++++++++++++++++++++++++++ lib/ddl/sql_generator.php | 40 +++++++++++++++++++++++ lib/ddl/tests/ddl_test.php | 22 +++++++++++++ lib/dml/tests/dml_test.php | 38 ++++++++++++++++++++++ 5 files changed, 185 insertions(+) diff --git a/lib/ddl/mssql_sql_generator.php b/lib/ddl/mssql_sql_generator.php index e3543d763fc..bfa03753d56 100644 --- a/lib/ddl/mssql_sql_generator.php +++ b/lib/ddl/mssql_sql_generator.php @@ -140,6 +140,36 @@ class mssql_sql_generator extends sql_generator { return $tablename; } + public function getCreateIndexSQL($xmldb_table, $xmldb_index) { + list($indexsql) = parent::getCreateIndexSQL($xmldb_table, $xmldb_index); + + // Unique indexes need to work-around non-standard SQL server behaviour. + if ($xmldb_index->getUnique()) { + // Find any nullable columns. We need to add a + // WHERE field IS NOT NULL to the index definition for each one. + // + // For example if you have a unique index on the three columns + // (required, option1, option2) where the first one is non-null, + // and the others nullable, then the SQL will end up as + // + // CREATE UNIQUE INDEX index_name ON table_name (required, option1, option2) + // WHERE option1 IS NOT NULL AND option2 IS NOT NULL + // + // The first line comes from parent calls above. The WHERE is added below. + $extraconditions = []; + foreach ($this->get_nullable_fields_in_index($xmldb_table, $xmldb_index) as $fieldname) { + $extraconditions[] = $this->getEncQuoted($fieldname) . + ' IS NOT NULL'; + } + + if ($extraconditions) { + $indexsql .= ' WHERE ' . implode(' AND ', $extraconditions); + } + } + + return [$indexsql]; + } + /** * Given one correct xmldb_table, returns the SQL statements * to create temporary table (inside one array). diff --git a/lib/ddl/oracle_sql_generator.php b/lib/ddl/oracle_sql_generator.php index 8deccf98fbe..5e1325c53b6 100644 --- a/lib/ddl/oracle_sql_generator.php +++ b/lib/ddl/oracle_sql_generator.php @@ -131,6 +131,61 @@ class oracle_sql_generator extends sql_generator { return $tablename; } + public function getCreateIndexSQL($xmldb_table, $xmldb_index) { + if ($error = $xmldb_index->validateDefinition($xmldb_table)) { + throw new coding_exception($error); + } + + $indexfields = $this->getEncQuoted($xmldb_index->getFields()); + + $unique = ''; + $suffix = 'ix'; + if ($xmldb_index->getUnique()) { + $unique = ' UNIQUE'; + $suffix = 'uix'; + + $nullablefields = $this->get_nullable_fields_in_index($xmldb_table, $xmldb_index); + if ($nullablefields) { + // If this is a unique index with nullable fields, then we have to + // apply the work-around from https://community.oracle.com/message/9518046#9518046. + // + // For example if you have a unique index on the three columns + // (required, option1, option2) where the first one is non-null, + // and the others nullable, then the SQL will end up as + // + // CREATE UNIQUE INDEX index_name ON table_name ( + // CASE WHEN option1 IS NOT NULL AND option2 IS NOT NULL THEN required ELSE NULL END, + // CASE WHEN option1 IS NOT NULL AND option2 IS NOT NULL THEN option1 ELSE NULL END, + // CASE WHEN option1 IS NOT NULL AND option2 IS NOT NULL THEN option2 ELSE NULL END) + // + // Basically Oracle behaves according to the standard if either + // none of the columns are NULL or all columns contain NULL. Therefore, + // if any column is NULL, we treat them all as NULL for the index. + $conditions = []; + foreach ($nullablefields as $fieldname) { + $conditions[] = $this->getEncQuoted($fieldname) . + ' IS NOT NULL'; + } + $condition = implode(' AND ', $conditions); + + $updatedindexfields = []; + foreach ($indexfields as $fieldname) { + $updatedindexfields[] = 'CASE WHEN ' . $condition . ' THEN ' . + $fieldname . ' ELSE NULL END'; + } + $indexfields = $updatedindexfields; + } + + } + + $index = 'CREATE' . $unique . ' INDEX '; + $index .= $this->getNameForObject($xmldb_table->getName(), implode(', ', $xmldb_index->getFields()), $suffix); + $index .= ' ON ' . $this->getTableName($xmldb_table); + $index .= ' (' . implode(', ', $indexfields) . ')'; + + return array($index); + } + /** * Given one correct xmldb_table, returns the SQL statements * to create temporary table (inside one array). diff --git a/lib/ddl/sql_generator.php b/lib/ddl/sql_generator.php index 64fa5d73177..9ad419f512d 100644 --- a/lib/ddl/sql_generator.php +++ b/lib/ddl/sql_generator.php @@ -1408,4 +1408,44 @@ abstract class sql_generator { $s = str_replace("'", "\\'", $s); return $s; } + + /** + * Get the fields from an index definition that might be null. + * @param xmldb_table $xmldb_table the table + * @param xmldb_index $xmldb_index the index + * @return array list of fields in the index definition that might be null. + */ + public function get_nullable_fields_in_index($xmldb_table, $xmldb_index) { + global $DB; + + // If we don't have the field info passed in, we need to query it from the DB. + $fieldsfromdb = null; + + $nullablefields = []; + foreach ($xmldb_index->getFields() as $fieldname) { + if ($field = $xmldb_table->getField($fieldname)) { + // We have the field details in the table definition. + if ($field->getNotNull() !== XMLDB_NOTNULL) { + $nullablefields[] = $fieldname; + } + + } else { + // We don't have the table definition loaded. Need to + // inspect the database. + if ($fieldsfromdb === null) { + $fieldsfromdb = $DB->get_columns($xmldb_table->getName(), false); + } + if (!isset($fieldsfromdb[$fieldname])) { + throw new coding_exception('Unknown field ' . $fieldname . + ' in index ' . $xmldb_index->getName()); + } + + if (!$fieldsfromdb[$fieldname]->not_null) { + $nullablefields[] = $fieldname; + } + } + } + + return $nullablefields; + } } diff --git a/lib/ddl/tests/ddl_test.php b/lib/ddl/tests/ddl_test.php index fb01903e1aa..4f0b190ab5e 100644 --- a/lib/ddl/tests/ddl_test.php +++ b/lib/ddl/tests/ddl_test.php @@ -2291,6 +2291,28 @@ class core_ddl_testcase extends database_driver_testcase { } } + public function test_get_nullable_fields_in_index() { + $DB = $this->tdb; + $gen = $DB->get_manager()->generator; + + $indexwithoutnulls = $this->tables['test_table0']->getIndex('type-name'); + $this->assertSame([], $gen->get_nullable_fields_in_index( + $this->tables['test_table0'], $indexwithoutnulls)); + + $indexwithnulls = new xmldb_index('course-grade', XMLDB_INDEX_UNIQUE, ['course', 'grade']); + $this->assertSame(['grade'], $gen->get_nullable_fields_in_index( + $this->tables['test_table0'], $indexwithnulls)); + + $this->create_deftable('test_table0'); + + // Now test using a minimal xmldb_table, to ensure we get the data from the DB. + $table = new xmldb_table('test_table0'); + $this->assertSame([], $gen->get_nullable_fields_in_index( + $table, $indexwithoutnulls)); + $this->assertSame(['grade'], $gen->get_nullable_fields_in_index( + $table, $indexwithnulls)); + } + // Following methods are not supported == Do not test. /* public function testRenameIndex() { diff --git a/lib/dml/tests/dml_test.php b/lib/dml/tests/dml_test.php index 866625da722..cea23a8de36 100644 --- a/lib/dml/tests/dml_test.php +++ b/lib/dml/tests/dml_test.php @@ -2407,6 +2407,44 @@ class core_dml_testcase extends database_driver_testcase { } } + public function test_insert_record_with_nullable_unique_index() { + $DB = $this->tdb; + $dbman = $DB->get_manager(); + + $table = $this->get_test_table(); + $tablename = $table->getName(); + + $table->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null); + $table->add_field('notnull1', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, '0'); + $table->add_field('nullable1', XMLDB_TYPE_INTEGER, '10', null, null, null, null); + $table->add_field('nullable2', XMLDB_TYPE_INTEGER, '10', null, null, null, null); + $table->add_key('primary', XMLDB_KEY_PRIMARY, array('id')); + $table->add_index('notnull1-nullable1-nullable2', XMLDB_INDEX_UNIQUE, + array('notnull1', 'nullable1', 'nullable2')); + $dbman->create_table($table); + + // Insert one record. Should be OK (no exception). + $DB->insert_record($tablename, (object) ['notnull1' => 1, 'nullable1' => 1, 'nullable2' => 1]); + + // Inserting a duplicate should fail. + try { + $DB->insert_record($tablename, (object) ['notnull1' => 1, 'nullable1' => 1, 'nullable2' => 1]); + $this->fail('dml_write_exception expected when a record violates a unique index'); + } catch (moodle_exception $e) { + $this->assertInstanceOf('dml_write_exception', $e); + } + + // Inserting a record with nulls in the nullable columns should work. + $DB->insert_record($tablename, (object) ['notnull1' => 1, 'nullable1' => null, 'nullable2' => null]); + + // And it should be possible to insert a duplicate. + $DB->insert_record($tablename, (object) ['notnull1' => 1, 'nullable1' => null, 'nullable2' => null]); + + // Same, but with only one of the nullable columns being null. + $DB->insert_record($tablename, (object) ['notnull1' => 1, 'nullable1' => 1, 'nullable2' => null]); + $DB->insert_record($tablename, (object) ['notnull1' => 1, 'nullable1' => 1, 'nullable2' => null]); + } + public function test_import_record() { // All the information in this test is fetched from DB by get_recordset() so we // have such method properly tested against nulls, empties and friends...