Skip to content

Commit

Permalink
database MDL-24863
Browse files Browse the repository at this point in the history 
- added restricting text conditions to where_clause , it throws a dml_exception when detected.
- also added to where_clause throwing dml_exception for field that doesn't exist in table.
- added unit tests for text condition restricting.
- added 2 unit tests for set_field_select() testing 'auto-casting params to int problem' fix
  • Loading branch information
@nebgor
nebgor committed Nov 18, 2010
1 parent fa8f03e commit 011bfd2
Show file tree
Hide file tree
Showing 4 changed files with 153 additions and 11 deletions.
1 change: 1 addition & 0 deletions lang/en/error.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@
$string['tagdisabled'] = 'Tags are disabled!';
$string['tagnotfound'] = 'The specified tag was not found in the database';
$string['targetdatabasenotempty'] = 'The target database is not empty. Transfer aborted for safety reasons.';
$string['textconditionsnotallowed'] = 'Comparisons of text column conditions are not allowed. Please use sql_compare_text() in your query.';
$string['themenotinstall'] = 'This theme is not installed!';
$string['TODO'] = 'TODO';
$string['tokengenerationfailed'] = 'Cannot generate a new token.';
Expand Down
33 changes: 23 additions & 10 deletions lib/dml/moodle_database.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -487,18 +487,31 @@ protected function print_debug($sql, array $params=null, $obj=null) {

/**
* Returns SQL WHERE conditions.
*
* @param string $table - the table name that these conditions will be validated against.
* @param array conditions - must not contain numeric indexes
* @return array sql part and params
*/
protected function where_clause(array $conditions=null) {
protected function where_clause($table, array $conditions=null) {
$allowed_types = $this->allowed_param_types();
if (empty($conditions)) {
return array('', array());
}
$where = array();
$params = array();

$columns = $this->get_columns($table);
foreach ($conditions as $key=>$value) {
if (!isset($columns[$key])) {
$a = new stdClass();
$a->fieldname = $key;
$a->tablename = $table;
throw new dml_exception('ddlfieldnotexist', $a);
}
$column = $columns[$key];
if ($column->meta_type == 'X') {
//ok so the column is a text column. sorry no text columns in the where clause conditions
throw new dml_exception('textconditionsnotallowed', $conditions);
}
if (is_int($key)) {
throw new dml_exception('invalidnumkey');
}
Expand Down Expand Up @@ -921,7 +934,7 @@ public abstract function execute($sql, array $params=null);
* @throws dml_exception if error
*/
public function get_recordset($table, array $conditions=null, $sort='', $fields='*', $limitfrom=0, $limitnum=0) {
list($select, $params) = $this->where_clause($conditions);
list($select, $params) = $this->where_clause($table, $conditions);
return $this->get_recordset_select($table, $select, $params, $sort, $fields, $limitfrom, $limitnum);
}

Expand Down Expand Up @@ -1020,7 +1033,7 @@ public abstract function get_recordset_sql($sql, array $params=null, $limitfrom=
* @throws dml_exception if error
*/
public function get_records($table, array $conditions=null, $sort='', $fields='*', $limitfrom=0, $limitnum=0) {
list($select, $params) = $this->where_clause($conditions);
list($select, $params) = $this->where_clause($table, $conditions);
return $this->get_records_select($table, $select, $params, $sort, $fields, $limitfrom, $limitnum);
}

Expand Down Expand Up @@ -1191,7 +1204,7 @@ public function get_records_sql_menu($sql, array $params=null, $limitfrom=0, $li
* @throws dml_exception if error
*/
public function get_record($table, array $conditions, $fields='*', $strictness=IGNORE_MISSING) {
list($select, $params) = $this->where_clause($conditions);
list($select, $params) = $this->where_clause($table, $conditions);
return $this->get_record_select($table, $select, $params, $fields, $strictness);
}

Expand Down Expand Up @@ -1272,7 +1285,7 @@ public function get_record_sql($sql, array $params=null, $strictness=IGNORE_MISS
* @throws dml_exception if error
*/
public function get_field($table, $return, array $conditions, $strictness=IGNORE_MISSING) {
list($select, $params) = $this->where_clause($conditions);
list($select, $params) = $this->where_clause($table, $conditions);
return $this->get_field_select($table, $return, $select, $params, $strictness);
}

Expand Down Expand Up @@ -1424,7 +1437,7 @@ public abstract function update_record($table, $dataobject, $bulk=false);
* @throws dml_exception if error
*/
public function set_field($table, $newfield, $newvalue, array $conditions=null) {
list($select, $params) = $this->where_clause($conditions);
list($select, $params) = $this->where_clause($table, $conditions);
return $this->set_field_select($table, $newfield, $newvalue, $select, $params);
}

Expand All @@ -1451,7 +1464,7 @@ public abstract function set_field_select($table, $newfield, $newvalue, $select,
* @throws dml_exception if error
*/
public function count_records($table, array $conditions=null) {
list($select, $params) = $this->where_clause($conditions);
list($select, $params) = $this->where_clause($table, $conditions);
return $this->count_records_select($table, $select, $params);
}

Expand Down Expand Up @@ -1505,7 +1518,7 @@ public function count_records_sql($sql, array $params=null) {
* @throws dml_exception if error
*/
public function record_exists($table, array $conditions) {
list($select, $params) = $this->where_clause($conditions);
list($select, $params) = $this->where_clause($table, $conditions);
return $this->record_exists_select($table, $select, $params);
}

Expand Down Expand Up @@ -1558,7 +1571,7 @@ public function delete_records($table, array $conditions=null) {
if (is_null($conditions)) {
return $this->execute("TRUNCATE TABLE {".$table."}");
}
list($select, $params) = $this->where_clause($conditions);
list($select, $params) = $this->where_clause($table, $conditions);
return $this->delete_records_select($table, $select, $params);
}

Expand Down
128 changes: 128 additions & 0 deletions lib/dml/simpletest/testdml.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,7 @@ public function test_get_recordset() {
$table->add_field('id', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, XMLDB_SEQUENCE, null);
$table->add_field('course', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, '0');
$table->add_field('name', XMLDB_TYPE_CHAR, '255', null, null, null, '0');
$table->add_field('onetext', XMLDB_TYPE_TEXT, 'big', null, null, null);
$table->add_index('course', XMLDB_INDEX_NOTUNIQUE, array('course'));
$table->add_key('primary', XMLDB_KEY_PRIMARY, array('id'));
$dbman->create_table($table);
Expand Down Expand Up @@ -694,6 +695,19 @@ public function test_get_recordset() {
}
$rs->close();

// test for exception throwing on text conditions being compared. (MDL-24863, unwanted auto conversion of param to int)
$conditions = array('onetext' => '1');
try {
$rs = $DB->get_recordset($tablename, $conditions);
$this->assertFalse(true, 'An Exception is missing, expected due to equating of text fields');
} catch (dml_exception $e) {
if ($e->errorcode == 'textconditionsnotallowed') {
$this->assertTrue(true, 'The Expected exception was caught.');
} else {
throw $e;
}
}

// notes:
// * limits are tested in test_get_recordset_sql()
// * where_clause() is used internally and is tested in test_get_records()
Expand Down Expand Up @@ -894,6 +908,7 @@ public function test_get_records() {

$table->add_field('id', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, XMLDB_SEQUENCE, null);
$table->add_field('course', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, '0');
$table->add_field('onetext', XMLDB_TYPE_TEXT, 'big', null, null, null);
$table->add_key('primary', XMLDB_KEY_PRIMARY, array('id'));
$dbman->create_table($table);

Expand Down Expand Up @@ -940,6 +955,19 @@ public function test_get_records() {
$records = $DB->get_records($tablename, array('course' => false));
$this->assertEqual(0, count($records));

// test for exception throwing on text conditions being compared. (MDL-24863, unwanted auto conversion of param to int)
$conditions = array('onetext' => '1');
try {
$records = $DB->get_records($tablename, $conditions);
$this->assertFalse(true, 'An Exception is missing, expected due to equating of text fields');
} catch (dml_exception $e) {
if ($e->errorcode == 'textconditionsnotallowed') {
$this->assertTrue(true, 'The Expected exception was caught.');
} else {
throw $e;
}
}

// note: delegate limits testing to test_get_records_sql()
}

Expand Down Expand Up @@ -1251,6 +1279,7 @@ public function test_get_field() {

$table->add_field('id', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, XMLDB_SEQUENCE, null);
$table->add_field('course', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, '0');
$table->add_field('onetext', XMLDB_TYPE_TEXT, 'big', null, null, null);
$table->add_key('primary', XMLDB_KEY_PRIMARY, array('id'));
$dbman->create_table($table);

Expand All @@ -1276,6 +1305,19 @@ public function test_get_field() {
$this->enable_debugging();
$this->assertEqual(5, $DB->get_field($tablename, 'course', array('course' => 5), IGNORE_MISSING));
$this->assertFalse($this->get_debugging() === '');

// test for exception throwing on text conditions being compared. (MDL-24863, unwanted auto conversion of param to int)
$conditions = array('onetext' => '1');
try {
$DB->get_field($tablename, 'course', $conditions);
$this->assertFalse(true, 'An Exception is missing, expected due to equating of text fields');
} catch (dml_exception $e) {
if ($e->errorcode == 'textconditionsnotallowed') {
$this->assertTrue(true, 'The Expected exception was caught.');
} else {
throw $e;
}
}
}

public function test_get_field_select() {
Expand Down Expand Up @@ -2018,6 +2060,8 @@ public function test_set_field() {

$table->add_field('id', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, XMLDB_SEQUENCE, null);
$table->add_field('course', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, '0');
$table->add_field('onechar', XMLDB_TYPE_CHAR, '100', null, null, null);
$table->add_field('onetext', XMLDB_TYPE_TEXT, 'big', null, null, null);
$table->add_key('primary', XMLDB_KEY_PRIMARY, array('id'));
$dbman->create_table($table);

Expand Down Expand Up @@ -2060,6 +2104,19 @@ public function test_set_field() {
$this->assertEqual(5, $DB->get_field($tablename, 'course', array('id' => $id2)));
$this->assertEqual(5, $DB->get_field($tablename, 'course', array('id' => $id3)));

// test for exception throwing on text conditions being compared. (MDL-24863, unwanted auto conversion of param to int)
$conditions = array('onetext' => '1');
try {
$DB->set_field($tablename, 'onechar', 'frog', $conditions);
$this->assertFalse(true, 'An Exception is missing, expected due to equating of text fields');
} catch (dml_exception $e) {
if ($e->errorcode == 'textconditionsnotallowed') {
$this->assertTrue(true, 'The Expected exception was caught.');
} else {
throw $e;
}
}

// Note: All the nulls, booleans, empties, quoted and backslashes tests
// go to set_field_select() because set_field() is just one wrapper over it
}
Expand Down Expand Up @@ -2190,6 +2247,22 @@ public function test_set_field_select() {
$DB->set_field_select($tablename, 'onebinary', $newblob, 'id = ?', array(1));
$this->assertEqual($newclob, $DB->get_field($tablename, 'onetext', array('id' => 1)), 'Test "small" CLOB set_field (full contents output disabled)');
$this->assertEqual($newblob, $DB->get_field($tablename, 'onebinary', array('id' => 1)), 'Test "small" BLOB set_field (full contents output disabled)');

// This is the failure from MDL-24863. This was giving an error on MSSQL,
// which converts the '1' to an integer, which cannot then be compared with
// onetext cast to a varchar. This should be fixed and working now.
$newchar = 'frog';
// test for exception throwing on text conditions being compared. (MDL-24863, unwanted auto conversion of param to int)
$params = array('onetext' => '1');
try {
$DB->set_field_select($tablename, 'onechar', $newchar, $DB->sql_compare_text('onetext') . ' = ?', $params);
$this->assertTrue(true, 'No exceptions thrown with numerical text param comparison for text field.');
} catch (dml_exception $e) {
$this->assertFalse(true, 'We have an unexpected exception.');
throw $e;
}


}

public function test_count_records() {
Expand All @@ -2202,6 +2275,7 @@ public function test_count_records() {

$table->add_field('id', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, XMLDB_SEQUENCE, null);
$table->add_field('course', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, '0');
$table->add_field('onetext', XMLDB_TYPE_TEXT, 'big', null, null, null);
$table->add_key('primary', XMLDB_KEY_PRIMARY, array('id'));
$dbman->create_table($table);

Expand All @@ -2212,6 +2286,19 @@ public function test_count_records() {
$DB->insert_record($tablename, array('course' => 5));

$this->assertEqual(3, $DB->count_records($tablename));

// test for exception throwing on text conditions being compared. (MDL-24863, unwanted auto conversion of param to int)
$conditions = array('onetext' => '1');
try {
$DB->count_records($tablename, $conditions);
$this->assertFalse(true, 'An Exception is missing, expected due to equating of text fields');
} catch (dml_exception $e) {
if ($e->errorcode == 'textconditionsnotallowed') {
$this->assertTrue(true, 'The Expected exception was caught.');
} else {
throw $e;
}
}
}

public function test_count_records_select() {
Expand Down Expand Up @@ -2266,6 +2353,7 @@ public function test_record_exists() {

$table->add_field('id', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, XMLDB_SEQUENCE, null);
$table->add_field('course', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, '0');
$table->add_field('onetext', XMLDB_TYPE_TEXT, 'big', null, null, null);
$table->add_key('primary', XMLDB_KEY_PRIMARY, array('id'));
$dbman->create_table($table);

Expand All @@ -2276,6 +2364,19 @@ public function test_record_exists() {

$this->assertTrue($DB->record_exists($tablename, array('course' => 3)));


// test for exception throwing on text conditions being compared. (MDL-24863, unwanted auto conversion of param to int)
$conditions = array('onetext' => '1');
try {
$DB->record_exists($tablename, $conditions);
$this->assertFalse(true, 'An Exception is missing, expected due to equating of text fields');
} catch (dml_exception $e) {
if ($e->errorcode == 'textconditionsnotallowed') {
$this->assertTrue(true, 'The Expected exception was caught.');
} else {
throw $e;
}
}
}

public function test_record_exists_select() {
Expand Down Expand Up @@ -2327,6 +2428,7 @@ public function test_delete_records() {

$table->add_field('id', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, XMLDB_SEQUENCE, null);
$table->add_field('course', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, '0');
$table->add_field('onetext', XMLDB_TYPE_TEXT, 'big', null, null, null);
$table->add_key('primary', XMLDB_KEY_PRIMARY, array('id'));
$dbman->create_table($table);

Expand All @@ -2349,6 +2451,32 @@ public function test_delete_records() {
// delete all
$this->assertTrue($DB->delete_records($tablename, array()));
$this->assertEqual(0, $DB->count_records($tablename));

// test for exception throwing on text conditions being compared. (MDL-24863, unwanted auto conversion of param to int)
$conditions = array('onetext'=>'1');
try {
$DB->delete_records($tablename, $conditions);
$this->assertFalse(true, 'An Exception is missing, expected due to equating of text fields');
} catch (dml_exception $e) {
if ($e->errorcode == 'textconditionsnotallowed') {
$this->assertTrue(true, 'The Expected exception was caught.');
} else {
throw $e;
}
}

// test for exception throwing on text conditions being compared. (MDL-24863, unwanted auto conversion of param to int)
$conditions = array('onetext' => 1);
try {
$DB->delete_records($tablename, $conditions);
$this->assertFalse(true, 'An Exception is missing, expected due to equating of text fields');
} catch (dml_exception $e) {
if ($e->errorcode == 'textconditionsnotallowed') {
$this->assertTrue(true, 'The Expected exception was caught.');
} else {
throw $e;
}
}
}

public function test_delete_records_select() {
Expand Down
2 changes: 1 addition & 1 deletion lib/dml/sqlite3_pdo_moodle_database.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ public function delete_records($table, array $conditions=null) {
if (is_null($conditions)) {
return $this->execute("DELETE FROM {{$table}}");
}
list($select, $params) = $this->where_clause($conditions);
list($select, $params) = $this->where_clause($table, $conditions);
return $this->delete_records_select($table, $select, $params);
}

Expand Down

0 comments on commit 011bfd2

Please sign in to comment.