Skip to content

Commit e3841a7

Browse files
efd6efd6
authored
internal/testrunner/runners/pipeline: unmarshal test results using UseNumber (#717)
By default, numbers are unmarshaled as float64 resulting in low bits truncation in longs that are above 53 bits wide, so use a decoder and UseNumber to ensure results are not corrupted.
1 parent 99cfd37 commit e3841a7

17 files changed

Lines changed: 295 additions & 7 deletions

File tree

‎internal/testrunner/runners/pipeline/runner.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,8 @@ func (r *runner) run() ([]testrunner.TestResult, error) {
103103
}
104104
startTime := time.Now()
105105

106+
// TODO: Add tests to cover regressive use of json.Unmarshal in loadTestCaseFile.
107+
// See https://github.com/elastic/elastic-package/pull/717.
106108
tc, err := r.loadTestCaseFile(testCaseFile)
107109
if err != nil {
108110
err := errors.Wrap(err, "loading test case failed")
@@ -141,6 +143,8 @@ func (r *runner) run() ([]testrunner.TestResult, error) {
141143
return nil, errors.Wrapf(err, "creating fields validator for data stream failed (path: %s, test case file: %s)", dataStreamPath, testCaseFile)
142144
}
143145

146+
// TODO: Add tests to cover regressive use of json.Unmarshal in verifyResults.
147+
// See https://github.com/elastic/elastic-package/pull/717.
144148
err = r.verifyResults(testCaseFile, tc.config, result, fieldsValidator)
145149
if e, ok := err.(testrunner.ErrTestCaseFailed); ok {
146150
tr.FailureMsg = e.Error()
@@ -232,6 +236,8 @@ func (r *runner) verifyResults(testCaseFile string, config *testConfig, result *
232236
testCasePath := filepath.Join(r.options.TestFolder.Path, testCaseFile)
233237

234238
if r.options.GenerateTestResult {
239+
// TODO: Add tests to cover regressive use of json.Unmarshal in writeTestResult.
240+
// See https://github.com/elastic/elastic-package/pull/717.
235241
err := writeTestResult(testCasePath, result)
236242
if err != nil {
237243
return errors.Wrap(err, "writing test result failed")
@@ -281,7 +287,7 @@ func verifyDynamicFields(result *testResult, config *testConfig) error {
281287
var multiErr multierror.Error
282288
for _, event := range result.events {
283289
var m common.MapStr
284-
err := json.Unmarshal(event, &m)
290+
err := jsonUnmarshalUsingNumber(event, &m)
285291
if err != nil {
286292
return errors.Wrap(err, "can't unmarshal event")
287293
}
@@ -348,7 +354,7 @@ func checkErrorMessage(event json.RawMessage) error {
348354
Message interface{}
349355
}
350356
}
351-
err := json.Unmarshal(event, &pipelineError)
357+
err := jsonUnmarshalUsingNumber(event, &pipelineError)
352358
if err != nil {
353359
return errors.Wrapf(err, "can't unmarshal event to check pipeline error: %#q", event)
354360
}

‎internal/testrunner/runners/pipeline/runner_test.go‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ package pipeline
66

77
import (
88
"encoding/json"
9+
"fmt"
10+
"strings"
911
"testing"
1012

1113
"github.com/google/go-cmp/cmp"
@@ -318,3 +320,108 @@ func TestDiffUlite(t *testing.T) {
318320
})
319321
}
320322
}
323+
324+
var jsonUnmarshalUsingNumberTests = []struct {
325+
name string
326+
msg string
327+
}{
328+
{
329+
name: "empty",
330+
msg: "", // Will error "unexpected end of JSON input".
331+
},
332+
{
333+
name: "string",
334+
msg: `"message"`,
335+
},
336+
{
337+
name: "array",
338+
msg: "[1,2,3,4,5]",
339+
},
340+
{
341+
name: "object",
342+
msg: `{"key":42}`,
343+
},
344+
{
345+
name: "object",
346+
msg: `{"key":42}answer`, // Will error "invalid character 'a' after top-level value".
347+
},
348+
// Test extra data whitespace parity with json.Unmarshal for error parity.
349+
{
350+
name: "object",
351+
msg: `{"key":42} `,
352+
},
353+
{
354+
name: "object",
355+
msg: `{"key":42}` + "\t",
356+
},
357+
{
358+
name: "object",
359+
msg: `{"key":42}` + "\r",
360+
},
361+
{
362+
name: "object",
363+
msg: `{"key":42}` + "\n",
364+
},
365+
{
366+
name: "0x1p52+1",
367+
msg: fmt.Sprint(uint64(0x1p52) + 1),
368+
},
369+
{
370+
name: "0x1p53-1",
371+
msg: fmt.Sprint(uint64(0x1p53) - 1),
372+
},
373+
// The following three cases will fail if json.Unmarshal is used in place
374+
// of jsonUnmarshalUsingNumber, as they are past the cutover.
375+
{
376+
name: "0x1p53+1",
377+
msg: fmt.Sprint(uint64(0x1p53) + 1),
378+
},
379+
{
380+
name: "0x1p54+1",
381+
msg: fmt.Sprint(uint64(0x1p54) + 1),
382+
},
383+
{
384+
name: "long",
385+
msg: "9223372036854773807",
386+
},
387+
}
388+
389+
func TestJsonUnmarshalUsingNumberRoundTrip(t *testing.T) {
390+
// This tests that jsonUnmarshalUsingNumber behaves the same
391+
// way as json.Unmarshal with the exception that numbers are
392+
// not unmarshaled through float64. This is important to avoid
393+
// low-bit truncation of long numeric values that are greater
394+
// than or equal to 0x1p53, the limit of bijective equivalence
395+
// with 64 bit-integers.
396+
397+
for _, test := range jsonUnmarshalUsingNumberTests {
398+
t.Run(test.name, func(t *testing.T) {
399+
var val interface{}
400+
err := jsonUnmarshalUsingNumber([]byte(test.msg), &val)
401+
402+
// Confirm that we get the same errors with jsonUnmarshalUsingNumber
403+
// as are returned by json.Unmarshal.
404+
jerr := json.Unmarshal([]byte(test.msg), new(interface{}))
405+
if (err == nil) != (jerr == nil) {
406+
t.Errorf("unexpected error: got:%#v want:%#v", err, jerr)
407+
}
408+
if err != nil {
409+
return
410+
}
411+
412+
// Confirm that we round-trip the message correctly without
413+
// alteration beyond trailing whitespace.
414+
got, err := json.Marshal(val)
415+
if err != nil {
416+
t.Errorf("unexpected error: got:%#v want:%#v", err, jerr)
417+
}
418+
// Truncate trailing whitespace from the input since it won't
419+
// be rendered in the output. This set of space characters is
420+
// defined in encoding/json/scanner.go as func isSpace.
421+
want := strings.TrimRight(test.msg, " \t\r\n")
422+
if string(got) != want {
423+
t.Errorf("unexpected result: got:%v want:%v", val, want)
424+
}
425+
})
426+
}
427+
}

‎internal/testrunner/runners/pipeline/test_case.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ type testCaseDefinition struct {
2828

2929
func readTestCaseEntriesForEvents(inputData []byte) ([]json.RawMessage, error) {
3030
var tcd testCaseDefinition
31-
err := json.Unmarshal(inputData, &tcd)
31+
err := jsonUnmarshalUsingNumber(inputData, &tcd)
3232
if err != nil {
3333
return nil, errors.Wrap(err, "unmarshalling input data failed")
3434
}
@@ -59,7 +59,7 @@ func createTestCase(filename string, entries []json.RawMessage, config *testConf
5959
var events []json.RawMessage
6060
for _, entry := range entries {
6161
var m common.MapStr
62-
err := json.Unmarshal(entry, &m)
62+
err := jsonUnmarshalUsingNumber(entry, &m)
6363
if err != nil {
6464
return nil, errors.Wrap(err, "can't unmarshal test case entry")
6565
}

‎internal/testrunner/runners/pipeline/test_result.go‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"bytes"
99
"encoding/json"
1010
"fmt"
11+
"io"
1112
"os"
1213
"path/filepath"
1314
"strings"
@@ -188,7 +189,7 @@ func adjustTestResult(result *testResult, config *testConfig) (*testResult, erro
188189
}
189190

190191
var m common.MapStr
191-
err := json.Unmarshal(event, &m)
192+
err := jsonUnmarshalUsingNumber(event, &m)
192193
if err != nil {
193194
return nil, errors.Wrapf(err, "can't unmarshal event: %s", string(event))
194195
}
@@ -212,7 +213,7 @@ func adjustTestResult(result *testResult, config *testConfig) (*testResult, erro
212213

213214
func unmarshalTestResult(body []byte) (*testResult, error) {
214215
var trd testResultDefinition
215-
err := json.Unmarshal(body, &trd)
216+
err := jsonUnmarshalUsingNumber(body, &trd)
216217
if err != nil {
217218
return nil, errors.Wrap(err, "unmarshalling test result failed")
218219
}
@@ -222,6 +223,28 @@ func unmarshalTestResult(body []byte) (*testResult, error) {
222223
return &tr, nil
223224
}
224225

226+
// jsonUnmarshalUsingNumber is a drop-in replacement for json.Unmarshal that
227+
// does not default to unmarshaling numeric values to float64 in order to
228+
// prevent low bit truncation of values greater than 1<<53.
229+
// See https://golang.org/cl/6202068 for details.
230+
func jsonUnmarshalUsingNumber(data []byte, v interface{}) error {
231+
dec := json.NewDecoder(bytes.NewReader(data))
232+
dec.UseNumber()
233+
err := dec.Decode(v)
234+
if err != nil {
235+
if err == io.EOF {
236+
return errors.New("unexpected end of JSON input")
237+
}
238+
return err
239+
}
240+
// Make sure there is no more data after the message
241+
// to approximate json.Unmarshal's behaviour.
242+
if dec.More() {
243+
return fmt.Errorf("more data after top-level value")
244+
}
245+
return nil
246+
}
247+
225248
func marshalTestResultDefinition(result *testResult) ([]byte, error) {
226249
var trd testResultDefinition
227250
trd.Expected = result.events
@@ -241,7 +264,7 @@ func marshalNormalizedJSON(v testResultDefinition) ([]byte, error) {
241264
return msg, err
242265
}
243266
var obj interface{}
244-
err = json.Unmarshal(msg, &obj)
267+
err = jsonUnmarshalUsingNumber(msg, &obj)
245268
if err != nil {
246269
return msg, err
247270
}

‎test/packages/other/long_integers/_dev/build/build.yml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
dependencies:
2+
ecs:
3+
reference: git@1.10

‎test/packages/other/long_integers/_dev/build/docs/README.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
# Long Integer Tests
2+
3+
{{event "test"}}
4+
5+
{{fields "test"}}

‎test/packages/other/long_integers/changelog.yml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
# newer versions go on top
2+
- version: "0.0.1"
3+
changes:
4+
- description: Initial draft of the package
5+
type: enhancement
6+
link: https://github.com/elastic/integrations/pull/0 # FIXME Replace with the real PR link

‎test/packages/other/long_integers/data_stream/test/_dev/test/pipeline/test-sequences.log‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
4503599627370497,9007199254740991,9007199254740993,18014398509481985,9223372036854773807

‎test/packages/other/long_integers/data_stream/test/_dev/test/pipeline/test-sequences.log-config.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
fields:
2+
"@warning": "The values in sequence_number must match the values in message."

‎test/packages/other/long_integers/data_stream/test/_dev/test/pipeline/test-sequences.log-expected.json‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
{
2+
"expected": [
3+
{
4+
"@warning": "The values in sequence_number must match the values in message.",
5+
"message": "4503599627370497,9007199254740991,9007199254740993,18014398509481985,9223372036854773807",
6+
"sequence_number": [
7+
4503599627370497,
8+
9007199254740991,
9+
9007199254740993,
10+
18014398509481985,
11+
9223372036854773807
12+
]
13+
}
14+
]
15+
}

0 commit comments

Comments
 (0)