Skip to content

Commit

Permalink
cleanup: move ArchivalMaybeBinaryData to legacymodel (ooni#1336)
Browse files Browse the repository at this point in the history
This diff concludes most of the development work associated with
ooni/probe#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
ooni/probe#2543.
  • Loading branch information
bassosimone authored and Murphy-OrangeMud committed Feb 13, 2024
1 parent 99123e6 commit 21b8ef6
Show file tree
Hide file tree
Showing 9 changed files with 199 additions and 163 deletions.
13 changes: 7 additions & 6 deletions internal/experiment/hirl/hirl.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand Down Expand Up @@ -151,7 +152,7 @@ type MethodConfig struct {
type MethodResult struct {
Err error
Name string
Received tracex.MaybeBinaryValue
Received legacymodel.ArchivalMaybeBinaryData
Sent string
Tampering bool
}
Expand Down
6 changes: 3 additions & 3 deletions internal/experiment/hirl/hirl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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,
}
Expand All @@ -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,
}
Expand Down
31 changes: 16 additions & 15 deletions internal/experiment/quicping/quicping.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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,
})
Expand Down
60 changes: 60 additions & 0 deletions internal/legacy/legacymodel/archival.go
Original file line number Diff line number Diff line change
@@ -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
}
111 changes: 111 additions & 0 deletions internal/legacy/legacymodel/archival_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
})
}
})
}
2 changes: 2 additions & 0 deletions internal/legacy/legacymodel/doc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// Package legacymodel contains legacy content that used to be in internal/model
package legacymodel
1 change: 0 additions & 1 deletion internal/legacy/tracex/archival.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
53 changes: 0 additions & 53 deletions internal/model/archival.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ package model

import (
"bytes"
"encoding/base64"
"encoding/json"
"errors"
"fmt"
Expand Down Expand Up @@ -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
//
Expand Down
Loading

0 comments on commit 21b8ef6

Please sign in to comment.