Skip to content

Commit

Permalink
cleanup(model): use ArchivalMaybeBinaryString for HTTP bodies (ooni#1331
Browse files Browse the repository at this point in the history
)

This diff modifies the model for HTTP requests and responses to use the
ArchivalBinaryString type to represent bodies.

The tests I have introduced in previous commits are still passing, so we
can have good confidence that this change is actually WAI.

Part of ooni/probe#2531
  • Loading branch information
bassosimone authored and Murphy-OrangeMud committed Feb 13, 2024
1 parent e3e60d9 commit 6daa1e6
Show file tree
Hide file tree
Showing 14 changed files with 90 additions and 126 deletions.
2 changes: 1 addition & 1 deletion internal/experiment/hhfm/hhfm.go
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ func NewRequestEntryList(req *http.Request, headers map[string]string) (out []tr
// specific *http.Response instance and its body.
func NewHTTPResponse(resp *http.Response, data []byte) (out tracex.HTTPResponse) {
out = tracex.HTTPResponse{
Body: tracex.HTTPBody{Value: string(data)},
Body: model.ArchivalMaybeBinaryString(data),
Code: int64(resp.StatusCode),
Headers: make(map[string]tracex.MaybeBinaryValue),
HeadersList: []tracex.HTTPHeader{},
Expand Down
12 changes: 6 additions & 6 deletions internal/experiment/hhfm/hhfm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func TestSuccess(t *testing.T) {
if request.Failure != nil {
t.Fatal("invalid Requests[0].Failure")
}
if request.Request.Body.Value != "" {
if request.Request.Body != "" {
t.Fatal("invalid Requests[0].Request.Body.Value")
}
if request.Request.BodyIsTruncated != false {
Expand Down Expand Up @@ -99,7 +99,7 @@ func TestSuccess(t *testing.T) {
if request.Request.URL != ths[0].Address {
t.Fatal("invalid Requests[0].Request.URL")
}
if len(request.Response.Body.Value) < 1 {
if len(request.Response.Body) < 1 {
t.Fatal("invalid Requests[0].Response.Body.Value length")
}
if request.Response.BodyIsTruncated != false {
Expand Down Expand Up @@ -181,7 +181,7 @@ func TestCancelledContext(t *testing.T) {
if *request.Failure != netxlite.FailureInterrupted {
t.Fatal("invalid Requests[0].Failure")
}
if request.Request.Body.Value != "" {
if request.Request.Body != "" {
t.Fatal("invalid Requests[0].Request.Body.Value")
}
if request.Request.BodyIsTruncated != false {
Expand Down Expand Up @@ -212,7 +212,7 @@ func TestCancelledContext(t *testing.T) {
if request.Request.URL != ths[0].Address {
t.Fatal("invalid Requests[0].Request.URL")
}
if len(request.Response.Body.Value) != 0 {
if len(request.Response.Body) != 0 {
t.Fatal("invalid Requests[0].Response.Body.Value length")
}
if request.Response.BodyIsTruncated != false {
Expand Down Expand Up @@ -811,7 +811,7 @@ func TestNewHTTPResponse(t *testing.T) {
data: []byte("deadbeef"),
},
wantOut: tracex.HTTPResponse{
Body: tracex.MaybeBinaryValue{Value: "deadbeef"},
Body: model.ArchivalMaybeBinaryString("deadbeef"),
Code: 200,
HeadersList: []tracex.HTTPHeader{{
Key: "Content-Type",
Expand All @@ -831,7 +831,7 @@ func TestNewHTTPResponse(t *testing.T) {
resp: &http.Response{StatusCode: 200},
},
wantOut: tracex.HTTPResponse{
Body: tracex.MaybeBinaryValue{Value: ""},
Body: model.ArchivalMaybeBinaryString(""),
Code: 200,
HeadersList: []tracex.HTTPHeader{},
Headers: map[string]tracex.MaybeBinaryValue{},
Expand Down
2 changes: 1 addition & 1 deletion internal/experiment/riseupvpn/riseupvpn.go
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ func parseGateways(testKeys *TestKeys) []GatewayV3 {
// TODO(bassosimone,cyberta): is it reasonable that we discard
// the error when the JSON we fetched cannot be parsed?
// See https://github.com/ooni/probe/issues/1432
eipService, err := DecodeEIP3(requestEntry.Response.Body.Value)
eipService, err := DecodeEIP3(string(requestEntry.Response.Body))
if err == nil {
return eipService.Gateways
}
Expand Down
8 changes: 4 additions & 4 deletions internal/experiment/riseupvpn/riseupvpn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -773,13 +773,13 @@ func generateMockGetter(requestResponse map[string]string, responseStatus map[st
Failure: failure,
Request: tracex.HTTPRequest{
URL: url,
Body: tracex.MaybeBinaryValue{},
Body: model.ArchivalMaybeBinaryString(""),
BodyIsTruncated: false,
},
Response: tracex.HTTPResponse{
Body: tracex.HTTPBody{
Value: responseBody,
},
Body: model.ArchivalMaybeBinaryString(
responseBody,
),
BodyIsTruncated: false,
}},
},
Expand Down
2 changes: 1 addition & 1 deletion internal/experiment/urlgetter/getter.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func (g Getter) Get(ctx context.Context) (TestKeys, error) {
if len(tk.Requests) > 0 {
// OONI's convention is that the last request appears first
tk.HTTPResponseStatus = tk.Requests[0].Response.Code
tk.HTTPResponseBody = tk.Requests[0].Response.Body.Value
tk.HTTPResponseBody = string(tk.Requests[0].Response.Body)
tk.HTTPResponseLocations = tk.Requests[0].Response.Locations
}
tk.TCPConnect = append(
Expand Down
4 changes: 2 additions & 2 deletions internal/experiment/webconnectivity/httpanalysis.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func HTTPBodyLengthChecks(
if response.BodyIsTruncated {
return
}
measurement := int64(len(response.Body.Value))
measurement := int64(len(response.Body))
if measurement <= 0 {
return
}
Expand Down Expand Up @@ -201,7 +201,7 @@ func HTTPTitleMatch(tk urlgetter.TestKeys, ctrl ControlResponse) (out *bool) {
return
}
control := ctrl.HTTPRequest.Title
measurementBody := response.Body.Value
measurementBody := string(response.Body)
measurement := measurexlite.WebGetTitle(measurementBody)
if measurement == "" {
return
Expand Down
55 changes: 28 additions & 27 deletions internal/experiment/webconnectivity/httpanalysis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/ooni/probe-cli/v3/internal/experiment/urlgetter"
"github.com/ooni/probe-cli/v3/internal/experiment/webconnectivity"
"github.com/ooni/probe-cli/v3/internal/legacy/tracex"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/randx"
)

Expand Down Expand Up @@ -76,9 +77,9 @@ func TestHTTPBodyLengthChecks(t *testing.T) {
tk: urlgetter.TestKeys{
Requests: []tracex.RequestEntry{{
Response: tracex.HTTPResponse{
Body: tracex.MaybeBinaryValue{
Value: randx.Letters(768),
},
Body: model.ArchivalMaybeBinaryString(
randx.Letters(768),
),
},
}},
},
Expand All @@ -95,9 +96,9 @@ func TestHTTPBodyLengthChecks(t *testing.T) {
tk: urlgetter.TestKeys{
Requests: []tracex.RequestEntry{{
Response: tracex.HTTPResponse{
Body: tracex.MaybeBinaryValue{
Value: randx.Letters(768),
},
Body: model.ArchivalMaybeBinaryString(
randx.Letters(768),
),
},
}},
},
Expand All @@ -115,9 +116,9 @@ func TestHTTPBodyLengthChecks(t *testing.T) {
tk: urlgetter.TestKeys{
Requests: []tracex.RequestEntry{{
Response: tracex.HTTPResponse{
Body: tracex.MaybeBinaryValue{
Value: randx.Letters(1024),
},
Body: model.ArchivalMaybeBinaryString(
randx.Letters(1024),
),
},
}},
},
Expand All @@ -135,9 +136,9 @@ func TestHTTPBodyLengthChecks(t *testing.T) {
tk: urlgetter.TestKeys{
Requests: []tracex.RequestEntry{{
Response: tracex.HTTPResponse{
Body: tracex.MaybeBinaryValue{
Value: randx.Letters(8),
},
Body: model.ArchivalMaybeBinaryString(
randx.Letters(8),
),
},
}},
},
Expand All @@ -155,9 +156,9 @@ func TestHTTPBodyLengthChecks(t *testing.T) {
tk: urlgetter.TestKeys{
Requests: []tracex.RequestEntry{{
Response: tracex.HTTPResponse{
Body: tracex.MaybeBinaryValue{
Value: randx.Letters(16),
},
Body: model.ArchivalMaybeBinaryString(
randx.Letters(16),
),
},
}},
},
Expand Down Expand Up @@ -698,7 +699,7 @@ func TestTitleMatch(t *testing.T) {
Requests: []tracex.RequestEntry{{
Response: tracex.HTTPResponse{
Code: 200,
Body: tracex.MaybeBinaryValue{Value: "<HTML/>"},
Body: model.ArchivalMaybeBinaryString("<HTML/>"),
},
}},
},
Expand All @@ -711,7 +712,7 @@ func TestTitleMatch(t *testing.T) {
Requests: []tracex.RequestEntry{{
Response: tracex.HTTPResponse{
Code: 200,
Body: tracex.MaybeBinaryValue{Value: "<HTML/>"},
Body: model.ArchivalMaybeBinaryString("<HTML/>"),
},
}},
},
Expand All @@ -730,8 +731,8 @@ func TestTitleMatch(t *testing.T) {
Requests: []tracex.RequestEntry{{
Response: tracex.HTTPResponse{
Code: 200,
Body: tracex.MaybeBinaryValue{
Value: "<HTML><TITLE>La community di MSN</TITLE></HTML>"},
Body: model.ArchivalMaybeBinaryString(
"<HTML><TITLE>La community di MSN</TITLE></HTML>"),
},
}},
},
Expand All @@ -750,8 +751,8 @@ func TestTitleMatch(t *testing.T) {
Requests: []tracex.RequestEntry{{
Response: tracex.HTTPResponse{
Code: 200,
Body: tracex.MaybeBinaryValue{
Value: "<HTML><TITLE>La communità di MSN</TITLE></HTML>"},
Body: model.ArchivalMaybeBinaryString(
"<HTML><TITLE>La communità di MSN</TITLE></HTML>"),
},
}},
},
Expand All @@ -770,8 +771,8 @@ func TestTitleMatch(t *testing.T) {
Requests: []tracex.RequestEntry{{
Response: tracex.HTTPResponse{
Code: 200,
Body: tracex.MaybeBinaryValue{
Value: "<HTML><TITLE>" + randx.Letters(1024) + "</TITLE></HTML>"},
Body: model.ArchivalMaybeBinaryString(
"<HTML><TITLE>" + randx.Letters(1024) + "</TITLE></HTML>"),
},
}},
},
Expand All @@ -790,8 +791,8 @@ func TestTitleMatch(t *testing.T) {
Requests: []tracex.RequestEntry{{
Response: tracex.HTTPResponse{
Code: 200,
Body: tracex.MaybeBinaryValue{
Value: "<HTML><TiTLe>La commUNity di MSN</tITLE></HTML>"},
Body: model.ArchivalMaybeBinaryString(
"<HTML><TiTLe>La commUNity di MSN</tITLE></HTML>"),
},
}},
},
Expand All @@ -810,8 +811,8 @@ func TestTitleMatch(t *testing.T) {
Requests: []tracex.RequestEntry{{
Response: tracex.HTTPResponse{
Code: 200,
Body: tracex.MaybeBinaryValue{
Value: "<HTML><TiTLe>La commUNity di MSN</tITLE></HTML>"},
Body: model.ArchivalMaybeBinaryString(
"<HTML><TiTLe>La commUNity di MSN</tITLE></HTML>"),
},
}},
},
Expand Down
4 changes: 2 additions & 2 deletions internal/experiment/webconnectivitylte/analysishttpdiff.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func (tk *TestKeys) httpDiffBodyLengthChecks(
if response.BodyIsTruncated {
return // cannot trust body length in this case
}
measurement := int64(len(response.Body.Value))
measurement := int64(len(response.Body))
if measurement <= 0 {
return // no actual length
}
Expand Down Expand Up @@ -230,7 +230,7 @@ func (tk *TestKeys) httpDiffTitleMatch(
return
}
control := ctrl.Title
measurementBody := response.Body.Value
measurementBody := string(response.Body)
measurement := measurexlite.WebGetTitle(measurementBody)
if control == "" || measurement == "" {
return
Expand Down
3 changes: 1 addition & 2 deletions internal/legacy/tracex/archival.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ type (
DNSQueryEntry = model.ArchivalDNSLookupResult
DNSAnswerEntry = model.ArchivalDNSAnswer
TLSHandshake = model.ArchivalTLSOrQUICHandshakeResult
HTTPBody = model.ArchivalHTTPBody
HTTPHeader = model.ArchivalHTTPHeader
RequestEntry = model.ArchivalHTTPRequestResult
HTTPRequest = model.ArchivalHTTPRequest
Expand Down Expand Up @@ -147,7 +146,7 @@ func newRequestList(begin time.Time, events []Event) (out []RequestEntry) {
ev.HTTPResponseHeaders, &entry.Response.HeadersList, &entry.Response.Headers)
entry.Response.Code = int64(ev.HTTPStatusCode)
entry.Response.Locations = ev.HTTPResponseHeaders.Values("Location")
entry.Response.Body.Value = string(ev.HTTPResponseBody)
entry.Response.Body = model.ArchivalMaybeBinaryString(ev.HTTPResponseBody)
entry.Response.BodyIsTruncated = ev.HTTPResponseBodyIsTruncated
entry.Failure = ev.Err.ToFailure()
out = append(out, entry)
Expand Down
8 changes: 2 additions & 6 deletions internal/legacy/tracex/archival_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,7 @@ func TestNewRequestList(t *testing.T) {
T: 0.02,
}, {
Request: HTTPRequest{
Body: MaybeBinaryValue{
Value: "",
},
Body: model.ArchivalMaybeBinaryString(""),
HeadersList: []HTTPHeader{{
Key: "User-Agent",
Value: MaybeBinaryValue{
Expand All @@ -207,9 +205,7 @@ func TestNewRequestList(t *testing.T) {
URL: "https://www.example.com/submit",
},
Response: HTTPResponse{
Body: MaybeBinaryValue{
Value: "{}",
},
Body: model.ArchivalMaybeBinaryString("{}"),
Code: 200,
HeadersList: []HTTPHeader{{
Key: "Server",
Expand Down
12 changes: 2 additions & 10 deletions internal/measurexlite/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func NewArchivalHTTPRequestResult(index int64, started time.Duration, network, a
ALPN: alpn,
Failure: NewFailure(err),
Request: model.ArchivalHTTPRequest{
Body: model.ArchivalMaybeBinaryData{},
Body: model.ArchivalMaybeBinaryString(""),
BodyIsTruncated: false,
HeadersList: newHTTPRequestHeaderList(req),
Headers: newHTTPRequestHeaderMap(req),
Expand All @@ -62,7 +62,7 @@ func NewArchivalHTTPRequestResult(index int64, started time.Duration, network, a
URL: httpRequestURL(req),
},
Response: model.ArchivalHTTPResponse{
Body: httpResponseBody(body),
Body: model.ArchivalMaybeBinaryString(body),
BodyIsTruncated: httpResponseBodyIsTruncated(body, maxRespBodySize),
Code: httpResponseStatusCode(resp),
HeadersList: newHTTPResponseHeaderList(resp),
Expand Down Expand Up @@ -112,14 +112,6 @@ func httpRequestURL(req *http.Request) (out string) {
return
}

// httpResponseBody returns the response body, if possible, or an empty body.
func httpResponseBody(body []byte) (out model.ArchivalMaybeBinaryData) {
if body != nil {
out.Value = string(body)
}
return
}

// httpResponseBodyIsTruncated determines whether the body is truncated (if possible)
func httpResponseBodyIsTruncated(body []byte, maxSnapSize int64) (out bool) {
if len(body) > 0 && maxSnapSize > 0 {
Expand Down
Loading

0 comments on commit 6daa1e6

Please sign in to comment.