Fix request filters with multi-method routes #9743
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Request
participant Route
participant SDKMethod
participant Filter
Client->>Request: Send request with parameters
Request->>Route: Retrieve SDK methods for current route
Route-->>Request: Return list of SDK methods
loop For each SDK method
Request->>SDKMethod: Compare parameter keys with method's parameters
end
alt Matching method found
Request->>Request: Filter parameters by method's definition
loop For each filter
Request->>Filter: Apply filter.parse(endpoint, param)
end
Request-->>Client: Return filtered parameters
else No matching method
Request-->>Client: Return raw parameters
end
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/Appwrite/Utopia/Request.php (3)
62-70: Filtering block can be simplified & made safer
array_intersect_key()plusarray_flip()is correct but a bit heavy; additionally,$parametersisn’t re-indexed, which may surpris 8000 e consumers expecting consecutive numeric keys for lists. A clearer and slightly faster alternative:- $parameters = \array_intersect_key($parameters, \array_flip($definedNames)); + $parameters = \array_filter( + $parameters, + fn($key) => \in_array($key, $definedNames, true), + ARRAY_FILTER_USE_KEY + );Minor, but improves readability and avoids the temporary flipped array.
73-76: Potential performance hit from reassigning$parametersin every loop
parse()already returns the transformed array; however, if many filters are chained the whole parameter array is re-allocated each time.
If filters are numerous or the array is large, consider streaming or at least cloning only when the filter actually mutates data (many implementations might be no-ops).Not urgent, but worth profiling.
1-223: Fix style issue flagged by PintCI reports a Pint failure for a “function declaration” style problem.
Runvendor/bin/pintorcomposer pintlocally and commit the auto-generated diff to keep the pipeline green.🧰 Tools
🪛 GitHub Actions: Linter
[error] 1-1: PHP CS Fixer (Pint) style check failed: 1 style issue detected related to function declaration. Run 'vendor/bin/pint' to fix code style issues.
tests/unit/Utopia/RequestTest.php (1)
150-186: Helper builds Parameter objects without specifying optionality flag name
new Parameter('bar', optional: true)relies on a named parameteroptional, which is great for clarity.
Consider also settingtypeordescriptionif those fields exist inParameterfor richer tests, and add a comment noting which param is required vs optional to ease future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
src/Appwrite/Utopia/Request.php(2 hunks)tests/unit/Utopia/RequestTest.php(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linter
src/Appwrite/Utopia/Request.php
[error] 1-1: PHP CS Fixer (Pint) style check failed: 1 style issue detected related to function declaration. Run 'vendor/bin/pint' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup & Build Appwrite Image
🔇 Additional comments (1)
tests/unit/Utopia/RequestTest.php (1)
96-112: Unit test legitimises behaviour that conflicts with SDK contract
testGetParamsWithAllOptional()passes only the optionalbarparam while omitting requiredfoo, yet expects the call to be accepted.
After tightening required-parameter checks (see code review above) this test will fail – and should fail.Please reassess the intended contract: if
foois mandatory, the test should assert a fallback or an exception, not a silent match.If
foois not mandatory, mark it accordingly insetupMultiMethodRoute()by settingoptional: true.
✨ Benchmark results
⚡ Benchmark Comparison
|
|
Validated on stage - reproduced issue first, then deployed, then made sure issue is nomore. Merging now |
What does this PR do?
The request getParams method was updated to handle request filters when multiple methods are present on a single route, but the implementation did not merge/check parameters correctly, so if a request filter was run, only the explicit parameters defined on last method in the list were sent to the route.
This meant that when calling
createDocumentfrom a version lower than 1.6.0, thecreateDocumentsparams were used, sodocumentIdanddatawere being stripped out. This lead to valid requests returning "The document data is missing. Try again with document data populated".Test Plan
Updated existing tests + manual tests.
Related PRs and Issues
Checklist
Summary by CodeRabbit