Skip to content

fix(json): preserve string map keys - #820

Open
nnunley wants to merge 1 commit into
nooga:mainfrom
nnunley:fix/json-string-map-keys
Open

fix(json): preserve string map keys#820
nnunley wants to merge 1 commit into
nooga:mainfrom
nnunley:fix/json-string-map-keys

Conversation

@nnunley

@nnunley nnunley commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fix json/write-json corrupting VM string map keys by serializing their underlying text instead of their readable EDN representation.

  • preserve raw string keys at every nesting level
  • retain existing keyword-key spelling and fallback behavior for other key types
  • cover plain, nested, empty, quote-escaped, Unicode, and slash-containing keys

Closes #817.

Reproduction

Before:

(json/write-json {"key" "value"})
;; => {"\"key\"":"value"}

After:

(json/write-json {"key" "value"})
;; => {"key":"value"}

Validation

  • go test ./test -run 'TestRunner/json_test\.lg$' -count=1
  • go test -race ./test -run 'TestRunner/json_test\.lg$' -count=1
  • go test ./pkg/rt -count=1
  • go vet ./pkg/rt ./test
  • make generate
  • make check-generated
  • git diff --check
  • independent code review: PASS, no findings

A broader go test ./pkg/rt ./test -count=1 pass before the final mechanical restack had pkg/rt green; the test package was blocked only by this sandbox denying the suite's existing literal /tmp writes. The focused JSON suite is green before and after the restack; CI remains the unrestricted full-suite gate.

@nnunley
nnunley requested review from mparrett and nooga September 7, 2026 18:13

@mparrett mparrett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one P2 correctness issue, commented inline. Focused JSON and race tests pass; vet and generated-artifact validation also pass.

Comment thread pkg/rt/json.go
default:
nk = k.String()
}
r[nk] = vv

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Reject collisions after coercing map keys

Now that both "x" and :x become "x", a valid VM map containing both silently drops one entry here because r is a Go map[string]any. This is particularly easy to hit in the advertised read-modify-write flow: (assoc (read-json "{\"x\":1}") :x 2) retains two VM keys, but write-json emits only {"x":2}. Please detect a previously emitted nk (and return an error, or otherwise define a deterministic policy) rather than silently losing data.

@mparrett mparrett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for the unresolved P2 inline finding: key coercion can silently collapse distinct VM map entries (for example, string "x" and keyword :x) into one JSON object key.

@nooga nooga left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string-key fix is correct and minimal. Verified locally: the new tests fail on main and pass here; go test ./pkg/rt, go vet, and make check-generated are all clean; and the generated.manifest hash matches the new json.go sha256. The pods JSON encoder (JSONEncodeArgs in transit.go) and js/emit share fromMapValue and pick up the fix with no dependency on the old quoted form.

+1 to @mparrett's P2. The silent drop on a "x" / :x collision is a Go-map artifact rather than JSON semantics (data.json and cheshire would emit duplicate keys instead), and it is a few lines to detect in this loop. Note it was already reachable for other type pairs ({1 1 "1" 2}, {:x 1 'x 2}), so this PR widens the path rather than creating the class; it could equally land as a follow-up if you prefer to keep this PR to the issue's scope.

One small pre-existing thing in the same switch: the default branch still emits reader syntax for the remaining key types, so (write-json {\a 1}) gives {"\\a":1} and a nil key gives "nil". Routing vm.Char through string(rune(k)) here would be a one-liner if you want to sweep it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

json/write-json adds literal quotes to string map keys

3 participants