fix(parser): write numeric/bool values unquoted for newly created config files#194
fix(parser): write numeric/bool values unquoted for newly created config files#194thenull99 wants to merge 3 commits into
Conversation
…fig files When a server is first created its configuration file is often generated from scratch, so the target key does not exist yet. setValueWithSjson previously inferred a value's type solely by mirroring the type already present in the document, which meant a freshly created key had no type to mirror and every value fell back to a quoted string. Numeric values such as ports were therefore written as `server-port: "25565"` instead of `server-port: 25565` in YAML (and quoted in JSON/TOML). Changes: - Infer the natural type from the value itself when the key does not exist yet, writing clean JSON numbers and booleans unquoted. - Honour an explicitly declared numeric type from the egg definition (previously only an explicit boolean type was honoured). - Replace the lax gjson.Parse number check with a strict canonical JSON-number match so values like "007", "+5", "1.20.1" or "25565x" stay strings instead of being coerced (which could also have produced invalid JSON via SetRaw). Existing string-typed keys are intentionally left untouched, so values like numeric passwords in an existing template are not coerced to numbers.
|
Warning Review limit reached
More reviews will be available in 4 minutes and 18 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthrough
ChangesJSON Literal Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
parser/helpers.go (2)
173-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated boolean parse/fallback logic.
The explicit-boolean case (Lines 176-181) and the mirror-existing-boolean case (Lines 193-198) are identical. Consider extracting a small helper to keep them in sync.
♻️ Sketch
func (cfr *ConfigurationFileReplacement) boolOrStringFallback(jsonStr, path, value string) (any, *string, error) { v, err := strconv.ParseBool(value) if err != nil { log.WithFields(log.Fields{"value": value, "path": path, "match": cfr.Match}). Warn("cannot parse replacement as boolean, falling back to string value") s, e := sjson.Set(jsonStr, path, value) return nil, &s, e } return v, nil, nil }🤖 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 `@parser/helpers.go` around lines 173 - 216, The explicit boolean branch and the existing-boolean mirror branch in the replacement logic duplicate the same parse-and-fallback behavior. Extract that shared `strconv.ParseBool`/warning/`sjson.Set` flow into a small helper on `ConfigurationFileReplacement` (or a local helper in `parser/helpers.go`) and use it from both the explicit `cfr.ReplaceWith.Type()` boolean case and the `existing.Type == gjson.True || gjson.False` case so the behavior stays identical in one place.
182-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNumber path silently degrades when value isn't canonical.
When the egg declares
NumberbutvaluefailsisJSONNumber(e.g.25565x,007), this case is skipped and the value silently falls through to the mirror/default branch and is written as a string. The analogous explicit-Booleanpath (Lines 177-179) logs a warning on bad input. Consider adding a matching warning here so misconfigured eggs are diagnosable.♻️ Possible adjustment
- case cfr.ReplaceWith.Type() == jsonparser.Number && isJSONNumber(value): - // Explicit numeric type declared in the egg definition. Write the literal - // as-is via SetRaw to avoid float64 precision loss for large integers (> 2^53). - return sjson.SetRaw(jsonStr, path, value) + case cfr.ReplaceWith.Type() == jsonparser.Number: + // Explicit numeric type declared in the egg definition. Write the literal + // as-is via SetRaw to avoid float64 precision loss for large integers (> 2^53). + if isJSONNumber(value) { + return sjson.SetRaw(jsonStr, path, value) + } + log.WithFields(log.Fields{"value": value, "path": path, "match": cfr.Match}). + Warn("cannot parse replacement as number, falling back to string value") + return sjson.Set(jsonStr, path, value)Note: this also changes the non-numeric fallback from "mirror existing type" to "string"; confirm that matches intended behavior before adopting.
🤖 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 `@parser/helpers.go` around lines 182 - 185, The explicit Number branch in parser/helpers.go silently falls through to the default string/mirror handling when isJSONNumber(value) fails, which hides misconfigured egg values. Update the case around cfr.ReplaceWith.Type() and isJSONNumber to emit a warning similar to the Boolean path before falling back, and review the fallback behavior in the default branch to ensure Number-typed values are handled intentionally rather than being silently written as strings.
🤖 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 `@parser/helpers.go`:
- Around line 173-216: The explicit boolean branch and the existing-boolean
mirror branch in the replacement logic duplicate the same parse-and-fallback
behavior. Extract that shared `strconv.ParseBool`/warning/`sjson.Set` flow into
a small helper on `ConfigurationFileReplacement` (or a local helper in
`parser/helpers.go`) and use it from both the explicit `cfr.ReplaceWith.Type()`
boolean case and the `existing.Type == gjson.True || gjson.False` case so the
behavior stays identical in one place.
- Around line 182-185: The explicit Number branch in parser/helpers.go silently
falls through to the default string/mirror handling when isJSONNumber(value)
fails, which hides misconfigured egg values. Update the case around
cfr.ReplaceWith.Type() and isJSONNumber to emit a warning similar to the Boolean
path before falling back, and review the fallback behavior in the default branch
to ensure Number-typed values are handled intentionally rather than being
silently written as strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6e355c5-d720-4362-9c3e-fb2d0b35dae0
📒 Files selected for processing (1)
parser/helpers.go
📜 Review details
🔇 Additional comments (1)
parser/helpers.go (1)
38-51: LGTM!
The explicit-boolean and mirror-existing-boolean cases in setValueWithSjson contained identical parse/log/fallback logic. Extract a setBool helper so the two paths stay in sync.
8438c6d to
081e75e
Compare
The explicit-Number path previously matched on `type == Number && isJSONNumber`, so when an egg declared a numeric type but the expanded value was not a canonical JSON number (e.g. "25565x" or "007"), the case was skipped and the value silently fell through to the mirror/default branch and was written as a string. Extract a setNumber helper that, like setBool, logs a warning and falls back to writing the raw string when the value is not a canonical JSON number, so a misconfigured egg is diagnosable.
2 participants
Problem
When a server is created for the first time, its configuration file is often generated from scratch, so the target key does not exist yet.
setValueWithSjson(parser/helpers.go) previously inferred a value's type solely by mirroring the type already present in the document. A freshly created key therefore has no existing type to mirror, and every value falls back to a quoted string.As a result numeric values such as ports are written quoted:
The same applies to booleans and to JSON/TOML output. This breaks games/servers that require a real number (or boolean) for those keys.
Fix
In
setValueWithSjson:!existing.Exists()) — infer the natural type from the value itself, writing clean JSON numbers and booleans unquoted (SetRawfor numbers to preserve large-integer precision).isJSONNumber) replaces the laxgjson.Parsecheck.gjson.Parsetreats values like007,+5,1.20.1and25565xas numbers, which would coerce them incorrectly and could even produce invalid JSON viaSetRaw. The new regex matches the JSON number grammar exactly, so those stay strings.Existing string-typed keys are intentionally left untouched, so values like a numeric password in an existing template are not coerced to numbers — this keeps the change low-risk and avoids regressions.
Verification
go buildandgo vetpass. Verified behaviour (new-key and existing-key paths):{}unless noted)server-port="25565"25565✅ (the reported bug)online-mode="true"true✅motd="Welcome""Welcome"✅id="007""007"✅ (leading zero preserved)key="25565x""25565x"✅owner="1234567890123456789"1234567890123456789✅ (precise){"pass":"old"}←"12345""12345"✅ (no regression){"port":1}←"25565"25565✅{"flag":false}←"true"true✅100100✅Summary by CodeRabbit