refactor(session): observe full check-in response (#1037)

This diff refactors engine/session.go such that the session code can observe the full check-in response. We're still returning the nettests subset to the caller, since normally callers only care about that. By observing the full response, we'll be able to cache the feature flags, which will then be useful to implement ooni/probe#2396.

internal/engine/session.go

@@ -111,7 +111,7 @@ type Session struct {
 // sessionProbeServicesClientForCheckIn returns the probe services
 // client that we should be using for performing the check-in.
 type sessionProbeServicesClientForCheckIn interface {
-	CheckIn(ctx context.Context, config model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error)
+	CheckIn(ctx context.Context, config model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error)
 }
 
 // NewSession creates a new session. This factory function will
@@ -265,6 +265,8 @@ func (s *Session) KibiBytesSent() float64 {
 // The return value is either the check-in response or an error.
 func (s *Session) CheckIn(
 	ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error) {
+	// TODO(bassosimone): consider refactoring this function to return
+	// the whole check-in response to the caller.
 	if err := s.maybeLookupLocationContext(ctx); err != nil {
 		return nil, err
 	}
@@ -293,7 +295,13 @@ func (s *Session) CheckIn(
 	if config.WebConnectivity.CategoryCodes == nil {
 		config.WebConnectivity.CategoryCodes = []string{}
 	}
-	return client.CheckIn(ctx, *config)
+	resp, err := client.CheckIn(ctx, *config)
+	if err != nil {
+		return nil, err
+	}
+	// TODO(bassosimone): here is where we should implement
+	// caching of the check-in response.
+	return &resp.Tests, nil
 }
 
 // maybeLookupLocationContext is a wrapper for MaybeLookupLocationContext that calls

internal/engine/session_internal_test.go

@@ -33,7 +33,7 @@ type mockableProbeServicesClientForCheckIn struct {
 
 	// Results contains the results of the call. This field MUST be
 	// non-nil if and only if Error is nil.
-	Results *model.OOAPICheckInResultNettests
+	Results *model.OOAPICheckInResult
 
 	// Error indicates whether the call failed. This field MUST be
 	// non-nil if and only if Error is nil.
@@ -45,7 +45,7 @@ type mockableProbeServicesClientForCheckIn struct {
 
 // CheckIn implements sessionProbeServicesClientForCheckIn.CheckIn.
 func (c *mockableProbeServicesClientForCheckIn) CheckIn(
-	ctx context.Context, config model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error) {
+	ctx context.Context, config model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) {
 	defer c.mu.Unlock()
 	c.mu.Lock()
 	if c.Config != nil {
@@ -74,7 +74,9 @@ func TestSessionCheckInSuccessful(t *testing.T) {
 		},
 	}
 	mockedClnt := &mockableProbeServicesClientForCheckIn{
-		Results: results,
+		Results: &model.OOAPICheckInResult{
+			Tests: *results,
+		},
 	}
 	s := &Session{
 		location: &geolocate.Results{
@@ -121,6 +123,35 @@ func TestSessionCheckInSuccessful(t *testing.T) {
 	}
 }
 
+func TestSessionCheckInNetworkError(t *testing.T) {
+	expect := errors.New("mocked error")
+	mockedClnt := &mockableProbeServicesClientForCheckIn{
+		Error: expect,
+	}
+	s := &Session{
+		location: &geolocate.Results{
+			ASN:         137,
+			CountryCode: "IT",
+		},
+		softwareName:    "miniooni",
+		softwareVersion: "0.1.0-dev",
+		testMaybeLookupLocationContext: func(ctx context.Context) error {
+			return nil
+		},
+		testNewProbeServicesClientForCheckIn: func(
+			ctx context.Context) (sessionProbeServicesClientForCheckIn, error) {
+			return mockedClnt, nil
+		},
+	}
+	out, err := s.CheckIn(context.Background(), &model.OOAPICheckInConfig{})
+	if !errors.Is(err, expect) {
+		t.Fatal("unexpected err", err)
+	}
+	if out != nil {
+		t.Fatal("expected nil out")
+	}
+}
+
 func TestSessionCheckInCannotLookupLocation(t *testing.T) {
 	errMocked := errors.New("mocked error")
 	s := &Session{

internal/model/ooapi.go

@@ -64,6 +64,9 @@ type OOAPICheckInResultNettests struct {
 
 // OOAPICheckInResult is the result returned by the checkin API.
 type OOAPICheckInResult struct {
+	// Conf contains configuration.
+	Conf OOAPICheckInResultConfig `json:"conf"`
+
 	// ProbeASN contains the probe's ASN.
 	ProbeASN string `json:"probe_asn"`
 
@@ -73,10 +76,19 @@ type OOAPICheckInResult struct {
 	// Tests contains information about nettests.
 	Tests OOAPICheckInResultNettests `json:"tests"`
 
+	// UTCTime contains the time in UTC.
+	UTCTime time.Time `json:"utc_time"`
+
 	// V is the version.
 	V int64 `json:"v"`
 }
 
+// OOAPICheckInResultConfig contains configuration.
+type OOAPICheckInResultConfig struct {
+	// Feature contains feature flags.
+	Feature map[string]bool `json:"feature"`
+}
+
 // OOAPICheckReportIDResponse is the check-report-id API response.
 type OOAPICheckReportIDResponse struct {
 	Error string `json:"error"`

internal/probeservices/checkin.go

@@ -13,12 +13,12 @@ import (
 // Returns the list of tests to run and the URLs, on success,
 // or an explanatory error, in case of failure.
 func (c Client) CheckIn(
-	ctx context.Context, config model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error) {
+	ctx context.Context, config model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) {
 	epnt := c.newHTTPAPIEndpoint()
 	desc := ooapi.NewDescriptorCheckIn(&config)
 	resp, err := httpapi.Call(ctx, desc, epnt)
 	if err != nil {
 		return nil, err
 	}
-	return &resp.Tests, nil
+	return resp, nil
 }

internal/probeservices/checkin_test.go

@@ -29,16 +29,16 @@ func TestCheckInSuccess(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if result == nil || result.WebConnectivity == nil {
+	if result == nil || result.Tests.WebConnectivity == nil {
 		t.Fatal("got nil result or WebConnectivity")
 	}
-	if result.WebConnectivity.ReportID == "" {
+	if result.Tests.WebConnectivity.ReportID == "" {
 		t.Fatal("ReportID is empty")
 	}
-	if len(result.WebConnectivity.URLs) < 1 {
+	if len(result.Tests.WebConnectivity.URLs) < 1 {
 		t.Fatal("unexpected number of URLs")
 	}
-	for _, entry := range result.WebConnectivity.URLs {
+	for _, entry := range result.Tests.WebConnectivity.URLs {
 		if entry.CategoryCode != "NEWS" && entry.CategoryCode != "CULTR" {
 			t.Fatalf("unexpected category code: %+v", entry)
 		}

pkg/oonimkall/session.go

@@ -481,6 +481,6 @@ func (sess *Session) CheckIn(ctx *Context, config *CheckInConfig) (*CheckInInfo,
 		return nil, err
 	}
 	return &CheckInInfo{
-		WebConnectivity: newCheckInInfoWebConnectivity(result.WebConnectivity),
+		WebConnectivity: newCheckInInfoWebConnectivity(result.Tests.WebConnectivity),
 	}, nil
 }