fix: coerce empty maps structurally instead of corrupting string payloads#25
Conversation
…oads Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Greptile SummaryThis PR fixes a production bug where
Confidence Score: 4/5Safe to merge — the structural coercion correctly replaces the string-manipulation approach and all three previous exemptions are preserved. The implementation is sound: string scalars inside the attribute tree are never touched, empty map fields still serialize to No files require special attention; both changed files look correct. Important Files Changed
Reviews (1): Last reviewed commit: "fix: coerce empty maps structurally inst..." | Re-trigger Greptile |
| */ | ||
| protected function coerceEmptyArraysToObjects(array $attributes): array | ||
| { | ||
| $emptyArrayLists = ['allowedTopologies', 'mountOptions', 'accessModes']; |
There was a problem hiding this comment.
The
$emptyArrayLists array literal is allocated fresh on every recursive invocation. For a deeply nested resource structure this creates unnecessary allocations on each stack frame. Declaring it as a static local avoids the per-call cost and makes the intent clearer.
| $emptyArrayLists = ['allowedTopologies', 'mountOptions', 'accessModes']; | |
| static $emptyArrayLists = ['allowedTopologies', 'mountOptions', 'accessModes']; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
The bug
K8sResource::toJsonPayload()coerced empty PHP arrays ([]) into empty JSON objects ({}) for Kubernetes map fields using a blind global string replace over the entire JSON-encoded payload:Because this operates on the raw JSON string, it also rewrites the
: []byte sequence wherever it appears inside string values — not just at structural map positions. Containercommandarrays that embed inline scripts are corrupted.Repro
The PHP expression
glob(...) ?: []json-encodes as?: []inside the command string and gets rewritten to?: {}, which is a syntax error (unexpected token "{").The fix
Replace the blind string replace with a structure-aware coercion. Instead of rewriting the encoded string, walk the resource attribute tree before encoding and convert empty-array nodes to empty objects. String scalars are leaves in the tree and are left byte-identical, so embedded scripts are never touched:
The recursive walk preserves the exact prior semantics: every empty array in the structure becomes
{}, with the same three genuine list fields (allowedTopologies,mountOptions,accessModes) exempted by key name at any depth — previously handled by follow-upstr_replacecalls that re-inverted them. The public contract oftoJsonPayload()is unchanged.Impact on Appwrite Edge
Appwrite Edge provisions dedicated-database backup Jobs whose containers run inline
php -rmaintenance scripts containingglob(...) ?: []. With the old coercion, every such Job was serialized with corrupted command strings and crash-looped on startup withParse error: syntax error, unexpected token "{", breaking dedicated-database backups in production. This fix makes those Jobs serialize correctly while preserving all existing empty-map behavior.Tests
Added
tests/JsonPayloadTest.php:?: [](and: []generally) serialize byte-identical — fails on the old code, passes nowdata/labelsmaps still coerce to{}[]speccoerce to{}The two string-corruption tests fail against the unfixed method and pass with the fix; the full suite shows no new failures (remaining errors are pre-existing and require a live cluster /
ext-yaml).🤖 Generated with Claude Code