Skip to content

Commit

Permalink
refactor(model): simplify using ArchivalBinaryData (ooni#1323)
Browse files Browse the repository at this point in the history 
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 ooni/probe#2531
  • Loading branch information
@bassosimone @Murphy-OrangeMud
bassosimone authored and Murphy-OrangeMud committed Feb 13, 2024
1 parent 267780c commit da20e69
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 30 deletions.
12 changes: 5 additions & 7 deletions internal/model/archival.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
}

Expand All @@ -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
}

Expand All @@ -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
}

Expand Down
46 changes: 23 additions & 23 deletions internal/model/archival_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}}
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit da20e69

Please sign in to comment.