From ea1f32e86a5a3e3deb2189481a547b67189be5d3 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 16 Oct 2026 23:55:23 +1300 Subject: [PATCH 1/2] chore(deps): bump utopia-php/query to 0.7.2 0.7.2 reads a join given no alias under the collection it joins and exposes isAliasDefaulted(), which the join validator needs to explain why a table ID cannot serve as an alias. It also refuses a NaN or infinite distance when compiling a distance filter, so the Postgres builder test now expects that refusal instead of a compiled exact check for those three inputs. Co-Authored-By: Claude Opus 5.5 --- composer.json | 2 +- composer.lock | 14 ++++++------ tests/unit/Builder/PostgresDistanceTest.php | 25 ++++++++++++++++++--- 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/composer.json b/composer.json index bedf0a71a..e35127263 100755 --- a/composer.json +++ b/composer.json @@ -54,7 +54,7 @@ "utopia-php/cache": "^4.0 || ^5.0", "utopia-php/pools": "^2.0", "utopia-php/mongo": "^1.5.4", - "utopia-php/query": "^0.7.1", + "utopia-php/query": "^0.7.2", "utopia-php/async": "^0.2" }, "require-dev": { diff --git a/composer.lock b/composer.lock index 2ecf01fdb..6893e9e93 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "45b5cf01aef6a3cd2ad9b296d965edfb", + "content-hash": "166a6243504fe68982cde7684c933e3d", "packages": [ { "name": "brick/math", @@ -2278,16 +2278,16 @@ }, { "name": "utopia-php/query", - "version": "0.7.1", + "version": "0.7.2", "source": { "type": "git", "url": "https://github.com/utopia-php/query.git", - "reference": "291ec903812f6b752991c02ee4ea50e835c23b9b" + "reference": "945ef1c9f1c863b0e6e8bbac4c81c1b667dbfc9c" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/utopia-php/query/zipball/291ec903812f6b752991c02ee4ea50e835c23b9b", - "reference": "291ec903812f6b752991c02ee4ea50e835c23b9b", + "url": "https://api.github.com/repos/utopia-php/query/zipball/945ef1c9f1c863b0e6e8bbac4c81c1b667dbfc9c", + "reference": "945ef1c9f1c863b0e6e8bbac4c81c1b667dbfc9c", "shasum": "" }, "require": { @@ -2316,9 +2316,9 @@ ], "support": { "issues": "https://github.com/utopia-php/query/issues", - "source": "https://github.com/utopia-php/query/tree/0.7.1" + "source": "https://github.com/utopia-php/query/tree/0.7.2" }, - "time": "2026-10-15T06:11:20+00:00" + "time": "2026-10-16T10:52:31+00:00" }, { "name": "utopia-php/telemetry", diff --git a/tests/unit/Builder/PostgresDistanceTest.php b/tests/unit/Builder/PostgresDistanceTest.php index bdcb4d4fe..6c7aedf3b 100644 --- a/tests/unit/Builder/PostgresDistanceTest.php +++ b/tests/unit/Builder/PostgresDistanceTest.php @@ -6,6 +6,7 @@ use PHPUnit\Framework\TestCase; use Utopia\Database\Builder\Postgres; use Utopia\Database\Query; +use Utopia\Query\Exception\ValidationException; use Utopia\Query\Schema\ColumnType; final class PostgresDistanceTest extends TestCase @@ -46,9 +47,6 @@ public static function exactOnly(): array 'a polygon value' => [self::typed(Query::distanceLessThan('location', [[[0, 0], [0, 1], [1, 1], [0, 0]]], 1000, true), ColumnType::Point)], 'a box reaching a pole' => [self::typed(Query::distanceLessThan('location', [10, 89.99], 5000, true), ColumnType::Point)], 'a box reaching the antimeridian' => [self::typed(Query::distanceLessThan('location', [179.99, 0], 5000, true), ColumnType::Point)], - 'a distance that is not a number' => [self::typed(Query::distanceLessThan('location', [10, 20], NAN, true), ColumnType::Point)], - 'an infinite distance' => [self::typed(Query::distanceLessThan('location', [10, 20], INF, true), ColumnType::Point)], - 'a negatively infinite distance' => [self::typed(Query::distanceLessThan('location', [10, 20], -INF, true), ColumnType::Point)], ]; } @@ -61,6 +59,27 @@ public function testDistanceLessThanInMetersKeepsTheExactCheckOnlyWhenNoBoxHolds $this->assertCount(2, $condition->bindings); } + /** + * @return array + */ + public static function nonFiniteDistances(): array + { + return [ + 'a distance that is not a number' => [self::typed(Query::distanceLessThan('location', [10, 20], NAN, true), ColumnType::Point)], + 'an infinite distance' => [self::typed(Query::distanceLessThan('location', [10, 20], INF, true), ColumnType::Point)], + 'a negatively infinite distance' => [self::typed(Query::distanceLessThan('location', [10, 20], -INF, true), ColumnType::Point)], + ]; + } + + #[DataProvider('nonFiniteDistances')] + public function testDistanceLessThanInMetersRefusesADistanceThatIsNotFinite(Query $query): void + { + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Invalid distance query: the distance must be a finite number'); + + (new Postgres())->compileFilters([$query]); + } + public function testDistanceGreaterThanIsUnchanged(): void { $condition = (new Postgres())->compileFilters([self::typed(Query::distanceGreaterThan('location', [10, 20], 5), ColumnType::Point)]); From 2e6e085375d9081dac54439d4a77a75c3b738b0d Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 16 Oct 2026 23:55:23 +1300 Subject: [PATCH 2/2] feat(join): read a join without an alias under its table ID A join given no alias is now read under the ID of the table it joins, so rows come back keyed ".". Query's constructor applies the same default as the query library's factories and parser, so joins built directly read the same way. Self-joins need no alias: the main collection is always read under the reserved alias, never under its own ID. When the table ID can't be an alias, the query is refused with the reason, the ID, and a request for an alias. That covers an ID that isn't an identifier (Appwrite IDs may hold '-' and '.', and '.' would clash with alias.column), the reserved main alias, a relationship key, and an ID another join already uses (joining a table twice). An explicit alias keeps today's messages. The defaulted state comes from Query::isAliasDefaulted(), which is set when the query is built and kept through clone + setAttribute(). Query hooks rewrite the joined table (Appwrite swaps its public ID for the internal collection) before validation runs, so the alias and the error wording both stay on the ID the caller gave, never the internal name. An e2e test with a rewriting hook pins this. Co-Authored-By: Claude Opus 5.5 --- src/Database/Adapter/SQL.php | 4 +- src/Database/Query.php | 6 + src/Database/Validator/Query/Join.php | 35 +- tests/e2e/Adapter/Base.php | 2 + tests/e2e/Adapter/Scopes/JoinAliasTests.php | 414 +++++++++++++++++++ tests/e2e/Adapter/Support/JoinTables.php | 34 ++ tests/unit/JoinAliasTest.php | 44 +- tests/unit/QuerySerializationTest.php | 55 ++- tests/unit/Validator/Query/JoinAliasTest.php | 92 ++++- 9 files changed, 669 insertions(+), 17 deletions(-) create mode 100644 tests/e2e/Adapter/Scopes/JoinAliasTests.php create mode 100644 tests/e2e/Adapter/Support/JoinTables.php diff --git a/src/Database/Adapter/SQL.php b/src/Database/Adapter/SQL.php index b12a0ba53..7fedaab1c 100644 --- a/src/Database/Adapter/SQL.php +++ b/src/Database/Adapter/SQL.php @@ -4507,14 +4507,14 @@ private function assertJoinAliases(array $queries): void } $alias = $query->getAlias(); - $invalid = JoinValidator::describeInvalidAlias($alias); + $invalid = JoinValidator::describeInvalidAlias($alias, $query->isAliasDefaulted()); if ($invalid !== null) { throw new QueryException($invalid); } $key = \strtolower($alias); if (isset($declared[$key])) { - throw new QueryException("Join alias \"{$alias}\" is declared more than once"); + throw new QueryException(JoinValidator::describeDuplicateAlias($alias, $query->isAliasDefaulted())); } $declared[$key] = true; } diff --git a/src/Database/Query.php b/src/Database/Query.php index b3e6b3dac..31fbe763d 100644 --- a/src/Database/Query.php +++ b/src/Database/Query.php @@ -46,6 +46,8 @@ class Query extends BaseQuery public const int SHAPE_MAX_NODES = 10000; /** + * A join given no alias is read under the collection it joins. + * * @param array $values */ public function __construct(Method|string $method, string $attribute = '', array $values = [], string $alias = '') @@ -56,6 +58,10 @@ public function __construct(Method|string $method, string $attribute = '', array $attribute = Document::SEQUENCE; } + if ($alias === '' && $methodEnum->isJoin()) { + $alias = $attribute; + } + parent::__construct($methodEnum, $attribute, $values, $alias); } diff --git a/src/Database/Validator/Query/Join.php b/src/Database/Validator/Query/Join.php index e97197458..681d3f49e 100644 --- a/src/Database/Validator/Query/Join.php +++ b/src/Database/Validator/Query/Join.php @@ -122,24 +122,46 @@ public function resetJoinAliases(): void /** * Why a join alias cannot be used, or null when it can. The reserved alias is matched without * case: SQLite, and MySQL on case-insensitive file systems, treat `x` and `X` as one alias. + * A defaulted alias is the table ID the join names, so the reason says that ID cannot serve as + * the alias and asks for one. */ - public static function describeInvalidAlias(string $alias): ?string + public static function describeInvalidAlias(string $alias, bool $defaulted = false): ?string { if ($alias === '') { return 'Join alias is required'; } if (\preg_match(self::ALIAS_PATTERN, $alias) !== 1) { - return 'Join alias must start with a letter or an underscore and contain only letters, digits and underscores'; + return $defaulted + ? self::describeUnusableTable($alias, 'an alias must start with a letter or an underscore and contain only letters, digits and underscores') + : 'Join alias must start with a letter or an underscore and contain only letters, digits and underscores'; } if (\strcasecmp($alias, Query::DEFAULT_ALIAS) === 0) { - return "Join alias \"{$alias}\" is reserved for the main collection"; + return $defaulted + ? self::describeUnusableTable($alias, 'it is reserved for the main collection') + : "Join alias \"{$alias}\" is reserved for the main collection"; } return null; } + /** + * Why a join alias another join of the same query set already declared cannot be used. The + * adapters check it, as they read `x` and `X` as one alias. + */ + public static function describeDuplicateAlias(string $alias, bool $defaulted = false): string + { + return $defaulted + ? self::describeUnusableTable($alias, 'another join of the query already uses it as its alias') + : "Join alias \"{$alias}\" is declared more than once"; + } + + private static function describeUnusableTable(string $table, string $reason): string + { + return "Table ID \"{$table}\" cannot be used as the join alias: {$reason}. Pass an alias for the join"; + } + /** * Get the method type this validator handles. * @@ -187,7 +209,8 @@ protected function isValidQuery(Query $query): bool } $alias = $query->getAlias(); - $invalidAlias = self::describeInvalidAlias($alias); + $defaulted = $query->isAliasDefaulted(); + $invalidAlias = self::describeInvalidAlias($alias, $defaulted); if ($invalidAlias !== null) { $this->message = $invalidAlias; @@ -195,7 +218,9 @@ protected function isValidQuery(Query $query): bool } if (isset($this->relationships[$alias])) { - $this->message = "Join alias \"{$alias}\" is the key of the relationship attribute \"{$alias}\": give the join another alias"; + $this->message = $defaulted + ? self::describeUnusableTable($alias, "it is the key of the relationship attribute \"{$alias}\"") + : "Join alias \"{$alias}\" is the key of the relationship attribute \"{$alias}\": give the join another alias"; return false; } diff --git a/tests/e2e/Adapter/Base.php b/tests/e2e/Adapter/Base.php index 22669f79b..06a0e15fa 100644 --- a/tests/e2e/Adapter/Base.php +++ b/tests/e2e/Adapter/Base.php @@ -15,6 +15,7 @@ use Tests\E2E\Adapter\Scopes\GeneralTests; use Tests\E2E\Adapter\Scopes\IndexTests; use Tests\E2E\Adapter\Scopes\IntegerRoundingTests; +use Tests\E2E\Adapter\Scopes\JoinAliasTests; use Tests\E2E\Adapter\Scopes\JoinComboTests; use Tests\E2E\Adapter\Scopes\JoinReadRuleTests; use Tests\E2E\Adapter\Scopes\JoinRelationshipTests; @@ -58,6 +59,7 @@ abstract class Base extends TestCase use GeneralTests; use IndexTests; use IntegerRoundingTests; + use JoinAliasTests; use JoinComboTests; use JoinReadRuleTests; use JoinRelationshipTests; diff --git a/tests/e2e/Adapter/Scopes/JoinAliasTests.php b/tests/e2e/Adapter/Scopes/JoinAliasTests.php new file mode 100644 index 000000000..a60f62ad5 --- /dev/null +++ b/tests/e2e/Adapter/Scopes/JoinAliasTests.php @@ -0,0 +1,414 @@ +getAdapter()->supports(Capability::Joins)) { + $this->expectNotToPerformAssertions(); + + return; + } + + $this->createAliasFixtures($database); + + try { + $rows = $database->find(self::ALIAS_ORDERS, [ + Query::join(self::ALIAS_CUSTOMERS, '', [Query::on('customerId', '$id')]), + Query::select(['total', self::ALIAS_CUSTOMERS.'.name']), + Query::orderAsc('total'), + ]); + + $this->assertSame( + [[10, 'Ann'], [20, 'Bob']], + \array_map(static fn (Document $row): array => [$row->getAttribute('total'), $row->getAttribute(self::ALIAS_CUSTOMERS.'.name')], $rows), + ); + $this->assertArrayHasKey(self::ALIAS_CUSTOMERS.'.name', $rows[0]->getArrayCopy()); + + $this->assertSame(2, $database->count(self::ALIAS_ORDERS, [ + Query::join(self::ALIAS_CUSTOMERS, '', [Query::on('customerId', '$id')]), + Query::equal(self::ALIAS_CUSTOMERS.'.name', ['Ann', 'Bob']), + ])); + + $parsed = Query::parse(Query::join(self::ALIAS_CUSTOMERS, '', [Query::on('customerId', '$id')])->toString()); + $rows = $database->find(self::ALIAS_ORDERS, [ + $parsed, + Query::select(['total', self::ALIAS_CUSTOMERS.'.name']), + Query::equal(self::ALIAS_CUSTOMERS.'.name', ['Bob']), + ]); + $this->assertCount(1, $rows); + $this->assertSame('Bob', $rows[0]->getAttribute(self::ALIAS_CUSTOMERS.'.name'), 'a parsed join without an alias reads the same'); + } finally { + $this->cleanupAggCollections($database, $this->aliasFixtureCollections()); + } + } + + public function testLeftJoinWithoutAliasIsReadUnderItsTableId(): void + { + $database = static::getDatabase(); + if (! $database->getAdapter()->supports(Capability::Joins)) { + $this->expectNotToPerformAssertions(); + + return; + } + + $this->createAliasFixtures($database); + + try { + $rows = $database->find(self::ALIAS_ORDERS, [ + Query::leftJoin(self::ALIAS_CUSTOMERS, '', [Query::on('customerId', '$id')]), + Query::select(['total', self::ALIAS_CUSTOMERS.'.name']), + Query::orderAsc('total'), + ]); + + $this->assertSame( + [[10, 'Ann'], [20, 'Bob'], [30, null]], + \array_map(static fn (Document $row): array => [$row->getAttribute('total'), $row->getAttribute(self::ALIAS_CUSTOMERS.'.name')], $rows), + ); + } finally { + $this->cleanupAggCollections($database, $this->aliasFixtureCollections()); + } + } + + public function testCrossJoinWithoutAliasIsReadUnderItsTableId(): void + { + $database = static::getDatabase(); + if (! $database->getAdapter()->supports(Capability::Joins)) { + $this->expectNotToPerformAssertions(); + + return; + } + + $this->createAliasFixtures($database); + + try { + $rows = $database->find(self::ALIAS_ORDERS, [ + Query::crossJoin(self::ALIAS_COLORS, ''), + Query::select(['total', self::ALIAS_COLORS.'.name']), + Query::lessThan('total', 30), + Query::orderAsc('total'), + Query::orderAsc(self::ALIAS_COLORS.'.name'), + ]); + + $this->assertSame( + [[10, 'blue'], [10, 'red'], [20, 'blue'], [20, 'red']], + \array_map(static fn (Document $row): array => [$row->getAttribute('total'), $row->getAttribute(self::ALIAS_COLORS.'.name')], $rows), + ); + } finally { + $this->cleanupAggCollections($database, $this->aliasFixtureCollections()); + } + } + + /** + * The main collection is never referenced by its own ID, so a self-join without an alias reads the + * joined copy under the table ID and the main copy bare. + */ + public function testSelfJoinWithoutAliasIsReadUnderItsTableId(): void + { + $database = static::getDatabase(); + if (! $database->getAdapter()->supports(Capability::Joins)) { + $this->expectNotToPerformAssertions(); + + return; + } + + $this->createAliasFixtures($database); + + try { + $rows = $database->find(self::ALIAS_STAFF, [ + Query::join(self::ALIAS_STAFF, '', [Query::on('managerId', '$id')]), + Query::select(['name', self::ALIAS_STAFF.'.name']), + Query::orderAsc('name'), + ]); + + $this->assertSame( + [['Cal', 'Ann'], ['Dee', 'Cal']], + \array_map(static fn (Document $row): array => [$row->getAttribute('name'), $row->getAttribute(self::ALIAS_STAFF.'.name')], $rows), + ); + + $this->assertRejectedJoin( + 'The left column of a join condition must belong to the main collection or to a join declared before it: '.self::ALIAS_STAFF.'.managerId', + fn (): mixed => $database->find(self::ALIAS_STAFF, [Query::join(self::ALIAS_STAFF, '', [Query::on(self::ALIAS_STAFF.'.managerId', '$id')])]), + ); + } finally { + $this->cleanupAggCollections($database, $this->aliasFixtureCollections()); + } + } + + public function testJoinWithoutAliasRefusesATableIdThatIsNotAnAlias(): void + { + $database = static::getDatabase(); + if (! $database->getAdapter()->supports(Capability::Joins)) { + $this->expectNotToPerformAssertions(); + + return; + } + + $this->createAliasFixtures($database); + + try { + foreach ([self::ALIAS_HYPHEN, self::ALIAS_DOT] as $table) { + $this->assertRejectedJoin( + "Table ID \"{$table}\" cannot be used as the join alias: an alias must start with a letter or an underscore and contain only letters, digits and underscores. Pass an alias for the join", + fn (): mixed => $database->find(self::ALIAS_ORDERS, [Query::join($table, '', [Query::on('customerId', '$id')])]), + $table, + ); + $this->assertRejectedJoin( + "Table ID \"{$table}\" cannot be used as the join alias: an alias must start with a letter or an underscore and contain only letters, digits and underscores. Pass an alias for the join", + fn (): mixed => $database->find(self::ALIAS_ORDERS, [Query::crossJoin($table, '')]), + $table.' cross join', + ); + + $rows = $database->find(self::ALIAS_ORDERS, [ + Query::join($table, 'item', [Query::on('customerId', 'customerId')]), + Query::select(['total', 'item.label']), + ]); + $this->assertSame(['first'], \array_map(static fn (Document $row): mixed => $row->getAttribute('item.label'), $rows), $table.' with an alias'); + } + + $this->assertRejectedJoin( + 'Join alias must start with a letter or an underscore and contain only letters, digits and underscores', + fn (): mixed => $database->find(self::ALIAS_ORDERS, [Query::join(self::ALIAS_CUSTOMERS, 'my-alias', [Query::on('customerId', '$id')])]), + 'an explicit alias keeps its message', + ); + } finally { + $this->cleanupAggCollections($database, $this->aliasFixtureCollections()); + } + } + + public function testJoinWithoutAliasRefusesTheReservedMainAlias(): void + { + $database = static::getDatabase(); + if (! $database->getAdapter()->supports(Capability::Joins)) { + $this->expectNotToPerformAssertions(); + + return; + } + + $this->createAliasFixtures($database); + $this->createAliasCollection($database, Query::DEFAULT_ALIAS, [Attribute::string(key: 'customerId', size: 32)]); + + try { + $this->assertRejectedJoin( + 'Table ID "'.Query::DEFAULT_ALIAS.'" cannot be used as the join alias: it is reserved for the main collection. Pass an alias for the join', + fn (): mixed => $database->find(self::ALIAS_ORDERS, [Query::join(Query::DEFAULT_ALIAS, '', [Query::on('customerId', 'customerId')])]), + ); + $this->assertRejectedJoin( + 'Join alias "'.Query::DEFAULT_ALIAS.'" is reserved for the main collection', + fn (): mixed => $database->find(self::ALIAS_ORDERS, [Query::join(self::ALIAS_CUSTOMERS, Query::DEFAULT_ALIAS, [Query::on('customerId', '$id')])]), + 'an explicit alias keeps its message', + ); + } finally { + $this->cleanupAggCollections($database, [...$this->aliasFixtureCollections(), Query::DEFAULT_ALIAS]); + } + } + + public function testJoinWithoutAliasRefusesARelationshipKey(): void + { + $database = static::getDatabase(); + if (! $database->getAdapter()->supports(Capability::Joins) || ! $database->getAdapter()->hasFeature(Feature\Relationships::class)) { + $this->expectNotToPerformAssertions(); + + return; + } + + $this->createAliasFixtures($database); + $database->createRelationship(self::ALIAS_ORDERS, Relationship::manyToOne(relatedCollection: self::ALIAS_CUSTOMERS, key: self::ALIAS_CUSTOMERS, twoWayKey: 'orders')); + + try { + $this->assertRejectedJoin( + 'Table ID "'.self::ALIAS_CUSTOMERS.'" cannot be used as the join alias: it is the key of the relationship attribute "'.self::ALIAS_CUSTOMERS.'". Pass an alias for the join', + fn (): mixed => $database->find(self::ALIAS_ORDERS, [Query::join(self::ALIAS_CUSTOMERS, '', [Query::on('customerId', '$id')])]), + ); + $this->assertRejectedJoin( + 'Join alias "'.self::ALIAS_CUSTOMERS.'" is the key of the relationship attribute "'.self::ALIAS_CUSTOMERS.'": give the join another alias', + fn (): mixed => $database->find(self::ALIAS_ORDERS, [Query::join(self::ALIAS_COLORS, self::ALIAS_CUSTOMERS, [Query::on('customerId', '$id')])]), + 'an explicit alias keeps its message', + ); + } finally { + $this->cleanupAggCollections($database, $this->aliasFixtureCollections()); + } + } + + public function testJoiningATableTwiceWithoutAliasesIsRefused(): void + { + $database = static::getDatabase(); + if (! $database->getAdapter()->supports(Capability::Joins)) { + $this->expectNotToPerformAssertions(); + + return; + } + + $this->createAliasFixtures($database); + + try { + $twice = [ + Query::join(self::ALIAS_CUSTOMERS, '', [Query::on('customerId', '$id')]), + Query::leftJoin(self::ALIAS_CUSTOMERS, '', [Query::on('customerId', '$id')]), + ]; + $message = 'Table ID "'.self::ALIAS_CUSTOMERS.'" cannot be used as the join alias: another join of the query already uses it as its alias. Pass an alias for the join'; + + $this->assertRejectedJoin($message, fn (): mixed => $database->find(self::ALIAS_ORDERS, $twice), 'find()'); + $this->assertRejectedJoin($message, fn (): mixed => $database->count(self::ALIAS_ORDERS, $twice), 'count()'); + $this->assertRejectedJoin($message, fn (): mixed => $database->find(self::ALIAS_ORDERS, [$twice[0], Query::crossJoin(self::ALIAS_CUSTOMERS, '')]), 'a cross join'); + + $this->assertRejectedJoin( + 'Join alias "c" is declared more than once', + fn (): mixed => $database->find(self::ALIAS_ORDERS, [ + Query::join(self::ALIAS_CUSTOMERS, 'c', [Query::on('customerId', '$id')]), + Query::join(self::ALIAS_COLORS, 'c', [Query::on('customerId', '$id')]), + ]), + 'an explicit alias keeps its message', + ); + + $rows = $database->find(self::ALIAS_ORDERS, [ + Query::join(self::ALIAS_CUSTOMERS, '', [Query::on('customerId', '$id')]), + Query::leftJoin(self::ALIAS_CUSTOMERS, 'again', [Query::on('customerId', '$id')]), + Query::select(['total', self::ALIAS_CUSTOMERS.'.name', 'again.name']), + Query::orderAsc('total'), + ]); + $this->assertSame( + [['Ann', 'Ann'], ['Bob', 'Bob']], + \array_map(static fn (Document $row): array => [$row->getAttribute(self::ALIAS_CUSTOMERS.'.name'), $row->getAttribute('again.name')], $rows), + 'aliasing the second join reads both', + ); + } finally { + $this->cleanupAggCollections($database, $this->aliasFixtureCollections()); + } + } + + /** + * A query hook may point a join at another table, as Appwrite points a public table ID at its + * internal collection. The alias is fixed when the join is built, so rows stay keyed by the ID + * the caller gave and a refusal names that ID, never the internal one. + */ + public function testJoinWithoutAliasKeepsTheTableIdAHookRewrites(): void + { + $database = static::getDatabase(); + if (! $database->getAdapter()->supports(Capability::Joins)) { + $this->expectNotToPerformAssertions(); + + return; + } + + $this->createAliasFixtures($database); + $hook = new JoinTables(['customers' => self::ALIAS_CUSTOMERS, 'public-customers' => self::ALIAS_CUSTOMERS]); + $database->addHook($hook); + + try { + $rows = $database->find(self::ALIAS_ORDERS, [ + Query::parseQuery(['method' => 'join', 'attribute' => 'customers', 'values' => [Query::on('customerId', '$id')->toArray()]]), + Query::select(['total', 'customers.name']), + Query::orderAsc('total'), + ]); + + $this->assertSame(['Ann', 'Bob'], \array_map(static fn (Document $row): mixed => $row->getAttribute('customers.name'), $rows)); + $this->assertArrayNotHasKey(self::ALIAS_CUSTOMERS.'.name', $rows[0]->getArrayCopy(), 'rows are not keyed by the internal table'); + + $this->assertRejectedJoin( + 'Table ID "public-customers" cannot be used as the join alias: an alias must start with a letter or an underscore and contain only letters, digits and underscores. Pass an alias for the join', + fn (): mixed => $database->find(self::ALIAS_ORDERS, [Query::join('public-customers', '', [Query::on('customerId', '$id')])]), + ); + } finally { + $database->removeHook($hook); + $this->cleanupAggCollections($database, $this->aliasFixtureCollections()); + } + } + + /** + * @param non-empty-string $message + */ + private function assertRejectedJoin(string $message, Closure $read, string $case = ''): void + { + try { + $read(); + $this->fail(($case === '' ? '' : $case.': ').'the join ran instead of being refused with "'.$message.'"'); + } catch (QueryException $error) { + $this->assertStringEndsWith($message, $error->getMessage(), $case); + } + } + + /** + * @return list + */ + private function aliasFixtureCollections(): array + { + return [self::ALIAS_ORDERS, self::ALIAS_CUSTOMERS, self::ALIAS_COLORS, self::ALIAS_STAFF, self::ALIAS_HYPHEN, self::ALIAS_DOT]; + } + + private function createAliasFixtures(Database $database): void + { + $this->cleanupAggCollections($database, [...$this->aliasFixtureCollections(), Query::DEFAULT_ALIAS]); + + $this->createAliasCollection($database, self::ALIAS_ORDERS, [Attribute::string(key: 'customerId', size: 32), Attribute::integer(key: 'total')]); + $this->createAliasCollection($database, self::ALIAS_CUSTOMERS, [Attribute::string(key: 'name', size: 32)]); + $this->createAliasCollection($database, self::ALIAS_COLORS, [Attribute::string(key: 'name', size: 32)]); + $this->createAliasCollection($database, self::ALIAS_STAFF, [Attribute::string(key: 'name', size: 32), Attribute::string(key: 'managerId', size: 32)]); + foreach ([self::ALIAS_HYPHEN, self::ALIAS_DOT] as $table) { + $this->createAliasCollection($database, $table, [Attribute::string(key: 'customerId', size: 32), Attribute::string(key: 'label', size: 32)]); + } + + foreach ([ + [self::ALIAS_ORDERS, 'o1', ['customerId' => 'ann', 'total' => 10]], + [self::ALIAS_ORDERS, 'o2', ['customerId' => 'bob', 'total' => 20]], + [self::ALIAS_ORDERS, 'o3', ['customerId' => 'nobody', 'total' => 30]], + [self::ALIAS_CUSTOMERS, 'ann', ['name' => 'Ann']], + [self::ALIAS_CUSTOMERS, 'bob', ['name' => 'Bob']], + [self::ALIAS_COLORS, 'red', ['name' => 'red']], + [self::ALIAS_COLORS, 'blue', ['name' => 'blue']], + [self::ALIAS_STAFF, 'ann', ['name' => 'Ann', 'managerId' => '']], + [self::ALIAS_STAFF, 'cal', ['name' => 'Cal', 'managerId' => 'ann']], + [self::ALIAS_STAFF, 'dee', ['name' => 'Dee', 'managerId' => 'cal']], + [self::ALIAS_HYPHEN, 'i1', ['customerId' => 'ann', 'label' => 'first']], + [self::ALIAS_DOT, 'i1', ['customerId' => 'ann', 'label' => 'first']], + ] as [$collection, $id, $attributes]) { + $database->createDocument($collection, new Document(['$id' => $id, '$permissions' => [Permission::read(Role::any())], ...$attributes])); + } + } + + /** + * @param list $attributes + */ + private function createAliasCollection(Database $database, string $id, array $attributes): void + { + $database->createCollection(Collection::create( + id: $id, + attributes: $attributes, + permissions: [Permission::create(Role::any()), Permission::read(Role::any())], + documentSecurity: false, + )); + } +} diff --git a/tests/e2e/Adapter/Support/JoinTables.php b/tests/e2e/Adapter/Support/JoinTables.php new file mode 100644 index 000000000..a0fc3f631 --- /dev/null +++ b/tests/e2e/Adapter/Support/JoinTables.php @@ -0,0 +1,34 @@ + $tables The table each joined collection is read from + */ + public function __construct(private array $tables) + { + } + + #[\Override] + public function rewrite(Event $event, Collection $collection, array $queries): array + { + foreach ($queries as $index => $query) { + $table = $this->tables[$query->getAttribute()] ?? null; + if ($table !== null && $query->getMethod()->isJoin()) { + $queries[$index] = (clone $query)->setAttribute($table); + } + } + + return $queries; + } +} diff --git a/tests/unit/JoinAliasTest.php b/tests/unit/JoinAliasTest.php index 0a671780e..a984d9e32 100644 --- a/tests/unit/JoinAliasTest.php +++ b/tests/unit/JoinAliasTest.php @@ -54,16 +54,40 @@ public function testAnAliasSortingBeforeAnEarlierOneStillReadsItsOwnJoin(): void ])); } - public function testAJoinWithoutAnAliasIsRejectedWithoutQueryValidation(): void + public function testAJoinWithoutAnAliasIsReadUnderItsCollection(): void { $database = $this->database(); - $this->expectException(QueryException::class); - $this->expectExceptionMessage('Join alias is required'); + foreach ([ + 'factory' => Query::join('b', '', [Query::on('$id', 'mainId')]), + 'constructor' => new Query(Method::Join, 'b', [Query::on('$id', 'mainId')]), + 'parsed' => Query::parseQuery(['method' => 'join', 'attribute' => 'b', 'values' => ['$id', '=', 'mainId']]), + ] as $case => $join) { + $rows = $database->find('main', [$join, Query::select(['name', 'b.score'])]); + $this->assertCount(1, $rows, $case); + $this->assertSame(1, $rows[0]->getAttribute('b.score'), $case); + + $rows = $database->skipValidation(fn (): array => $database->find('main', [$join, Query::select(['name', 'b.score'])])); + $this->assertSame(1, $rows[0]->getAttribute('b.score'), $case.' without query validation'); + } + } - $database->skipValidation(fn (): array => $database->find('main', [ - new Query(Method::Join, 'b', [Query::on('$id', 'mainId')], ''), - ])); + public function testJoiningACollectionTwiceWithoutAliasesNamesTheCollection(): void + { + $database = $this->database(); + $joins = [ + Query::join('b', '', [Query::on('$id', 'mainId')]), + Query::leftJoin('b', '', [Query::on('$id', 'mainId')]), + ]; + + foreach (['with query validation' => fn (): array => $database->find('main', $joins), 'without query validation' => fn (): array => $database->skipValidation(fn (): array => $database->find('main', $joins))] as $case => $read) { + try { + $read(); + $this->fail($case.': the joins ran'); + } catch (QueryException $error) { + $this->assertSame('Table ID "b" cannot be used as the join alias: another join of the query already uses it as its alias. Pass an alias for the join', $error->getMessage(), $case); + } + } } /** @@ -144,6 +168,14 @@ public static function collidingAliases(): iterable yield 'an alias that is not an identifier' => [[ Query::join('b', 'my-alias', [Query::on('$id', 'mainId')]), ]]; + yield 'a collection joined twice without aliases' => [[ + Query::join('b', '', [Query::on('$id', 'mainId')]), + Query::crossJoin('b', ''), + ]]; + yield 'a join without an alias that repeats an earlier alias' => [[ + Query::join('c', 'b', [Query::on('$id', 'mainId')]), + Query::join('b', '', [Query::on('$id', 'mainId')]), + ]]; yield 'a nested join alias that repeats an earlier one' => [[ Query::join('b', 'x', [Query::on('$id', 'mainId')]), Query::leftJoin('c', 'x', [Query::on('$id', 'mainId')]), diff --git a/tests/unit/QuerySerializationTest.php b/tests/unit/QuerySerializationTest.php index 5de062d12..4fbb9aa49 100644 --- a/tests/unit/QuerySerializationTest.php +++ b/tests/unit/QuerySerializationTest.php @@ -130,12 +130,61 @@ public function testParsedColumnFormJoinBecomesAnOnCondition(): void ); } - public function testParsedJoinWithoutAnAliasIsAQueryException(): void + /** + * @return iterable, Method}> + */ + public static function joinsWithoutAlias(): iterable + { + $on = ['method' => 'on', 'values' => ['$id', '=', 'user']]; + + yield 'the nested ON form' => [['method' => 'join', 'attribute' => 'orders', 'values' => [$on]], Method::Join]; + yield 'an empty alias property' => [['method' => 'leftJoin', 'attribute' => 'orders', 'alias' => '', 'values' => [$on]], Method::LeftJoin]; + yield 'an empty legacy alias first' => [['method' => 'join', 'attribute' => 'orders', 'values' => ['', $on]], Method::Join]; + yield 'the column form' => [['method' => 'join', 'attribute' => 'orders', 'values' => ['$id', '=', 'user']], Method::Join]; + yield 'a cross join' => [['method' => 'crossJoin', 'attribute' => 'orders', 'values' => []], Method::CrossJoin]; + } + + /** + * @param array $array + */ + #[DataProvider('joinsWithoutAlias')] + public function testParsedJoinWithoutAnAliasIsReadUnderItsCollection(array $array, Method $method): void + { + $parsed = Query::parseQuery($array); + + $this->assertSame($method, $parsed->getMethod()); + $this->assertSame('orders', $parsed->getAlias()); + $this->assertTrue($parsed->isAliasDefaulted()); + + $serialized = $parsed->toArray(); + $this->assertSame('orders', $serialized['alias']); + $this->assertSame($serialized, Query::parseQuery($serialized)->toArray()); + $this->assertTrue(Query::parse($parsed->toString())->isAliasDefaulted()); + } + + public function testAJoinBuiltWithoutAnAliasIsReadUnderItsCollection(): void + { + $join = new Query(Method::Join, 'orders', [Query::on('$id', 'user')]); + + $this->assertSame('orders', $join->getAlias()); + $this->assertTrue($join->isAliasDefaulted()); + $this->assertFalse(Query::join('orders', 'ord', [Query::on('$id', 'ord.user')])->isAliasDefaulted()); + } + + public function testADefaultedAliasSurvivesRenamingTheJoinedCollection(): void + { + $renamed = (clone Query::join('orders', '', [Query::on('$id', 'user')]))->setAttribute('database_1_collection_2'); + + $this->assertSame('orders', $renamed->getAlias()); + $this->assertTrue($renamed->isAliasDefaulted()); + } + + public function testParsedJoinWithoutAnAliasOrCollectionIsAQueryException(): void { $this->expectException(QueryException::class); - $this->expectExceptionMessage('Join alias is required: join orders'); + $this->expectExceptionMessage('Join alias is required: join '); - Query::parseQuery(['method' => 'join', 'attribute' => 'orders', 'values' => [['method' => 'on', 'values' => ['$id', '=', 'user']]]]); + Query::parseQuery(['method' => 'join', 'values' => [['method' => 'on', 'values' => ['$id', '=', 'user']]]]); } /** diff --git a/tests/unit/Validator/Query/JoinAliasTest.php b/tests/unit/Validator/Query/JoinAliasTest.php index 6fe43cc79..63bebb75a 100644 --- a/tests/unit/Validator/Query/JoinAliasTest.php +++ b/tests/unit/Validator/Query/JoinAliasTest.php @@ -4,9 +4,13 @@ use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; +use Utopia\Database\Document; use Utopia\Database\Query; +use Utopia\Database\RelationshipType; use Utopia\Database\Validator\Query\Join; +use Utopia\Database\Validator\Query\Joined\Collection as JoinedCollection; use Utopia\Query\Method; +use Utopia\Query\Schema\ColumnType; final class JoinAliasTest extends TestCase { @@ -28,6 +32,11 @@ public static function acceptedJoins(): iterable yield 'an identifier with digits and underscores' => [Query::leftJoin('orders', '_order_2', [Query::on('$id', 'customerId')])]; yield 'a cross join alias' => [Query::crossJoin('orders', 'ord')]; yield 'a nested join alias' => [Query::rightJoin('orders', 'ord', [Query::on('$id', 'customerId')])]; + yield 'no alias' => [Query::join('orders', '', [Query::on('$id', 'customerId')])]; + yield 'no alias on a left join' => [Query::leftJoin('orders', '', [Query::on('$id', 'orders.customerId')])]; + yield 'no alias on a cross join' => [Query::crossJoin('orders', '')]; + yield 'no alias on a join built without one' => [new Query(Method::Join, 'orders', [Query::on('$id', 'customerId')])]; + yield 'no alias on a join of a table named like the main collection' => [Query::join('main', '', [Query::on('$id', 'main.customerId')])]; } #[DataProvider('rejectedJoins')] @@ -48,7 +57,6 @@ public static function rejectedJoins(): iterable $upper = \strtoupper(Query::DEFAULT_ALIAS); $invalid = 'Join alias must start with a letter or an underscore and contain only letters, digits and underscores'; - yield 'no alias' => [new Query(Method::Join, 'orders', [Query::on('$id', 'customerId')], ''), 'Join alias is required']; yield 'the main collection alias' => [Query::join('orders', $main, [Query::on('$id', 'customerId')]), "Join alias \"{$main}\" is reserved for the main collection"]; yield 'the main collection alias in upper case' => [Query::join('orders', $upper, [Query::on('$id', 'customerId')]), "Join alias \"{$upper}\" is reserved for the main collection"]; yield 'the main collection alias on a cross join' => [Query::crossJoin('orders', $main), "Join alias \"{$main}\" is reserved for the main collection"]; @@ -58,4 +66,86 @@ public static function rejectedJoins(): iterable yield 'a dot' => [Query::join('orders', 'a.b', [Query::on('$id', 'customerId')]), $invalid]; yield 'a quote' => [Query::crossJoin('orders', 'x`y'), $invalid]; } + + #[DataProvider('rejectedTableIds')] + public function testRejectsATableIdThatCannotBeTheAlias(Query $join, string $message): void + { + $validator = new Join(); + + $this->assertTrue($join->isAliasDefaulted()); + $this->assertFalse($validator->isValid($join)); + $this->assertSame($message, $validator->getDescription()); + } + + /** + * @return iterable + */ + public static function rejectedTableIds(): iterable + { + $main = Query::DEFAULT_ALIAS; + $upper = \strtoupper(Query::DEFAULT_ALIAS); + $pattern = static fn (string $table): string => "Table ID \"{$table}\" cannot be used as the join alias: an alias must start with a letter or an underscore and contain only letters, digits and underscores. Pass an alias for the join"; + $reserved = static fn (string $table): string => "Table ID \"{$table}\" cannot be used as the join alias: it is reserved for the main collection. Pass an alias for the join"; + + yield 'a hyphen' => [Query::join('my-orders', '', [Query::on('$id', 'customerId')]), $pattern('my-orders')]; + yield 'a dot' => [Query::leftJoin('orders.v2', '', [Query::on('$id', 'customerId')]), $pattern('orders.v2')]; + yield 'a leading digit' => [Query::rightJoin('2024orders', '', [Query::on('$id', 'customerId')]), $pattern('2024orders')]; + yield 'a hyphen on a cross join' => [Query::crossJoin('my-orders', ''), $pattern('my-orders')]; + yield 'a hyphen on a join built without an alias' => [new Query(Method::FullOuterJoin, 'my-orders', [Query::on('$id', 'customerId')]), $pattern('my-orders')]; + yield 'a hyphen on a parsed join' => [Query::parseQuery(['method' => 'join', 'attribute' => 'my-orders', 'values' => ['$id', '=', 'customerId']]), $pattern('my-orders')]; + yield 'the main collection alias' => [Query::join($main, '', [Query::on('$id', 'customerId')]), $reserved($main)]; + yield 'the main collection alias in upper case' => [Query::crossJoin($upper, ''), $reserved($upper)]; + } + + public function testRejectsATableIdThatIsARelationshipKey(): void + { + $validator = new Join([ + new Document(['$id' => 'customerId', 'key' => 'customerId', 'type' => ColumnType::String->value]), + new Document(['$id' => 'customers', 'key' => 'customers', 'type' => ColumnType::Relationship->value, 'options' => ['relationType' => RelationshipType::ManyToOne->value, 'side' => 'parent', 'relatedCollection' => 'customers']]), + ]); + + $this->assertFalse($validator->isValid(Query::join('customers', '', [Query::on('customerId', '$id')]))); + $this->assertSame('Table ID "customers" cannot be used as the join alias: it is the key of the relationship attribute "customers". Pass an alias for the join', $validator->getDescription()); + + $validator->resetJoinAliases(); + $this->assertFalse($validator->isValid(Query::join('people', 'customers', [Query::on('customerId', '$id')]))); + $this->assertSame('Join alias "customers" is the key of the relationship attribute "customers": give the join another alias', $validator->getDescription(), 'an explicit alias keeps its message'); + + $validator->resetJoinAliases(); + $this->assertTrue($validator->isValid(Query::join('customers', 'buyer', [Query::on('customerId', '$id')])), $validator->getDescription()); + } + + /** + * The main collection is read under the reserved alias, never under its own ID, so a self-join's + * defaulted alias names only the joined copy: its columns are qualified by it on the right, and a + * left column qualified by it is not the main collection's. + */ + public function testASelfJoinWithoutAnAliasIsReadUnderTheTableId(): void + { + $columns = [new Document(['$id' => 'managerId', 'key' => 'managerId', 'type' => ColumnType::String->value])]; + $validator = new Join($columns); + $validator->allowJoins([JoinedCollection::of('staff', new Document(['$id' => 'staff', 'attributes' => $columns]))]); + + $this->assertTrue($validator->isValid(Query::join('staff', '', [Query::on('managerId', '$id')])), $validator->getDescription()); + + $validator->resetJoinAliases(); + $validator->allowJoins([JoinedCollection::of('staff', new Document(['$id' => 'staff', 'attributes' => $columns]))]); + $this->assertTrue($validator->isValid(Query::join('staff', '', [Query::on('$id', 'staff.managerId')])), $validator->getDescription()); + + $validator->resetJoinAliases(); + $this->assertFalse($validator->isValid(Query::join('staff', '', [Query::on('staff.managerId', '$id')]))); + $this->assertSame('The left column of a join condition must belong to the main collection or to a join declared before it: staff.managerId', $validator->getDescription()); + } + + public function testAnEmptyAliasIsStillDescribedAsRequired(): void + { + $this->assertSame('Join alias is required', Join::describeInvalidAlias('')); + $this->assertSame('Join alias is required', Join::describeInvalidAlias('', true)); + } + + public function testDescribesADuplicateAliasByHowItWasGiven(): void + { + $this->assertSame('Join alias "c" is declared more than once', Join::describeDuplicateAlias('c')); + $this->assertSame('Table ID "customers" cannot be used as the join alias: another join of the query already uses it as its alias. Pass an alias for the join', Join::describeDuplicateAlias('customers', true)); + } }