fix: stop internal keys stealing user attributes named after them - #961
fix: stop internal keys stealing user attributes named after them#961HarshMN2345 wants to merge 1 commit into
Conversation
decode() falls back to the adapter-filtered key when an attribute's value is null, and filter() strips the leading "$" off an internal key, so "$collection" resolves to "collection". find() stamps $collection after decode, unlike every other read path, so on a list read the fallback took the user's "collection" attribute, removed it from the document, and wrote its value into $collection. "$tenant" is reachable the same way on shared tables, where a select query leaves _tenant out of the projection. Guarding on the internal-attribute set rather than the "$" prefix keeps the alias fallback for relationship keys, which start with "$" when the related collection id does. Refs appwrite/appwrite#6944
836d5bf to
5d7b005
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesAttribute decoding
Priority: ➖ Normal — Schedule the database decoding fix because user attributes named `collection` or `tenant` can otherwise be consumed by internal keys, with regression coverage for both cases. Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change preserves user attributes named like internal database keys without altering relationship-key fallback behavior. Coverage includes the affected null-value and projected-query paths, with no current merge-blocking risk identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 prevents null internal metadata fields from consuming user attributes whose names match their filtered aliases.
Confidence Score: 5/5The PR appears safe to merge, with the decoder change narrowly addressing internal-key alias collisions and focused regressions covering the reachable failure paths. No actionable failures remain; internal metadata is mapped independently by adapters, symbol-prefixed relationship aliases retain their fallback behavior, and the new tests verify preservation of both non-null and explicit-null colliding user attributes. Important Files Changed
Reviews (1): Last reviewed commit: "fix: stop internal keys stealing user at..." | Re-trigger Greptile |
1 participant
What
Database::decode()falls back to the adapter-filtered key when an attribute's value is null:Adapter::filter()strips everything outside[A-Za-z0-9_-], so for an internal key it strips the leading$:filter('$collection') === 'collection', which is a name a user attribute is allowed to have. An internal value never reaches the document under that name — the adapters map their own_-prefixed columns back themselves — so the lookup has nothing of its own to find and can only take the user's attribute.Two keys are actually reachable with a null value at decode time:
$collection—find()stamps it after decode ($node->setAttribute('$collection', ...)), whilegetDocument(),createDocument(),createDocuments()andupsertDocuments()all stamp it before. So on a list read the fallback removed the user'scollectionattribute from the document and wrote its value into$collection;find()then overwrote$collectionwith the real id, and the user's value was simply gone. Where the value was null it was worse: theelseifalias cleanup deleted the key outright, so the attribute vanished from the response entirely.$tenant— with shared tables on,getAttributeProjection()does not project_tenant, so anyQuery::select(...)returns a row without it and$tenantis null at decode.validateSelections()does not list$tenanteither, so the stolen value was not even re-exposed under$tenant— it disappeared silently.$permissionsis safe (an earliercontinue), and$id/$sequence/$createdAt/$updatedAtare always projected and mapped back, so they are never null on either read path.Fix
Skip the alias fallback for internal keys.
The guard keys off the internal-attribute set rather than the
$prefix on purpose: a collection id may itself start with$, and the derived relationship attribute key then also starts with$and does need the fallback (its column is the filtered name). Guarding on the prefix breakstestManyToManyRelationshipKeyWithSymbols.Tests
Two regression tests in
DocumentTests, so every adapter scope picks them up:testFindAttributeNamedAfterInternalKey— an attribute namedcollection, with a non-null and a null row. The null row asserts onoffsetExists(), sincegetAttribute()cannot tell a dropped key from a null value.testFindAttributeNamedAfterTenantKey— the shared-tablestenantcase behind agetSharedTables()guard, read throughQuery::select(['tenant']), which is what drops_tenantfrom the projection.Both fail on
mainand pass here. Verified across all 16 adapter suites: MariaDB, MySQL, SQLite, Redis, Mirror and SharedTables/{MariaDB,MySQL,Redis,SQLite} are green; Postgres, SharedTables/Postgres, Memory and MongoDB carry only their existingtestObjectAttribute*/testIntegersBeyondInt32failures; Pool, Schemaless/MongoDB and SharedTables/MongoDB fail wholesale on cleanmainlocally with the same counts, so they are unchanged by this.Refs appwrite/appwrite#6944 — Appwrite needs a dependency bump before that issue closes.
Summary by CodeRabbit
Bug Fixes
collectionortenantare preserved correctly.Tests