feat: preserve key order in decode-modify-encode workflow#60
Merged
Conversation
- Add _keys and _values to INTERNAL_KEYS - Implement ordered materialization in LazyObject.__newindex - Add materialized state check in read_object_field - Update encode_lazy_object_walking to use _keys order - Update LazyObject.__pairs for ordered iteration - Add comprehensive tests for ordered encode scenarios Closes #59
📝 WalkthroughWalkthroughPreserves JSON key order across decode-modify-encode by adding ORDER_KEYS/ORDER_VALUES caches and a per-view CHILD_CACHE, updating read, iteration, mutation, materialize, and JSON-encode paths to prefer cached ordering and values while preserving lazy proxies. ChangesOrdered key preservation in lazy objects
sequenceDiagram
participant Decoder
participant LazyObject
participant ReadPath as "__index / __pairs"
participant MutationPath as "__newindex"
participant EncodePath as "encode_lazy_object_walking"
Decoder->>LazyObject: decode JSON to lazy proxy
ReadPath->>LazyObject: read field before mutation (cursor)
LazyObject-->>ReadPath: resolve via cursor (or cached child)
MutationPath->>LazyObject: first mutation triggers ensure_object_order_state
LazyObject->>LazyObject: build ORDER_KEYS and ORDER_VALUES from cursor
LazyObject->>LazyObject: apply mutation to ORDER_KEYS/ORDER_VALUES
ReadPath->>LazyObject: read field after mutation
LazyObject-->>ReadPath: return from ORDER_VALUES
EncodePath->>LazyObject: encode output
LazyObject->>LazyObject: iterate ORDER_KEYS and emit values from ORDER_VALUES
LazyObject-->>EncodePath: JSON output with preserved key order
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
The new ordered encode implementation preserves LazyObject metatable after modification instead of converting to plain table. Update tests to reflect this intentional behavior change.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/lua/ordered_encode_spec.lua (1)
52-57: ⚡ Quick winCover the cached-child read-before-modify path too.
This only exercises a scalar read. The fragile case here is
local child = t.nested, then a parent write, then mutatingchildthrough that saved reference. Adding that regression test would catch the current lost-update path inLazyObject.__newindex.Suggested test case
it("read before modify works correctly", function() local t = qjson.decode('{"a":1,"b":2,"c":3}') local _ = t.b -- read first t.b = 999 assert.are.equal('{"a":1,"b":999,"c":3}', qjson.encode(t)) end) + + it("keeps a pre-read nested proxy wired after parent materialization", function() + local t = qjson.decode('{"nested":{"a":1}}') + local nested = t.nested + t.extra = 1 + nested.a = 2 + assert.are.equal('{"nested":{"a":2},"extra":1}', qjson.encode(t)) + end)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/lua/ordered_encode_spec.lua` around lines 52 - 57, Add a test that exercises the cached-child read-before-modify path: decode an object with a nested table into t (using qjson.decode), read and store the nested child (e.g., local child = t.nested), then mutate the parent (e.g., t.nested = <new table> or change parent keys) and mutate the saved child (e.g., child.x = ...), and finally assert qjson.encode(t) reflects the mutation; this targets the LazyObject.__newindex lost-update path and should use the same variable names (t, child, nested) and qjson.encode/qjson.decode so the test fails if cached-child updates are lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lua/qjson/table.lua`:
- Around line 299-315: The materialize() logic currently rebuilds the object
solely from the cursor (`t._cur`) and ignores any prior mutations stored in
`_keys`/`_values`, so after `t.a = 2` `qjson.materialize(t)` can return stale
data; change materialize() (the function that uses
`qjson_cursor_object_entry_at`, `decode_cursor`, `rawget`, `rawset`) to first
check `local keys = rawget(t, "_keys")` and if present iterate `for _, key in
ipairs(keys) do result[key] = values[key]` (honoring nil to represent deletions)
and return that materialized table, otherwise fall back to the existing cursor
loop that populates `_keys` and `_values` as it currently does. Ensure the code
references `LazyObject`’s `_keys`/`_values` storage and does not re-read the
cursor when `_keys` exists so mutations are preserved.
- Around line 299-315: During first materialization (in the loop that populates
_keys/_values) reuse any already-cached child proxy instead of always creating a
fresh one: for each key, check for an existing child proxy (e.g. via rawget(t,
key) or t[key]) and if present assign that proxy into values[key] and update its
contents from the cursor (or call a decode-into helper) rather than calling
decode_cursor to produce a new table; otherwise fall back to decode_cursor as
now. This ensures later mutations on the previously-read proxy are preserved
when encode_lazy_object_walking() reads _values[k].
---
Nitpick comments:
In `@tests/lua/ordered_encode_spec.lua`:
- Around line 52-57: Add a test that exercises the cached-child
read-before-modify path: decode an object with a nested table into t (using
qjson.decode), read and store the nested child (e.g., local child = t.nested),
then mutate the parent (e.g., t.nested = <new table> or change parent keys) and
mutate the saved child (e.g., child.x = ...), and finally assert qjson.encode(t)
reflects the mutation; this targets the LazyObject.__newindex lost-update path
and should use the same variable names (t, child, nested) and
qjson.encode/qjson.decode so the test fails if cached-child updates are lost.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0d93637-eeab-4649-9e0f-8d200076e489
📒 Files selected for processing (2)
lua/qjson/table.luatests/lua/ordered_encode_spec.lua
- __newindex now checks for cached child proxies before decoding - materialize() uses _keys/_values when object is already materialized - Add tests for cached nested mutations and materialize on modified objects Addresses CodeRabbit review feedback.
membphis
commented
May 28, 2026
Collaborator
Author
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
结论
暂不建议合并。
PR 目标清楚，方向也合理，但当前实现会在部分 decode-modify-encode 流程中静默丢失子对象修改，并且让 qjson.materialize 对已修改 LazyObject 返回旧数据。这两个都是用户可见正确性问题，需要先修。
阻塞项
[P1] 已缓存子对象的后续修改会被 encode 静默丢弃
- 问题：
LazyObject首次写入时_values通过decode_cursor重新创建子对象，没有复用之前 rawset 缓存过的子 proxy；之后 encode 只读取_values，不读取 raw 缓存。 - 为什么阻塞：decode-modify-encode 会静默输出旧值，属于核心功能正确性回退。
- 影响范围：调用方先读取子对象，再修改父对象触发 ordered materialization，然后继续修改之前拿到的子对象引用。
- 触发条件：
local qjson = require("qjson")
local t = qjson.decode('{"a":{"x":1},"b":2}')
local inner = t.a
t.c = 3
inner.x = 99
print(qjson.encode(t))
-- actual: {"a":{"x":1},"b":2,"c":3}
-- expected: {"a":{"x":99},"b":2,"c":3}- 证据位置：
lua/qjson/table.lua中首次物化直接decode_cursor填充_values；encode_lazy_object_walking对 materialized object 只按_values输出。 - 建议修复：首次建立
_values时合并已有非内部 raw 缓存，优先使用已缓存子 proxy；或迁移/清理 raw 用户 key，让读取、写入、encode 始终只有一个权威数据源。补充该触发序列的回归测试。
[P2] qjson.materialize 对已修改 LazyObject 会丢失修改
- 问题：PR 保留
LazyObjectmetatable 后，qjson.materialize仍调用materialize_object_contents从原始 cursor 读取内容，完全忽略_keys/_values。 - 为什么阻塞：
qjson.materialize是公开 API，修改后的对象 materialize 出旧数据会误导调用方。 - 影响范围：所有先修改
LazyObject再调用qjson.materialize的对象，包括新增、删除、更新字段。 - 触发条件：
local qjson = require("qjson")
local t = qjson.decode('{"a":1}')
t.b = 2
local m = qjson.materialize(t)
print(m.a, m.b, qjson.encode(t))
-- actual: 1 nil {"a":1,"b":2}- 证据位置：
materialize的LazyObject路径仍只走materialize_object_contents，而该 cursor walker 不读取_values。 - 建议修复：
materialize遇到带_keys/_values的LazyObject时按_keys遍历_values并递归 materialize；同时覆盖新增、删除、更新和嵌套修改测试。
其他检查
- 新增/更新依赖：无。PR 仅涉及
lua/qjson/table.lua、tests/lua/lazy_table_spec.lua、tests/lua/ordered_encode_spec.lua，没有 manifest、lockfile、rockspec 或 Cargo 依赖声明变更。 - Breaking change：首次写入后的
LazyObject从 plain table 改为继续保留 lazy metatable，这是可观察行为变化；更关键的是当前实现引入了上面的正确性回退。 - 重复实现风险：
__newindex新增了一套对象物化循环，和既有materialize_object_contents/旧缓存保留逻辑分叉，直接导致缓存 proxy 未被合并。建议把“按顺序物化并合并缓存”的逻辑集中成一个内部 helper，供 encode/materialize/write 路径共享。
membphis
commented
May 28, 2026
Collaborator
Author
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
阻塞项
[P1] JSON 字段 _keys / _values 为容器时会和新增内部状态撞名
- 问题：PR 新增
rawget(self, "_values")作为 materialized 状态判断，并用rawget(t, "_keys")作为 key-order 存储。但read_object_field会把用户 JSON 容器字段 rawset 到同一个 lazy object 上；如果 payload 本身含有_keys或_values容器字段，用户字段会被误当成内部状态。 - 为什么阻塞：这是合法 JSON key，且
_keys/_values在实际业务 payload 中并不罕见。当前行为会导致读取错误或写入崩溃，属于用户可见正确性回退。 - 影响范围：读取过
_values容器字段后，后续普通字段读取会从该_values子对象里查；读取过_keys容器字段后，后续对父对象写入会把该用户字段误当内部 key order。 - 触发条件：
local t = qjson.decode('{"_keys":["user"],"a":1}')
local user_keys = t._keys
t.b = 2
-- lua/qjson/table.lua:334: attempt to index local 'values' (a nil value)local t = qjson.decode('{"_values":{"z":9},"a":1}')
local user_values = t._values
print(t.a)
-- actual: nil
-- expected: 1- 证据位置：
lua/qjson/table.lua中read_object_field用 raw_values判断 materialized 状态；LazyObject.__newindex用 raw_keys判断是否已建立 key order；同时read_object_field会 rawset 缓存用户容器字段。 - 建议修复：不要用用户字符串 key 存储新增内部状态。可以改为弱表 side table，或用本地 sentinel table/cdata 作为 raw key，例如
ORDER_KEYS = {}、ORDER_VALUES = {}，避免和任意 JSON string key 冲突。补充_keys/_values作为用户字段的读、写、encode 回归测试。
Replace string keys '_keys' and '_values' with sentinel table keys ORDER_KEYS and ORDER_VALUES. This prevents collision when user JSON contains fields named '_keys' or '_values'. Add regression tests for _keys and _values as user JSON fields.
membphis
commented
May 28, 2026
Collaborator
Author
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
阻塞项
[P1] 父对象物化后整体替换/删除已缓存子容器字段会被 encode 忽略
- 问题：
read_object_field会把子容器 rawset 到父 lazy object；父对象第一次写入时又把该 cached proxy 放进ORDER_VALUES。之后如果直接给这个已存在 raw key 赋新值或nil，Lua 会绕过LazyObject.__newindex，只改 raw 表项，不会同步ORDER_VALUES，最终 encode 仍输出旧子对象。 - 为什么阻塞：这是 decode-modify-encode 的静默数据错误。对比
main，同样在父对象物化后替换已缓存子字段，main会输出替换后的值；最新 PR 会输出旧对象。 - 影响范围：调用方先读取某个子 object/array，再修改父对象触发 materialization，然后整体替换或删除这个子字段。
- 触发条件：
local qjson = require("qjson")
local t = qjson.decode('{"a":{"x":1},"b":2}')
local old = t.a
t.c = 3
t.a = "replaced"
print(qjson.encode(t))
-- actual: {"a":{"x":1},"b":2,"c":3}
-- expected: {"a":"replaced","b":2,"c":3}删除也同样失败：
local qjson = require("qjson")
local t = qjson.decode('{"a":{"x":1},"b":2}')
local old = t.a
t.c = 3
t.a = nil
print(qjson.encode(t))
-- actual still includes "a":{"x":1}- 证据位置：
lua/qjson/table.lua中read_object_fieldrawset 缓存子容器；首次物化把 cached proxy 存入ORDER_VALUES；encode_lazy_object_walking对 materialized object 只读取ORDER_VALUES。 - 建议修复：父对象建立
ORDER_VALUES后，不要继续保留可被普通赋值绕过__newindex的 raw 用户字段；可以把 cached proxy 迁移到ORDER_VALUES后清掉 raw string key，让后续t.a = ...进入__newindex。补充替换和删除已缓存子容器字段的回归测试。
Collaborator
Author
|
Follow-up from re-review: the previous fix only covered replacing/deleting a cached child after the parent had already materialized via another write. The remaining root cause is that |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/lua/ordered_encode_spec.lua (1)
130-153: 💤 Low valueConsider verifying full value preservation in collision tests.
The collision safety tests correctly verify that user fields
_keysand_valuesdon't collide with internal bookkeeping. However, lines 138 and 150 only check that the field names appear in the encoded output (out:find('"_keys"')andout:find('"_values"')), without verifying that the nested values are correctly preserved.For more thorough coverage, consider decoding the output and asserting the full structure, e.g.:
local parsed = require("cjson").decode(out) assert.are.same({"user"}, parsed._keys) -- for line 138 assert.are.equal(9, parsed._values.z) -- for line 150That said, the current tests are sufficient as regression tests for the collision fix and align with the focused goal of ensuring no key loss.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/lua/ordered_encode_spec.lua` around lines 130 - 153, The tests for collision safety should also assert that the user-provided nested values survive round-trip encoding; after calling qjson.encode(t) (variable out) decode the encoded string (e.g., into parsed) and assert that parsed._keys equals the original {"user"} for the _keys test and parsed._values.z equals 9 for the _values test, leaving the existing name-presence checks intact; modify the two specs that create t, set t.b, and produce out to add these decode-and-assert steps to fully verify value preservation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/lua/ordered_encode_spec.lua`:
- Around line 130-153: The tests for collision safety should also assert that
the user-provided nested values survive round-trip encoding; after calling
qjson.encode(t) (variable out) decode the encoded string (e.g., into parsed) and
assert that parsed._keys equals the original {"user"} for the _keys test and
parsed._values.z equals 9 for the _values test, leaving the existing
name-presence checks intact; modify the two specs that create t, set t.b, and
produce out to add these decode-and-assert steps to fully verify value
preservation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f976046-0eaa-457a-b918-a5921a09b597
📒 Files selected for processing (3)
lua/qjson/table.luatests/lua/lazy_table_spec.luatests/lua/ordered_encode_spec.lua
🚧 Files skipped from review as they are similar to previous changes (1)
- lua/qjson/table.lua
The old comment described the now-removed behavior of stripping the LazyObject metatable on first write. Rewrite to describe the current approach: building ORDER_KEYS/ORDER_VALUES under sentinel keys while keeping the metatable alive. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
Quick re-check on
So from merge-readiness perspective: one old bug, plus one old bug amplified into a regression in this branch. I still consider the latter a blocker for this PR. |
Restores the pre-PR behavior: writing a non-string key to a lazy object now raises 'object key must be a string' immediately, instead of silently inserting the key into ORDER_KEYS and producing an inconsistent state (unreadable via __index, broken qjson.len, crash in encode_string).
When pairs() iterates an unmodified lazy object, check CHILD_CACHE before calling decode_cursor so mutations on previously accessed child containers are visible to the caller.
ensure_object_order_state always returns a non-nil order table, so the post-materialization branch in encode_lazy_object_walking made the old cursor-walk path unreachable; remove it and flatten the now-constant guard. INTERNAL_KEYS was only referenced from that dead branch, so drop it and its stale comment. No behavior change; 156/156 Lua tests pass.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lua/qjson/table.lua (1)
591-596: 💤 Low valueConsider aligning nil-value handling with
materialize().
materialize()at line 494 asserts thatvalues[k] ~= nilfor any key inORDER_KEYS, treating a nil value as an invariant violation. Here,encode_lazy_object_walkingsilently skips nil values. If the invariant holds, thev ~= nilcheck is redundant; if it doesn't, the silent skip may mask a bug thatmaterialize()would catch.Either remove the check (relying on the invariant) or add an assertion here for consistency. Given that deletions remove keys from
ORDER_KEYS, the check is technically unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/qjson/table.lua` around lines 591 - 596, The loop in encode_lazy_object_walking silently skips nil values which is inconsistent with materialize()'s invariant (materialize asserts values[k] ~= nil for keys in ORDER_KEYS); update encode_lazy_object_walking to enforce the same invariant by replacing the silent nil-check with an assertion (e.g., assert(v ~= nil, "ORDER_KEYS invariant violated for key "..tostring(k))) so missing values surface as errors; reference encode_lazy_object_walking, materialize, ORDER_KEYS, encode_string, and encode when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lua/qjson/table.lua`:
- Around line 591-596: The loop in encode_lazy_object_walking silently skips nil
values which is inconsistent with materialize()'s invariant (materialize asserts
values[k] ~= nil for keys in ORDER_KEYS); update encode_lazy_object_walking to
enforce the same invariant by replacing the silent nil-check with an assertion
(e.g., assert(v ~= nil, "ORDER_KEYS invariant violated for key "..tostring(k)))
so missing values surface as errors; reference encode_lazy_object_walking,
materialize, ORDER_KEYS, encode_string, and encode when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fc7a909-6334-447e-ae8b-821be261f183
📒 Files selected for processing (2)
lua/qjson/table.luatests/lua/ordered_encode_spec.lua
This was referenced May 29, 2026
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enhance qjson's lazy table API to preserve original JSON key order when encoding after modifications. This enables API gateway use cases where JSON payloads are decoded, lightly modified, and re-encoded while maintaining key order consistency.
Changes
_keysand_valuestoINTERNAL_KEYSLazyObject.__newindexread_object_fieldencode_lazy_object_walkingto use_keysorderLazyObject.__pairsfor ordered iterationSupported Scenarios
t.key = nilto remove a key from output; remaining keys preserve orderExample
Closes #59
Closes #61
Summary by CodeRabbit
Bug Fixes
Tests