Skip to content

Commit

Permalink
FIX Allow selecting multiple tables
Browse files Browse the repository at this point in the history
  • Loading branch information
GuySartorelli committed Sep 21, 2023
1 parent b8665a7 commit f40885a
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 11 deletions.
18 changes: 17 additions & 1 deletion src/ORM/Connect/DBQueryBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,25 @@ public function buildUpdateFragment(SQLUpdate $query, array &$parameters)
public function buildFromFragment(SQLConditionalExpression $query, array &$parameters)
{
$from = $query->getJoins($joinParameters);
$tables = [];
$joins = [];

// E.g. a naive "Select 1" statemnt is valid SQL
if (empty($from)) {
return '';
}

foreach ($from as $joinOrTable) {
if (preg_match(SQLConditionalExpression::JOIN_REGEX, $joinOrTable)) {
$joins[] = $joinOrTable;
} else {
$tables[] = $joinOrTable;
}
}

$parameters = array_merge($parameters, $joinParameters);
$nl = $this->getSeparator();
return "{$nl}FROM " . implode(' ', $from);
return "{$nl}FROM " . implode(', ', $tables) . ' ' . implode(' ', $joins);
}

/**
Expand Down
59 changes: 49 additions & 10 deletions src/ORM/Queries/SQLConditionalExpression.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@

namespace SilverStripe\ORM\Queries;

use LogicException;

/**
* Represents a SQL query for an expression which interacts with existing rows
* (SELECT / DELETE / UPDATE) with a WHERE clause
*/
abstract class SQLConditionalExpression extends SQLExpression
{
public const JOIN_REGEX = '/JOIN +.*? +(AS|ON|USING\(?) +/i';

/**
* An array of WHERE clauses.
Expand Down Expand Up @@ -149,6 +152,34 @@ public function addLeftJoin($table, $onPredicate, $tableAlias = '', $order = 20,
return $this;
}

/**
* Add a RIGHT JOIN criteria to the tables list.
*
* @param string $table Unquoted table name
* @param string $onPredicate The "ON" SQL fragment in a "RIGHT JOIN ... AS ... ON ..." statement, Needs to be valid
* (quoted) SQL.
* @param string $tableAlias Optional alias which makes it easier to identify and replace joins later on
* @param int $order A numerical index to control the order that joins are added to the query; lower order values
* will cause the query to appear first. The default is 20, and joins created automatically by the
* ORM have a value of 10.
* @param array $parameters Any additional parameters if the join is a parameterized subquery
* @return $this Self reference
*/
public function addRightJoin($table, $onPredicate, $tableAlias = '', $order = 20, $parameters = [])
{
if (!$tableAlias) {
$tableAlias = $table;
}
$this->from[$tableAlias] = [
'type' => 'RIGHT',
'table' => $table,
'filter' => [$onPredicate],
'order' => $order,
'parameters' => $parameters
];
return $this;
}

/**
* Add an INNER JOIN criteria
*
Expand Down Expand Up @@ -226,7 +257,7 @@ public function queriedTables()
foreach ($this->from as $key => $tableClause) {
if (is_array($tableClause)) {
$table = '"' . $tableClause['table'] . '"';
} elseif (is_string($tableClause) && preg_match('/JOIN +("[^"]+") +(AS|ON) +/i', $tableClause ?? '', $matches)) {
} elseif (is_string($tableClause) && preg_match(self::JOIN_REGEX, $tableClause ?? '', $matches)) {
$table = $matches[1];
} else {
$table = $tableClause;
Expand Down Expand Up @@ -325,11 +356,16 @@ protected function getOrderedJoins($from)
return $from;
}

// shift the first FROM table out from so we only deal with the JOINs
reset($from);
$baseFromAlias = key($from ?? []);
$baseFrom = array_shift($from);
// Remove the regular FROM tables out so we only deal with the JOINs
$regularTables = [];
foreach ($from as $alias => $tableClause) {
if (is_string($tableClause) && !preg_match(self::JOIN_REGEX, $tableClause)) {
$regularTables[$alias] = $tableClause;
unset($from[$alias]);
}
}

// Sort the joins
$this->mergesort($from, function ($firstJoin, $secondJoin) {
if (!is_array($firstJoin)
|| !is_array($secondJoin)
Expand All @@ -341,11 +377,14 @@ protected function getOrderedJoins($from)
}
});

// Put the first FROM table back into the results
if (!empty($baseFromAlias) && !is_numeric($baseFromAlias)) {
$from = array_merge([$baseFromAlias => $baseFrom], $from);
} else {
array_unshift($from, $baseFrom);
// Put the regular FROM tables back into the results
$regularTables = array_reverse($regularTables, true);
foreach ($regularTables as $alias => $tableName) {
if (!empty($alias) && !is_numeric($alias)) {
$from = array_merge([$alias => $tableName], $from);
} else {
array_unshift($from, $tableName);
}
}

return $from;
Expand Down
16 changes: 16 additions & 0 deletions tests/php/ORM/DataQueryTest/DateAndPriceObject.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php

namespace SilverStripe\ORM\Tests\DataQueryTest;

use SilverStripe\Dev\TestOnly;
use SilverStripe\ORM\DataObject;

class DateAndPriceObject extends DataObject implements TestOnly
{
private static $table_name = 'DataQueryTest_DateAndPriceObject';

private static $db = [
'Date' => 'Date',
'Price' => 'Int',
];
}
16 changes: 16 additions & 0 deletions tests/php/ORM/SQLSelectTest/CteDatesObject.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php

namespace SilverStripe\ORM\Tests\SQLSelectTest;

use SilverStripe\Dev\TestOnly;
use SilverStripe\ORM\DataObject;

class CteDatesObject extends DataObject implements TestOnly
{
private static $table_name = 'SQLSelectTestCteDates';

private static $db = [
'Date' => 'Date',
'Price' => 'Int',
];
}
23 changes: 23 additions & 0 deletions tests/php/ORM/SQLSelectTest/CteRecursiveObject.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

namespace SilverStripe\ORM\Tests\SQLSelectTest;

use SilverStripe\Dev\TestOnly;
use SilverStripe\ORM\DataObject;

class CteRecursiveObject extends DataObject implements TestOnly
{
private static $table_name = 'SQLSelectTestCteRecursive';

private static $db = [
'Title' => 'Varchar',
];

private static $has_one = [
'Parent' => self::class,
];

private static $has_many = [
'Children' => self::class . '.Parent',
];
}

0 comments on commit f40885a

Please sign in to comment.