Skip to content

refactor: centralize Query Builder join compilation #10237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
memleakd wants to merge 1 commit into
base: 4.8
Choose a base branch
from
+106 −124
Open

refactor: centralize Query Builder join compilation #10237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 77 additions & 51 deletions system/Database/BaseBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,15 +642,7 @@ public function fromSubquery(BaseBuilder $from, string $alias): self
*/
public function join(string $table, $cond, string $type = '', ?bool $escape = null)
{
if ($type !== '') {
$type = strtoupper(trim($type));

if (! in_array($type, $this->joinTypes, true)) {
$type = '';
} else {
$type .= ' ';
}
}
$type = $this->compileJoinType($type);

// Extract any aliases that might exist. We use this information
// in the protectIdentifiers to know whether to add a table prefix
Expand All @@ -660,62 +652,96 @@ public function join(string $table, $cond, string $type = '', ?bool $escape = nu
$escape = $this->db->protectIdentifiers;
}

// Do we want to escape the table name?
if ($escape === true) {
$table = $this->db->protectIdentifiers($table, true, null, false);
$table = $this->compileJoinTable($table, $escape);
$cond = $this->compileJoinCondition($cond, $escape);

// Assemble the JOIN statement
$this->QBJoin[] = $type . 'JOIN ' . $table . $cond;

return $this;
}

/**
* Compiles the JOIN table name.
*/
protected function compileJoinTable(string $table, bool $escape): string
{
if ($escape) {
return $this->db->protectIdentifiers($table, true, null, false);
}

if ($cond instanceof RawSql) {
$this->QBJoin[] = $type . 'JOIN ' . $table . ' ON ' . $cond;
return $table;
}

return $this;
private function compileJoinType(string $type): string
{
if ($type === '') {
return '';
}

if (! $this->hasOperator($cond)) {
$cond = ' USING (' . ($escape ? $this->db->escapeIdentifiers($cond) : $cond) . ')';
} elseif ($escape === false) {
$cond = ' ON ' . $cond;
} else {
// Split multiple conditions
// @TODO This does not parse `BETWEEN a AND b` correctly.
if (preg_match_all('/\sAND\s|\sOR\s/i', $cond, $joints, PREG_OFFSET_CAPTURE) >= 1) {
$conditions = [];
$joints = $joints[0];
array_unshift($joints, ['', 0]);

for ($i = count($joints) - 1, $pos = strlen($cond); $i >= 0; $i--) {
$joints[$i][1] += strlen($joints[$i][0]); // offset
$conditions[$i] = substr($cond, $joints[$i][1], $pos - $joints[$i][1]);
$pos = $joints[$i][1] - strlen($joints[$i][0]);
$joints[$i] = $joints[$i][0];
}
ksort($conditions);
} else {
$conditions = [$cond];
$joints = [''];
}
$type = strtoupper(trim($type));

$cond = ' ON ';
return in_array($type, $this->joinTypes, true) ? $type . ' ' : '';
}

foreach ($conditions as $i => $condition) {
$operator = $this->getOperator($condition);
/**
* Compiles the JOIN ON or USING condition.
*/
private function compileJoinCondition(RawSql|string $condition, bool $escape): string
{
if ($condition instanceof RawSql) {
return ' ON ' . $condition;
}

// Workaround for BETWEEN
if ($operator === false) {
$cond .= $joints[$i] . $condition;
if (! $this->hasOperator($condition)) {
return ' USING (' . ($escape ? $this->db->escapeIdentifiers($condition) : $condition) . ')';
}

continue;
}
if ($escape === false) {
return ' ON ' . $condition;
}

return $this->compileProtectedJoinCondition($condition);
}

$cond .= $joints[$i];
$cond .= preg_match('/(\(*)?([\[\]\w\.\'-]+)' . preg_quote($operator, '/') . '(.*)/i', $condition, $match) ? $match[1] . $this->db->protectIdentifiers($match[2]) . $operator . $this->db->protectIdentifiers($match[3]) : $condition;
private function compileProtectedJoinCondition(string $condition): string
{
// Split multiple conditions
// @TODO This does not parse `BETWEEN a AND b` correctly.
if (preg_match_all('/\sAND\s|\sOR\s/i', $condition, $joints, PREG_OFFSET_CAPTURE) >= 1) {
$conditions = [];
$joints = $joints[0];
array_unshift($joints, ['', 0]);

for ($i = count($joints) - 1, $pos = strlen($condition); $i >= 0; $i--) {
$joints[$i][1] += strlen($joints[$i][0]); // offset
$conditions[$i] = substr($condition, $joints[$i][1], $pos - $joints[$i][1]);
$pos = $joints[$i][1] - strlen($joints[$i][0]);
$joints[$i] = $joints[$i][0];
}
ksort($conditions);
} else {
$conditions = [$condition];
$joints = [''];
}

// Assemble the JOIN statement
$this->QBJoin[] = $type . 'JOIN ' . $table . $cond;
$condition = ' ON ';

return $this;
foreach ($conditions as $i => $conditionPart) {
$operator = $this->getOperator($conditionPart);

// Workaround for BETWEEN
if ($operator === false) {
$condition .= $joints[$i] . $conditionPart;

continue;
}

$condition .= $joints[$i];
$condition .= preg_match('/(\(*)?([\[\]\w\.\'-]+)' . preg_quote($operator, '/') . '(.*)/i', $conditionPart, $match) ? $match[1] . $this->db->protectIdentifiers($match[2]) . $operator . $this->db->protectIdentifiers($match[3]) : $conditionPart;
}

return $condition;
}

/**
Expand Down
76 changes: 3 additions & 73 deletions system/Database/SQLSRV/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,83 +101,13 @@ protected function _truncate(string $table): string
return 'TRUNCATE TABLE ' . $this->getFullName($table);
}

/**
* Generates the JOIN portion of the query
*
* @param RawSql|string $cond
*
* @return $this
*/
public function join(string $table, $cond, string $type = '', ?bool $escape = null)
protected function compileJoinTable(string $table, bool $escape): string
{
if ($type !== '') {
$type = strtoupper(trim($type));

if (! in_array($type, $this->joinTypes, true)) {
$type = '';
} else {
$type .= ' ';
}
}

// Extract any aliases that might exist. We use this information
// in the protectIdentifiers to know whether to add a table prefix
$this->trackAliases($table);

if (! is_bool($escape)) {
$escape = $this->db->protectIdentifiers;
}

if (! $this->hasOperator($cond)) {
$cond = ' USING (' . ($escape ? $this->db->escapeIdentifiers($cond) : $cond) . ')';
} elseif ($escape === false) {
$cond = ' ON ' . $cond;
} else {
// Split multiple conditions
if (preg_match_all('/\sAND\s|\sOR\s/i', $cond, $joints, PREG_OFFSET_CAPTURE) >= 1) {
$conditions = [];
$joints = $joints[0];
array_unshift($joints, ['', 0]);

for ($i = count($joints) - 1, $pos = strlen($cond); $i >= 0; $i--) {
$joints[$i][1] += strlen($joints[$i][0]); // offset
$conditions[$i] = substr($cond, $joints[$i][1], $pos - $joints[$i][1]);
$pos = $joints[$i][1] - strlen($joints[$i][0]);
$joints[$i] = $joints[$i][0];
}

ksort($conditions);
} else {
$conditions = [$cond];
$joints = [''];
}

$cond = ' ON ';

foreach ($conditions as $i => $condition) {
$operator = $this->getOperator($condition);

// Workaround for BETWEEN
if ($operator === false) {
$cond .= $joints[$i] . $condition;

continue;
}

$cond .= $joints[$i];
$cond .= preg_match('/(\(*)?([\[\]\w\.\'-]+)' . preg_quote($operator, '/') . '(.*)/i', $condition, $match) ? $match[1] . $this->db->protectIdentifiers($match[2]) . $operator . $this->db->protectIdentifiers($match[3]) : $condition;
}
}

// Do we want to escape the table name?
if ($escape === true) {
if ($escape) {
$table = $this->db->protectIdentifiers($table, true, null, false);
}

// Assemble the JOIN statement
$this->QBJoin[] = $type . 'JOIN ' . $this->getFullName($table) . $cond;

return $this;
return $this->getFullName($table);
}

/**
Expand Down
26 changes: 26 additions & 0 deletions tests/system/Database/Builder/JoinTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,4 +144,30 @@ public function testJoinWithAlias(): void

$this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect()));
}

public function testSqlsrvJoinMultipleConditions(): void
{
$this->db = new MockConnection(['DBDriver' => 'SQLSRV', 'database' => 'test', 'schema' => 'dbo']);

$builder = new SQLSRVBuilder('jobs', $this->db);
$builder->testMode();
$builder->join('users u', "u.id = jobs.id AND u.status = 'active'", 'LEFT');

$expectedSQL = 'SELECT * FROM "test"."dbo"."jobs" LEFT JOIN "test"."dbo"."users" "u" ON "u"."id" = "jobs"."id" AND "u"."status" = \'active\'';

$this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect()));
}

public function testSqlsrvJoinRawSql(): void
{
$this->db = new MockConnection(['DBDriver' => 'SQLSRV', 'database' => 'test', 'schema' => 'dbo']);

$builder = new SQLSRVBuilder('jobs', $this->db);
$builder->testMode();
$builder->join('users u', new RawSql('u.id = jobs.id AND u.deleted_at IS NULL'), 'LEFT');

$expectedSQL = 'SELECT * FROM "test"."dbo"."jobs" LEFT JOIN "test"."dbo"."users" "u" ON u.id = jobs.id AND u.deleted_at IS NULL';

$this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect()));
}
}
Loading