refactor(sessionresolver): minor changes in files and types naming (#810

)

Part of ooni/probe#2135

internal/engine/internal/sessionresolver/clientmaker.go

@@ -1,5 +1,9 @@
 package sessionresolver
 
+//
+// Code for mocking the creation of a client.
+//
+
 import (
 	"github.com/ooni/probe-cli/v3/internal/engine/netx"
 	"github.com/ooni/probe-cli/v3/internal/model"

internal/engine/internal/sessionresolver/doc.go

@@ -0,0 +1,24 @@
+// Package sessionresolver contains the resolver used by the session. This
+// resolver will try to figure out which is the best service for running
+// domain name resolutions and will consistently use it.
+//
+// Occasionally this code will also swap the best resolver with other
+// ~good resolvers to give them a chance to perform.
+//
+// The penalty/reward mechanism is strongly derivative, so the code should
+// adapt ~quickly to changing network conditions. Occasionally, we will
+// have longer resolutions when trying out other resolvers.
+//
+// At the beginning we randomize the known resolvers so that we do not
+// have any preferential ordering. The initial resolutions may be slower
+// if there are many issues with resolvers.
+//
+// The system resolver is given the lowest priority at the beginning
+// but it will of course be the most popular resolver if anything else
+// is failing us. (We will still occasionally probe for other working
+// resolvers and increase their score on success.)
+//
+// We also support a socks5 proxy. When such a proxy is configured,
+// the code WILL skip http3 resolvers AS WELL AS the system
+// resolver, in an attempt to avoid leaking your queries.
+package sessionresolver

internal/engine/internal/sessionresolver/errwrapper.go

@@ -1,23 +1,40 @@
 package sessionresolver
 
+//
+// Error wrapping
+//
+
 import (
 	"errors"
 	"fmt"
 )
 
-// errwrapper wraps an error to include the URL of the
+// errWrapper wraps an error to include the URL of the
 // resolver that we're currently using.
-type errwrapper struct {
-	error
-	URL string
+type errWrapper struct {
+	err error
+	url string
+}
+
+// newErrWrapper creates a new err wrapper.
+func newErrWrapper(err error, URL string) *errWrapper {
+	return &errWrapper{
+		err: err,
+		url: URL,
+	}
 }
 
 // Error implements error.Error.
-func (ew *errwrapper) Error() string {
-	return fmt.Sprintf("<%s> %s", ew.URL, ew.error.Error())
+func (ew *errWrapper) Error() string {
+	return fmt.Sprintf("<%s> %s", ew.url, ew.err.Error())
 }
 
 // Is allows consumers to query for the type of the underlying error.
-func (ew *errwrapper) Is(target error) bool {
-	return errors.Is(ew.error, target)
+func (ew *errWrapper) Is(target error) bool {
+	return errors.Is(ew.err, target)
+}
+
+// Unwrap returns the underlying error.
+func (ew *errWrapper) Unwrap() error {
+	return ew.err
 }

internal/engine/internal/sessionresolver/errwrapper_test.go

@@ -9,10 +9,7 @@ import (
 )
 
 func TestErrWrapper(t *testing.T) {
-	ew := &errwrapper{
-		error: io.EOF,
-		URL:   "https://dns.quad9.net/dns-query",
-	}
+	ew := newErrWrapper(io.EOF, "https://dns.quad9.net/dns-query")
 	o := ew.Error()
 	expect := "<https://dns.quad9.net/dns-query> EOF"
 	if diff := cmp.Diff(expect, o); diff != "" {
@@ -21,4 +18,7 @@ func TestErrWrapper(t *testing.T) {
 	if !errors.Is(ew, io.EOF) {
 		t.Fatal("not the sub-error we expected")
 	}
+	if errors.Unwrap(ew) != io.EOF {
+		t.Fatal("unwrap failed")
+	}
 }

internal/engine/internal/sessionresolver/jsoncodec.go

@@ -0,0 +1,37 @@
+package sessionresolver
+
+//
+// JSON codec
+//
+
+import "encoding/json"
+
+// jsonCodec encodes to/decodes from JSON.
+type jsonCodec interface {
+	// Encode encodes v as a JSON stream of bytes.
+	Encode(v interface{}) ([]byte, error)
+
+	// Decode decodes b from a JSON stream of bytes.
+	Decode(b []byte, v interface{}) error
+}
+
+// codec always returns a valid jsonCodec.
+func (r *Resolver) codec() jsonCodec {
+	if r.jsonCodec != nil {
+		return r.jsonCodec
+	}
+	return &jsonCodecStdlib{}
+}
+
+// jsonCodecStdlib is the default codec.
+type jsonCodecStdlib struct{}
+
+// Decode implements jsonCodec.Decode.
+func (*jsonCodecStdlib) Decode(b []byte, v interface{}) error {
+	return json.Unmarshal(b, v)
+}
+
+// Encode implements jsonCodec.Encode.
+func (*jsonCodecStdlib) Encode(v interface{}) ([]byte, error) {
+	return json.Marshal(v)
+}

internal/engine/internal/sessionresolver/codec_test.go → internal/engine/internal/sessionresolver/jsoncodec_test.go

@@ -6,40 +6,40 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
-type FakeCodec struct {
+type jsonCodecMockable struct {
 	EncodeData []byte
 	EncodeErr  error
 	DecodeErr  error
 }
 
-func (c *FakeCodec) Encode(v interface{}) ([]byte, error) {
+func (c *jsonCodecMockable) Encode(v interface{}) ([]byte, error) {
 	return c.EncodeData, c.EncodeErr
 }
 
-func (c *FakeCodec) Decode(b []byte, v interface{}) error {
+func (c *jsonCodecMockable) Decode(b []byte, v interface{}) error {
 	return c.DecodeErr
 }
 
-func TestCodecCustom(t *testing.T) {
-	c := &FakeCodec{}
-	reso := &Resolver{codec: c}
-	if r := reso.getCodec(); r != c {
+func TestJSONCodecCustom(t *testing.T) {
+	c := &jsonCodecMockable{}
+	reso := &Resolver{jsonCodec: c}
+	if r := reso.codec(); r != c {
 		t.Fatal("not the codec we expected")
 	}
 }
 
-func TestCodecDefault(t *testing.T) {
+func TestJSONCodecDefault(t *testing.T) {
 	reso := &Resolver{}
 	in := resolverinfo{
 		URL:   "https://dns.google/dns.query",
 		Score: 0.99,
 	}
-	data, err := reso.getCodec().Encode(in)
+	data, err := reso.codec().Encode(in)
 	if err != nil {
 		t.Fatal(err)
 	}
 	var out resolverinfo
-	if err := reso.getCodec().Decode(data, &out); err != nil {
+	if err := reso.codec().Decode(data, &out); err != nil {
 		t.Fatal(err)
 	}
 	if diff := cmp.Diff(in, out); diff != "" {

internal/engine/internal/sessionresolver/childresolver.go → internal/engine/internal/sessionresolver/lookup.go

@@ -1,5 +1,9 @@
 package sessionresolver
 
+//
+// Actual lookup code
+//
+
 import (
 	"context"
 	"time"

internal/engine/internal/sessionresolver/sessionresolver.go → internal/engine/internal/sessionresolver/resolver.go

@@ -1,27 +1,8 @@
-// Package sessionresolver contains the resolver used by the session. This
-// resolver will try to figure out which is the best service for running
-// domain name resolutions and will consistently use it.
-//
-// Occasionally this code will also swap the best resolver with other
-// ~good resolvers to give them a chance to perform.
-//
-// The penalty/reward mechanism is strongly derivative, so the code should
-// adapt ~quickly to changing network conditions. Occasionally, we will
-// have longer resolutions when trying out other resolvers.
-//
-// At the beginning we randomize the known resolvers so that we do not
-// have any preferential ordering. The initial resolutions may be slower
-// if there are many issues with resolvers.
+package sessionresolver
+
 //
-// The system resolver is given the lowest priority at the beginning
-// but it will of course be the most popular resolver if anything else
-// is failing us. (We will still occasionally probe for other working
-// resolvers and increase their score on success.)
+// Implementation of Resolver
 //
-// We also support a socks5 proxy. When such a proxy is configured,
-// the code WILL skip http3 resolvers AS WELL AS the system
-// resolver, in an attempt to avoid leaking your queries.
-package sessionresolver
 
 import (
 	"context"
@@ -52,11 +33,8 @@ import (
 //
 // You MUST NOT modify public fields of this structure once it
 // has been created, because that MAY lead to data races.
-//
-// You should create an instance of this structure and use
-// it in internal/engine/session.go.
 type Resolver struct {
-	// ByteCounter is the optional byte counter. It will count
+	// ByteCounter is the OPTIONAL byte counter. It will count
 	// the bytes used by any child resolver except for the
 	// system resolver, whose bytes ARE NOT counted. If this
 	// field is not set, then we won't count the bytes.
@@ -67,21 +45,21 @@ type Resolver struct {
 	// working better in your network.
 	KVStore model.KeyValueStore
 
-	// Logger is the optional logger you want us to use
+	// Logger is the OPTIONAL logger you want us to use
 	// to emit log messages.
 	Logger model.Logger
 
-	// ProxyURL is the optional URL of the socks5 proxy
+	// ProxyURL is the OPTIONAL URL of the socks5 proxy
 	// we should be using. If not set, then we WON'T use
 	// any proxy. If set, then we WON'T use any http3
 	// based resolvers and we WON'T use the system resolver.
 	ProxyURL *url.URL
 
-	// codec is the optional codec to use. If not set, we
-	// will construct a default codec.
-	codec codec
+	// jsonCodec is the OPTIONAL JSON Codec to use. If not set,
+	// we will construct a default codec.
+	jsonCodec jsonCodec
 
-	// dnsClientMaker is the optional dnsclientmaker to
+	// dnsClientMaker is the OPTIONAL dnsclientmaker to
 	// use. If not set, we will use the default.
 	dnsClientMaker dnsclientmaker
 
@@ -111,16 +89,17 @@ func (r *Resolver) Stats() string {
 	return fmt.Sprintf("sessionresolver: %s", string(data))
 }
 
-var errNotImplemented = errors.New("not implemented")
+// errLookupNotImplemented indicates a given lookup type is not implemented.
+var errLookupNotImplemented = errors.New("sessionresolver: lookup not implemented")
 
 // LookupHTTPS implements Resolver.LookupHTTPS.
 func (r *Resolver) LookupHTTPS(ctx context.Context, domain string) (*model.HTTPSSvc, error) {
-	return nil, errNotImplemented
+	return nil, errLookupNotImplemented
 }
 
 // LookupNS implements Resolver.LookupNS.
 func (r *Resolver) LookupNS(ctx context.Context, domain string) ([]*net.NS, error) {
-	return nil, errNotImplemented
+	return nil, errLookupNotImplemented
 }
 
 // ErrLookupHost indicates that LookupHost failed.
@@ -143,7 +122,7 @@ func (r *Resolver) LookupHost(ctx context.Context, hostname string) ([]string, e
 		if err == nil {
 			return addrs, nil
 		}
-		me.Add(&errwrapper{error: err, URL: e.URL})
+		me.Add(newErrWrapper(err, e.URL))
 	}
 	return nil, me
 }

internal/engine/internal/sessionresolver/sessionresolver_test.go → internal/engine/internal/sessionresolver/resolver_test.go

@@ -47,14 +47,14 @@ func TestTypicalUsageWithFailure(t *testing.T) {
 		// means that we need to go down hunting what's the
 		// real error that occurred and use more verbose code.
 		{
-			var errWrapper *errwrapper
-			if !errors.As(child, &errWrapper) {
+			var ew *errWrapper
+			if !errors.As(child, &ew) {
 				t.Fatal("not an instance of errwrapper")
 			}
-			var dnsError *net.DNSError
-			if errors.As(errWrapper.error, &dnsError) {
-				if !strings.HasSuffix(dnsError.Err, "operation was canceled") {
-					t.Fatal("not the error we expected", dnsError.Err)
+			var de *net.DNSError
+			if errors.As(ew, &de) {
+				if !strings.HasSuffix(de.Err, "operation was canceled") {
+					t.Fatal("not the error we expected", de.Err)
 				}
 				continue
 			}
@@ -361,7 +361,7 @@ func TestUnimplementedFunctions(t *testing.T) {
 	t.Run("LookupHTTPS", func(t *testing.T) {
 		r := &Resolver{}
 		https, err := r.LookupHTTPS(context.Background(), "dns.google")
-		if !errors.Is(err, errNotImplemented) {
+		if !errors.Is(err, errLookupNotImplemented) {
 			t.Fatal("unexpected error", err)
 		}
 		if https != nil {
@@ -372,7 +372,7 @@ func TestUnimplementedFunctions(t *testing.T) {
 	t.Run("LookupNS", func(t *testing.T) {
 		r := &Resolver{}
 		ns, err := r.LookupNS(context.Background(), "dns.google")
-		if !errors.Is(err, errNotImplemented) {
+		if !errors.Is(err, errLookupNotImplemented) {
 			t.Fatal("unexpected error", err)
 		}
 		if len(ns) > 0 {

internal/engine/internal/sessionresolver/resolvermaker.go

@@ -1,5 +1,9 @@
 package sessionresolver
 
+//
+// Code for creating a new child resolver
+//
+
 import (
 	"math/rand"
 	"strings"