Throw appropriate 400s from request filters#10502
Conversation
📝 WalkthroughWalkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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! |
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Appwrite/Utopia/Request/Filters/V17.php (2)
123-128: Fix fatal: non-static helpers called statically + unused params in appendSymbol.appendSymbol, isQuote, isSpecialChar, and parseValue are declared as instance methods but are called statically. This is a runtime fatal (cannot call non-static method statically). Also, appendSymbol’s $index and $filter params are unused (lint failure). Make helpers static and drop the unused params; update call sites.
- if (!(static::isSpecialChar($filter[$i + 1]))) { - static::appendSymbol($isStringStack, $filter[$i], $i, $filter, $currentParam); + if (!(static::isSpecialChar($filter[$i + 1]))) { + static::appendSymbol($isStringStack, $filter[$i], $currentParam); } - static::appendSymbol($isStringStack, $filter[$i + 1], $i, $filter, $currentParam); + static::appendSymbol($isStringStack, $filter[$i + 1], $currentParam); ... - static::appendSymbol( - $isStringStack, - $char, - $i, - $filter, - $currentParam, - ); + static::appendSymbol($isStringStack, $char, $currentParam); ... - static::appendSymbol( - $isStringStack, - $char, - $i, - $filter, - $currentParam, - ); + static::appendSymbol($isStringStack, $char, $currentParam);- private function appendSymbol( - bool $isStringStack, - string $char, - int $index, - string $filter, - string &$currentParam - ): void { + private static function appendSymbol( + bool $isStringStack, + string $char, + string &$currentParam + ): void {- private function isQuote(string $char): bool + private static function isQuote(string $char): bool- private function isSpecialChar(string $char): bool + private static function isSpecialChar(string $char): bool- private function parseValue(string $value): mixed + private static function parseValue(string $value): mixed- if (\is_array($param)) { - foreach ($param as $element) { - $arr[] = self::parseValue($element); - } - $parsedParams[] = $arr ?? []; + if (\is_array($param)) { + $arr = []; + foreach ($param as $element) { + $arr[] = static::parseValue($element); + } + $parsedParams[] = $arr; } else { - $parsedParams[] = self::parseValue($param); + $parsedParams[] = static::parseValue($param); }Also applies to: 139-156, 201-209, 309-316, 334-343, 345-360, 277-298, 218-227
91-96: Consistency + small correctness guardrails.
- Prefix core functions with global namespace for consistency/perf.
- Guard between() param count to avoid undefined offsets.
- Use global count() consistently.
- $paramsStart = mb_strpos($filter, '('); + $paramsStart = \mb_strpos($filter, '(');- if (strlen($currentParam)) { + if (\strlen($currentParam)) {- if (\strlen($currentParam)) { + if (\strlen($currentParam)) { $params[] = $currentParam; $currentParam = ''; }- $attribute = $parsedParams[0] ?? ''; - if (count($parsedParams) < 2) { + $attribute = $parsedParams[0] ?? ''; + if (\count($parsedParams) < 2) { return new Query($method, $attribute); } return new Query($method, $attribute, \is_array($parsedParams[1]) ? $parsedParams[1] : [$parsedParams[1]]);- case Query::TYPE_BETWEEN: - return new Query($method, $parsedParams[0], [$parsedParams[1], $parsedParams[2]]); + case Query::TYPE_BETWEEN: + if (\count($parsedParams) < 3) { + throw new Exception(Exception::GENERAL_QUERY_INVALID, 'between() expects 3 parameters'); + } + return new Query($method, $parsedParams[0], [$parsedParams[1], $parsedParams[2]]);- if (count($parsedParams) > 0) { + if (\count($parsedParams) > 0) { return new Query($method, values: [$parsedParams[0]]); }Also applies to: 173-175, 211-214, 244-247, 261-264
🧹 Nitpick comments (7)
src/Appwrite/Utopia/Request/Filters/V20.php (5)
57-61: Preserve original cause when rethrowing invalid query errors.Chaining the original QueryException improves debugging without changing the external API or HTTP code.
- } catch (QueryException $e) { - throw new Exception(Exception::GENERAL_QUERY_INVALID, $e->getMessage()); + } catch (QueryException $e) { + throw new Exception(Exception::GENERAL_QUERY_INVALID, $e->getMessage(), null, $e); }
37-56: Normalize queries to array-of-strings before parsing.If a single string slips through (SDKs/clients can differ), Query::parseQueries may receive an invalid type. Normalize defensively.
if (!isset($content['queries'])) { $content['queries'] = []; } - // Handle case where queries is an array but empty + // Normalize to array-of-strings + if (!\is_array($content['queries'])) { + $content['queries'] = \array_filter([$content['queries']]); + } + + // Handle case where queries is an array but empty if (\is_array($content['queries'])) { $content['queries'] = \array_filter($content['queries'], function ($q) { if (\is_object($q) && empty((array)$q)) { return false; } if (\is_string($q) && \trim($q) === '') { return false; } if (empty($q)) { return false; } return true; }); }Also applies to: 57-61
129-135: Use strict in_array for ID comparisons to avoid accidental matches.Prevents '1' == 1 style collisions when tracking visited collections.
- if (in_array($collectionId, $visited)) { + if (\in_array($collectionId, $visited, true)) { return []; } ... - if ($relatedCollectionId && in_array($relatedCollectionId, $visited)) { + if ($relatedCollectionId && \in_array($relatedCollectionId, $visited, true)) { continue; }Also applies to: 181-183
141-151: Propagate NotFound as previous cause on typed 404s.Keeps stack traces intact while mapping to DATABASE_NOT_FOUND and COLLECTION_NOT_FOUND.
- } catch (NotFound) { - throw new Exception(Exception::DATABASE_NOT_FOUND); + } catch (NotFound $e) { + throw new Exception(Exception::DATABASE_NOT_FOUND, null, null, $e); } ... - } catch (NotFound) { - throw new Exception(Exception::COLLECTION_NOT_FOUND); + } catch (NotFound $e) { + throw new Exception(Exception::COLLECTION_NOT_FOUND, null, null, $e); }Also applies to: 153-163
141-161: Optional: avoid repeated database fetches during recursion.Pass the database sequence (or the collection document) into recursive calls to reduce round-trips when expanding nested relationships.
If you decide to do it now, add an optional int $dbSeq = null param, compute it once, and pass it through recursive calls.
Also applies to: 188-199
src/Appwrite/Utopia/Request/Filters/V17.php (2)
71-74: Keep throwable as previous when wrapping to Appwrite exception.Preserves stack trace while still surfacing GENERAL_QUERY_INVALID.
- } catch (\Throwable $th) { - throw new Exception(Exception::GENERAL_QUERY_INVALID, $th->getMessage()); + } catch (\Throwable $th) { + throw new Exception(Exception::GENERAL_QUERY_INVALID, $th->getMessage(), null, $th); }
83-96: Optional: throw Appwrite\Extend\Exception from parseQuery or restrict its visibilityconvertOldQueries already catches any Throwable from parseQuery and rethrows Appwrite\Extend\Exception, so no immediate bug; parseQuery is public in src/Appwrite/Utopia/Request/Filters/V17.php — either make it private/protected or have it throw Appwrite\Extend\Exception directly to avoid leaking raw \Exception if reused.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Appwrite/Utopia/Request/Filters/V16.php(0 hunks)src/Appwrite/Utopia/Request/Filters/V17.php(9 hunks)src/Appwrite/Utopia/Request/Filters/V20.php(3 hunks)
💤 Files with no reviewable changes (1)
- src/Appwrite/Utopia/Request/Filters/V16.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-03T05:58:34.294Z
Learnt from: ItzNotABug
PR: appwrite/appwrite#9669
File: src/Appwrite/Utopia/Request/Filters/V19.php:56-71
Timestamp: 2025-05-03T05:58:34.294Z
Learning: According to the developer, wildcard select handling in V19 request filter is only needed for `listDocuments` operations, not for `getDocument` operations, despite the doc-block mentioning both.
Applied to files:
src/Appwrite/Utopia/Request/Filters/V20.php
🧬 Code graph analysis (2)
src/Appwrite/Utopia/Request/Filters/V20.php (2)
src/Appwrite/Extend/Exception.php (1)
Exception(7-447)src/Appwrite/Utopia/Request/Filter.php (1)
Filter(8-57)
src/Appwrite/Utopia/Request/Filters/V17.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-447)
🪛 GitHub Actions: Linter
src/Appwrite/Utopia/Request/Filters/V20.php
[error] 1-1: PSR-12: function_declaration – Function declaration missing visibility modifier.
🪛 PHPMD (2.15.0)
src/Appwrite/Utopia/Request/Filters/V17.php
312-312: Avoid unused parameters such as '$index'. (undefined)
(UnusedFormalParameter)
313-313: Avoid unused parameters such as '$filter'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Utopia/Request/Filters/V20.php (2)
59-61: LGTM — error mappings verifiedGENERAL_QUERY_INVALID → HTTP 400; DATABASE_NOT_FOUND and COLLECTION_NOT_FOUND → HTTP 404 (app/config/errors.php). V20 filter throws the correct exceptions (src/Appwrite/Utopia/Request/Filters/V20.php:59–61, 149–151, 161–163).
19-23: Scope wildcard/back-compat selects to listDocuments onlyLimit manageSelectQueries to 'databases.listDocuments' and update the docblock; confirm there are no callers relying on relationship expansion for 'databases.getDocument'.
- case 'databases.getDocument': - case 'databases.listDocuments': + case 'databases.listDocuments': $content = $this->manageSelectQueries($content); break;- * This filter preserves 1.7.x behavior by including all related documents for backward compatibility with - * `listDocuments` and `getDocument` calls. + * This filter preserves 1.7.x behavior by including all related documents for backward compatibility with + * `listDocuments` calls.src/Appwrite/Utopia/Request/Filters/V17.php (1)
1-1: Action: Re-run PSR-12 linter and scan for class methods missing visibilitySandbox checks failed; re-run the linter and run the local scan below to locate bare
functiondeclarations inside classes.#!/bin/bash set -euo pipefail # Run from repo root; excludes vendor. find . -type f -name '*.php' -not -path './vendor/*' -print0 | python3 - <<'PY' import sys,re from pathlib import Path pat_class = re.compile(r'\b(class|interface|trait)\b', re.IGNORECASE) pat_func = re.compile(r'\bfunction\b\s*([A-Za-z_]\w*)\s*\(') vis_re = re.compile(r'\b(public|protected|private)\b', re.IGNORECASE) data = sys.stdin.buffer.read() if not data: sys.exit(0) files = data.split(b'\x00') results = [] for bpath in files: if not bpath: continue path = bpath.decode('utf-8', errors='replace') try: text = Path(path).read_text(encoding='utf-8', errors='replace') except Exception: continue lines = text.splitlines() brace_level = 0 pending_class = False class_stack = [] last_class_name = None for lineno, line in enumerate(lines, start=1): if pat_class.search(line): pending_class = True m = re.search(r'\b(class|interface|trait)\s+([A-Za-z_]\w*)', line, re.IGNORECASE) if m: last_class_name = m.group(2) for ch in line: if ch == '{': brace_level += 1 if pending_class: class_stack.append((brace_level, last_class_name)) pending_class = False elif ch == '}': if class_stack and brace_level == class_stack[-1][0]: class_stack.pop() brace_level -= 1 if class_stack: m = pat_func.search(line) if m: func_pos = m.start() prefix = line[:func_pos] if '->' in prefix or '::' in prefix: continue if vis_re.search(prefix): continue if re.search(r'[\$\)=,]', prefix): continue clsname = class_stack[-1][1] or "<anonymous>" results.append(f"{path}:{lineno}: in class {clsname}: {line.strip()}") if not results: print("NO_MISSING_VISIBILITY") else: for r in results: print(r) PYIf the script reports matches, add an explicit visibility (public/protected/private) to each reported method and re-run the linter.
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