From 21b8ef66006b523fd15c62078359babc92d70815 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 28 Sep 2023 23:34:30 +0200 Subject: [PATCH] cleanup: move ArchivalMaybeBinaryData to legacymodel (#1336) This diff concludes most of the development work associated with https://github.com/ooni/probe/issues/2531. We move the nearly unused ArchivalMaybeBinaryData to the internal/legacy/legacymodel package to clearly mark it deprecated. Finishing removing this struct is a task that is documented by https://github.com/ooni/probe/issues/2543. --- internal/experiment/hirl/hirl.go | 13 ++- internal/experiment/hirl/hirl_test.go | 6 +- internal/experiment/quicping/quicping.go | 31 +++--- internal/legacy/legacymodel/archival.go | 60 ++++++++++ internal/legacy/legacymodel/archival_test.go | 111 +++++++++++++++++++ internal/legacy/legacymodel/doc.go | 2 + internal/legacy/tracex/archival.go | 1 - internal/model/archival.go | 53 --------- internal/model/archival_test.go | 85 -------------- 9 files changed, 199 insertions(+), 163 deletions(-) create mode 100644 internal/legacy/legacymodel/archival.go create mode 100644 internal/legacy/legacymodel/archival_test.go create mode 100644 internal/legacy/legacymodel/doc.go diff --git a/internal/experiment/hirl/hirl.go b/internal/experiment/hirl/hirl.go index 8e1938944b..d7b41a3eb9 100644 --- a/internal/experiment/hirl/hirl.go +++ b/internal/experiment/hirl/hirl.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/ooni/probe-cli/v3/internal/legacy/legacymodel" "github.com/ooni/probe-cli/v3/internal/legacy/netx" "github.com/ooni/probe-cli/v3/internal/legacy/tracex" "github.com/ooni/probe-cli/v3/internal/model" @@ -29,11 +30,11 @@ type Config struct{} // TestKeys contains the experiment test keys. type TestKeys struct { - FailureList []*string `json:"failure_list"` - Received []tracex.MaybeBinaryValue `json:"received"` - Sent []string `json:"sent"` - TamperingList []bool `json:"tampering_list"` - Tampering bool `json:"tampering"` + FailureList []*string `json:"failure_list"` + Received []legacymodel.ArchivalMaybeBinaryData `json:"received"` + Sent []string `json:"sent"` + TamperingList []bool `json:"tampering_list"` + Tampering bool `json:"tampering"` } // NewExperimentMeasurer creates a new ExperimentMeasurer. @@ -151,7 +152,7 @@ type MethodConfig struct { type MethodResult struct { Err error Name string - Received tracex.MaybeBinaryValue + Received legacymodel.ArchivalMaybeBinaryData Sent string Tampering bool } diff --git a/internal/experiment/hirl/hirl_test.go b/internal/experiment/hirl/hirl_test.go index 7685261419..4a46f28c18 100644 --- a/internal/experiment/hirl/hirl_test.go +++ b/internal/experiment/hirl/hirl_test.go @@ -8,9 +8,9 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/experiment/hirl" + "github.com/ooni/probe-cli/v3/internal/legacy/legacymodel" "github.com/ooni/probe-cli/v3/internal/legacy/mockable" "github.com/ooni/probe-cli/v3/internal/legacy/netx" - "github.com/ooni/probe-cli/v3/internal/legacy/tracex" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" ) @@ -159,7 +159,7 @@ func (FakeMethodSuccessful) Name() string { func (meth FakeMethodSuccessful) Run(ctx context.Context, config hirl.MethodConfig) { config.Out <- hirl.MethodResult{ Name: meth.Name(), - Received: tracex.MaybeBinaryValue{Value: "antani"}, + Received: legacymodel.ArchivalMaybeBinaryData{Value: "antani"}, Sent: "antani", Tampering: false, } @@ -174,7 +174,7 @@ func (FakeMethodFailure) Name() string { func (meth FakeMethodFailure) Run(ctx context.Context, config hirl.MethodConfig) { config.Out <- hirl.MethodResult{ Name: meth.Name(), - Received: tracex.MaybeBinaryValue{Value: "antani"}, + Received: legacymodel.ArchivalMaybeBinaryData{Value: "antani"}, Sent: "melandri", Tampering: true, } diff --git a/internal/experiment/quicping/quicping.go b/internal/experiment/quicping/quicping.go index 4061b35e26..c3955f4af4 100644 --- a/internal/experiment/quicping/quicping.go +++ b/internal/experiment/quicping/quicping.go @@ -17,6 +17,7 @@ import ( _ "crypto/sha256" + "github.com/ooni/probe-cli/v3/internal/legacy/legacymodel" "github.com/ooni/probe-cli/v3/internal/legacy/tracex" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -78,26 +79,26 @@ type TestKeys struct { // SinglePing is a result of a single ping operation. type SinglePing struct { - ConnIdDst string `json:"conn_id_dst"` - ConnIdSrc string `json:"conn_id_src"` - Failure *string `json:"failure"` - Request *model.ArchivalMaybeBinaryData `json:"request"` - T float64 `json:"t"` - Responses []*SinglePingResponse `json:"responses"` + ConnIdDst string `json:"conn_id_dst"` + ConnIdSrc string `json:"conn_id_src"` + Failure *string `json:"failure"` + Request *legacymodel.ArchivalMaybeBinaryData `json:"request"` + T float64 `json:"t"` + Responses []*SinglePingResponse `json:"responses"` } type SinglePingResponse struct { - Data *model.ArchivalMaybeBinaryData `json:"response_data"` - Failure *string `json:"failure"` - T float64 `json:"t"` - SupportedVersions []uint32 `json:"supported_versions"` + Data *legacymodel.ArchivalMaybeBinaryData `json:"response_data"` + Failure *string `json:"failure"` + T float64 `json:"t"` + SupportedVersions []uint32 `json:"supported_versions"` } // makeResponse is a utility function to create a SinglePingResponse func makeResponse(resp *responseInfo) *SinglePingResponse { - var data *model.ArchivalMaybeBinaryData + var data *legacymodel.ArchivalMaybeBinaryData if resp.raw != nil { - data = &model.ArchivalMaybeBinaryData{Value: string(resp.raw)} + data = &legacymodel.ArchivalMaybeBinaryData{Value: string(resp.raw)} } return &SinglePingResponse{ Data: data, @@ -272,7 +273,7 @@ L: ConnIdDst: req.dstID, ConnIdSrc: req.srcID, Failure: tracex.NewFailure(req.err), - Request: &model.ArchivalMaybeBinaryData{Value: string(req.raw)}, + Request: &legacymodel.ArchivalMaybeBinaryData{Value: string(req.raw)}, T: req.t, }) continue @@ -312,7 +313,7 @@ L: ConnIdDst: ping.request.dstID, ConnIdSrc: ping.request.srcID, Failure: tracex.NewFailure(timeoutErr), - Request: &model.ArchivalMaybeBinaryData{Value: string(ping.request.raw)}, + Request: &legacymodel.ArchivalMaybeBinaryData{Value: string(ping.request.raw)}, T: ping.request.t, }) continue @@ -325,7 +326,7 @@ L: ConnIdDst: ping.request.dstID, ConnIdSrc: ping.request.srcID, Failure: nil, - Request: &model.ArchivalMaybeBinaryData{Value: string(ping.request.raw)}, + Request: &legacymodel.ArchivalMaybeBinaryData{Value: string(ping.request.raw)}, T: ping.request.t, Responses: responses, }) diff --git a/internal/legacy/legacymodel/archival.go b/internal/legacy/legacymodel/archival.go new file mode 100644 index 0000000000..e11203e5a3 --- /dev/null +++ b/internal/legacy/legacymodel/archival.go @@ -0,0 +1,60 @@ +package legacymodel + +import ( + "encoding/base64" + "encoding/json" + "errors" + "unicode/utf8" +) + +// ArchivalMaybeBinaryData is a possibly binary string. We use this helper class +// to define a custom JSON encoder that allows us to choose the proper +// representation depending on whether the Value field is valid UTF-8 or not. +// +// See https://github.com/ooni/spec/blob/master/data-formats/df-001-httpt.md#maybebinarydata +// +// Deprecated: do not use this type in new code. +// +// Removing this struct is TODO(https://github.com/ooni/probe/issues/2543). +type ArchivalMaybeBinaryData struct { + Value string +} + +// MarshalJSON marshals a string-like to JSON following the OONI spec that +// says that UTF-8 content is represented as string and non-UTF-8 content is +// instead represented using `{"format":"base64","data":"..."}`. +func (hb ArchivalMaybeBinaryData) MarshalJSON() ([]byte, error) { + // if we can serialize as UTF-8 string, do that + if utf8.ValidString(hb.Value) { + return json.Marshal(hb.Value) + } + + // otherwise fallback to the ooni/spec representation for binary data + er := make(map[string]string) + er["format"] = "base64" + er["data"] = base64.StdEncoding.EncodeToString([]byte(hb.Value)) + return json.Marshal(er) +} + +// UnmarshalJSON is the opposite of MarshalJSON. +func (hb *ArchivalMaybeBinaryData) UnmarshalJSON(d []byte) error { + if err := json.Unmarshal(d, &hb.Value); err == nil { + return nil + } + er := make(map[string]string) + if err := json.Unmarshal(d, &er); err != nil { + return err + } + if v, ok := er["format"]; !ok || v != "base64" { + return errors.New("missing or invalid format field") + } + if _, ok := er["data"]; !ok { + return errors.New("missing data field") + } + b64, err := base64.StdEncoding.DecodeString(er["data"]) + if err != nil { + return err + } + hb.Value = string(b64) + return nil +} diff --git a/internal/legacy/legacymodel/archival_test.go b/internal/legacy/legacymodel/archival_test.go new file mode 100644 index 0000000000..f1feb830c3 --- /dev/null +++ b/internal/legacy/legacymodel/archival_test.go @@ -0,0 +1,111 @@ +package legacymodel_test + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/legacy/legacymodel" +) + +// we use this value below to test we can handle binary data +var archivalBinaryInput = []uint8{ + 0x57, 0xe5, 0x79, 0xfb, 0xa6, 0xbb, 0x0d, 0xbc, 0xce, 0xbd, 0xa7, 0xa0, + 0xba, 0xa4, 0x78, 0x78, 0x12, 0x59, 0xee, 0x68, 0x39, 0xa4, 0x07, 0x98, + 0xc5, 0x3e, 0xbc, 0x55, 0xcb, 0xfe, 0x34, 0x3c, 0x7e, 0x1b, 0x5a, 0xb3, + 0x22, 0x9d, 0xc1, 0x2d, 0x6e, 0xca, 0x5b, 0xf1, 0x10, 0x25, 0x47, 0x1e, + 0x44, 0xe2, 0x2d, 0x60, 0x08, 0xea, 0xb0, 0x0a, 0xcc, 0x05, 0x48, 0xa0, + 0xf5, 0x78, 0x38, 0xf0, 0xdb, 0x3f, 0x9d, 0x9f, 0x25, 0x6f, 0x89, 0x00, + 0x96, 0x93, 0xaf, 0x43, 0xac, 0x4d, 0xc9, 0xac, 0x13, 0xdb, 0x22, 0xbe, + 0x7a, 0x7d, 0xd9, 0x24, 0xa2, 0x52, 0x69, 0xd8, 0x89, 0xc1, 0xd1, 0x57, + 0xaa, 0x04, 0x2b, 0xa2, 0xd8, 0xb1, 0x19, 0xf6, 0xd5, 0x11, 0x39, 0xbb, + 0x80, 0xcf, 0x86, 0xf9, 0x5f, 0x9d, 0x8c, 0xab, 0xf5, 0xc5, 0x74, 0x24, + 0x3a, 0xa2, 0xd4, 0x40, 0x4e, 0xd7, 0x10, 0x1f, +} + +// we use this value below to test we can handle binary data +var archivalEncodedBinaryInput = []byte(`{"data":"V+V5+6a7DbzOvaeguqR4eBJZ7mg5pAeYxT68Vcv+NDx+G1qzIp3BLW7KW/EQJUceROItYAjqsArMBUig9Xg48Ns/nZ8lb4kAlpOvQ6xNyawT2yK+en3ZJKJSadiJwdFXqgQrotixGfbVETm7gM+G+V+djKv1xXQkOqLUQE7XEB8=","format":"base64"}`) + +func TestArchivalMaybeBinaryData(t *testing.T) { + t.Run("MarshalJSON", func(t *testing.T) { + tests := []struct { + name string // test name + input string // value to marshal + want []byte // expected result + wantErr bool // whether we expect an error + }{{ + name: "with string input", + input: "antani", + want: []byte(`"antani"`), + wantErr: false, + }, { + name: "with binary input", + input: string(archivalBinaryInput), + want: archivalEncodedBinaryInput, + wantErr: false, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hb := legacymodel.ArchivalMaybeBinaryData{ + Value: tt.input, + } + got, err := hb.MarshalJSON() + if (err != nil) != tt.wantErr { + t.Fatalf("ArchivalMaybeBinaryData.MarshalJSON() error = %v, wantErr %v", err, tt.wantErr) + } + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Fatal(diff) + } + }) + } + }) + + t.Run("UnmarshalJSON", func(t *testing.T) { + tests := []struct { + name string // test name + input []byte // value to unmarshal + want string // expected result + wantErr bool // whether we want an error + }{{ + name: "with string input", + input: []byte(`"xo"`), + want: "xo", + wantErr: false, + }, { + name: "with nil input", + input: nil, + want: "", + wantErr: true, + }, { + name: "with missing/invalid format", + input: []byte(`{"format": "foo"}`), + want: "", + wantErr: true, + }, { + name: "with missing data", + input: []byte(`{"format": "base64"}`), + want: "", + wantErr: true, + }, { + name: "with invalid base64 data", + input: []byte(`{"format": "base64", "data": "x"}`), + want: "", + wantErr: true, + }, { + name: "with valid base64 data", + input: archivalEncodedBinaryInput, + want: string(archivalBinaryInput), + wantErr: false, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hb := &legacymodel.ArchivalMaybeBinaryData{} + if err := hb.UnmarshalJSON(tt.input); (err != nil) != tt.wantErr { + t.Fatalf("ArchivalMaybeBinaryData.UnmarshalJSON() error = %v, wantErr %v", err, tt.wantErr) + } + if d := cmp.Diff(tt.want, hb.Value); d != "" { + t.Fatal(d) + } + }) + } + }) +} diff --git a/internal/legacy/legacymodel/doc.go b/internal/legacy/legacymodel/doc.go new file mode 100644 index 0000000000..de378bc7db --- /dev/null +++ b/internal/legacy/legacymodel/doc.go @@ -0,0 +1,2 @@ +// Package legacymodel contains legacy content that used to be in internal/model +package legacymodel diff --git a/internal/legacy/tracex/archival.go b/internal/legacy/tracex/archival.go index 0fae83d0a7..55f58714ba 100644 --- a/internal/legacy/tracex/archival.go +++ b/internal/legacy/tracex/archival.go @@ -21,7 +21,6 @@ type ( ExtSpec = model.ArchivalExtSpec TCPConnectEntry = model.ArchivalTCPConnectResult TCPConnectStatus = model.ArchivalTCPConnectStatus - MaybeBinaryValue = model.ArchivalMaybeBinaryData DNSQueryEntry = model.ArchivalDNSLookupResult DNSAnswerEntry = model.ArchivalDNSAnswer TLSHandshake = model.ArchivalTLSOrQUICHandshakeResult diff --git a/internal/model/archival.go b/internal/model/archival.go index 474e2163c3..ecef3c2427 100644 --- a/internal/model/archival.go +++ b/internal/model/archival.go @@ -12,7 +12,6 @@ package model import ( "bytes" - "encoding/base64" "encoding/json" "errors" "fmt" @@ -172,58 +171,6 @@ func (value *ArchivalScrubbedMaybeBinaryString) UnmarshalJSON(rawData []byte) er return nil } -// ArchivalMaybeBinaryData is a possibly binary string. We use this helper class -// to define a custom JSON encoder that allows us to choose the proper -// representation depending on whether the Value field is valid UTF-8 or not. -// -// See https://github.com/ooni/spec/blob/master/data-formats/df-001-httpt.md#maybebinarydata -// -// Deprecated: do not use this type in new code. -// -// Removing this struct is TODO(https://github.com/ooni/probe/issues/2543). -type ArchivalMaybeBinaryData struct { - Value string -} - -// MarshalJSON marshals a string-like to JSON following the OONI spec that -// says that UTF-8 content is represented as string and non-UTF-8 content is -// instead represented using `{"format":"base64","data":"..."}`. -func (hb ArchivalMaybeBinaryData) MarshalJSON() ([]byte, error) { - // if we can serialize as UTF-8 string, do that - if utf8.ValidString(hb.Value) { - return json.Marshal(hb.Value) - } - - // otherwise fallback to the ooni/spec representation for binary data - er := make(map[string]string) - er["format"] = "base64" - er["data"] = base64.StdEncoding.EncodeToString([]byte(hb.Value)) - return json.Marshal(er) -} - -// UnmarshalJSON is the opposite of MarshalJSON. -func (hb *ArchivalMaybeBinaryData) UnmarshalJSON(d []byte) error { - if err := json.Unmarshal(d, &hb.Value); err == nil { - return nil - } - er := make(map[string]string) - if err := json.Unmarshal(d, &er); err != nil { - return err - } - if v, ok := er["format"]; !ok || v != "base64" { - return errors.New("missing or invalid format field") - } - if _, ok := er["data"]; !ok { - return errors.New("missing data field") - } - b64, err := base64.StdEncoding.DecodeString(er["data"]) - if err != nil { - return err - } - hb.Value = string(b64) - return nil -} - // // DNS lookup // diff --git a/internal/model/archival_test.go b/internal/model/archival_test.go index 6567ce4cfa..0e0800c022 100644 --- a/internal/model/archival_test.go +++ b/internal/model/archival_test.go @@ -550,91 +550,6 @@ func TestArchivalScrubbedMaybeBinaryString(t *testing.T) { }) } -func TestMaybeBinaryValue(t *testing.T) { - t.Run("MarshalJSON", func(t *testing.T) { - tests := []struct { - name string // test name - input string // value to marshal - want []byte // expected result - wantErr bool // whether we expect an error - }{{ - name: "with string input", - input: "antani", - want: []byte(`"antani"`), - wantErr: false, - }, { - name: "with binary input", - input: string(archivalBinaryInput), - want: archivalEncodedBinaryInput, - wantErr: false, - }} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - hb := model.ArchivalMaybeBinaryData{ - Value: tt.input, - } - got, err := hb.MarshalJSON() - if (err != nil) != tt.wantErr { - t.Fatalf("ArchivalMaybeBinaryData.MarshalJSON() error = %v, wantErr %v", err, tt.wantErr) - } - if diff := cmp.Diff(tt.want, got); diff != "" { - t.Fatal(diff) - } - }) - } - }) - - t.Run("UnmarshalJSON", func(t *testing.T) { - tests := []struct { - name string // test name - input []byte // value to unmarshal - want string // expected result - wantErr bool // whether we want an error - }{{ - name: "with string input", - input: []byte(`"xo"`), - want: "xo", - wantErr: false, - }, { - name: "with nil input", - input: nil, - want: "", - wantErr: true, - }, { - name: "with missing/invalid format", - input: []byte(`{"format": "foo"}`), - want: "", - wantErr: true, - }, { - name: "with missing data", - input: []byte(`{"format": "base64"}`), - want: "", - wantErr: true, - }, { - name: "with invalid base64 data", - input: []byte(`{"format": "base64", "data": "x"}`), - want: "", - wantErr: true, - }, { - name: "with valid base64 data", - input: archivalEncodedBinaryInput, - want: string(archivalBinaryInput), - wantErr: false, - }} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - hb := &model.ArchivalMaybeBinaryData{} - if err := hb.UnmarshalJSON(tt.input); (err != nil) != tt.wantErr { - t.Fatalf("ArchivalMaybeBinaryData.UnmarshalJSON() error = %v, wantErr %v", err, tt.wantErr) - } - if d := cmp.Diff(tt.want, hb.Value); d != "" { - t.Fatal(d) - } - }) - } - }) -} - func TestArchivalHTTPHeader(t *testing.T) { t.Run("MarshalJSON", func(t *testing.T) { tests := []struct {