Fix json parser for egg configs #161
Conversation
📝 WalkthroughWalkthroughThe PR refactors JSON handling by replacing the gabs library with tidwall's gjson and sjson libraries. Dependencies are updated in go.mod, JSON processing methods in parser/helpers.go are rewritten to work with JSON byte arrays instead of containers, and JSON/YAML output processing in parser/parser.go is enhanced with pretty-printing and updated serialization logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
i can confirm; this works fine with json and the config.json dont getting messed up like before, where the parser sort all strings in alphabetical order and remove important parts |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@parser/helpers.go`:
- Around line 12-13: The generated paths sometimes get a leading dot when a
wildcard starts at root (v.Match begins with ".*") because basePath becomes
empty and code naively does fullPath := basePath + "." + key.String(); update
both places where fullPath is constructed (the array-iteration branch and the
object-iteration branch) to conditionally build the path without the dot when
basePath == "" — e.g., if basePath == "" set fullPath = key.String() else
fullPath = basePath + "." + key.String(); apply this change wherever fullPath is
computed (referencing variables basePath and key.String()) to avoid producing
".someKey" and produce "someKey" instead.
🧹 Nitpick comments (1)
parser/helpers.go (1)
174-185: Preserve non‑integer numbers when ReplaceWith.Type is Number.Right now only
Atoiis attempted, so floats become strings. Consider parsing floats to keep numeric types stable.♻️ Proposed refactor
- var setValue interface{} - if cfr.ReplaceWith.Type() == jsonparser.Boolean { - v, _ := strconv.ParseBool(value) - setValue = v - } else if v, err := strconv.Atoi(value); err == nil { - setValue = v - } else { - setValue = value - } + var setValue interface{} + switch cfr.ReplaceWith.Type() { + case jsonparser.Boolean: + v, _ := strconv.ParseBool(value) + setValue = v + case jsonparser.Number: + if i, err := strconv.ParseInt(value, 10, 64); err == nil { + setValue = i + } else if f, err := strconv.ParseFloat(value, 64); err == nil { + setValue = f + } else { + setValue = value + } + default: + setValue = value + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modparser/helpers.goparser/parser.go
🧰 Additional context used
🧬 Code graph analysis (1)
parser/helpers.go (1)
parser/parser.go (2)
ConfigurationFile(107-116)ConfigurationFileReplacement(158-162)
🔇 Additional comments (3)
parser/parser.go (2)
17-17: Pretty JSON output is a nice improvement—please sanity‑check formatting expectations.Confirm the width/indent settings and key order behavior match the desired output for representative egg configs.
Also applies to: 418-425
444-447: Verify YAML → JSON → YAML round‑trip keeps expected scalar types.The extra JSON unmarshal can coerce YAML scalars (timestamps, large ints, custom tags). Please validate with representative configs to ensure type fidelity is acceptable.
Also applies to: 459-465
go.mod (1)
43-44: Tidwall dependencies are current and safe.Verified:
github.com/tidwall/gjson v1.18.0— latest stable; all historical CVEs (2020–2021) fixed in v1.6.x+github.com/tidwall/sjson v1.2.5— latest stable; no known advisoriesgithub.com/tidwall/match v1.1.1— safe; v1.2.0 available (optional)github.com/tidwall/pretty v1.2.0— safe; v1.2.1 available (optional)No action required.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "github.com/tidwall/gjson" | ||
| "github.com/tidwall/sjson" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat parser/helpers.go | head -80Repository: pelican-dev/wings
Length of output: 2741
🏁 Script executed:
sed -n '60,150p' parser/helpers.goRepository: pelican-dev/wings
Length of output: 2769
🏁 Script executed:
# Check if there are more occurrences of this pattern
rg -n "fullPath\s*:=\s*basePath\s*\+" parser/helpers.goRepository: pelican-dev/wings
Length of output: 161
🏁 Script executed:
# Let me check gjson/sjson path behavior and documentation
web_search="gjson path format leading dot valid"Repository: pelican-dev/wings
Length of output: 43
🏁 Script executed:
# Check if there are test files or examples showing gjson path usage
fd -t f ".*test.*\.go$" | xargs rg -l "gjson\|sjson" | head -5Repository: pelican-dev/wings
Length of output: 43
🏁 Script executed:
# Let's trace through the logic more carefully
# When v.Match starts with ".*", what would happen?
python3 << 'EOF'
# Simulate the path building logic
test_cases = [
".*", # Starts with wildcard
"foo.*", # Wildcard in middle
"foo.bar.*", # Wildcard at end
]
for match in test_cases:
print(f"\nMatch: {match}")
parts = match.split(".*", 1) # SplitN with 2 would keep both
print(f" parts: {parts}")
part0 = parts[0]
basePath = part0.strip(".")
remaining = parts[1] if len(parts) > 1 else ""
remainingPath = remaining.strip(".")
print(f" basePath: '{basePath}'")
print(f" remainingPath: '{remainingPath}'")
# Simulate building fullPath with an example key "123"
key = "123"
if basePath:
fullPath = basePath + "." + key
else:
fullPath = "." + key # Current code behavior
if remainingPath:
fullPath += "." + remainingPath
print(f" fullPath (current): '{fullPath}'")
# What it should be:
if basePath:
fullPath_fixed = basePath + "." + key
else:
fullPath_fixed = key # Fixed behavior
if remainingPath:
fullPath_fixed += "." + remainingPath
print(f" fullPath (fixed): '{fullPath_fixed}'")
EOFRepository: pelican-dev/wings
Length of output: 147
🏁 Script executed:
# Let's manually trace the logic without running python
# Look at what happens when v.Match = ".*"
cat << 'EOF'
When v.Match = ".*":
parts := strings.SplitN(".*", ".*", 2)
// Result: ["", ""]
basePath := strings.Trim("", ".")
// Result: "" (empty string)
remainingPath := strings.Trim("", ".")
// Result: "" (empty string)
// Then in the ForEach loop with key = "someKey":
fullPath := "" + "." + "someKey"
// Result: ".someKey" (LEADING DOT!)
EOFRepository: pelican-dev/wings
Length of output: 420
🏁 Script executed:
# Search for gjson documentation or any comments about path format
rg -i "path.*format|leading.*dot|valid.*path" parser/helpers.goRepository: pelican-dev/wings
Length of output: 43
🏁 Script executed:
# Look for actual test cases or usage examples with gjson paths
rg -n "gjson\.Get|sjson\.Set" parser/ -A 2 -B 2 | head -40Repository: pelican-dev/wings
Length of output: 1216
🏁 Script executed:
# Check the actual gjson package to understand path requirements
# First, let's look at go.mod to see which version is used
cat go.mod | grep "tidwall"Repository: pelican-dev/wings
Length of output: 218
🏁 Script executed:
web_search
gjson path syntax leading dot root elementRepository: pelican-dev/wings
Length of output: 155
🌐 Web query:
gjson path syntax leading dot root element
💡 Result:
Short answer: GJSON paths are dot-separated keys (e.g., "name.last"); you do not need a leading dot for the root and typical examples omit it. Use backslash to escape literal dots in keys (e.g., "fav.movie"). [1][2]
References:
- tidwall/gjson README (path syntax overview). [1]
- gjson documentation (crate/docs mirror with examples). [2]
🏁 Script executed:
# Verify both occurrences are identical and need the same fix
sed -n '90,115p' parser/helpers.goRepository: pelican-dev/wings
Length of output: 881
Fix leading-dot path generation when wildcard starts at root.
When v.Match begins with .*, basePath becomes empty after trimming. The current code then builds fullPath := basePath + "." + key.String(), resulting in paths like .someKey. gjson/sjson expect paths without leading dots (e.g., someKey not .someKey).
This occurs in both the array iteration (line 92) and object iteration (line 109) branches. Apply the conditional fix to both:
🔧 Proposed fix
- fullPath := basePath + "." + key.String()
+ fullPath := key.String()
+ if basePath != "" {
+ fullPath = basePath + "." + key.String()
+ }
if remainingPath != "" {
fullPath += "." + remainingPath
}🤖 Prompt for AI Agents
In `@parser/helpers.go` around lines 12 - 13, The generated paths sometimes get a
leading dot when a wildcard starts at root (v.Match begins with ".*") because
basePath becomes empty and code naively does fullPath := basePath + "." +
key.String(); update both places where fullPath is constructed (the
array-iteration branch and the object-iteration branch) to conditionally build
the path without the dot when basePath == "" — e.g., if basePath == "" set
fullPath = key.String() else fullPath = basePath + "." + key.String(); apply
this change wherever fullPath is computed (referencing variables basePath and
key.String()) to avoid producing ".someKey" and produce "someKey" instead.
2 participants
This pull request should fix the json parser that messes up with some json files.
This pr has some AI parts due to me not knowing a lot of go.
I removed most of the useless code but let me know if I need to make changes, and I'm sorry for using AI, but right now go is not my priority, and we need this for Hytale.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.