Skip to content

Commit

Permalink
Always escape identifiers in the set(), setUpdateBatch(), and `in…
Browse files Browse the repository at this point in the history 
…sertBatch()` in `BaseBuilder`
  • Loading branch information
@ytetsuro
ytetsuro authored Oct 3, 2021
1 parent 9f63232 commit add6083
Show file tree
Hide file tree
Showing 7 changed files with 102 additions and 17 deletions.
4 changes: 2 additions & 2 deletions system/BaseModel.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ abstract protected function doInsert(array $data);
* This methods works only with dbCalls
*
* @param array|null $set An associative array of insert values
* @param bool|null $escape Whether to escape values and identifiers
* @param bool|null $escape Whether to escape values
* @param int $batchSize The size of the batch to run
* @param bool $testing True means only number of records is returned, false will execute the query
*
Expand Down Expand Up @@ -763,7 +763,7 @@ public function insert($data = null, bool $returnID = true)
* Compiles batch insert runs the queries, validating each row prior.
*
* @param array|null $set an associative array of insert values
* @param bool|null $escape Whether to escape values and identifiers
* @param bool|null $escape Whether to escape values
* @param int $batchSize The size of the batch to run
* @param bool $testing True means only number of records is returned, false will execute the query
*
Expand Down
12 changes: 6 additions & 6 deletions system/Database/BaseBuilder.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1340,7 +1340,7 @@ protected function _limit(string $sql, bool $offsetIgnore = false): string
*
* @param array|object|string $key Field name, or an array of field/value pairs
* @param mixed $value Field value, if $key is a single field
* @param bool|null $escape Whether to escape values and identifiers
* @param bool|null $escape Whether to escape values
*
* @return $this
*/
Expand All @@ -1358,9 +1358,9 @@ public function set($key, $value = '', ?bool $escape = null)
if ($escape) {
$bind = $this->setBind($k, $v, $escape);

$this->QBSet[$this->db->protectIdentifiers($k, false, $escape)] = ":{$bind}:";
$this->QBSet[$this->db->protectIdentifiers($k, false)] = ":{$bind}:";
} else {
$this->QBSet[$this->db->protectIdentifiers($k, false, $escape)] = $v;
$this->QBSet[$this->db->protectIdentifiers($k, false)] = $v;
}
}

Expand Down Expand Up @@ -1604,7 +1604,7 @@ public function insertBatch(?array $set = null, ?bool $escape = null, int $batch
$affectedRows = 0;

for ($i = 0, $total = count($this->QBSet); $i < $total; $i += $batchSize) {
$sql = $this->_insertBatch($this->db->protectIdentifiers($table, true, $escape, false), $this->QBKeys, array_slice($this->QBSet, $i, $batchSize));
$sql = $this->_insertBatch($this->db->protectIdentifiers($table, true, null, false), $this->QBKeys, array_slice($this->QBSet, $i, $batchSize));

if ($this->testMode) {
$affectedRows++;
Expand Down Expand Up @@ -1672,7 +1672,7 @@ public function setInsertBatch($key, string $value = '', ?bool $escape = null)
}

foreach ($keys as $k) {
$this->QBKeys[] = $this->db->protectIdentifiers($k, false, $escape);
$this->QBKeys[] = $this->db->protectIdentifiers($k, false);
}

return $this;
Expand Down Expand Up @@ -2052,7 +2052,7 @@ public function setUpdateBatch($key, string $index = '', ?bool $escape = null)

$bind = $this->setBind($k2, $v2, $escape);

$clean[$this->db->protectIdentifiers($k2, false, $escape)] = ":{$bind}:";
$clean[$this->db->protectIdentifiers($k2, false)] = ":{$bind}:";
}

if ($indexSet === false) {
Expand Down
4 changes: 2 additions & 2 deletions system/Model.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ protected function doInsert(array $data)
* This methods works only with dbCalls
*
* @param array|null $set An associative array of insert values
* @param bool|null $escape Whether to escape values and identifiers
* @param bool|null $escape Whether to escape values
* @param int $batchSize The size of the batch to run
* @param bool $testing True means only number of records is returned, false will execute the query
*
Expand Down Expand Up @@ -559,7 +559,7 @@ public function builder(?string $table = null)
*
* @param mixed $key Field name, or an array of field/value pairs
* @param mixed $value Field value, if $key is a single field
* @param bool|null $escape Whether to escape values and identifiers
* @param bool|null $escape Whether to escape values
*
* @return $this
*/
Expand Down
30 changes: 30 additions & 0 deletions tests/system/Database/Builder/InsertTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,36 @@ public function testInsertBatch()
$this->assertSame($expected, str_replace("\n", ' ', $query->getQuery()));
}

public function testInsertBatchWithoutEscape()
{
$builder = $this->db->table('jobs');

$insertData = [
[
'id' => 2,
'name' => '1 + 1',
'description' => '1 + 2',
],
[
'id' => 3,
'name' => '2 + 1',
'description' => '2 + 2',
],
];

$this->db->shouldReturn('execute', 1)->shouldReturn('affectedRows', 1);
$builder->insertBatch($insertData, false);

$query = $this->db->getLastQuery();
$this->assertInstanceOf(Query::class, $query);

$raw = 'INSERT INTO "jobs" ("description", "id", "name") VALUES (:description:,:id:,:name:), (:description.1:,:id.1:,:name.1:)';
$this->assertSame($raw, str_replace("\n", ' ', $query->getOriginalQuery()));

$expected = 'INSERT INTO "jobs" ("description", "id", "name") VALUES (1 + 2,2,1 + 1), (2 + 2,3,2 + 1)';
$this->assertSame($expected, str_replace("\n", ' ', $query->getQuery()));
}

/**
* @see https://github.com/codeigniter4/CodeIgniter4/issues/4345
*/
Expand Down
57 changes: 55 additions & 2 deletions tests/system/Database/Builder/UpdateTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,59 @@ public function testUpdateBatch()
$this->assertSame($expected, $query->getQuery());
}

public function testSetUpdateBatchWithoutEscape()
{
$builder = new BaseBuilder('jobs', $this->db);
$escape = false;

$builder->setUpdateBatch([
[
'id' => 2,
'name' => 'SUBSTRING(name, 1)',
'description' => 'SUBSTRING(description, 3)',
],
[
'id' => 3,
'name' => 'SUBSTRING(name, 2)',
'description' => 'SUBSTRING(description, 4)',
],
], 'id', $escape);

$this->db->shouldReturn('execute', 1)->shouldReturn('affectedRows', 1);
$builder->updateBatch(null, 'id');

$query = $this->db->getLastQuery();
$this->assertInstanceOf(MockQuery::class, $query);

$space = ' ';

$expected = <<<EOF
UPDATE "jobs" SET "name" = CASE{$space}
WHEN "id" = :id: THEN :name:
WHEN "id" = :id.1: THEN :name.1:
ELSE "name" END, "description" = CASE{$space}
WHEN "id" = :id: THEN :description:
WHEN "id" = :id.1: THEN :description.1:
ELSE "description" END
WHERE "id" IN(:id:,:id.1:)
EOF;

$this->assertSame($expected, $query->getOriginalQuery());

$expected = <<<EOF
UPDATE "jobs" SET "name" = CASE{$space}
WHEN "id" = 2 THEN SUBSTRING(name, 1)
WHEN "id" = 3 THEN SUBSTRING(name, 2)
ELSE "name" END, "description" = CASE{$space}
WHEN "id" = 2 THEN SUBSTRING(description, 3)
WHEN "id" = 3 THEN SUBSTRING(description, 4)
ELSE "description" END
WHERE "id" IN(2,3)
EOF;

$this->assertSame($expected, $query->getQuery());
}

public function testUpdateBatchThrowsExceptionWithNoData()
{
$builder = new BaseBuilder('jobs', $this->db);
Expand Down Expand Up @@ -350,7 +403,7 @@ public function testSetWithoutEscape()
->where('id', 2)
->update(null, null, null);

$expectedSQL = 'UPDATE "mytable" SET field = field+1 WHERE "id" = 2';
$expectedSQL = 'UPDATE "mytable" SET "field" = field+1 WHERE "id" = 2';
$expectedBinds = [
'id' => [
2,
Expand All @@ -372,7 +425,7 @@ public function testSetWithAndWithoutEscape()
->where('id', 2)
->update(null, null, null);

$expectedSQL = 'UPDATE "mytable" SET "foo" = \'bar\', field = field+1 WHERE "id" = 2';
$expectedSQL = 'UPDATE "mytable" SET "foo" = \'bar\', "field" = field+1 WHERE "id" = 2';
$expectedBinds = [
'foo' => [
'bar',
Expand Down
2 changes: 2 additions & 0 deletions user_guide_src/source/changelogs/v4.1.5.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ Enhancements:

Changes:

- Always escape identifiers in the ``set``, ``setUpdateBatch``, and ``insertBatch`` functions in ``BaseBuilder``.

Deprecations:

- Deprecated ``CodeIgniter\\Cache\\Handlers\\BaseHandler::RESERVED_CHARACTERS`` in favor of the new config property
Expand Down
10 changes: 5 additions & 5 deletions user_guide_src/source/database/query_builder.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1701,7 +1701,7 @@ Class Reference
:param mixed $key: Field name, or an array of field/value pairs
:param mixed $value: Field value, if $key is a single field
:param bool $escape: Whether to escape values and identifiers
:param bool $escape: Whether to escape values
:returns: ``BaseBuilder`` instance (method chaining)
:rtype: ``BaseBuilder``

Expand All @@ -1710,7 +1710,7 @@ Class Reference
.. php:method:: insert([$set = null[, $escape = null]])
:param array $set: An associative array of field/value pairs
:param bool $escape: Whether to escape values and identifiers
:param bool $escape: Whether to escape values
:returns: ``true`` on success, ``false`` on failure
:rtype: bool

Expand All @@ -1719,7 +1719,7 @@ Class Reference
.. php:method:: insertBatch([$set = null[, $escape = null[, $batch_size = 100]]])
:param array $set: Data to insert
:param bool $escape: Whether to escape values and identifiers
:param bool $escape: Whether to escape values
:param int $batch_size: Count of rows to insert at once
:returns: Number of rows inserted or ``false`` on failure
:rtype: int|false
Expand All @@ -1734,7 +1734,7 @@ Class Reference
:param mixed $key: Field name or an array of field/value pairs
:param string $value: Field value, if $key is a single field
:param bool $escape: Whether to escape values and identifiers
:param bool $escape: Whether to escape values
:returns: ``BaseBuilder`` instance (method chaining)
:rtype: ``BaseBuilder``

Expand Down Expand Up @@ -1768,7 +1768,7 @@ Class Reference
:param mixed $key: Field name or an array of field/value pairs
:param string $value: Field value, if $key is a single field
:param bool $escape: Whether to escape values and identifiers
:param bool $escape: Whether to escape values
:returns: ``BaseBuilder`` instance (method chaining)
:rtype: ``BaseBuilder``

Expand Down

0 comments on commit add6083

Please sign in to comment.