refactor: Replace usage of utopia injection with route#11435
Open
HarshMN2345 wants to merge 21 commits into1.8.xfrom
Open
refactor: Replace usage of utopia injection with route#11435HarshMN2345 wants to merge 21 commits into1.8.xfrom
HarshMN2345 wants to merge 21 commits into1.8.xfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces injected Http $utopia with Route $route across routing, init, shutdown, action, and resource handlers and adds use Utopia\Http\Route where required. Removes local calls to $utopia->getRoute() / $utopia->match($request) and uses the injected $route directly, including in router labeling and error/telemetry callbacks. Updates injection tokens from 'utopia' to 'route' on Http registrations. Additionally, adds JSON-body parsing for empty GraphQL queries and normalizes x-sdk-graphql payloads in GraphQL endpoints. Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.29s | Logs |
LegacyTransactionsConsoleClientTest::testBulkUpdateOperations |
1 | 240.31s | Logs |
Commit 3f0a341 - 3 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.27s | Logs |
LegacyCustomServerTest::testSpatialQuery |
1 | 240.55s | Logs |
TablesDBConsoleClientTest::testSpatialQuery |
1 | 240.52s | Logs |
Commit 03aa85b - 3 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.31s | Logs |
RealtimeCustomClientQueryTest::testDatabaseChannelWithQuery |
1 | 46.48s | Logs |
RealtimeCustomClientTest::testChannelTablesDBRowUpdate |
1 | 644ms | Logs |
Commit 3dacb1e - 9 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.27s | Logs |
LegacyCustomClientTest::testOrQueries |
1 | 240.28s | Logs |
LegacyCustomServerTest::testInvalidDocumentStructure |
1 | 240.97s | Logs |
LegacyCustomServerTest::testNotStartsWith |
1 | 241.20s | Logs |
TablesDBCustomServerTest::testCreateIndexes |
1 | 241.14s | Logs |
TablesDBTransactionsConsoleClientTest::testUpdateDocument |
1 | 241.01s | Logs |
TablesDBTransactionsCustomClientTest::testBulkUpsert |
1 | 240.52s | Logs |
TablesDBTransactionsCustomServerTest::testRollback |
1 | 240.71s | Logs |
TablesDBTransactionsCustomServerTest::testCrossAPIIncrementDecrement |
1 | 240.72s | Logs |
Commit 836c4f2 - 4 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.26s | Logs |
TablesDBConsoleClientTest::testNotContains |
1 | 240.40s | Logs |
TablesDBCustomServerTest::testCreateIndexes |
1 | 241.13s | Logs |
LegacyTransactionsConsoleClientTest::testBulkUpsert |
1 | 240.23s | Logs |
Note: Flaky test results are tracked for the last 5 commits
✨ Benchmark results
⚡ Benchmark Comparison
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/init/resources.php (1)
1268-1307:⚠️ Potential issue | 🔴 CriticalInjection token mismatch:
'utopia'should be'route'.The function signature expects
Route $route, but the injection array on line 1307 still specifies'utopia'. This will cause a runtime error when the resource is resolved.🐛 Proposed fix
-}, ['project', 'dbForPlatform', 'utopia', 'request', 'authorization']); +}, ['project', 'dbForPlatform', 'route', 'request', 'authorization']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/init/resources.php` around lines 1268 - 1307, The resource registered with Http::setResource declares a parameter Route $route but the injection array still contains the wrong token 'utopia', causing a mismatch at resolution; update the injection tokens for this resource so they match the function signature by replacing 'utopia' with 'route' in the array passed as the third argument to Http::setResource so the container injects Route $route correctly (ensure the rest of tokens: 'project', 'dbForPlatform', 'request', 'authorization' remain in the same order).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/init/resources.php`:
- Around line 1268-1307: The resource registered with Http::setResource declares
a parameter Route $route but the injection array still contains the wrong token
'utopia', causing a mismatch at resolution; update the injection tokens for this
resource so they match the function signature by replacing 'utopia' with 'route'
in the array passed as the third argument to Http::setResource so the container
injects Route $route correctly (ensure the rest of tokens: 'project',
'dbForPlatform', 'request', 'authorization' remain in the same order).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
app/controllers/general.phpapp/controllers/mock.phpapp/controllers/shared/api.phpapp/controllers/shared/api/auth.phpapp/init/resources.php
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/controllers/api/graphql.php (2)
120-126: Consider providing explicit error feedback for malformed JSON.When
json_decodefails on an invalid JSON payload, the code silently continues with an empty$query, which will eventually throwGRAPHQL_NO_QUERY. This could confuse users who sent malformed JSON but receive a "no query" error instead of a JSON parsing error.♻️ Proposed improvement for better error handling
if (empty($query) && \str_starts_with($type, 'application/json')) { $rawPayload = $request->getRawPayload(); $decoded = \json_decode($rawPayload, true); - if (\is_array($decoded)) { + if ($decoded === null && \json_last_error() !== JSON_ERROR_NONE && !empty($rawPayload)) { + throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, 'Invalid JSON payload: ' . \json_last_error_msg()); + } + if (\is_array($decoded)) { $query = $decoded; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/graphql.php` around lines 120 - 126, The code silently ignores json_decode failures causing malformed JSON to surface as GRAPHQL_NO_QUERY; update the block handling $rawPayload/$decoded to detect json_decode errors (use json_last_error() / json_last_error_msg()) when $rawPayload is non-empty and $decoded is not an array, and return or throw a clear client error (e.g., a 400 response or a specific exception/message indicating malformed JSON) referencing the same $query/$rawPayload variables so callers get an explicit "malformed JSON" error instead of GRAPHQL_NO_QUERY.
176-184: Duplicated JSON parsing logic could be extracted into a helper function.This JSON body parsing block (lines 178-184) is identical to lines 120-126 in the mutation endpoint. Consider extracting this into a shared helper function similar to
parseGraphql()andparseMultipart()to reduce duplication and centralize the logic.♻️ Proposed helper function
Add a helper function:
/** * Parse a JSON request body when query params are empty * * `@param` array $query * `@param` Request $request * `@return` array */ function parseJsonBody(array $query, Request $request): array { $type = $request->getHeader('content-type', ''); if (empty($query) && \str_starts_with($type, 'application/json')) { $rawPayload = $request->getRawPayload(); $decoded = \json_decode($rawPayload, true); if (\is_array($decoded)) { return $decoded; } } return $query; }Then use in both endpoints:
$query = $request->getParams(); -$type = $request->getHeader('content-type', ''); - -if (empty($query) && \str_starts_with($type, 'application/json')) { - $rawPayload = $request->getRawPayload(); - $decoded = \json_decode($rawPayload, true); - if (\is_array($decoded)) { - $query = $decoded; - } -} +$query = parseJsonBody($query, $request); +$type = $request->getHeader('content-type', '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/graphql.php` around lines 176 - 184, Extract the duplicated JSON body parsing into a shared helper named parseJsonBody(array $query, Request $request): array and replace the inlined blocks in both the GraphQL query handler and the mutation endpoint with calls to parseJsonBody($query, $request); the helper should check the Content-Type header with str_starts_with($type, 'application/json'), json_decode the raw payload via $request->getRawPayload(), return the decoded array if is_array($decoded) or the original $query otherwise, and be used alongside existing parseGraphql() and parseMultipart() flows to centralize logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/controllers/api/graphql.php`:
- Around line 120-126: The code silently ignores json_decode failures causing
malformed JSON to surface as GRAPHQL_NO_QUERY; update the block handling
$rawPayload/$decoded to detect json_decode errors (use json_last_error() /
json_last_error_msg()) when $rawPayload is non-empty and $decoded is not an
array, and return or throw a clear client error (e.g., a 400 response or a
specific exception/message indicating malformed JSON) referencing the same
$query/$rawPayload variables so callers get an explicit "malformed JSON" error
instead of GRAPHQL_NO_QUERY.
- Around line 176-184: Extract the duplicated JSON body parsing into a shared
helper named parseJsonBody(array $query, Request $request): array and replace
the inlined blocks in both the GraphQL query handler and the mutation endpoint
with calls to parseJsonBody($query, $request); the helper should check the
Content-Type header with str_starts_with($type, 'application/json'), json_decode
the raw payload via $request->getRawPayload(), return the decoded array if
is_array($decoded) or the original $query otherwise, and be used alongside
existing parseGraphql() and parseMultipart() flows to centralize logic.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors request-context access by replacing utopia injection usage (and related match()/getRoute() calls) with direct route injection across HTTP controllers/resources, alongside dependency lockfile updates and improved GraphQL request payload parsing for JSON bodies.
Changes:
- Replace
utopiainjection withrouteinjection in multiple init/shutdown handlers and resources. - Update GraphQL POST handlers to fall back to decoding raw JSON payload when params are empty.
- Bump several dependencies in
composer.lock(Symfony, Utopia packages, phpstan, etc.).
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| composer.lock | Updates locked dependency versions and composer lock metadata. |
| app/init/resources.php | Updates team resource to depend on injected Route instead of matching via Http. |
| app/controllers/shared/api/auth.php | Switches auth init middleware from utopia-based route matching to injected Route. |
| app/controllers/shared/api.php | Switches API init/shutdown handlers to injected Route and removes redundant route matching calls. |
| app/controllers/mock.php | Uses injected Route in mock shutdown handler instead of utopia->getRoute(). |
| app/controllers/general.php | Injects Route into init/options/error handlers and replaces utopia->getRoute() usage. |
| app/controllers/api/graphql.php | Adds JSON raw-payload decode fallback; adjusts x-sdk-graphql query extraction timing. |
Comments suppressed due to low confidence (1)
app/controllers/general.php:920
- This
if ($route === null)block is unreachable with the currentRoute $routetype-hint (and would be aTypeErrorbefore reaching here ifnullwere injected). Either make$routenullable (?Route) or remove this null handling and ensure a route is always available at this point.
if ($route === null) {
return $response
->setStatusCode(404)
->send('Not Found');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/controllers/general.php
Outdated
| ->inject('devKey') | ||
| ->inject('authorization') | ||
| ->action(function (Throwable $error, Http $utopia, Request $request, Response $response, Document $project, ?Logger $logger, Log $log, StatsUsage $queueForStatsUsage, Document $devKey, Authorization $authorization) { | ||
| ->action(function (Throwable $error, Http $utopia, Request $request, Route $route, Response $response, Document $project, ?Logger $logger, Log $log, StatsUsage $queueForStatsUsage, Document $devKey, Authorization $authorization) { |
There was a problem hiding this comment.
This error handler receives $route as non-nullable Route, but later uses $route?->... and checks $route === null. To avoid dead code and potential TypeError when handling unmatched routes/errors, consider changing the parameter type to ?Route (and keep nullsafe usage) or remove the null checks if route is guaranteed.
Suggested change
| ->action(function (Throwable $error, Http $utopia, Request $request, Route $route, Response $response, Document $project, ?Logger $logger, Log $log, StatsUsage $queueForStatsUsage, Document $devKey, Authorization $authorization) { | |
| ->action(function (Throwable $error, Http $utopia, Request $request, ?Route $route, Response $response, Document $project, ?Logger $logger, Log $log, StatsUsage $queueForStatsUsage, Document $devKey, Authorization $authorization) { |
app/init/resources.php
Outdated
| } else { | ||
| $route = $utopia->match($request); | ||
| $path = !empty($route) ? $route->getPath() : $request->getURI(); | ||
| $path = $route->getPath() ?? $request->getURI(); |
There was a problem hiding this comment.
Previously this logic handled the case where route matching returned no route; now it assumes $route is always present. If route can be missing for some requests, $route->getPath() will throw. Consider making the injected parameter ?Route and using $route?->getPath() ?? $request->getURI() (and/or retaining the old match() fallback) to preserve the prior behavior.
app/controllers/api/graphql.php
Outdated
| $query = parseMultipart($query, $request); | ||
| } | ||
|
|
||
| if ($request->getHeader('x-sdk-graphql') == 'true' && isset($query['query'])) { |
There was a problem hiding this comment.
execute() is type-hinted to accept array $query, but this branch can replace $query with $query['query'] which may be a string depending on the request payload shape. If that happens, calling execute() will raise a TypeError. Consider validating that $query['query'] is an array (or wrapping it into the expected array shape) before reassigning.
Suggested change
| if ($request->getHeader('x-sdk-graphql') == 'true' && isset($query['query'])) { | |
| if ($request->getHeader('x-sdk-graphql') == 'true' && isset($query['query']) && \is_array($query['query'])) { |
app/controllers/api/graphql.php
Outdated
| } | ||
|
|
||
| if ($request->getHeader('x-sdk-graphql') == 'true' && isset($query['query'])) { | ||
| $query = $query['query']; |
There was a problem hiding this comment.
Same as above: execute() requires an array, but $query = $query['query'] can turn $query into a string if the decoded payload uses a plain query string. Guard for array shape (or normalize into the expected {query, variables, operationName} structure) before calling execute().
Suggested change
| $query = $query['query']; | |
| if (\is_array($query['query'])) { | |
| // If the SDK wrapped the full GraphQL payload in a "query" field, unwrap it. | |
| $query = $query['query']; | |
| } else { | |
| // If the SDK sent a plain query string, normalize into the expected structure. | |
| $normalized = [ | |
| 'query' => $query['query'], | |
| ]; | |
| if (isset($query['variables'])) { | |
| $normalized['variables'] = $query['variables']; | |
| } | |
| if (isset($query['operationName'])) { | |
| $normalized['operationName'] = $query['operationName']; | |
| } | |
| $query = $normalized; | |
| } |
app/controllers/general.php
Outdated
| ->inject('queueForDeletes') | ||
| ->inject('executionsRetentionCount') | ||
| ->action(function (Http $utopia, SwooleRequest $swooleRequest, Request $request, Response $response, Log $log, Document $project, Database $dbForPlatform, callable $getProjectDB, Locale $locale, array $localeCodes, Reader $geodb, StatsUsage $queueForStatsUsage, Event $queueForEvents, Execution $queueForExecutions, Executor $executor, array $platform, callable $isResourceBlocked, string $previewHostname, Document $devKey, ?Key $apiKey, Cors $cors, Authorization $authorization, DeleteEvent $queueForDeletes, int $executionsRetentionCount) { | ||
| ->action(function (Http $utopia, SwooleRequest $swooleRequest, Request $request, Route $route, Response $response, Log $log, Document $project, Database $dbForPlatform, callable $getProjectDB, Locale $locale, array $localeCodes, Reader $geodb, StatsUsage $queueForStatsUsage, Event $queueForEvents, Execution $queueForExecutions, Executor $executor, array $platform, callable $isResourceBlocked, string $previewHostname, Document $devKey, ?Key $apiKey, Cors $cors, Authorization $authorization, DeleteEvent $queueForDeletes, int $executionsRetentionCount) { |
There was a problem hiding this comment.
$route is type-hinted as non-nullable Route, but this handler later treats it as nullable (null check + nullsafe usage). If the injector can provide null, this will crash with a TypeError; otherwise the null-handling becomes dead code. Consider changing the parameter to ?Route and using nullsafe access, or remove the null-handling if a route is guaranteed here.
…njection Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/init/resources.php`:
- Around line 1269-1274: In the Http::setResource('team') callback there is a
potential null dereference calling $route->getPath(); update the call to guard
against a null $route by using the null-safe operator on the $route before
calling getPath (i.e., use $route?->getPath() with the existing fallback to
$request->getURI()), or alternatively ensure and document that the Route
injection can never be null—prefer the null-safe change for $route and getPath
to prevent fatal errors during error/early-exit flows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 752c2c8e-1ec8-4c23-a77e-69c9f88cfd17
📒 Files selected for processing (3)
app/controllers/general.phpapp/controllers/shared/api.phpapp/init/resources.php
loks0n
reviewed
Mar 11, 2026
app/controllers/general.php
Outdated
Comment on lines
+1272
to
+1273
| if ($logger && $publish) { | ||
| try { | ||
| /** @var Utopia\Database\Document $user */ | ||
| $user = $utopia->getResource('user'); | ||
| } catch (\Throwable) { | ||
| // All good, user is optional information for logger | ||
| } | ||
|
|
||
| if (isset($user) && !$user->isEmpty()) { | ||
| $log->setUser(new User($user->getId())); | ||
| } else { | ||
| $log->setUser(new User('guest-' . hash('sha256', $request->getIP()))); | ||
| } | ||
| $log->setUser(new User('guest-' . hash('sha256', $request->getIP()))); |
Member
There was a problem hiding this comment.
do we no longer set the user using the user resource? why?
loks0n
reviewed
Mar 11, 2026
Comment on lines
+270
to
+277
| $previousRoute = Request::getRoute(); | ||
| $previousRouteResource = null; | ||
|
|
||
| try { | ||
| $previousRouteResource = $utopia->getResource('route', true); | ||
| } catch (\Throwable) { | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
This looks wrong, why is it needed?
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist