From f5a7cfd2ea0135d1b01a01fd60d378bd81acba8c Mon Sep 17 00:00:00 2001 From: ArnabChatterjee20k Date: Mon, 4 May 2026 12:22:48 +0530 Subject: [PATCH] fix: resolve query syntax errors and improve error handling in Request class --- src/Appwrite/Utopia/Request.php | 65 ++++++----- .../e2e/Services/Databases/DatabasesBase.php | 45 ++++++++ .../Utopia/Request/Filters/ThrowingFilter.php | 24 +++++ tests/unit/Utopia/RequestTest.php | 101 ++++++++++++++++++ 4 files changed, 207 insertions(+), 28 deletions(-) create mode 100644 tests/unit/Utopia/Request/Filters/ThrowingFilter.php diff --git a/src/Appwrite/Utopia/Request.php b/src/Appwrite/Utopia/Request.php index 3004392f76..945a3d2bf6 100644 --- a/src/Appwrite/Utopia/Request.php +++ b/src/Appwrite/Utopia/Request.php @@ -51,38 +51,47 @@ class Request extends UtopiaRequest if (!\is_array($methods)) { $id = $methods->getNamespace() . '.' . $methods->getMethodName(); + } else { + $matched = null; + foreach ($methods as $method) { + /** @var Method|null $method */ + if ($method === null) { + continue; + } + + // Find the method that matches the parameters passed + $methodParamNames = \array_map(fn ($param) => $param->getName(), $method->getParameters()); + $invalidParams = \array_diff(\array_keys($parameters), $methodParamNames); + + // No params defined, or all params are valid + if (empty($methodParamNames) || empty($invalidParams)) { + $matched = $method; + break; + } + } + + $id = $matched !== null + ? $matched->getNamespace() . '.' . $matched->getMethodName() + : 'unknown.unknown'; + } + + try { foreach ($this->getFilters() as $filter) { $parameters = $filter->parse($parameters, $id); } - $this->filteredParams = $parameters; - return $parameters; - } - - $matched = null; - foreach ($methods as $method) { - /** @var Method|null $method */ - if ($method === null) { - continue; + } catch (\Throwable $e) { + // 4xx filter throws are user-input errors that the action layer + // revalidates and reports. Cache the raw, pre-filter parameters + // so a subsequent getParams() — e.g. when the framework builds + // arguments for an error hook — returns without re-running + // filters. Otherwise the second throw gets wrapped as + // "Error handler had an error: ..." (HTTP 500), masking the + // intended 400. + $code = $e->getCode(); + if (\is_int($code) && $code >= 400 && $code < 500) { + $this->filteredParams = parent::getParams(); } - - // Find the method that matches the parameters passed - $methodParamNames = \array_map(fn ($param) => $param->getName(), $method->getParameters()); - $invalidParams = \array_diff(\array_keys($parameters), $methodParamNames); - - // No params defined, or all params are valid - if (empty($methodParamNames) || empty($invalidParams)) { - $matched = $method; - break; - } - } - - $id = $matched !== null - ? $matched->getNamespace() . '.' . $matched->getMethodName() - : 'unknown.unknown'; - - // Apply filters - foreach ($this->getFilters() as $filter) { - $parameters = $filter->parse($parameters, $id); + throw $e; } $this->filteredParams = $parameters; diff --git a/tests/e2e/Services/Databases/DatabasesBase.php b/tests/e2e/Services/Databases/DatabasesBase.php index e3efe3bbd9..315f40475a 100644 --- a/tests/e2e/Services/Databases/DatabasesBase.php +++ b/tests/e2e/Services/Databases/DatabasesBase.php @@ -4339,6 +4339,51 @@ trait DatabasesBase // $this->assertEquals('Invalid query: Cannot query search on attribute "actors" because it is an array.', $documents['body']['message']); } + public function testDocumentsListInvalidQuerySyntax(): void + { + $data = $this->setupDocuments(); + $databaseId = $data['databaseId']; + + // Each entry in `queries` must be a JSON-encoded object (e.g. + // `{"method":"limit","values":[5]}`). Anything else — including the + // legacy SDK shorthand `limit(5)`, a hand-rolled non-JSON string, or a + // JSON value that isn't an object — must be rejected by Query::parse + // with the same `Invalid query: Syntax error` message so callers get + // a consistent 400. + $invalidQueries = [ + 'legacy shorthand' => 'limit(5)', + 'plain string' => 'not-json', + 'json non-object' => '"limit"', + 'malformed json' => '{"method":"limit","values":[5}', + 'unquoted attribute' => '{method:"limit",values:[5]}', + ]; + + foreach ($invalidQueries as $label => $rawQuery) { + $documents = $this->client->call(Client::METHOD_GET, $this->getRecordUrl($databaseId, $data['moviesId']), array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'queries' => [$rawQuery], + ]); + + $this->assertEquals(400, $documents['headers']['status-code'], "Expected 400 for [$label]: $rawQuery"); + $this->assertEquals('Invalid query: Syntax error', $documents['body']['message'], "Wrong error for [$label]: $rawQuery"); + } + + // Sanity check: the JSON form the SDK actually emits still works. + $documents = $this->client->call(Client::METHOD_GET, $this->getRecordUrl($databaseId, $data['moviesId']), array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'queries' => [ + Query::limit(5)->toString(), + ], + ]); + + $this->assertEquals(200, $documents['headers']['status-code']); + $this->assertLessThanOrEqual(5, count($documents['body'][$this->getRecordResource()])); + } + public function testUpdateDocument(): void { $data = $this->setupDocuments(); diff --git a/tests/unit/Utopia/Request/Filters/ThrowingFilter.php b/tests/unit/Utopia/Request/Filters/ThrowingFilter.php new file mode 100644 index 0000000000..8e02b92e39 --- /dev/null +++ b/tests/unit/Utopia/Request/Filters/ThrowingFilter.php @@ -0,0 +1,24 @@ +calls++; + throw new \Exception($this->reason, $this->code); + } +} diff --git a/tests/unit/Utopia/RequestTest.php b/tests/unit/Utopia/RequestTest.php index 57ebae6d1e..d6f911da8d 100644 --- a/tests/unit/Utopia/RequestTest.php +++ b/tests/unit/Utopia/RequestTest.php @@ -5,10 +5,12 @@ namespace Tests\Unit\Utopia; use Appwrite\SDK\Method; use Appwrite\SDK\Parameter; use Appwrite\Utopia\Request; +use Appwrite\Utopia\Request\Filter; use PHPUnit\Framework\TestCase; use Swoole\Http\Request as SwooleRequest; use Tests\Unit\Utopia\Request\Filters\First; use Tests\Unit\Utopia\Request\Filters\Second; +use Tests\Unit\Utopia\Request\Filters\ThrowingFilter; use Utopia\Http\Route; class RequestTest extends TestCase @@ -192,6 +194,105 @@ class RequestTest extends TestCase $this->assertSame('fallback', $request->getHeader('referer', 'fallback')); } + public function testGetParamsCachesRawParamsWhenFilterThrows4xx(): void + { + // Regression: when a request filter throws a 4xx exception during + // Request::getParams() (e.g. RequestV20 rejecting an unparseable + // queries[]), the framework's error path calls getParams() again to + // build error-hook arguments. Without caching, that second call + // re-runs the filter and re-throws, which the framework wraps as + // "Error handler had an error: ..." (HTTP 500), masking the intended + // 400. This test pins that behavior: the first call throws (so the + // action's argument resolution aborts), but the second call returns + // the raw, pre-filter params without re-invoking filters. + $filter = new ThrowingFilter(400, 'invalid input'); + + $this->setupSingleMethodRoute($filter); + $this->request->setQueryString(['foo' => 'bar']); + + $threw = false; + try { + $this->request->getParams(); + } catch (\Throwable $e) { + $threw = true; + $this->assertSame(400, $e->getCode()); + $this->assertSame('invalid input', $e->getMessage()); + } + $this->assertTrue($threw, 'First getParams() call must rethrow the filter exception.'); + $this->assertSame(1, $filter->calls, 'Filter ran once on the first call.'); + + // Second call: framework's error hook arg resolution. Must return raw + // params without re-invoking the filter. + $params = $this->request->getParams(); + $this->assertSame(['foo' => 'bar'], $params); + $this->assertSame(1, $filter->calls, 'Filter must not run again after a cached 4xx failure.'); + } + + public function testGetParamsDoesNotCacheRawParamsForServerError(): void + { + // 5xx filter throws indicate genuine server-side problems, not + // user-input mistakes. They must keep rethrowing on every call so + // the framework's normal error handling sees the failure each time + // — caching raw params would silently swallow real bugs. + $filter = new ThrowingFilter(500, 'boom'); + + $this->setupSingleMethodRoute($filter); + $this->request->setQueryString(['foo' => 'bar']); + + for ($attempt = 1; $attempt <= 2; $attempt++) { + $threw = false; + try { + $this->request->getParams(); + } catch (\Throwable $e) { + $threw = true; + $this->assertSame(500, $e->getCode()); + } + $this->assertTrue($threw, "Call #$attempt must rethrow."); + $this->assertSame($attempt, $filter->calls, "Filter must run on call #$attempt."); + } + } + + public function testGetParamsDoesNotCacheRawParamsForUncodedException(): void + { + // \Exception with the default code of 0 is treated as "unknown" and + // must propagate every call — same reasoning as 5xx. + $filter = new ThrowingFilter(0, 'unknown'); + + $this->setupSingleMethodRoute($filter); + $this->request->setQueryString(['foo' => 'bar']); + + for ($attempt = 1; $attempt <= 2; $attempt++) { + $threw = false; + try { + $this->request->getParams(); + } catch (\Throwable) { + $threw = true; + } + $this->assertTrue($threw, "Call #$attempt must rethrow."); + $this->assertSame($attempt, $filter->calls, "Filter must run on call #$attempt."); + } + } + + /** + * Helper to attach a route with a single SDK method and one filter. + */ + private function setupSingleMethodRoute(Filter $filter): void + { + $route = new Route(Request::METHOD_GET, '/single'); + $route->label('sdk', new Method( + namespace: 'namespace', + group: 'group', + name: 'method', + description: 'description', + auth: [], + responses: [], + )); + + $this->request->addHeader('EXAMPLE', 'VALUE'); + $this->request->setRoute($route); + $this->request->addFilter($filter); + } + /** * Helper to attach a route with multiple SDK methods to the request. */