Skip to content

Commit

Permalink
cleanup(model): use ArchivalMaybeBinaryString for HTTP bodies (#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 Sep 28, 2023
1 parent b9282c7 commit 0d5702c
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 0d5702c

Please sign in to comment.