From 257ad88f8924f481769516e802582853d244380c Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Sun, 11 Sep 2011 20:30:42 +0200 Subject: [PATCH] MDL-29313 enforce varchar size limit --- admin/xmldb/actions/edit_field/edit_field.js | 2 +- .../edit_field_save/edit_field_save.class.php | 2 +- lib/ddl/mysql_sql_generator.php | 2 +- lib/ddl/postgres_sql_generator.php | 2 +- lib/ddl/simpletest/testddl.php | 50 ++++++++++++++++ lib/ddl/sql_generator.php | 11 ++-- lib/xmldb/xmldb_field.php | 58 +++++++++++++++++++ 7 files changed, 119 insertions(+), 8 deletions(-) diff --git a/admin/xmldb/actions/edit_field/edit_field.js b/admin/xmldb/actions/edit_field/edit_field.js index 5fcf76cce28..065828d883c 100644 --- a/admin/xmldb/actions/edit_field/edit_field.js +++ b/admin/xmldb/actions/edit_field/edit_field.js @@ -99,7 +99,7 @@ function transformForm(event) { decimalsTip.innerHTML = ' 0...length or empty'; break; case '4': // XMLDB_TYPE_CHAR - lengthTip.innerHTML = ' 1...255'; + lengthTip.innerHTML = ' 1...'.xmldb_field::CHAR_MAX_LENGTH; decimalsTip.innerHTML = ''; decimalsField.disabled = true; decimalsField.value = ''; diff --git a/admin/xmldb/actions/edit_field_save/edit_field_save.class.php b/admin/xmldb/actions/edit_field_save/edit_field_save.class.php index b762063bd7d..710c36c5ab3 100644 --- a/admin/xmldb/actions/edit_field_save/edit_field_save.class.php +++ b/admin/xmldb/actions/edit_field_save/edit_field_save.class.php @@ -192,7 +192,7 @@ class edit_field_save extends XMLDBAction { /// Char checks if ($type == XMLDB_TYPE_CHAR) { if (!(is_numeric($length) && !empty($length) && intval($length)==floatval($length) && - $length > 0 && $length <= 255)) { + $length > 0 && $length <= xmldb_field::CHAR_MAX_LENGTH)) { $errors[] = $this->str['charincorrectlength']; } if ($default !== NULL && $default !== '') { diff --git a/lib/ddl/mysql_sql_generator.php b/lib/ddl/mysql_sql_generator.php index 38b2d7b3579..3004c7680c8 100644 --- a/lib/ddl/mysql_sql_generator.php +++ b/lib/ddl/mysql_sql_generator.php @@ -281,7 +281,7 @@ class mysql_sql_generator extends sql_generator { /// Change the name of the field to perform the change $xmldb_field_clone->setName($xmldb_field_clone->getName() . ' ' . $newname); - $fieldsql = $this->getFieldSQL($xmldb_field_clone); + $fieldsql = $this->getFieldSQL($xmldb_table, $xmldb_field_clone); $sql = 'ALTER TABLE ' . $this->getTableName($xmldb_table) . ' CHANGE ' . $fieldsql; diff --git a/lib/ddl/postgres_sql_generator.php b/lib/ddl/postgres_sql_generator.php index cb8e00b06b4..5e2a3c8ee48 100644 --- a/lib/ddl/postgres_sql_generator.php +++ b/lib/ddl/postgres_sql_generator.php @@ -280,7 +280,7 @@ class postgres_sql_generator extends sql_generator { $results[] = 'ALTER TABLE ' . $tablename . ' ALTER COLUMN ' . $fieldname . ' DROP DEFAULT'; /// Drop default clause } $alterstmt = 'ALTER TABLE ' . $tablename . ' ALTER COLUMN ' . $this->getEncQuoted($xmldb_field->getName()) . - ' TYPE' . $this->getFieldSQL($xmldb_field, null, true, true, null, false); + ' TYPE' . $this->getFieldSQL($xmldb_table, $xmldb_field, null, true, true, null, false); /// Some castings must be performed explicity (mainly from text|char to numeric|integer) if (($oldmetatype == 'C' || $oldmetatype == 'X') && ($xmldb_field->getType() == XMLDB_TYPE_NUMBER || $xmldb_field->getType() == XMLDB_TYPE_FLOAT)) { diff --git a/lib/ddl/simpletest/testddl.php b/lib/ddl/simpletest/testddl.php index 66b8e7b3f58..2266721c533 100644 --- a/lib/ddl/simpletest/testddl.php +++ b/lib/ddl/simpletest/testddl.php @@ -1596,6 +1596,56 @@ class ddl_test extends UnitTestCase { } } + public function test_char_size_limit() { + $DB = $this->tdb; + $dbman = $DB->get_manager(); + + $maxstr = ''; + for($i=0; $iadd_field('id', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, XMLDB_SEQUENCE, null); + $table->add_field('name', XMLDB_TYPE_CHAR, xmldb_field::CHAR_MAX_LENGTH, null, XMLDB_NOTNULL, null); + $table->add_key('primary', XMLDB_KEY_PRIMARY, array('id')); + + // Drop if exists + if ($dbman->table_exists($table)) { + $dbman->drop_table($table); + } + $dbman->create_table($table); + $tablename = $table->getName(); + $this->tables[$tablename] = $table; + + $rec = new stdClass(); + $rec->name = $maxstr; + + $id = $DB->insert_record($tablename, $rec); + $this->assertTrue(!empty($id)); + + $rec = $DB->get_record($tablename, array('id'=>$id)); + $this->assertIdentical($rec->name, $maxstr); + + + $table = new xmldb_table('testtable'); + $table->add_field('id', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, XMLDB_SEQUENCE, null); + $table->add_field('name', XMLDB_TYPE_CHAR, xmldb_field::CHAR_MAX_LENGTH+1, null, XMLDB_NOTNULL, null); + $table->add_key('primary', XMLDB_KEY_PRIMARY, array('id')); + + // Drop if exists + if ($dbman->table_exists($table)) { + $dbman->drop_table($table); + } + + try { + $dbman->create_table($table); + $this->assertTrue(false); + } catch (Exception $e) { + $this->assertTrue($e instanceof coding_exception); + } + } + // Following methods are not supported == Do not test /* public function testRenameIndex() { diff --git a/lib/ddl/sql_generator.php b/lib/ddl/sql_generator.php index 0717e9a1bf0..203ccf3686f 100644 --- a/lib/ddl/sql_generator.php +++ b/lib/ddl/sql_generator.php @@ -240,7 +240,7 @@ abstract class sql_generator { if ($xmldb_field->getSequence()) { $sequencefield = $xmldb_field->getName(); } - $table .= "\n " . $this->getFieldSQL($xmldb_field); + $table .= "\n " . $this->getFieldSQL($xmldb_table, $xmldb_field); $table .= ','; } /// Add the keys, separated by commas @@ -370,7 +370,10 @@ abstract class sql_generator { /** * Given one correct xmldb_field, returns the complete SQL line to create it */ - public function getFieldSQL($xmldb_field, $skip_type_clause = NULL, $skip_default_clause = NULL, $skip_notnull_clause = NULL, $specify_nulls_clause = NULL, $specify_field_name = true) { + public function getFieldSQL($xmldb_table, $xmldb_field, $skip_type_clause = NULL, $skip_default_clause = NULL, $skip_notnull_clause = NULL, $specify_nulls_clause = NULL, $specify_field_name = true) { + if ($error = $xmldb_field->validateDefinition($xmldb_table)) { + throw new coding_exception($error); + } $skip_type_clause = is_null($skip_type_clause) ? $this->alter_column_skip_type : $skip_type_clause; $skip_default_clause = is_null($skip_default_clause) ? $this->alter_column_skip_default : $skip_default_clause; @@ -595,7 +598,7 @@ abstract class sql_generator { $tablename = $this->getTableName($xmldb_table); /// Build the standard alter table add - $sql = $this->getFieldSQL($xmldb_field, $skip_type_clause, + $sql = $this->getFieldSQL($xmldb_table, $xmldb_field, $skip_type_clause, $skip_default_clause, $skip_notnull_clause); $altertable = 'ALTER TABLE ' . $tablename . ' ADD ' . $sql; @@ -642,7 +645,7 @@ abstract class sql_generator { /// Build de alter sentence using the alter_column_sql template $alter = str_replace('TABLENAME', $this->getTableName($xmldb_table), $this->alter_column_sql); - $colspec = $this->getFieldSQL($xmldb_field, $skip_type_clause, + $colspec = $this->getFieldSQL($xmldb_table, $xmldb_field, $skip_type_clause, $skip_default_clause, $skip_notnull_clause, true); diff --git a/lib/xmldb/xmldb_field.php b/lib/xmldb/xmldb_field.php index cd00b77f687..171fed9cda6 100644 --- a/lib/xmldb/xmldb_field.php +++ b/lib/xmldb/xmldb_field.php @@ -36,6 +36,17 @@ class xmldb_field extends xmldb_object { var $sequence; var $decimals; + /** + * Note: + * - Oracle: VARCHAR2 has a limit of 4000 bytes + * - SQL Server: NVARCHAR has a limit of 40000 chars + * - MySQL: VARCHAR 65,535 chars + * - PostgreSQL: no limit + * + * @const maximum length of text field + */ + const CHAR_MAX_LENGTH = 255; //TODO: bump up to 1333 + /** * Creates one new xmldb_field */ @@ -785,6 +796,53 @@ class xmldb_field extends xmldb_object { return $o; } + + /** + * Validates the field restrictions. + * + * The error message should not be localised because it is intended for developers, + * end users and admins should never see these problems! + * + * @param xmldb_table $xmldb_table optional when object is table + * @return string null if ok, error message if problem found + */ + function validateDefinition(xmldb_table $xmldb_table=null) { + if (!$xmldb_table) { + return 'Invalid xmldb_field->validateDefinition() call, $xmldb_table si required.'; + } + + switch ($this->getType()) { + case XMLDB_TYPE_INTEGER: + break; + + case XMLDB_TYPE_NUMBER: + break; + + case XMLDB_TYPE_FLOAT: + break; + + case XMLDB_TYPE_CHAR: + if ($this->getLength() > self::CHAR_MAX_LENGTH) { + return 'Invalid field definition in table {'.$xmldb_table->getName(). '}: XMLDB_TYPE_CHAR field "'.$this->getName().'" is too long.' + .' Limit is '.self::CHAR_MAX_LENGTH.' chars.'; + } + break; + + case XMLDB_TYPE_TEXT: + break; + + case XMLDB_TYPE_BINARY: + break; + + case XMLDB_TYPE_DATETIME: + break; + + case XMLDB_TYPE_TIMESTAMP: + break; + } + + return null; + } } /// TODO: Delete for 2.1 (deprecated in 2.0).