Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: split geolocate in three packages #1150

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

refactor: split geolocate in three packages #1150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions internal/engine/experiment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@ package engine
import (
"testing"

"github.com/ooni/probe-cli/v3/internal/geolocate"
"github.com/ooni/probe-cli/v3/internal/model"
)

func TestExperimentHonoursSharingDefaults(t *testing.T) {
measure := func(info *geolocate.Results) *model.Measurement {
measure := func(info *GeolocateResults) *model.Measurement {
sess := &Session{location: info}
builder, err := sess.NewExperimentBuilder("example")
if err != nil {
Expand All @@ -19,48 +18,48 @@ func TestExperimentHonoursSharingDefaults(t *testing.T) {
}
type spec struct {
name string
locationInfo *geolocate.Results
locationInfo *GeolocateResults
expect func(*model.Measurement) bool
}
allspecs := []spec{{
name: "probeIP",
locationInfo: &geolocate.Results{ProbeIP: "8.8.8.8"},
locationInfo: &GeolocateResults{ProbeIP: "8.8.8.8"},
expect: func(m *model.Measurement) bool {
return m.ProbeIP == model.DefaultProbeIP
},
}, {
name: "probeASN",
locationInfo: &geolocate.Results{ASN: 30722},
locationInfo: &GeolocateResults{ASN: 30722},
expect: func(m *model.Measurement) bool {
return m.ProbeASN == "AS30722"
},
}, {
name: "probeCC",
locationInfo: &geolocate.Results{CountryCode: "IT"},
locationInfo: &GeolocateResults{CountryCode: "IT"},
expect: func(m *model.Measurement) bool {
return m.ProbeCC == "IT"
},
}, {
name: "probeNetworkName",
locationInfo: &geolocate.Results{NetworkName: "Vodafone Italia"},
locationInfo: &GeolocateResults{NetworkName: "Vodafone Italia"},
expect: func(m *model.Measurement) bool {
return m.ProbeNetworkName == "Vodafone Italia"
},
}, {
name: "resolverIP",
locationInfo: &geolocate.Results{ResolverIP: "9.9.9.9"},
locationInfo: &GeolocateResults{ResolverIP: "9.9.9.9"},
expect: func(m *model.Measurement) bool {
return m.ResolverIP == "9.9.9.9"
},
}, {
name: "resolverASN",
locationInfo: &geolocate.Results{ResolverASN: 44},
locationInfo: &GeolocateResults{ResolverASN: 44},
expect: func(m *model.Measurement) bool {
return m.ResolverASN == "AS44"
},
}, {
name: "resolverNetworkName",
locationInfo: &geolocate.Results{ResolverNetworkName: "Google LLC"},
locationInfo: &GeolocateResults{ResolverNetworkName: "Google LLC"},
expect: func(m *model.Measurement) bool {
return m.ResolverNetworkName == "Google LLC"
},
Expand Down
35 changes: 17 additions & 18 deletions internal/geolocate/geolocate.go → internal/engine/geolocate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
// Package geolocate implements IP lookup, resolver lookup, and geolocation.
package geolocate
package engine

import (
"context"
"fmt"

"github.com/ooni/probe-cli/v3/internal/iplookup"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/netxlite"
"github.com/ooni/probe-cli/v3/internal/resolverlookup"
"github.com/ooni/probe-cli/v3/internal/version"
)

// Results contains geolocate results.
type Results struct {
type GeolocateResults struct {
// ASN is the autonomous system number.
ASN uint

Expand All @@ -38,7 +39,7 @@ type Results struct {
}

// ASNString returns the ASN as a string.
func (r *Results) ASNString() string {
func (r *GeolocateResults) ASNString() string {
return fmt.Sprintf("AS%d", r.ASN)
}

Expand All @@ -59,7 +60,7 @@ type resolverIPLookupper interface {
}

// Config contains configuration for a geolocate Task.
type Config struct {
type GeolocateConfig struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering the extent to which we need to expose details of how we implement geolocation outside of the engine package. It may be desirable to make all the types we moved here from the former geolocate package private and just accept that they're an implementation detail of a session. What do you reckon?

// Resolver is the resolver we should use when
// making requests for discovering the IP. When
// this field is not set, we use the stdlib.
Expand All @@ -74,8 +75,8 @@ type Config struct {
UserAgent string
}

// NewTask creates a new instance of Task from config.
func NewTask(config Config) *Task {
// NewIplookupTask creates a new instance of IpLookupTask from config.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// NewIplookupTask creates a new instance of IpLookupTask from config.
// NewGeolocateTask creates a new instance of GeolocateTask from config.

func NewGeolocateTask(config GeolocateConfig) *GeolocateTask {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something to think about: we may want to pass a *GeolocateConfig here (which is what we try to do in many cases to have consistent coding style). However, in such a case, we should refactor the code below such that we always avoid mutating the config structure that was passed to us (for correctness). We may also want to consider checking what the calls of NewGeolocateTask do and, if they always initialize all the fields, then we can just avoid setting default values and make all the fields in the config struct mandatory.

if config.Logger == nil {
config.Logger = model.DiscardLogger
}
Expand All @@ -85,20 +86,18 @@ func NewTask(config Config) *Task {
if config.Resolver == nil {
config.Resolver = netxlite.NewStdlibResolver(config.Logger)
}
return &Task{
countryLookupper: mmdbLookupper{},
probeIPLookupper: ipLookupClient(config),
probeASNLookupper: mmdbLookupper{},
resolverASNLookupper: mmdbLookupper{},
resolverIPLookupper: resolverLookupClient{
Logger: config.Logger,
},
return &GeolocateTask{
countryLookupper: iplookup.MMDBLookupper{},
probeIPLookupper: iplookup.IpLookupClient(config),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be cleaner to have a constructor to which we pass the resolver, the logger, and the user agent, rather than relying on a cast. Because it's just three arguments, I would say we do not need to have a structure and it's just fine to pass the three arguments to the constructor instead.

This also provides us with an opportunity to make the iplookup.Client fields private. (We tend to adopt an approach based on constructors and private fields, which is a bit more verbose than initializing structs, but is also useful in that refactoring is a bit easier to implement.)

probeASNLookupper: iplookup.MMDBLookupper{},
resolverASNLookupper: iplookup.MMDBLookupper{},
resolverIPLookupper: resolverlookup.NewResolverLookupClient(config.Logger),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best name here is resolverlookup.NewClient and the corresponding type should also be named resolverlookup.Client, since we don't need to repeat "ResolverLookup" in the name of the type.

}
}

// Task performs a geolocation. You must create a new
// instance of Task using the NewTask factory.
type Task struct {
type GeolocateTask struct {
countryLookupper countryLookupper
probeIPLookupper probeIPLookupper
probeASNLookupper asnLookupper
Expand All @@ -107,9 +106,9 @@ type Task struct {
}

// Run runs the task.
func (op Task) Run(ctx context.Context) (*Results, error) {
func (op *GeolocateTask) Run(ctx context.Context) (*GeolocateResults, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you converted from (op Task) to (op *GeolocateTask). This change is ~good because we tend to prefer always using the pointer receiver in OONI. However, I would like you to double check that there is no method that changes any field of GeolocateTask, otherwise we'll see a change in behavior.

var err error
out := &Results{
out := &GeolocateResults{
ASN: model.DefaultProbeASN,
CountryCode: model.DefaultProbeCC,
NetworkName: model.DefaultProbeNetworkName,
Expand Down
20 changes: 10 additions & 10 deletions internal/geolocate/geolocate_test.go → internal/engine/geolocate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package geolocate
package engine

import (
"context"
Expand All @@ -19,7 +19,7 @@ func (c taskProbeIPLookupper) LookupProbeIP(ctx context.Context) (string, error)

func TestLocationLookupCannotLookupProbeIP(t *testing.T) {
expected := errors.New("mocked error")
op := Task{
op := &GeolocateTask{
probeIPLookupper: taskProbeIPLookupper{err: expected},
}
ctx := context.Background()
Expand Down Expand Up @@ -62,7 +62,7 @@ func (c taskASNLookupper) LookupASN(ip string) (uint, string, error) {

func TestLocationLookupCannotLookupProbeASN(t *testing.T) {
expected := errors.New("mocked error")
op := Task{
op := &GeolocateTask{
probeIPLookupper: taskProbeIPLookupper{ip: "1.2.3.4"},
probeASNLookupper: taskASNLookupper{err: expected},
}
Expand Down Expand Up @@ -105,7 +105,7 @@ func (c taskCCLookupper) LookupCC(ip string) (string, error) {

func TestLocationLookupCannotLookupProbeCC(t *testing.T) {
expected := errors.New("mocked error")
op := Task{
op := &GeolocateTask{
probeIPLookupper: taskProbeIPLookupper{ip: "1.2.3.4"},
probeASNLookupper: taskASNLookupper{asn: 1234, name: "1234.com"},
countryLookupper: taskCCLookupper{cc: "US", err: expected},
Expand Down Expand Up @@ -149,7 +149,7 @@ func (c taskResolverIPLookupper) LookupResolverIP(ctx context.Context) (string,

func TestLocationLookupCannotLookupResolverIP(t *testing.T) {
expected := errors.New("mocked error")
op := Task{
op := &GeolocateTask{
probeIPLookupper: taskProbeIPLookupper{ip: "1.2.3.4"},
probeASNLookupper: taskASNLookupper{asn: 1234, name: "1234.com"},
countryLookupper: taskCCLookupper{cc: "IT"},
Expand Down Expand Up @@ -188,7 +188,7 @@ func TestLocationLookupCannotLookupResolverIP(t *testing.T) {

func TestLocationLookupCannotLookupResolverNetworkName(t *testing.T) {
expected := errors.New("mocked error")
op := Task{
op := &GeolocateTask{
probeIPLookupper: taskProbeIPLookupper{ip: "1.2.3.4"},
probeASNLookupper: taskASNLookupper{asn: 1234, name: "1234.com"},
countryLookupper: taskCCLookupper{cc: "IT"},
Expand Down Expand Up @@ -227,7 +227,7 @@ func TestLocationLookupCannotLookupResolverNetworkName(t *testing.T) {
}

func TestLocationLookupSuccessWithResolverLookup(t *testing.T) {
op := Task{
op := &GeolocateTask{
probeIPLookupper: taskProbeIPLookupper{ip: "1.2.3.4"},
probeASNLookupper: taskASNLookupper{asn: 1234, name: "1234.com"},
countryLookupper: taskCCLookupper{cc: "IT"},
Expand Down Expand Up @@ -266,8 +266,8 @@ func TestLocationLookupSuccessWithResolverLookup(t *testing.T) {
}

func TestSmoke(t *testing.T) {
config := Config{}
task := NewTask(config)
config := GeolocateConfig{}
task := NewGeolocateTask(config)
result, err := task.Run(context.Background())
if err != nil {
t.Fatal(err)
Expand All @@ -280,7 +280,7 @@ func TestSmoke(t *testing.T) {
}

func TestASNStringWorks(t *testing.T) {
r := Results{ASN: 1234}
r := &GeolocateResults{ASN: 1234}
if r.ASNString() != "AS1234" {
t.Fatal("unexpected result")
}
Expand Down
11 changes: 5 additions & 6 deletions internal/engine/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (

"github.com/ooni/probe-cli/v3/internal/bytecounter"
"github.com/ooni/probe-cli/v3/internal/checkincache"
"github.com/ooni/probe-cli/v3/internal/geolocate"
"github.com/ooni/probe-cli/v3/internal/kvstore"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/netxlite"
Expand Down Expand Up @@ -60,7 +59,7 @@ type Session struct {
byteCounter *bytecounter.Counter
httpDefaultTransport model.HTTPTransport
kvStore model.KeyValueStore
location *geolocate.Results
location *GeolocateResults
logger model.Logger
proxyURL *url.URL
queryProbeServicesCount *atomic.Int64
Expand All @@ -79,7 +78,7 @@ type Session struct {

// testLookupLocationContext is a an optional hook for testing
// allowing us to mock LookupLocationContext.
testLookupLocationContext func(ctx context.Context) (*geolocate.Results, error)
testLookupLocationContext func(ctx context.Context) (*GeolocateResults, error)

// testMaybeLookupBackendsContext is an optional hook for testing
// allowing us to mock MaybeLookupBackendsContext.
Expand Down Expand Up @@ -676,8 +675,8 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error {

// LookupLocationContext performs a location lookup. If you want memoisation
// of the results, you should use MaybeLookupLocationContext.
func (s *Session) LookupLocationContext(ctx context.Context) (*geolocate.Results, error) {
task := geolocate.NewTask(geolocate.Config{
func (s *Session) LookupLocationContext(ctx context.Context) (*GeolocateResults, error) {
task := NewGeolocateTask(GeolocateConfig{
Logger: s.Logger(),
Resolver: s.resolver,
UserAgent: s.UserAgent(),
Expand All @@ -687,7 +686,7 @@ func (s *Session) LookupLocationContext(ctx context.Context) (*geolocate.Results

// lookupLocationContext calls testLookupLocationContext if set and
// otherwise calls LookupLocationContext.
func (s *Session) lookupLocationContext(ctx context.Context) (*geolocate.Results, error) {
func (s *Session) lookupLocationContext(ctx context.Context) (*GeolocateResults, error) {
if s.testLookupLocationContext != nil {
return s.testLookupLocationContext(ctx)
}
Expand Down
9 changes: 4 additions & 5 deletions internal/engine/session_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
"github.com/ooni/probe-cli/v3/internal/checkincache"
"github.com/ooni/probe-cli/v3/internal/experiment/webconnectivity"
"github.com/ooni/probe-cli/v3/internal/experiment/webconnectivitylte"
"github.com/ooni/probe-cli/v3/internal/geolocate"
"github.com/ooni/probe-cli/v3/internal/kvstore"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/registry"
Expand Down Expand Up @@ -85,7 +84,7 @@ func TestSessionCheckInSuccessful(t *testing.T) {
},
}
s := &Session{
location: &geolocate.Results{
location: &GeolocateResults{
ASN: 137,
CountryCode: "IT",
},
Expand Down Expand Up @@ -136,7 +135,7 @@ func TestSessionCheckInNetworkError(t *testing.T) {
Error: expect,
}
s := &Session{
location: &geolocate.Results{
location: &GeolocateResults{
ASN: 137,
CountryCode: "IT",
},
Expand Down Expand Up @@ -178,7 +177,7 @@ func TestSessionCheckInCannotLookupLocation(t *testing.T) {
func TestSessionCheckInCannotCreateProbeServicesClient(t *testing.T) {
errMocked := errors.New("mocked error")
s := &Session{
location: &geolocate.Results{
location: &GeolocateResults{
ASN: 137,
CountryCode: "IT",
},
Expand Down Expand Up @@ -240,7 +239,7 @@ func TestSessionNewSubmitterWithCancelledContext(t *testing.T) {
func TestSessionMaybeLookupLocationContextLookupLocationContextFailure(t *testing.T) {
errMocked := errors.New("mocked error")
sess := newSessionForTestingNoLookups(t)
sess.testLookupLocationContext = func(ctx context.Context) (*geolocate.Results, error) {
sess.testLookupLocationContext = func(ctx context.Context) (*GeolocateResults, error) {
return nil, errMocked
}
err := sess.MaybeLookupLocationContext(context.Background())
Expand Down
15 changes: 0 additions & 15 deletions internal/geolocate/mmdblookup.go
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion internal/geolocate/cloudflare.go → internal/iplookup/cloudflare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package geolocate
package iplookup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure you add a doc.go file for each new package with minimal package-level documentation, please!


import (
"context"
Expand Down
2 changes: 1 addition & 1 deletion internal/geolocate/cloudflare_test.go → internal/iplookup/cloudflare_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package geolocate
package iplookup

import (
"context"
Expand Down
2 changes: 1 addition & 1 deletion internal/geolocate/fake_test.go → internal/iplookup/fake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package geolocate
package iplookup

import (
"net/http"
Expand Down
2 changes: 1 addition & 1 deletion internal/geolocate/invalid_test.go → internal/iplookup/invalid_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package geolocate
package iplookup

import (
"context"
Expand Down
Loading