Skip to content

Commit be70740

Browse files
memleakdmemleakd
authored
refactor(database): centralize join condition compilation (#10237)
1 parent ed854ac commit be70740

3 files changed

Lines changed: 106 additions & 124 deletions

File tree

‎system/Database/BaseBuilder.php‎

Lines changed: 77 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -642,15 +642,7 @@ public function fromSubquery(BaseBuilder $from, string $alias): self
642642
*/
643643
public function join(string $table, $cond, string $type = '', ?bool $escape = null)
644644
{
645-
if ($type !== '') {
646-
$type = strtoupper(trim($type));
647-
648-
if (! in_array($type, $this->joinTypes, true)) {
649-
$type = '';
650-
} else {
651-
$type .= ' ';
652-
}
653-
}
645+
$type = $this->compileJoinType($type);
654646

655647
// Extract any aliases that might exist. We use this information
656648
// 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
660652
$escape = $this->db->protectIdentifiers;
661653
}
662654

663-
// Do we want to escape the table name?
664-
if ($escape === true) {
665-
$table = $this->db->protectIdentifiers($table, true, null, false);
655+
$table = $this->compileJoinTable($table, $escape);
656+
$cond = $this->compileJoinCondition($cond, $escape);
657+
658+
// Assemble the JOIN statement
659+
$this->QBJoin[] = $type . 'JOIN ' . $table . $cond;
660+
661+
return $this;
662+
}
663+
664+
/**
665+
* Compiles the JOIN table name.
666+
*/
667+
protected function compileJoinTable(string $table, bool $escape): string
668+
{
669+
if ($escape) {
670+
return $this->db->protectIdentifiers($table, true, null, false);
666671
}
667672

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

671-
return $this;
676+
private function compileJoinType(string $type): string
677+
{
678+
if ($type === '') {
679+
return '';
672680
}
673681

674-
if (! $this->hasOperator($cond)) {
675-
$cond = ' USING (' . ($escape ? $this->db->escapeIdentifiers($cond) : $cond) . ')';
676-
} elseif ($escape === false) {
677-
$cond = ' ON ' . $cond;
678-
} else {
679-
// Split multiple conditions
680-
// @TODO This does not parse `BETWEEN a AND b` correctly.
681-
if (preg_match_all('/\sAND\s|\sOR\s/i', $cond, $joints, PREG_OFFSET_CAPTURE) >= 1) {
682-
$conditions = [];
683-
$joints = $joints[0];
684-
array_unshift($joints, ['', 0]);
685-
686-
for ($i = count($joints) - 1, $pos = strlen($cond); $i >= 0; $i--) {
687-
$joints[$i][1] += strlen($joints[$i][0]); // offset
688-
$conditions[$i] = substr($cond, $joints[$i][1], $pos - $joints[$i][1]);
689-
$pos = $joints[$i][1] - strlen($joints[$i][0]);
690-
$joints[$i] = $joints[$i][0];
691-
}
692-
ksort($conditions);
693-
} else {
694-
$conditions = [$cond];
695-
$joints = [''];
696-
}
682+
$type = strtoupper(trim($type));
697683

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

700-
foreach ($conditions as $i => $condition) {
701-
$operator = $this->getOperator($condition);
687+
/**
688+
* Compiles the JOIN ON or USING condition.
689+
*/
690+
private function compileJoinCondition(RawSql|string $condition, bool $escape): string
691+
{
692+
if ($condition instanceof RawSql) {
693+
return ' ON ' . $condition;
694+
}
702695

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

707-
continue;
708-
}
700+
if ($escape === false) {
701+
return ' ON ' . $condition;
702+
}
703+
704+
return $this->compileProtectedJoinCondition($condition);
705+
}
709706

710-
$cond .= $joints[$i];
711-
$cond .= preg_match('/(\(*)?([\[\]\w\.\'-]+)' . preg_quote($operator, '/') . '(.*)/i', $condition, $match) ? $match[1] . $this->db->protectIdentifiers($match[2]) . $operator . $this->db->protectIdentifiers($match[3]) : $condition;
707+
private function compileProtectedJoinCondition(string $condition): string
708+
{
709+
// Split multiple conditions
710+
// @TODO This does not parse `BETWEEN a AND b` correctly.
711+
if (preg_match_all('/\sAND\s|\sOR\s/i', $condition, $joints, PREG_OFFSET_CAPTURE) >= 1) {
712+
$conditions = [];
713+
$joints = $joints[0];
714+
array_unshift($joints, ['', 0]);
715+
716+
for ($i = count($joints) - 1, $pos = strlen($condition); $i >= 0; $i--) {
717+
$joints[$i][1] += strlen($joints[$i][0]); // offset
718+
$conditions[$i] = substr($condition, $joints[$i][1], $pos - $joints[$i][1]);
719+
$pos = $joints[$i][1] - strlen($joints[$i][0]);
720+
$joints[$i] = $joints[$i][0];
712721
}
722+
ksort($conditions);
723+
} else {
724+
$conditions = [$condition];
725+
$joints = [''];
713726
}
714727

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

718-
return $this;
730+
foreach ($conditions as $i => $conditionPart) {
731+
$operator = $this->getOperator($conditionPart);
732+
733+
// Workaround for BETWEEN
734+
if ($operator === false) {
735+
$condition .= $joints[$i] . $conditionPart;
736+
737+
continue;
738+
}
739+
740+
$condition .= $joints[$i];
741+
$condition .= preg_match('/(\(*)?([\[\]\w\.\'-]+)' . preg_quote($operator, '/') . '(.*)/i', $conditionPart, $match) ? $match[1] . $this->db->protectIdentifiers($match[2]) . $operator . $this->db->protectIdentifiers($match[3]) : $conditionPart;
742+
}
743+
744+
return $condition;
719745
}
720746

721747
/**

‎system/Database/SQLSRV/Builder.php‎

Lines changed: 3 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -101,83 +101,13 @@ protected function _truncate(string $table): string
101101
return 'TRUNCATE TABLE ' . $this->getFullName($table);
102102
}
103103

104-
/**
105-
* Generates the JOIN portion of the query
106-
*
107-
* @param RawSql|string $cond
108-
*
109-
* @return $this
110-
*/
111-
public function join(string $table, $cond, string $type = '', ?bool $escape = null)
104+
protected function compileJoinTable(string $table, bool $escape): string
112105
{
113-
if ($type !== '') {
114-
$type = strtoupper(trim($type));
115-
116-
if (! in_array($type, $this->joinTypes, true)) {
117-
$type = '';
118-
} else {
119-
$type .= ' ';
120-
}
121-
}
122-
123-
// Extract any aliases that might exist. We use this information
124-
// in the protectIdentifiers to know whether to add a table prefix
125-
$this->trackAliases($table);
126-
127-
if (! is_bool($escape)) {
128-
$escape = $this->db->protectIdentifiers;
129-
}
130-
131-
if (! $this->hasOperator($cond)) {
132-
$cond = ' USING (' . ($escape ? $this->db->escapeIdentifiers($cond) : $cond) . ')';
133-
} elseif ($escape === false) {
134-
$cond = ' ON ' . $cond;
135-
} else {
136-
// Split multiple conditions
137-
if (preg_match_all('/\sAND\s|\sOR\s/i', $cond, $joints, PREG_OFFSET_CAPTURE) >= 1) {
138-
$conditions = [];
139-
$joints = $joints[0];
140-
array_unshift($joints, ['', 0]);
141-
142-
for ($i = count($joints) - 1, $pos = strlen($cond); $i >= 0; $i--) {
143-
$joints[$i][1] += strlen($joints[$i][0]); // offset
144-
$conditions[$i] = substr($cond, $joints[$i][1], $pos - $joints[$i][1]);
145-
$pos = $joints[$i][1] - strlen($joints[$i][0]);
146-
$joints[$i] = $joints[$i][0];
147-
}
148-
149-
ksort($conditions);
150-
} else {
151-
$conditions = [$cond];
152-
$joints = [''];
153-
}
154-
155-
$cond = ' ON ';
156-
157-
foreach ($conditions as $i => $condition) {
158-
$operator = $this->getOperator($condition);
159-
160-
// Workaround for BETWEEN
161-
if ($operator === false) {
162-
$cond .= $joints[$i] . $condition;
163-
164-
continue;
165-
}
166-
167-
$cond .= $joints[$i];
168-
$cond .= preg_match('/(\(*)?([\[\]\w\.\'-]+)' . preg_quote($operator, '/') . '(.*)/i', $condition, $match) ? $match[1] . $this->db->protectIdentifiers($match[2]) . $operator . $this->db->protectIdentifiers($match[3]) : $condition;
169-
}
170-
}
171-
172-
// Do we want to escape the table name?
173-
if ($escape === true) {
106+
if ($escape) {
174107
$table = $this->db->protectIdentifiers($table, true, null, false);
175108
}
176109

177-
// Assemble the JOIN statement
178-
$this->QBJoin[] = $type . 'JOIN ' . $this->getFullName($table) . $cond;
179-
180-
return $this;
110+
return $this->getFullName($table);
181111
}
182112

183113
/**

‎tests/system/Database/Builder/JoinTest.php‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,4 +144,30 @@ public function testJoinWithAlias(): void
144144

145145
$this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect()));
146146
}
147+
148+
public function testSqlsrvJoinMultipleConditions(): void
149+
{
150+
$this->db = new MockConnection(['DBDriver' => 'SQLSRV', 'database' => 'test', 'schema' => 'dbo']);
151+
152+
$builder = new SQLSRVBuilder('jobs', $this->db);
153+
$builder->testMode();
154+
$builder->join('users u', "u.id = jobs.id AND u.status = 'active'", 'LEFT');
155+
156+
$expectedSQL = 'SELECT * FROM "test"."dbo"."jobs" LEFT JOIN "test"."dbo"."users" "u" ON "u"."id" = "jobs"."id" AND "u"."status" = \'active\'';
157+
158+
$this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect()));
159+
}
160+
161+
public function testSqlsrvJoinRawSql(): void
162+
{
163+
$this->db = new MockConnection(['DBDriver' => 'SQLSRV', 'database' => 'test', 'schema' => 'dbo']);
164+
165+
$builder = new SQLSRVBuilder('jobs', $this->db);
166+
$builder->testMode();
167+
$builder->join('users u', new RawSql('u.id = jobs.id AND u.deleted_at IS NULL'), 'LEFT');
168+
169+
$expectedSQL = 'SELECT * FROM "test"."dbo"."jobs" LEFT JOIN "test"."dbo"."users" "u" ON u.id = jobs.id AND u.deleted_at IS NULL';
170+
171+
$this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect()));
172+
}
147173
}

0 commit comments

Comments
 (0)