fix(sync): handle non-string YAML map keys in config parsing#1990
fix(sync): handle non-string YAML map keys in config parsing#1990anxkhn wants to merge 1 commit into
Conversation
YAMLToJSON unmarshaled YAML into map[string]interface{} with gopkg.in/yaml.v3
and then json.Marshal'd the result. yaml.v3 only forces string keys for the
top-level map; any nested mapping with a non-string key (e.g. an unquoted
numeric key in an object variant or under metadata) is decoded as
map[interface{}]interface{}, which encoding/json cannot marshal. The whole
config was then rejected with a cryptic internal error:
"json: unsupported type: map[interface {}]interface {}".
This is reachable from every YAML sync source (file, http, blob), all of
which route through ConvertToJSON -> YAMLToJSON.
Recursively convert nested map[interface{}]interface{} into
map[string]interface{} (stringifying the keys) before marshaling. yaml.v3 is
kept as the parser, so YAML 1.2 value semantics are unchanged: unquoted
on/off/yes/no/NO stay strings, timestamps keep their normalization, and
duplicate/top-level-scalar inputs still error exactly as before. The change is
purely additive: configs that already loaded are unaffected, and configs with
nested non-string keys now load instead of failing the entire file. No
dependency change.
Add regression tests: a nested numeric object-variant key now converts to
stringified JSON keys, and unquoted boolean-like keys and values still parse as
strings.
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
dosubot
Bot
added
the
size:M
📝 WalkthroughWalkthroughThe ChangesYAML Key Conversion Fix
Estimated code review effort: 2 (Simple) | ~10 minutes Related Issues: None specified. Related PRs: None specified. Suggested labels: bug, core Suggested reviewers: None specified. Poem A rabbit hopped through YAML's maze, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/pkg/utils/yaml.go (1)
29-56: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSolid fix; correctly targets yaml.v3's non-string-key mapping behavior.
The recursive
convertMapKeyscorrectly handles the documentedgopkg.in/yaml.v3behavior where mappings containing non-string keys are decoded asmap[interface{}]interface{}(as opposed tomap[string]interface{}when all keys are strings), avoiding thejson: unsupported typefailure described in the PR.One edge case worth being aware of: stringifying keys via
fmt.Sprintf("%v", key)(line 45) can cause silent key collisions if a mapping mixes keys that stringify identically (e.g., an int key1alongside a string key"1"in the same YAML mapping) — one entry would silently overwrite the other rather than surfacing an error. This is unlikely in practice given typical config shapes, but if strict correctness matters here, consider detecting/rejecting collisions rather than silently overwriting.🤖 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 `@core/pkg/utils/yaml.go` around lines 29 - 56, The recursive key conversion in convertMapKeys currently stringifies non-string YAML mapping keys with fmt.Sprintf, which can silently overwrite entries when different keys collapse to the same string. Update convertMapKeys to detect and reject key collisions (or otherwise surface an error) while preserving the existing recursive handling of map[interface{}]interface{} and []interface{} values so yaml.v3 inputs are converted safely for json.Marshal.
🤖 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 `@core/pkg/utils/yaml.go`:
- Around line 29-56: The recursive key conversion in convertMapKeys currently
stringifies non-string YAML mapping keys with fmt.Sprintf, which can silently
overwrite entries when different keys collapse to the same string. Update
convertMapKeys to detect and reject key collisions (or otherwise surface an
error) while preserving the existing recursive handling of
map[interface{}]interface{} and []interface{} values so yaml.v3 inputs are
converted safely for json.Marshal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef4b3b3d-51eb-49d7-ac10-9bda236e8b04
📒 Files selected for processing (2)
core/pkg/utils/yaml.gocore/pkg/utils/yaml_test.go
1 participant
What
A YAML flag config that contains a nested mapping with a non-string key (for
example an unquoted numeric key inside an object variant, or under
metadata)currently fails to load entirely, and the whole file is rejected with a cryptic
internal error:
This affects every YAML sync source (
file,http,blob), all of which routethrough
utils.ConvertToJSON->utils.YAMLToJSON.Reproduction
A minimal, realistic flag definition that uses an object variant with numeric
keys is enough to trigger it:
Loading this config (via a
file:,http:, orblob:sync) fails the entirefile rather than just that flag, with the
map[interface {}]interface {}errorabove.
Root cause
YAMLToJSONdecodes the document intomap[string]interface{}withgopkg.in/yaml.v3and thenjson.Marshals the result. yaml.v3 only forcesstring keys for the top-level map (because the decode target type does).
Any nested mapping with a non-string key is decoded as
map[interface{}]interface{}, whichencoding/jsoncannot marshal, so themarshal step fails and the whole config is rejected.
Fix
Recursively convert nested
map[interface{}]interface{}intomap[string]interface{}(stringifying the keys) before marshaling. yaml.v3 iskept as the parser, so YAML 1.2 value semantics are unchanged:
on/off/yes/no/NOstill parse as strings (no "Norway problem"),The change is purely additive: configs that already loaded are unaffected, and
configs with nested non-string keys now load instead of failing. No dependency
change.
Tests
Adds two table cases to
core/pkg/utils/yaml_test.go:nested non-string map keys- the object-variant config above now converts toJSON with stringified keys (
"1","2"); red before this change, green after.unquoted boolean-like keys and values stay strings- pins YAML 1.2 valuesemantics so a future parser swap that reintroduced boolean coercion would be
caught. Passes on
maintoo.go test -race -covermode=atomic -cover -short ./pkg/utils/...passes (97.2%coverage);
golangci-lint(v2.7.2) is clean;go mod tidyleaves go.mod/go.sumunchanged.