Skip to content

Commit 4d53d77

Browse files
wallrj-cyberarkwallrj-cyberark
authored
Merge pull request #710 from jetstack/VC-43403-outputmode-integration-tests-2
fix(datareading): improve DataReading JSON parsing and error handling
2 parents 7fc0481 + 0ef5837 commit 4d53d77

File tree

3 files changed

+219
-31
lines changed

3 files changed

+219
-31
lines changed

‎api/datareading.go‎

Lines changed: 36 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,10 @@ type DataReading struct {
3030
}
3131

3232
// UnmarshalJSON implements the json.Unmarshaler interface for DataReading.
33-
// It handles the dynamic parsing of the Data field based on the DataGatherer.
33+
// The function attempts to decode the Data field into known types in a prioritized order.
34+
// Empty data is considered an error, because there is no way to discriminate between data types.
35+
// TODO(wallrj): Add a discriminator field to DataReading to avoid this complex logic.
36+
// E.g. "data_type": "discovery"|"dynamic"
3437
func (o *DataReading) UnmarshalJSON(data []byte) error {
3538
var tmp struct {
3639
ClusterID string `json:"cluster_id,omitempty"`
@@ -40,45 +43,49 @@ func (o *DataReading) UnmarshalJSON(data []byte) error {
4043
SchemaVersion string `json:"schema_version"`
4144
}
4245

43-
d := json.NewDecoder(bytes.NewReader(data))
44-
d.DisallowUnknownFields()
45-
46-
if err := d.Decode(&tmp); err != nil {
47-
return err
46+
// Decode the top-level fields of DataReading
47+
if err := jsonUnmarshalStrict(data, &tmp); err != nil {
48+
return fmt.Errorf("failed to parse DataReading: %s", err)
4849
}
50+
51+
// Assign top-level fields to the DataReading object
4952
o.ClusterID = tmp.ClusterID
5053
o.DataGatherer = tmp.DataGatherer
5154
o.Timestamp = tmp.Timestamp
5255
o.SchemaVersion = tmp.SchemaVersion
5356

54-
{
55-
var discoveryData DiscoveryData
56-
d := json.NewDecoder(bytes.NewReader(tmp.Data))
57-
d.DisallowUnknownFields()
58-
if err := d.Decode(&discoveryData); err == nil {
59-
o.Data = &discoveryData
60-
return nil
61-
}
57+
// Return an error if data is empty
58+
if len(tmp.Data) == 0 || bytes.Equal(tmp.Data, []byte("null")) || bytes.Equal(tmp.Data, []byte("{}")) {
59+
return fmt.Errorf("failed to parse DataReading.Data for gatherer %q: empty data", o.DataGatherer)
6260
}
63-
{
64-
var dynamicData DynamicData
65-
d := json.NewDecoder(bytes.NewReader(tmp.Data))
66-
d.DisallowUnknownFields()
67-
if err := d.Decode(&dynamicData); err == nil {
68-
o.Data = &dynamicData
69-
return nil
70-
}
61+
62+
// Define a list of decoding attempts with prioritized types
63+
dataTypes := []struct {
64+
target interface{}
65+
assign func(interface{})
66+
}{
67+
{&DiscoveryData{}, func(v interface{}) { o.Data = v.(*DiscoveryData) }},
68+
{&DynamicData{}, func(v interface{}) { o.Data = v.(*DynamicData) }},
7169
}
72-
{
73-
var genericData map[string]interface{}
74-
d := json.NewDecoder(bytes.NewReader(tmp.Data))
75-
d.DisallowUnknownFields()
76-
if err := d.Decode(&genericData); err == nil {
77-
o.Data = genericData
70+
71+
// Attempt to decode the Data field into each type
72+
for _, dataType := range dataTypes {
73+
if err := jsonUnmarshalStrict(tmp.Data, dataType.target); err == nil {
74+
dataType.assign(dataType.target)
7875
return nil
7976
}
8077
}
81-
return fmt.Errorf("failed to parse DataReading.Data for gatherer %s", o.DataGatherer)
78+
79+
// Return an error if no type matches
80+
return fmt.Errorf("failed to parse DataReading.Data for gatherer %q: unknown type", o.DataGatherer)
81+
}
82+
83+
// jsonUnmarshalStrict unmarshals JSON data into the provided interface,
84+
// disallowing unknown fields to ensure strict adherence to the expected structure.
85+
func jsonUnmarshalStrict(data []byte, v interface{}) error {
86+
decoder := json.NewDecoder(bytes.NewReader(data))
87+
decoder.DisallowUnknownFields()
88+
return decoder.Decode(v)
8289
}
8390

8491
// GatheredResource wraps the raw k8s resource that is sent to the jetstack secure backend

‎api/datareading_test.go‎

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import (
44
"encoding/json"
55
"testing"
66
"time"
7+
8+
"github.com/stretchr/testify/assert"
79
)
810

911
func TestJSONGatheredResourceDropsEmptyTime(t *testing.T) {
@@ -34,3 +36,178 @@ func TestJSONGatheredResourceSetsTimeWhenPresent(t *testing.T) {
3436
t.Fatalf("unexpected json \ngot %s\nwant %s", string(bytes), expected)
3537
}
3638
}
39+
40+
// TestDataReading_UnmarshalJSON tests the UnmarshalJSON method of DataReading
41+
// with various scenarios including valid and invalid JSON inputs.
42+
func TestDataReading_UnmarshalJSON(t *testing.T) {
43+
tests := []struct {
44+
name string
45+
input string
46+
wantDataType interface{}
47+
expectError string
48+
}{
49+
{
50+
name: "DiscoveryData type",
51+
input: `{
52+
"cluster_id": "61b2db64-fd70-49a6-a257-08397b9b4bae",
53+
"data-gatherer": "discovery",
54+
"timestamp": "2024-06-01T12:00:00Z",
55+
"data": {
56+
"cluster_id": "60868ebf-6e47-4184-9bc0-20bb6824e210",
57+
"server_version": {
58+
"major": "1",
59+
"minor": "20",
60+
"gitVersion": "v1.20.0"
61+
}
62+
},
63+
"schema_version": "v1"
64+
}`,
65+
wantDataType: &DiscoveryData{},
66+
},
67+
{
68+
name: "DynamicData type",
69+
input: `{
70+
"cluster_id": "69050b54-c61a-4384-95c3-35f890377a67",
71+
"data-gatherer": "dynamic",
72+
"timestamp": "2024-06-01T12:00:00Z",
73+
"data": {"items": []},
74+
"schema_version": "v1"
75+
}`,
76+
wantDataType: &DynamicData{},
77+
},
78+
{
79+
name: "Invalid JSON",
80+
input: `not a json`,
81+
expectError: "failed to parse DataReading: invalid character 'o' in literal null (expecting 'u')",
82+
},
83+
{
84+
name: "Missing data field",
85+
input: `{
86+
"cluster_id": "cc5a0429-8dc4-42c8-8e3a-eece9bca15c3",
87+
"data-gatherer": "missing-data-field",
88+
"timestamp": "2024-06-01T12:00:00Z",
89+
"schema_version": "v1"
90+
}`,
91+
expectError: `failed to parse DataReading.Data for gatherer "missing-data-field": empty data`,
92+
},
93+
{
94+
name: "Mismatched data type",
95+
input: `{
96+
"cluster_id": "c272b13e-b19e-4782-833f-d55a305f3c9e",
97+
"data-gatherer": "unknown-data-type",
98+
"timestamp": "2024-06-01T12:00:00Z",
99+
"data": "this should be an object",
100+
"schema_version": "v1"
101+
}`,
102+
expectError: `failed to parse DataReading.Data for gatherer "unknown-data-type": unknown type`,
103+
},
104+
{
105+
name: "Empty data field",
106+
input: `{
107+
"cluster_id": "07909675-113f-4b59-ba5e-529571a191e6",
108+
"data-gatherer": "empty-data",
109+
"timestamp": "2024-06-01T12:00:00Z",
110+
"data": {},
111+
"schema_version": "v1"
112+
}`,
113+
expectError: `failed to parse DataReading.Data for gatherer "empty-data": empty data`,
114+
},
115+
{
116+
name: "Additional field",
117+
input: `{
118+
"cluster_id": "11df7332-4b32-4f5a-903b-0cbbef381850",
119+
"data-gatherer": "additional-field",
120+
"timestamp": "2024-06-01T12:00:00Z",
121+
"data": {
122+
"cluster_id": "60868ebf-6e47-4184-9bc0-20bb6824e210"
123+
},
124+
"extra_field": "should cause error",
125+
"schema_version": "v1"
126+
}`,
127+
expectError: `failed to parse DataReading: json: unknown field "extra_field"`,
128+
},
129+
{
130+
name: "Additional data field",
131+
input: `{
132+
"cluster_id": "ca44c338-987e-4d57-8320-63f538db4292",
133+
"data-gatherer": "additional-data-field",
134+
"timestamp": "2024-06-01T12:00:00Z",
135+
"data": {
136+
"cluster_id": "60868ebf-6e47-4184-9bc0-20bb6824e210",
137+
"server_version": {
138+
"major": "1",
139+
"minor": "20",
140+
"gitVersion": "v1.20.0"
141+
},
142+
"extra_field": "should cause error"
143+
},
144+
"schema_version": "v1"
145+
}`,
146+
expectError: `failed to parse DataReading.Data for gatherer "additional-data-field": unknown type`,
147+
},
148+
{
149+
name: "Empty JSON object",
150+
input: `{}`,
151+
expectError: `failed to parse DataReading.Data for gatherer "": empty data`,
152+
},
153+
{
154+
name: "Null data field",
155+
input: `{
156+
"cluster_id": "36281cb3-7f3a-4efa-9879-7c988a9715b0",
157+
"data-gatherer": "null-data",
158+
"timestamp": "2024-06-01T12:00:00Z",
159+
"data": null,
160+
"schema_version": "v1"
161+
}`,
162+
expectError: `failed to parse DataReading.Data for gatherer "null-data": empty data`,
163+
},
164+
{
165+
name: "Empty string data field",
166+
input: `{
167+
"cluster_id": "7b7aa8ee-58ac-4818-9b29-c0a76296ea1d",
168+
"data-gatherer": "empty-string-data",
169+
"timestamp": "2024-06-01T12:00:00Z",
170+
"data": "",
171+
"schema_version": "v1"
172+
}`,
173+
expectError: `failed to parse DataReading.Data for gatherer "empty-string-data": unknown type`,
174+
},
175+
{
176+
name: "Array instead of object in data field",
177+
input: `{
178+
"cluster_id": "94d7757f-d084-4ccb-963b-f60fece0df2d",
179+
"data-gatherer": "array-data",
180+
"timestamp": "2024-06-01T12:00:00Z",
181+
"data": [],
182+
"schema_version": "v1"
183+
}`,
184+
expectError: `failed to parse DataReading.Data for gatherer "array-data": unknown type`,
185+
},
186+
{
187+
name: "Incorrect timestamp format",
188+
input: `{
189+
"cluster_id": "d58f298d-b8c1-4d99-aa85-c27d9aec6f97",
190+
"data-gatherer": "bad-timestamp",
191+
"timestamp": "not-a-timestamp",
192+
"data": {
193+
"items": []
194+
},
195+
"schema_version": "v1"
196+
}`,
197+
expectError: `failed to parse DataReading: parsing time "not-a-timestamp" as "2006-01-02T15:04:05Z07:00": cannot parse "not-a-timestamp" as "2006"`,
198+
},
199+
}
200+
201+
for _, tt := range tests {
202+
t.Run(tt.name, func(t *testing.T) {
203+
var dr DataReading
204+
err := dr.UnmarshalJSON([]byte(tt.input))
205+
if tt.expectError != "" {
206+
assert.EqualError(t, err, tt.expectError)
207+
return
208+
}
209+
assert.NoError(t, err)
210+
assert.IsType(t, tt.wantDataType, dr.Data)
211+
})
212+
}
213+
}

‎pkg/echo/echo_test.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88
"testing"
99
"time"
1010

11+
"k8s.io/apimachinery/pkg/version"
12+
1113
"github.com/jetstack/preflight/api"
1214
)
1315

@@ -34,8 +36,10 @@ func TestEchoServerRequestResponse(t *testing.T) {
3436
ClusterID: "test_suite_cluster",
3537
DataGatherer: "dummy",
3638
Timestamp: api.Time{Time: time.Now()},
37-
Data: map[string]string{
38-
"test": "test",
39+
Data: &api.DiscoveryData{
40+
ServerVersion: &version.Info{
41+
GitVersion: "v1.20.0",
42+
},
3943
},
4044
SchemaVersion: "2.0.0",
4145
},

0 commit comments

Comments
 (0)