Skip to content

Commit

Permalink
fix: Double quoted JSON handling (#471)
Browse files Browse the repository at this point in the history
## Motivation

The issue is extensively covered in this issue:
goccy/go-yaml#455

## Summary

Added test case and replaced `goccy/go-yaml` with our fork.
For now I'm pinning the commit SHA from the PR branch.
I'm not sure if the upstream is actively maintained anymore, we might
just want to go further with the fork instead of the replace clause.

## Related changes

- goccy/go-yaml#457
- nobl9/go-yaml#3

## Testing

Test case was supplied.

## Release Notes

Fixed incorrect handling of double quoted JSON literals which caused
errors when unmarshaling CloudWatch v1alpha SLOs
with certain `json` queries.
  • Loading branch information
nieomylnieja authored Jun 27, 2024
1 parent 7d704be commit 4e54bd7
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 2 deletions.
3 changes: 3 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,6 @@ require (
golang.org/x/xerrors v0.0.0-20231012003039-104605ab7028 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)

// We might in the end decide to just go with the fork direcly.
replace github.com/goccy/go-yaml => github.com/nobl9/go-yaml v0.0.0-20240626115914-6b82fd0d61b9
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ github.com/go-playground/universal-translator v0.17.0 h1:icxd5fm+REJzpZx7ZfpaD87
github.com/go-playground/universal-translator v0.17.0/go.mod h1:UkSxE5sNxxRwHyU+Scu5vgOQjsIJAF8j9muTVoKLVtA=
github.com/go-playground/validator/v10 v10.4.1 h1:pH2c5ADXtd66mxoE0Zm9SUhxE20r7aM3F26W0hOn+GE=
github.com/go-playground/validator/v10 v10.4.1/go.mod h1:nlOn6nFhuKACm19sB/8EGNn9GlaMV7XkbRSipzJ0Ii4=
github.com/goccy/go-yaml v1.11.3 h1:B3W9IdWbvrUu2OYQGwvU1nZtvMQJPBKgBUuweJjLj6I=
github.com/goccy/go-yaml v1.11.3/go.mod h1:wKnAMd44+9JAAnGQpWVEgBzGt3YuTaQ4uXoHvE4m7WU=
github.com/golang-jwt/jwt/v5 v5.2.1 h1:OuVbFODueb089Lh128TAcimifWaLhJwVflnrgM17wHk=
github.com/golang-jwt/jwt/v5 v5.2.1/go.mod h1:pqrtFR0X4osieyHYxtmOUWsAWrfe1Q5UVIyoH402zdk=
github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
Expand All @@ -42,6 +40,8 @@ github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovk
github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM=
github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY=
github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y=
github.com/nobl9/go-yaml v0.0.0-20240626115914-6b82fd0d61b9 h1:HWAY7k8zA8T3dgQRg6llwyWshguD6bIeaVXukNrnen4=
github.com/nobl9/go-yaml v0.0.0-20240626115914-6b82fd0d61b9/go.mod h1:wKnAMd44+9JAAnGQpWVEgBzGt3YuTaQ4uXoHvE4m7WU=
github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
Expand Down
9 changes: 9 additions & 0 deletions manifest/v1alpha/parser/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,15 @@ func Test_ParseObject_EnsureAllKindsAreParsed(t *testing.T) {
}
}

// References:
// - https://github.com/nobl9/go-yaml/pull/3 (fork)
// - https://github.com/nobl9/go-yaml/pull/457 (upstream)
func Test_ParseObject_DoubleQuotedJSONHandling(t *testing.T) {
data, format := readParserTestFile(t, "cloudwatch_slo.yaml")
_, err := ParseObject(data, manifest.KindSLO, format)
require.NoError(t, err)
}

func readParserTestFile(t *testing.T, filename string) ([]byte, manifest.ObjectFormat) {
t.Helper()
data, err := parserTestData.ReadFile(filepath.Join("test_data", filename))
Expand Down
29 changes: 29 additions & 0 deletions manifest/v1alpha/parser/test_data/cloudwatch_slo.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
apiVersion: n9/v1alpha
kind: SLO
metadata:
name: cloudwatch-json
project: cloudwatch
spec:
budgetingMethod: Occurrences
description: ""
indicator:
metricSource:
kind: Agent
name: cloudwatch
project: cloudwatch
objectives:
- displayName: ""
op: lte
rawMetric:
query:
cloudWatch:
json: "[{\"Id\": \"e1\",\"Expression\": \"MAX(FILL(METRICS(), 0))\",\"Period\": 60},{\"Id\": \"m1\",\"MetricStat\": {\"Metric\": {\"Namespace\": \"AWS/ApplicationELB\",\"MetricName\": \"TargetResponseTime\",\"Dimensions\": [{\"Name\": \"LoadBalancer\",\"Value\": \"app/123/123\"},{\"Name\": \"TargetGroup\",\"Value\": \"targetgroup/123/123\"}]},\"Period\": 60,\"Stat\": \"Average\"},\"ReturnData\": false}]"
region: eu-central-1
target: 0.8
value: 0.9
name: objective-1
service: cloudwatch-service
timeWindows:
- count: 1
isRolling: true
unit: Hour

0 comments on commit 4e54bd7

Please sign in to comment.