From 6297953a07851a4cf51711612dfa8a1dbd691c32 Mon Sep 17 00:00:00 2001 From: memleakd <121398829+memleakd@users.noreply.github.com> Date: Mon, 25 May 2026 11:48:21 +0200 Subject: [PATCH] refactor(database): centralize join condition compilation - Move shared JOIN type and condition compilation into BaseBuilder - Let SQLSRV customize only JOIN table-name compilation - Add SQLSRV regression coverage for multi-condition and RawSql joins Signed-off-by: memleakd <121398829+memleakd@users.noreply.github.com> --- system/Database/BaseBuilder.php | 128 +++++++++++++-------- system/Database/SQLSRV/Builder.php | 76 +----------- tests/system/Database/Builder/JoinTest.php | 26 +++++ 3 files changed, 106 insertions(+), 124 deletions(-) diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php index 90af1b4ef409..0427b14ff721 100644 --- a/system/Database/BaseBuilder.php +++ b/system/Database/BaseBuilder.php @@ -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 @@ -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; } /** diff --git a/system/Database/SQLSRV/Builder.php b/system/Database/SQLSRV/Builder.php index 558f3f9232ee..959299b66341 100644 --- a/system/Database/SQLSRV/Builder.php +++ b/system/Database/SQLSRV/Builder.php @@ -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); } /** diff --git a/tests/system/Database/Builder/JoinTest.php b/tests/system/Database/Builder/JoinTest.php index 04145671e7df..ca275a25efcb 100644 --- a/tests/system/Database/Builder/JoinTest.php +++ b/tests/system/Database/Builder/JoinTest.php @@ -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())); + } }