fix: refuse a non-string select value instead of fatalling on it - #931
Conversation
A nested-array value on `select` reached str_contains() and raised a TypeError. A TypeError is an Error, not an Exception, so it escaped the QueryException catch in every caller and surfaced as a 500 — where the same malformation on `equal`, `limit` and every other method returns a typed refusal, because their payload either lives on Query's typed string $attribute or never meets a string function. select was the only method whose values array is consumed as a string without a type check; a sweep of all 49 methods confirms it was the only fatal. The check runs before the duplicate check on purpose: array_unique() casts every array to "Array", so two nested values collapsed into one and reported a duplicate that was not there, masking the real error. The two downstream sites in Database.php are guarded too. They are unreachable while the validator refuses first, but live whenever validation is skipped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughSelection handling now rejects non-string values with typed ChangesSelection validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR converts malformed non-string
Confidence Score: 5/5The PR appears safe to merge, with malformed selections consistently rejected before reaching string operations or adapters. The new checks enforce the existing string-only selection contract, and the downstream query flow rejects malformed values with the established query exception while preserving valid projections. Important Files Changed
Reviews (1): Last reviewed commit: "fix: refuse a non-string select value in..." | Re-trigger Greptile |
Fixes Appwrite DAT-2222.
The defect
A nested-array value on
selectreturns 500 Server Error. Every other query method answers the same malformation with a typed 400.{"method":"select","values":["sku"]}{"method":"select","values":[["sku"]]}Attribute selection must be a string, got array{"method":"select","values":[["*"]]}{"method":"equal","attribute":"sku","values":[["x"]]}{"method":"limit","values":[[5]]}Select::isValid()fed avalueselement straight intostr_contains(). ATypeErroris anError, not anException, so it escaped theQueryExceptioncatch in every caller and came out as an unhandled 500.Why
selectwas the only oneStructural, not accidental:
FilterandOrderread$query->getAttribute(), backed byprotected string $attributeonQuery— it can never be an array.LimitandOffsetreadgetValue()into aNumericvalidator, which does no string operation.CursorreadsgetValue()throughUID.Selectis the only method whose payload lives in the untypedvaluesarray and is consumed as a string.Query::parseQuery()validates thatvaluesis an array but never its element types, andgetValues()is honestly typedarray<mixed>— the callers were not.Sweep of the other query methods
Asked for by the issue, and answered by execution rather than reading: every method constant on
Querywas driven throughValidator\Queries\Documentswith the same nested-array value, before and after.Before — 1 fatal of 49:
After — 0 fatals of 49:
The remaining 48 already refused cleanly and are unchanged. Five (
exists,notExists,orderAsc,orderDesc,orderRandom) accept the query because they ignorevaluesentirely, which is correct.The fix
Select::isValid()rejects a non-string element with a typed message before any string operation.array_unique()casts every array to the string"Array", so[["a"],["b"]]collapsed to one element and reportedDuplicate attributes selected— a wrong answer that masked the real error. That is also why this looked intermittent: only value sets surviving the stringified dedupe reached the fatal.str_contains()sites inDatabase.php(validateSelections(),processRelationshipQueries()) are guarded too. They are unreachable while the validator refuses first, but live whenever validation is skipped (skipValidation(), internal and worker calls).[1]and[null]now report the type rather thanAttribute not found in schema: 1and a pair of PHP deprecations.Regression tests, seen red
Two levels.
tests/unit/SelectProjectionTest.phpdrivesDatabase::find()— the entry point the HTTP layer calls — not the validator alone.Red — fix reverted
Validator level, same revert:
Note error 3 lands on line 71, the
array_unique()duplicate check — the reason the type check has to precede it.Green
Coverage: nested array, nested wildcard, mixed flat+nested, two nested values, assoc array, int, null; that the refusal is catchable as an
Exception(aTypeErroris not, which is the whole defect); and that the legitimate flat form still projects, the wildcard still projects, and an unknown attribute is still refused by schema rather than swallowed by the new check.The
testTwoNestedSelectionsReportTheTypeNotAFalseDuplicatecase is built withQuery::parse()from JSON rather thanQuery::select(), because that is the path a hand-written HTTP client takes and the only one that can carry a value the constructor'sarray<string>type would reject.Verification
composer lint(Pint) — passedcomposer check(PHPStan level 7,src+tests) — no errorscomposer test --testsuite unit— 428 tests, 2309 assertions, OKThe
e2esuite needs MySQL/Postgres/Mongo/Redis via docker-compose and was not run locally; the change is adapter-independent (validator plus two guards inDatabase.php), and the new tests use the Memory adapter so they run in the unit suite everywhere.Blast radius for consumers
Queries\Base::isSelectQueryAllowed()wiresSelectwherever it is true, so in Appwrite the identical 500 is reachable onlistProjectsandlistDeployments, not only on documents/rows.Consumers need a release:
appwrite/cloudlocksutopia-php/databaseat 7.1.0 and resolves it throughappwrite/server-ce's^7.0.0, so a 7.1.1 patch tag flows to it with a plaincomposer update utopia-php/database.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests