diff --git a/src/Database/Database.php b/src/Database/Database.php index 69cabf1006..674969b0b2 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -9520,6 +9520,9 @@ private function validateSelections(Document $collection, array $queries): array foreach ($queries as $query) { if ($query->getMethod() == Query::TYPE_SELECT) { foreach ($query->getValues() as $value) { + if (!\is_string($value)) { + throw new QueryException('Attribute selection must be a string, got ' . \get_debug_type($value)); + } if (\str_contains($value, '.')) { $relationshipSelections[] = $value; continue; @@ -10011,7 +10014,7 @@ private function processRelationshipQueries( $values = $query->getValues(); foreach ($values as $valueIndex => $value) { - if (!\str_contains($value, '.')) { + if (!\is_string($value) || !\str_contains($value, '.')) { continue; } diff --git a/src/Database/Validator/Query/Select.php b/src/Database/Validator/Query/Select.php index b0ed9e564b..2df21c9a3c 100644 --- a/src/Database/Validator/Query/Select.php +++ b/src/Database/Validator/Query/Select.php @@ -68,6 +68,16 @@ public function isValid($value): bool return false; } + // Before the duplicate check: array_unique() stringifies every array element + // to "Array", so two nested values collapse into one and report a misleading + // duplicate instead of the type error that is actually there. + foreach ($value->getValues() as $attribute) { + if (!\is_string($attribute)) { + $this->message = 'Attribute selection must be a string, got ' . \get_debug_type($attribute); + return false; + } + } + if (\count($value->getValues()) !== \count(\array_unique($value->getValues()))) { $this->message = 'Duplicate attributes selected'; return false; diff --git a/tests/unit/SelectProjectionTest.php b/tests/unit/SelectProjectionTest.php new file mode 100644 index 0000000000..262e609e1b --- /dev/null +++ b/tests/unit/SelectProjectionTest.php @@ -0,0 +1,120 @@ +database = new Database(new DatabaseMemory(), new Cache(new CacheMemory())); + $this->database + ->setDatabase('utopiaTests') + ->setNamespace('select_' . \uniqid()); + + $this->database->create(); + $this->database->createCollection('widgets'); + $this->database->createAttribute('widgets', 'sku', Database::VAR_STRING, 255, false); + $this->database->createDocument('widgets', new Document([ + '$id' => 'widget', + '$permissions' => [Permission::read(Role::any())], + 'sku' => 'abc', + ])); + } + + /** + * @param array $values + * + * @dataProvider malformedSelections + */ + public function testAMalformedSelectionIsRefusedRatherThanFatal(array $values): void + { + $this->expectException(QueryException::class); + $this->expectExceptionMessage('Attribute selection must be a string, got'); + + $this->database->find('widgets', [Query::select($values)]); + } + + /** + * @return array}> + */ + public static function malformedSelections(): array + { + return [ + 'nested array' => [[['sku']]], + 'nested wildcard' => [[['*']]], + 'mixed flat and nested' => [['sku', ['x']]], + 'two nested values' => [[['a'], ['b']]], + ]; + } + + /** + * The refusal must be a catchable Exception. A TypeError is an Error, so a caller + * catching Exception — as the HTTP layer does — never sees it and returns a 500. + * + * @param array $values + * + * @dataProvider malformedSelections + */ + public function testTheRefusalIsCatchableAsAnException(array $values): void + { + $caught = null; + + try { + $this->database->find('widgets', [Query::select($values)]); + } catch (\Exception $exception) { + $caught = $exception; + } + + $this->assertInstanceOf( + QueryException::class, + $caught, + 'a malformed selection must be catchable as an Exception, otherwise it escapes as a 500', + ); + } + + public function testTheLegitimateFlatFormStillProjects(): void + { + $rows = $this->database->find('widgets', [Query::select(['sku'])]); + + $this->assertCount(1, $rows); + $this->assertSame('abc', $rows[0]->getAttribute('sku')); + } + + public function testTheWildcardStillProjects(): void + { + $this->assertCount(1, $this->database->find('widgets', [Query::select(['*'])])); + } + + /** + * The type check must not swallow the schema check that already worked. + */ + public function testAnUnknownAttributeIsStillRefusedBySchema(): void + { + $this->expectException(QueryException::class); + $this->expectExceptionMessage('Attribute not found in schema: nope'); + + $this->database->find('widgets', [Query::select(['nope'])]); + } +} diff --git a/tests/unit/Validator/Query/SelectTest.php b/tests/unit/Validator/Query/SelectTest.php index 2dafdb94c0..86b8d2495c 100644 --- a/tests/unit/Validator/Query/SelectTest.php +++ b/tests/unit/Validator/Query/SelectTest.php @@ -49,4 +49,60 @@ public function testValueFailure(): void $this->assertEquals('Invalid query', $this->validator->getDescription()); $this->assertFalse($this->validator->isValid(Query::select(['name.artist']))); } + + /** + * A non-string selection used to reach str_contains() and raise a TypeError, + * which is an Error rather than an Exception, so it escaped every catch on the + * way out and surfaced as a 500. Select was the only query method that answered + * a malformed value that way. + * + * @param array $values + * + * @dataProvider nonStringSelections + */ + public function testANonStringSelectionIsRefusedByType(array $values, string $expected): void + { + $this->assertFalse($this->validator->isValid(Query::select($values))); + $this->assertSame($expected, $this->validator->getDescription()); + } + + /** + * @return array, string}> + */ + public static function nonStringSelections(): array + { + return [ + 'nested array' => [[['attr']], 'Attribute selection must be a string, got array'], + 'nested wildcard' => [[['*']], 'Attribute selection must be a string, got array'], + 'mixed flat and nested' => [['attr', ['x']], 'Attribute selection must be a string, got array'], + 'assoc array' => [[['a' => 1]], 'Attribute selection must be a string, got array'], + 'integer' => [[1], 'Attribute selection must be a string, got int'], + 'null' => [[null], 'Attribute selection must be a string, got null'], + ]; + } + + /** + * Two nested values used to collapse to one under array_unique(), which casts + * every array to the string "Array", so the duplicate check tripped first and + * reported a duplicate that was not there. The type check has to run before it. + * + * Parsed from JSON rather than built with Query::select(), because that is the + * path a hand-written HTTP client takes and the only one that can carry a value + * the constructor's array type would reject. + */ + public function testTwoNestedSelectionsReportTheTypeNotAFalseDuplicate(): void + { + $query = Query::parse('{"method":"select","values":[["a"],["b"]]}'); + + $this->assertFalse($this->validator->isValid($query)); + $this->assertSame('Attribute selection must be a string, got array', $this->validator->getDescription()); + } + + public function testTheLegitimateFlatFormStillPasses(): void + { + $this->assertTrue($this->validator->isValid(Query::select(['attr']))); + $this->assertTrue($this->validator->isValid(Query::select(['*']))); + $this->assertTrue($this->validator->isValid(Query::select(['$id', '$createdAt']))); + $this->assertTrue($this->validator->isValid(Query::select(['artist.name']))); + } }