From da20e69d20373667942eb2148f71d028b0b39da2 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 28 Sep 2023 14:16:07 +0200 Subject: [PATCH] refactor(model): simplify using ArchivalBinaryData (#1323) It did not originally occur to me, but now it's clear that we can avoid using a struct to wrap the data type. It just suffices to use the new type. I think this is better in terms of writing code because the only two things we need to do are: 1. make sure we have serialization and unserialization tests; 2. use the correct data type in the struct. For all intents and purposes the ArchivalBinaryData is just a special kind of []byte attached to custom marshal/unmarshal rules. Part of https://github.com/ooni/probe/issues/2531 --- internal/model/archival.go | 12 ++++----- internal/model/archival_test.go | 46 ++++++++++++++++----------------- 2 files changed, 28 insertions(+), 30 deletions(-) diff --git a/internal/model/archival.go b/internal/model/archival.go index cdd6114618..ad59a51d98 100644 --- a/internal/model/archival.go +++ b/internal/model/archival.go @@ -65,9 +65,7 @@ var ( // data using the specific ooni/spec data format for binary data. // // See https://github.com/ooni/spec/blob/master/data-formats/df-001-httpt.md#maybebinarydata. -type ArchivalBinaryData struct { - Value []byte -} +type ArchivalBinaryData []byte // archivalBinaryDataRepr is the wire representation of binary data according to // https://github.com/ooni/spec/blob/master/data-formats/df-001-httpt.md#maybebinarydata. @@ -84,12 +82,12 @@ var ( // MarshalJSON implements json.Marshaler. func (value ArchivalBinaryData) MarshalJSON() ([]byte, error) { // special case: we need to marshal the empty data as the null value - if len(value.Value) <= 0 { + if len(value) <= 0 { return json.Marshal(nil) } // construct and serialize the OONI representation - repr := &archivalBinaryDataRepr{Format: "base64", Data: value.Value} + repr := &archivalBinaryDataRepr{Format: "base64", Data: value} return json.Marshal(repr) } @@ -101,7 +99,7 @@ var ErrInvalidBinaryDataFormat = errors.New("model: invalid binary data format") func (value *ArchivalBinaryData) UnmarshalJSON(raw []byte) error { // handle the case where input is a literal null if bytes.Equal(raw, []byte("null")) { - value.Value = nil + *value = nil return nil } @@ -117,7 +115,7 @@ func (value *ArchivalBinaryData) UnmarshalJSON(raw []byte) error { } // we're good because Go uses base64 for []byte automatically - value.Value = repr.Data + *value = repr.Data return nil } diff --git a/internal/model/archival_test.go b/internal/model/archival_test.go index eb23e2afc0..591b56afb0 100644 --- a/internal/model/archival_test.go +++ b/internal/model/archival_test.go @@ -62,22 +62,22 @@ func TestArchivalBinaryData(t *testing.T) { cases := []testcase{{ name: "with nil .Value", - input: model.ArchivalBinaryData{Value: nil}, + input: nil, expectErr: nil, expectData: []byte("null"), }, { name: "with zero length .Value", - input: model.ArchivalBinaryData{Value: []byte{}}, + input: []byte{}, expectErr: nil, expectData: []byte("null"), }, { name: "with .Value being a simple binary string", - input: model.ArchivalBinaryData{Value: []byte("Elliot")}, + input: []byte("Elliot"), expectErr: nil, expectData: []byte(`{"data":"RWxsaW90","format":"base64"}`), }, { name: "with .Value being a long binary string", - input: model.ArchivalBinaryData{Value: archivalBinaryInput}, + input: archivalBinaryInput, expectErr: nil, expectData: archivalEncodedBinaryInput, }} @@ -137,67 +137,67 @@ func TestArchivalBinaryData(t *testing.T) { name: "with nil input array", input: nil, expectErr: errors.New("unexpected end of JSON input"), - expectData: model.ArchivalBinaryData{Value: nil}, + expectData: nil, }, { name: "with zero-length input array", input: []byte{}, expectErr: errors.New("unexpected end of JSON input"), - expectData: model.ArchivalBinaryData{Value: nil}, + expectData: nil, }, { name: "with binary input that is not a complete JSON", input: []byte("{"), expectErr: errors.New("unexpected end of JSON input"), - expectData: model.ArchivalBinaryData{Value: nil}, + expectData: nil, }, { name: "with ~random binary data as input", input: archivalBinaryInput, expectErr: errors.New("invalid character 'W' looking for beginning of value"), - expectData: model.ArchivalBinaryData{}, + expectData: nil, }, { name: "with valid JSON of the wrong type (array)", input: []byte("[]"), expectErr: errors.New("json: cannot unmarshal array into Go value of type model.archivalBinaryDataRepr"), - expectData: model.ArchivalBinaryData{}, + expectData: nil, }, { name: "with valid JSON of the wrong type (number)", input: []byte("1.17"), expectErr: errors.New("json: cannot unmarshal number into Go value of type model.archivalBinaryDataRepr"), - expectData: model.ArchivalBinaryData{}, + expectData: nil, }, { name: "with input being the liternal null", input: []byte(`null`), expectErr: nil, - expectData: model.ArchivalBinaryData{Value: nil}, + expectData: nil, }, { name: "with empty JSON object", input: []byte("{}"), expectErr: errors.New("model: invalid binary data format: ''"), - expectData: model.ArchivalBinaryData{}, + expectData: nil, }, { name: "with correct data model but invalid format", input: []byte(`{"data":"","format":"antani"}`), expectErr: errors.New("model: invalid binary data format: 'antani'"), - expectData: model.ArchivalBinaryData{}, + expectData: nil, }, { name: "with correct data model and format but invalid base64 string", input: []byte(`{"data":"x","format":"base64"}`), expectErr: errors.New("illegal base64 data at input byte 0"), - expectData: model.ArchivalBinaryData{}, + expectData: nil, }, { name: "with correct data model and format but empty base64 string", input: []byte(`{"data":"","format":"base64"}`), expectErr: nil, - expectData: model.ArchivalBinaryData{Value: []byte{}}, + expectData: []byte{}, }, { name: "with the encoding of a simple binary string", input: []byte(`{"data":"RWxsaW90","format":"base64"}`), expectErr: nil, - expectData: model.ArchivalBinaryData{Value: []byte("Elliot")}, + expectData: []byte("Elliot"), }, { name: "with the encoding of a complex binary string", input: archivalEncodedBinaryInput, expectErr: nil, - expectData: model.ArchivalBinaryData{Value: archivalBinaryInput}, + expectData: archivalBinaryInput, }} for _, tc := range cases { @@ -207,8 +207,8 @@ func TestArchivalBinaryData(t *testing.T) { err := json.Unmarshal(tc.input, &abd) t.Log("got this error", err) - t.Log("got this .Value field", abd.Value) - t.Logf("converted to string: %s", string(abd.Value)) + t.Log("got this .Value field", abd) + t.Logf("converted to string: %s", string(abd)) // handle errors switch { @@ -247,16 +247,16 @@ func TestArchivalBinaryData(t *testing.T) { cases := []testcase{{ name: "with nil .Value", - input: model.ArchivalBinaryData{Value: nil}, + input: nil, }, { name: "with zero length .Value", - input: model.ArchivalBinaryData{Value: []byte{}}, + input: []byte{}, }, { name: "with .Value being a simple binary string", - input: model.ArchivalBinaryData{Value: []byte("Elliot")}, + input: []byte("Elliot"), }, { name: "with .Value being a long binary string", - input: model.ArchivalBinaryData{Value: archivalBinaryInput}, + input: archivalBinaryInput, }} for _, tc := range cases {