Skip to content

Commit a37f9c1

Browse files
authored
cleanup: move ArchivalMaybeBinaryData to legacymodel (#1336)
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.
1 parent 7a6fd91 commit a37f9c1

File tree

9 files changed

+199
-163
lines changed

9 files changed

+199
-163
lines changed

internal/experiment/hirl/hirl.go

+7-6
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"strings"
1212
"time"
1313

14+
"github.com/ooni/probe-cli/v3/internal/legacy/legacymodel"
1415
"github.com/ooni/probe-cli/v3/internal/legacy/netx"
1516
"github.com/ooni/probe-cli/v3/internal/legacy/tracex"
1617
"github.com/ooni/probe-cli/v3/internal/model"
@@ -29,11 +30,11 @@ type Config struct{}
2930

3031
// TestKeys contains the experiment test keys.
3132
type TestKeys struct {
32-
FailureList []*string `json:"failure_list"`
33-
Received []tracex.MaybeBinaryValue `json:"received"`
34-
Sent []string `json:"sent"`
35-
TamperingList []bool `json:"tampering_list"`
36-
Tampering bool `json:"tampering"`
33+
FailureList []*string `json:"failure_list"`
34+
Received []legacymodel.ArchivalMaybeBinaryData `json:"received"`
35+
Sent []string `json:"sent"`
36+
TamperingList []bool `json:"tampering_list"`
37+
Tampering bool `json:"tampering"`
3738
}
3839

3940
// NewExperimentMeasurer creates a new ExperimentMeasurer.
@@ -151,7 +152,7 @@ type MethodConfig struct {
151152
type MethodResult struct {
152153
Err error
153154
Name string
154-
Received tracex.MaybeBinaryValue
155+
Received legacymodel.ArchivalMaybeBinaryData
155156
Sent string
156157
Tampering bool
157158
}

internal/experiment/hirl/hirl_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ import (
88

99
"github.com/apex/log"
1010
"github.com/ooni/probe-cli/v3/internal/experiment/hirl"
11+
"github.com/ooni/probe-cli/v3/internal/legacy/legacymodel"
1112
"github.com/ooni/probe-cli/v3/internal/legacy/mockable"
1213
"github.com/ooni/probe-cli/v3/internal/legacy/netx"
13-
"github.com/ooni/probe-cli/v3/internal/legacy/tracex"
1414
"github.com/ooni/probe-cli/v3/internal/model"
1515
"github.com/ooni/probe-cli/v3/internal/netxlite"
1616
)
@@ -159,7 +159,7 @@ func (FakeMethodSuccessful) Name() string {
159159
func (meth FakeMethodSuccessful) Run(ctx context.Context, config hirl.MethodConfig) {
160160
config.Out <- hirl.MethodResult{
161161
Name: meth.Name(),
162-
Received: tracex.MaybeBinaryValue{Value: "antani"},
162+
Received: legacymodel.ArchivalMaybeBinaryData{Value: "antani"},
163163
Sent: "antani",
164164
Tampering: false,
165165
}
@@ -174,7 +174,7 @@ func (FakeMethodFailure) Name() string {
174174
func (meth FakeMethodFailure) Run(ctx context.Context, config hirl.MethodConfig) {
175175
config.Out <- hirl.MethodResult{
176176
Name: meth.Name(),
177-
Received: tracex.MaybeBinaryValue{Value: "antani"},
177+
Received: legacymodel.ArchivalMaybeBinaryData{Value: "antani"},
178178
Sent: "melandri",
179179
Tampering: true,
180180
}

internal/experiment/quicping/quicping.go

+16-15
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717

1818
_ "crypto/sha256"
1919

20+
"github.com/ooni/probe-cli/v3/internal/legacy/legacymodel"
2021
"github.com/ooni/probe-cli/v3/internal/legacy/tracex"
2122
"github.com/ooni/probe-cli/v3/internal/model"
2223
)
@@ -78,26 +79,26 @@ type TestKeys struct {
7879

7980
// SinglePing is a result of a single ping operation.
8081
type SinglePing struct {
81-
ConnIdDst string `json:"conn_id_dst"`
82-
ConnIdSrc string `json:"conn_id_src"`
83-
Failure *string `json:"failure"`
84-
Request *model.ArchivalMaybeBinaryData `json:"request"`
85-
T float64 `json:"t"`
86-
Responses []*SinglePingResponse `json:"responses"`
82+
ConnIdDst string `json:"conn_id_dst"`
83+
ConnIdSrc string `json:"conn_id_src"`
84+
Failure *string `json:"failure"`
85+
Request *legacymodel.ArchivalMaybeBinaryData `json:"request"`
86+
T float64 `json:"t"`
87+
Responses []*SinglePingResponse `json:"responses"`
8788
}
8889

8990
type SinglePingResponse struct {
90-
Data *model.ArchivalMaybeBinaryData `json:"response_data"`
91-
Failure *string `json:"failure"`
92-
T float64 `json:"t"`
93-
SupportedVersions []uint32 `json:"supported_versions"`
91+
Data *legacymodel.ArchivalMaybeBinaryData `json:"response_data"`
92+
Failure *string `json:"failure"`
93+
T float64 `json:"t"`
94+
SupportedVersions []uint32 `json:"supported_versions"`
9495
}
9596

9697
// makeResponse is a utility function to create a SinglePingResponse
9798
func makeResponse(resp *responseInfo) *SinglePingResponse {
98-
var data *model.ArchivalMaybeBinaryData
99+
var data *legacymodel.ArchivalMaybeBinaryData
99100
if resp.raw != nil {
100-
data = &model.ArchivalMaybeBinaryData{Value: string(resp.raw)}
101+
data = &legacymodel.ArchivalMaybeBinaryData{Value: string(resp.raw)}
101102
}
102103
return &SinglePingResponse{
103104
Data: data,
@@ -272,7 +273,7 @@ L:
272273
ConnIdDst: req.dstID,
273274
ConnIdSrc: req.srcID,
274275
Failure: tracex.NewFailure(req.err),
275-
Request: &model.ArchivalMaybeBinaryData{Value: string(req.raw)},
276+
Request: &legacymodel.ArchivalMaybeBinaryData{Value: string(req.raw)},
276277
T: req.t,
277278
})
278279
continue
@@ -312,7 +313,7 @@ L:
312313
ConnIdDst: ping.request.dstID,
313314
ConnIdSrc: ping.request.srcID,
314315
Failure: tracex.NewFailure(timeoutErr),
315-
Request: &model.ArchivalMaybeBinaryData{Value: string(ping.request.raw)},
316+
Request: &legacymodel.ArchivalMaybeBinaryData{Value: string(ping.request.raw)},
316317
T: ping.request.t,
317318
})
318319
continue
@@ -325,7 +326,7 @@ L:
325326
ConnIdDst: ping.request.dstID,
326327
ConnIdSrc: ping.request.srcID,
327328
Failure: nil,
328-
Request: &model.ArchivalMaybeBinaryData{Value: string(ping.request.raw)},
329+
Request: &legacymodel.ArchivalMaybeBinaryData{Value: string(ping.request.raw)},
329330
T: ping.request.t,
330331
Responses: responses,
331332
})
+60
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
package legacymodel
2+
3+
import (
4+
"encoding/base64"
5+
"encoding/json"
6+
"errors"
7+
"unicode/utf8"
8+
)
9+
10+
// ArchivalMaybeBinaryData is a possibly binary string. We use this helper class
11+
// to define a custom JSON encoder that allows us to choose the proper
12+
// representation depending on whether the Value field is valid UTF-8 or not.
13+
//
14+
// See https://github.com/ooni/spec/blob/master/data-formats/df-001-httpt.md#maybebinarydata
15+
//
16+
// Deprecated: do not use this type in new code.
17+
//
18+
// Removing this struct is TODO(https://github.com/ooni/probe/issues/2543).
19+
type ArchivalMaybeBinaryData struct {
20+
Value string
21+
}
22+
23+
// MarshalJSON marshals a string-like to JSON following the OONI spec that
24+
// says that UTF-8 content is represented as string and non-UTF-8 content is
25+
// instead represented using `{"format":"base64","data":"..."}`.
26+
func (hb ArchivalMaybeBinaryData) MarshalJSON() ([]byte, error) {
27+
// if we can serialize as UTF-8 string, do that
28+
if utf8.ValidString(hb.Value) {
29+
return json.Marshal(hb.Value)
30+
}
31+
32+
// otherwise fallback to the ooni/spec representation for binary data
33+
er := make(map[string]string)
34+
er["format"] = "base64"
35+
er["data"] = base64.StdEncoding.EncodeToString([]byte(hb.Value))
36+
return json.Marshal(er)
37+
}
38+
39+
// UnmarshalJSON is the opposite of MarshalJSON.
40+
func (hb *ArchivalMaybeBinaryData) UnmarshalJSON(d []byte) error {
41+
if err := json.Unmarshal(d, &hb.Value); err == nil {
42+
return nil
43+
}
44+
er := make(map[string]string)
45+
if err := json.Unmarshal(d, &er); err != nil {
46+
return err
47+
}
48+
if v, ok := er["format"]; !ok || v != "base64" {
49+
return errors.New("missing or invalid format field")
50+
}
51+
if _, ok := er["data"]; !ok {
52+
return errors.New("missing data field")
53+
}
54+
b64, err := base64.StdEncoding.DecodeString(er["data"])
55+
if err != nil {
56+
return err
57+
}
58+
hb.Value = string(b64)
59+
return nil
60+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
package legacymodel_test
2+
3+
import (
4+
"testing"
5+
6+
"github.com/google/go-cmp/cmp"
7+
"github.com/ooni/probe-cli/v3/internal/legacy/legacymodel"
8+
)
9+
10+
// we use this value below to test we can handle binary data
11+
var archivalBinaryInput = []uint8{
12+
0x57, 0xe5, 0x79, 0xfb, 0xa6, 0xbb, 0x0d, 0xbc, 0xce, 0xbd, 0xa7, 0xa0,
13+
0xba, 0xa4, 0x78, 0x78, 0x12, 0x59, 0xee, 0x68, 0x39, 0xa4, 0x07, 0x98,
14+
0xc5, 0x3e, 0xbc, 0x55, 0xcb, 0xfe, 0x34, 0x3c, 0x7e, 0x1b, 0x5a, 0xb3,
15+
0x22, 0x9d, 0xc1, 0x2d, 0x6e, 0xca, 0x5b, 0xf1, 0x10, 0x25, 0x47, 0x1e,
16+
0x44, 0xe2, 0x2d, 0x60, 0x08, 0xea, 0xb0, 0x0a, 0xcc, 0x05, 0x48, 0xa0,
17+
0xf5, 0x78, 0x38, 0xf0, 0xdb, 0x3f, 0x9d, 0x9f, 0x25, 0x6f, 0x89, 0x00,
18+
0x96, 0x93, 0xaf, 0x43, 0xac, 0x4d, 0xc9, 0xac, 0x13, 0xdb, 0x22, 0xbe,
19+
0x7a, 0x7d, 0xd9, 0x24, 0xa2, 0x52, 0x69, 0xd8, 0x89, 0xc1, 0xd1, 0x57,
20+
0xaa, 0x04, 0x2b, 0xa2, 0xd8, 0xb1, 0x19, 0xf6, 0xd5, 0x11, 0x39, 0xbb,
21+
0x80, 0xcf, 0x86, 0xf9, 0x5f, 0x9d, 0x8c, 0xab, 0xf5, 0xc5, 0x74, 0x24,
22+
0x3a, 0xa2, 0xd4, 0x40, 0x4e, 0xd7, 0x10, 0x1f,
23+
}
24+
25+
// we use this value below to test we can handle binary data
26+
var archivalEncodedBinaryInput = []byte(`{"data":"V+V5+6a7DbzOvaeguqR4eBJZ7mg5pAeYxT68Vcv+NDx+G1qzIp3BLW7KW/EQJUceROItYAjqsArMBUig9Xg48Ns/nZ8lb4kAlpOvQ6xNyawT2yK+en3ZJKJSadiJwdFXqgQrotixGfbVETm7gM+G+V+djKv1xXQkOqLUQE7XEB8=","format":"base64"}`)
27+
28+
func TestArchivalMaybeBinaryData(t *testing.T) {
29+
t.Run("MarshalJSON", func(t *testing.T) {
30+
tests := []struct {
31+
name string // test name
32+
input string // value to marshal
33+
want []byte // expected result
34+
wantErr bool // whether we expect an error
35+
}{{
36+
name: "with string input",
37+
input: "antani",
38+
want: []byte(`"antani"`),
39+
wantErr: false,
40+
}, {
41+
name: "with binary input",
42+
input: string(archivalBinaryInput),
43+
want: archivalEncodedBinaryInput,
44+
wantErr: false,
45+
}}
46+
for _, tt := range tests {
47+
t.Run(tt.name, func(t *testing.T) {
48+
hb := legacymodel.ArchivalMaybeBinaryData{
49+
Value: tt.input,
50+
}
51+
got, err := hb.MarshalJSON()
52+
if (err != nil) != tt.wantErr {
53+
t.Fatalf("ArchivalMaybeBinaryData.MarshalJSON() error = %v, wantErr %v", err, tt.wantErr)
54+
}
55+
if diff := cmp.Diff(tt.want, got); diff != "" {
56+
t.Fatal(diff)
57+
}
58+
})
59+
}
60+
})
61+
62+
t.Run("UnmarshalJSON", func(t *testing.T) {
63+
tests := []struct {
64+
name string // test name
65+
input []byte // value to unmarshal
66+
want string // expected result
67+
wantErr bool // whether we want an error
68+
}{{
69+
name: "with string input",
70+
input: []byte(`"xo"`),
71+
want: "xo",
72+
wantErr: false,
73+
}, {
74+
name: "with nil input",
75+
input: nil,
76+
want: "",
77+
wantErr: true,
78+
}, {
79+
name: "with missing/invalid format",
80+
input: []byte(`{"format": "foo"}`),
81+
want: "",
82+
wantErr: true,
83+
}, {
84+
name: "with missing data",
85+
input: []byte(`{"format": "base64"}`),
86+
want: "",
87+
wantErr: true,
88+
}, {
89+
name: "with invalid base64 data",
90+
input: []byte(`{"format": "base64", "data": "x"}`),
91+
want: "",
92+
wantErr: true,
93+
}, {
94+
name: "with valid base64 data",
95+
input: archivalEncodedBinaryInput,
96+
want: string(archivalBinaryInput),
97+
wantErr: false,
98+
}}
99+
for _, tt := range tests {
100+
t.Run(tt.name, func(t *testing.T) {
101+
hb := &legacymodel.ArchivalMaybeBinaryData{}
102+
if err := hb.UnmarshalJSON(tt.input); (err != nil) != tt.wantErr {
103+
t.Fatalf("ArchivalMaybeBinaryData.UnmarshalJSON() error = %v, wantErr %v", err, tt.wantErr)
104+
}
105+
if d := cmp.Diff(tt.want, hb.Value); d != "" {
106+
t.Fatal(d)
107+
}
108+
})
109+
}
110+
})
111+
}

internal/legacy/legacymodel/doc.go

+2
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
// Package legacymodel contains legacy content that used to be in internal/model
2+
package legacymodel

internal/legacy/tracex/archival.go

-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ type (
2121
ExtSpec = model.ArchivalExtSpec
2222
TCPConnectEntry = model.ArchivalTCPConnectResult
2323
TCPConnectStatus = model.ArchivalTCPConnectStatus
24-
MaybeBinaryValue = model.ArchivalMaybeBinaryData
2524
DNSQueryEntry = model.ArchivalDNSLookupResult
2625
DNSAnswerEntry = model.ArchivalDNSAnswer
2726
TLSHandshake = model.ArchivalTLSOrQUICHandshakeResult

internal/model/archival.go

-53
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ package model
1212

1313
import (
1414
"bytes"
15-
"encoding/base64"
1615
"encoding/json"
1716
"errors"
1817
"fmt"
@@ -172,58 +171,6 @@ func (value *ArchivalScrubbedMaybeBinaryString) UnmarshalJSON(rawData []byte) er
172171
return nil
173172
}
174173

175-
// ArchivalMaybeBinaryData is a possibly binary string. We use this helper class
176-
// to define a custom JSON encoder that allows us to choose the proper
177-
// representation depending on whether the Value field is valid UTF-8 or not.
178-
//
179-
// See https://github.com/ooni/spec/blob/master/data-formats/df-001-httpt.md#maybebinarydata
180-
//
181-
// Deprecated: do not use this type in new code.
182-
//
183-
// Removing this struct is TODO(https://github.com/ooni/probe/issues/2543).
184-
type ArchivalMaybeBinaryData struct {
185-
Value string
186-
}
187-
188-
// MarshalJSON marshals a string-like to JSON following the OONI spec that
189-
// says that UTF-8 content is represented as string and non-UTF-8 content is
190-
// instead represented using `{"format":"base64","data":"..."}`.
191-
func (hb ArchivalMaybeBinaryData) MarshalJSON() ([]byte, error) {
192-
// if we can serialize as UTF-8 string, do that
193-
if utf8.ValidString(hb.Value) {
194-
return json.Marshal(hb.Value)
195-
}
196-
197-
// otherwise fallback to the ooni/spec representation for binary data
198-
er := make(map[string]string)
199-
er["format"] = "base64"
200-
er["data"] = base64.StdEncoding.EncodeToString([]byte(hb.Value))
201-
return json.Marshal(er)
202-
}
203-
204-
// UnmarshalJSON is the opposite of MarshalJSON.
205-
func (hb *ArchivalMaybeBinaryData) UnmarshalJSON(d []byte) error {
206-
if err := json.Unmarshal(d, &hb.Value); err == nil {
207-
return nil
208-
}
209-
er := make(map[string]string)
210-
if err := json.Unmarshal(d, &er); err != nil {
211-
return err
212-
}
213-
if v, ok := er["format"]; !ok || v != "base64" {
214-
return errors.New("missing or invalid format field")
215-
}
216-
if _, ok := er["data"]; !ok {
217-
return errors.New("missing data field")
218-
}
219-
b64, err := base64.StdEncoding.DecodeString(er["data"])
220-
if err != nil {
221-
return err
222-
}
223-
hb.Value = string(b64)
224-
return nil
225-
}
226-
227174
//
228175
// DNS lookup
229176
//

0 commit comments

Comments
 (0)