cleanup: common funcs for setting headers (ooni#1332)

I was trying to make sure the code was using a new definition of headers
based on ArchivalMaybeBinaryString, but I stumbled upon a roadblock
where there were too many code paths settings headers.

I did not feel comfortable changing multiple code paths, so here is
instead a diff that unifies setting headers.

The slightly tricky part of this diff has been the requirement that we
preserve headers' capitalization for hhfm. The catch seems to be that we
should not use http.Header for setting the headers in hhfm, but rather
pass through map[string][]string, which is not attached case
normalization setters. I think we're fine in this department because we
already had test cases and I added a bunch more test cases.

After this diff is merged, I can resume with my plan to make headers use
ArchivalMaybeBinaryString, which, in turn, is functional to
automatically scrub headers, which is something we would like to happen
in light of the introduction of more happy eyeballs code due to
ooni/probe#2531.

internal/experiment/hhfm/hhfm.go

@@ -11,7 +11,6 @@ import (
 	"fmt"
 	"net"
 	"net/http"
-	"sort"
 	"time"
 
 	"github.com/ooni/probe-cli/v3/internal/experiment/urlgetter"
@@ -246,27 +245,32 @@ func (tk *TestKeys) FillTampering(
 	}
 }
 
+// newHeadersFromMap converts the definition of headers used when sending the
+// request to something that looks like http.Header. QUIRK: because we need to
+// preserve the original random casing of headers (which is what we are in
+// fact testing with this experiment), the implementation of this func should
+// stay clear of using ordinary http.Header and specifically its .Set func.
+func newHeadersFromMap(input map[string]string) map[string][]string {
+	out := map[string][]string{}
+	for key, value := range input {
+		out[key] = []string{value}
+	}
+	return out
+}
+
 // NewRequestEntryList creates a new []tracex.RequestEntry given a
 // specific *http.Request and headers with random case.
 func NewRequestEntryList(req *http.Request, headers map[string]string) (out []tracex.RequestEntry) {
+	// Note: using the random capitalization headers here
+	realHeaders := newHeadersFromMap(headers)
 	out = []tracex.RequestEntry{{
 		Request: tracex.HTTPRequest{
-			Headers:     make(map[string]tracex.MaybeBinaryValue),
-			HeadersList: []tracex.HTTPHeader{},
+			Headers:     model.ArchivalNewHTTPHeadersMap(realHeaders),
+			HeadersList: model.ArchivalNewHTTPHeadersList(realHeaders),
 			Method:      req.Method,
 			URL:         req.URL.String(),
 		},
 	}}
-	for key, value := range headers {
-		// Using the random capitalization headers here
-		mbv := tracex.MaybeBinaryValue{Value: value}
-		out[0].Request.Headers[key] = mbv
-		out[0].Request.HeadersList = append(out[0].Request.HeadersList,
-			tracex.HTTPHeader{Key: key, Value: mbv})
-	}
-	sort.Slice(out[0].Request.HeadersList, func(i, j int) bool {
-		return out[0].Request.HeadersList[i].Key < out[0].Request.HeadersList[j].Key
-	})
 	return
 }
 
@@ -276,17 +280,9 @@ func NewHTTPResponse(resp *http.Response, data []byte) (out tracex.HTTPResponse)
 	out = tracex.HTTPResponse{
 		Body:        model.ArchivalMaybeBinaryString(data),
 		Code:        int64(resp.StatusCode),
-		Headers:     make(map[string]tracex.MaybeBinaryValue),
-		HeadersList: []tracex.HTTPHeader{},
-	}
-	for key := range resp.Header {
-		mbv := tracex.MaybeBinaryValue{Value: resp.Header.Get(key)}
-		out.Headers[key] = mbv
-		out.HeadersList = append(out.HeadersList, tracex.HTTPHeader{Key: key, Value: mbv})
+		Headers:     model.ArchivalNewHTTPHeadersMap(resp.Header),
+		HeadersList: model.ArchivalNewHTTPHeadersList(resp.Header),
 	}
-	sort.Slice(out.HeadersList, func(i, j int) bool {
-		return out.HeadersList[i].Key < out.HeadersList[j].Key
-	})
 	return
 }
 

internal/experiment/hhfm/hhfm_internal_test.go

@@ -0,0 +1,49 @@
+package hhfm
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestNewHeadersFromMap(t *testing.T) {
+
+	// testcase is a test case run by this func
+	type testcase struct {
+		name   string
+		input  map[string]string
+		expect map[string][]string
+	}
+
+	cases := []testcase{{
+		name:   "with nil input",
+		input:  nil,
+		expect: http.Header{},
+	}, {
+		name:   "with empty input",
+		input:  map[string]string{},
+		expect: http.Header{},
+	}, {
+		name: "common case: headers with mixed casing should be preserved",
+		input: map[string]string{
+			"ConTent-TyPe": "text/html; charset=utf-8",
+			"ViA":          "a",
+			"User-AgeNt":   "miniooni/0.1.0",
+		},
+		expect: map[string][]string{
+			"ConTent-TyPe": {"text/html; charset=utf-8"},
+			"ViA":          {"a"},
+			"User-AgeNt":   {"miniooni/0.1.0"},
+		},
+	}}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := newHeadersFromMap(tc.input)
+			if diff := cmp.Diff(tc.expect, got); diff != "" {
+				t.Fatal(diff)
+			}
+		})
+	}
+}

internal/legacy/tracex/archival.go

@@ -7,8 +7,6 @@ package tracex
 import (
 	"errors"
 	"net"
-	"net/http"
-	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -95,31 +93,6 @@ func NewFailedOperation(err error) *string {
 	return &s
 }
 
-// httpAddHeaders adds the headers inside source into destList and destMap.
-func httpAddHeaders(source http.Header, destList *[]HTTPHeader,
-	destMap *map[string]MaybeBinaryValue) {
-	*destList = []HTTPHeader{}
-	*destMap = make(map[string]model.ArchivalMaybeBinaryData)
-	for key, values := range source {
-		for index, value := range values {
-			value := MaybeBinaryValue{Value: value}
-			// With the map representation we can only represent a single
-			// value for every key. Hence the list representation.
-			if index == 0 {
-				(*destMap)[key] = value
-			}
-			*destList = append(*destList, HTTPHeader{
-				Key:   key,
-				Value: value,
-			})
-		}
-	}
-	// Sorting helps with unit testing (map keys are unordered)
-	sort.Slice(*destList, func(i, j int) bool {
-		return (*destList)[i].Key < (*destList)[j].Key
-	})
-}
-
 // NewRequestList returns the list for "requests"
 func NewRequestList(begin time.Time, events []Event) (out []RequestEntry) {
 	// OONI wants the last request to appear first
@@ -137,13 +110,13 @@ func newRequestList(begin time.Time, events []Event) (out []RequestEntry) {
 		case *EventHTTPTransactionDone:
 			entry := RequestEntry{}
 			entry.T = ev.Time.Sub(begin).Seconds()
-			httpAddHeaders(
-				ev.HTTPRequestHeaders, &entry.Request.HeadersList, &entry.Request.Headers)
+			entry.Request.Headers = model.ArchivalNewHTTPHeadersMap(ev.HTTPRequestHeaders)
+			entry.Request.HeadersList = model.ArchivalNewHTTPHeadersList(ev.HTTPRequestHeaders)
 			entry.Request.Method = ev.HTTPMethod
 			entry.Request.URL = ev.HTTPURL
 			entry.Request.Transport = ev.Transport
-			httpAddHeaders(
-				ev.HTTPResponseHeaders, &entry.Response.HeadersList, &entry.Response.Headers)
+			entry.Response.Headers = model.ArchivalNewHTTPHeadersMap(ev.HTTPResponseHeaders)
+			entry.Response.HeadersList = model.ArchivalNewHTTPHeadersList(ev.HTTPResponseHeaders)
 			entry.Response.Code = int64(ev.HTTPStatusCode)
 			entry.Response.Locations = ev.HTTPResponseHeaders.Values("Location")
 			entry.Response.Body = model.ArchivalMaybeBinaryString(ev.HTTPResponseBody)

internal/measurexlite/http.go

@@ -6,7 +6,6 @@ package measurexlite
 
 import (
 	"net/http"
-	"sort"
 	"time"
 
 	"github.com/ooni/probe-cli/v3/internal/model"
@@ -91,7 +90,7 @@ func newHTTPRequestHeaderList(req *http.Request) []model.ArchivalHTTPHeader {
 	if req != nil {
 		m = req.Header
 	}
-	return newHTTPHeaderList(m)
+	return model.ArchivalNewHTTPHeadersList(m)
 }
 
 // newHTTPRequestHeaderMap calls newHTTPHeaderMap with the request headers or
@@ -101,7 +100,7 @@ func newHTTPRequestHeaderMap(req *http.Request) map[string]model.ArchivalMaybeBi
 	if req != nil {
 		m = req.Header
 	}
-	return newHTTPHeaderMap(m)
+	return model.ArchivalNewHTTPHeadersMap(m)
 }
 
 // httpRequestURL returns the req.URL.String() or an empty string.
@@ -135,7 +134,7 @@ func newHTTPResponseHeaderList(resp *http.Response) (out []model.ArchivalHTTPHea
 	if resp != nil {
 		m = resp.Header
 	}
-	return newHTTPHeaderList(m)
+	return model.ArchivalNewHTTPHeadersList(m)
 }
 
 // newHTTPResponseHeaderMap calls newHTTPHeaderMap with the request headers or
@@ -145,7 +144,7 @@ func newHTTPResponseHeaderMap(resp *http.Response) (out map[string]model.Archiva
 	if resp != nil {
 		m = resp.Header
 	}
-	return newHTTPHeaderMap(m)
+	return model.ArchivalNewHTTPHeadersMap(m)
 }
 
 // httpResponseLocations returns the locations inside the response (if possible)
@@ -159,43 +158,3 @@ func httpResponseLocations(resp *http.Response) []string {
 	}
 	return []string{loc.String()}
 }
-
-// newHTTPHeaderList creates a list representation of HTTP headers
-func newHTTPHeaderList(header http.Header) (out []model.ArchivalHTTPHeader) {
-	out = []model.ArchivalHTTPHeader{}
-	keys := []string{}
-	for key := range header {
-		keys = append(keys, key)
-	}
-
-	// ensure the output is consistent, which helps with testing;
-	// for an example of why we need to sort headers, see
-	// https://github.com/ooni/probe-engine/pull/751/checks?check_run_id=853562310
-	sort.Strings(keys)
-
-	for _, key := range keys {
-		for _, value := range header[key] {
-			out = append(out, model.ArchivalHTTPHeader{
-				Key: key,
-				Value: model.ArchivalMaybeBinaryData{
-					Value: value,
-				},
-			})
-		}
-	}
-	return
-}
-
-// newHTTPHeaderMap creates a map representation of HTTP headers
-func newHTTPHeaderMap(header http.Header) (out map[string]model.ArchivalMaybeBinaryData) {
-	out = make(map[string]model.ArchivalMaybeBinaryData)
-	for key, values := range header {
-		for _, value := range values {
-			out[key] = model.ArchivalMaybeBinaryData{
-				Value: value,
-			}
-			break
-		}
-	}
-	return
-}

internal/model/archival.go

@@ -16,6 +16,8 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"net/http"
+	"sort"
 	"unicode/utf8"
 )
 
@@ -349,6 +351,49 @@ type ArchivalHTTPResponse struct {
 	Locations []string `json:"-"`
 }
 
+// ArchivalNewHTTPHeadersList constructs a new ArchivalHTTPHeader list given HTTP headers.
+func ArchivalNewHTTPHeadersList(source http.Header) (out []ArchivalHTTPHeader) {
+	out = []ArchivalHTTPHeader{}
+
+	// obtain the header keys
+	keys := []string{}
+	for key := range source {
+		keys = append(keys, key)
+	}
+
+	// ensure the output is consistent, which helps with testing;
+	// for an example of why we need to sort headers, see
+	// https://github.com/ooni/probe-engine/pull/751/checks?check_run_id=853562310
+	sort.Strings(keys)
+
+	// insert into the output list
+	for _, key := range keys {
+		for _, value := range source[key] {
+			out = append(out, ArchivalHTTPHeader{
+				Key: key,
+				Value: ArchivalMaybeBinaryData{
+					Value: value,
+				},
+			})
+		}
+	}
+	return
+}
+
+// ArchivalNewHTTPHeadersMap creates a map representation of HTTP headers
+func ArchivalNewHTTPHeadersMap(header http.Header) (out map[string]ArchivalMaybeBinaryData) {
+	out = make(map[string]ArchivalMaybeBinaryData)
+	for key, values := range header {
+		for _, value := range values {
+			out[key] = ArchivalMaybeBinaryData{
+				Value: value,
+			}
+			break // just the first header
+		}
+	}
+	return
+}
+
 // ArchivalHTTPHeader is a single HTTP header.
 type ArchivalHTTPHeader struct {
 	Key   string