From b1afbf9ad122bc14b4d90b2d31fcc693b11dfc98 Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Fri, 12 Feb 2021 18:01:47 -0700 Subject: [PATCH 1/3] insights: add new discovery package for locating insights This will be used both from the GraphQL resolver layer, as well as the background workers - both of which need to scan the user/org/global settings for insights defined within. Signed-off-by: Stephen Gutekanst --- .../internal/insights/discovery/discovery.go | 41 +++++ .../insights/discovery/discovery_test.go | 155 ++++++++++++++++++ enterprise/internal/insights/discovery/gen.go | 4 + .../insights/discovery/mock_setting_store.go | 151 +++++++++++++++++ 4 files changed, 351 insertions(+) create mode 100644 enterprise/internal/insights/discovery/discovery.go create mode 100644 enterprise/internal/insights/discovery/discovery_test.go create mode 100644 enterprise/internal/insights/discovery/gen.go create mode 100644 enterprise/internal/insights/discovery/mock_setting_store.go diff --git a/enterprise/internal/insights/discovery/discovery.go b/enterprise/internal/insights/discovery/discovery.go new file mode 100644 index 000000000000..350a5cb54663 --- /dev/null +++ b/enterprise/internal/insights/discovery/discovery.go @@ -0,0 +1,41 @@ +package discovery + +import ( + "context" + + "github.com/sourcegraph/sourcegraph/internal/api" + "github.com/sourcegraph/sourcegraph/internal/jsonc" + "github.com/sourcegraph/sourcegraph/schema" +) + +// SettingStore is a subset of the API exposed by the database.Settings() store. +type SettingStore interface { + GetLatest(context.Context, api.SettingsSubject) (*api.Settings, error) +} + +// Discover uses the given settings store to look for insights in the global user settings. +// +// TODO(slimsag): future: include user/org settings and consider security implications of doing so. +// In the future, this will be expanded to also include insights from users/orgs. +func Discover(ctx context.Context, settingStore SettingStore) ([]*schema.Insight, error) { + // Get latest Global user settings. + subject := api.SettingsSubject{Site: true} + globalSettingsRaw, err := settingStore.GetLatest(ctx, subject) + if err != nil { + return nil, err + } + globalSettings, err := parseUserSettings(globalSettingsRaw) + return globalSettings.Insights, nil +} + +func parseUserSettings(settings *api.Settings) (*schema.Settings, error) { + if settings == nil { + // Settings have never been saved for this subject; equivalent to `{}`. + return &schema.Settings{}, nil + } + var v schema.Settings + if err := jsonc.Unmarshal(settings.Contents, &v); err != nil { + return nil, err + } + return &v, nil +} diff --git a/enterprise/internal/insights/discovery/discovery_test.go b/enterprise/internal/insights/discovery/discovery_test.go new file mode 100644 index 000000000000..e620ebdfd43d --- /dev/null +++ b/enterprise/internal/insights/discovery/discovery_test.go @@ -0,0 +1,155 @@ +package discovery + +import ( + "context" + "testing" + + "github.com/hexops/autogold" + + "github.com/sourcegraph/sourcegraph/internal/api" + "github.com/sourcegraph/sourcegraph/schema" +) + +var settingsExample = &api.Settings{ID: 1, Contents: `{ + "insights": [ + { + "title": "fmt usage", + "description": "fmt.Errorf/fmt.Printf usage", + "series": [ + { + "label": "fmt.Errorf", + "search": "errorf", + }, + { + "label": "printf", + "search": "fmt.Printf", + } + ] + }, + { + "title": "gitserver usage", + "description": "gitserver exec & close usage", + "series": [ + { + "label": "exec", + "search": "gitserver.Exec", + }, + { + "label": "close", + "search": "gitserver.Close", + } + ] + } + ] + } +`} + +func TestDiscover(t *testing.T) { + settingStore := NewMockSettingStore() + settingStore.GetLatestFunc.SetDefaultHook(func(ctx context.Context, subject api.SettingsSubject) (*api.Settings, error) { + if !subject.Site { // TODO: future: site is an extremely poor name for "global settings", we should change this. + t.Fatal("expected only to request settings from global user settings") + } + return settingsExample, nil + }) + ctx := context.Background() + insights, err := Discover(ctx, settingStore) + if err != nil { + t.Fatal(err) + } + autogold.Want("insights", []*schema.Insight{ + &schema.Insight{ + Description: "fmt.Errorf/fmt.Printf usage", + Series: []*schema.InsightSeries{ + &schema.InsightSeries{ + Label: "fmt.Errorf", + Search: "errorf", + }, + &schema.InsightSeries{ + Label: "printf", + Search: "fmt.Printf", + }, + }, + Title: "fmt usage", + }, + &schema.Insight{ + Description: "gitserver exec & close usage", + Series: []*schema.InsightSeries{ + &schema.InsightSeries{ + Label: "exec", + Search: "gitserver.Exec", + }, + &schema.InsightSeries{ + Label: "close", + Search: "gitserver.Close", + }, + }, + Title: "gitserver usage", + }, + }).Equal(t, insights) +} + +func Test_parseUserSettings(t *testing.T) { + tests := []struct { + name string + input *api.Settings + want autogold.Value + }{ + { + name: "nil", + input: nil, + want: autogold.Want("nil", [2]interface{}{&schema.Settings{}, nil}), + }, + { + name: "empty", + input: &api.Settings{ + Contents: "{}", + }, + want: autogold.Want("empty", [2]interface{}{&schema.Settings{}, nil}), + }, + { + name: "real", + input: settingsExample, + want: autogold.Want("real", [2]interface{}{ + &schema.Settings{Insights: []*schema.Insight{ + { + Description: "fmt.Errorf/fmt.Printf usage", + Series: []*schema.InsightSeries{ + { + Label: "fmt.Errorf", + Search: "errorf", + }, + { + Label: "printf", + Search: "fmt.Printf", + }, + }, + Title: "fmt usage", + }, + { + Description: "gitserver exec & close usage", + Series: []*schema.InsightSeries{ + { + Label: "exec", + Search: "gitserver.Exec", + }, + { + Label: "close", + Search: "gitserver.Close", + }, + }, + Title: "gitserver usage", + }, + }}, + nil, + }), + }, + } + for _, tst := range tests { + t.Run(tst.name, func(t *testing.T) { + got, err := parseUserSettings(tst.input) + tst.want.Equal(t, [2]interface{}{got, err}) + }) + } + +} diff --git a/enterprise/internal/insights/discovery/gen.go b/enterprise/internal/insights/discovery/gen.go new file mode 100644 index 000000000000..f1fc8a65bbdf --- /dev/null +++ b/enterprise/internal/insights/discovery/gen.go @@ -0,0 +1,4 @@ +package discovery + +//go:generate env GOBIN=$PWD/.bin GO111MODULE=on go install github.com/efritz/go-mockgen +//go:generate $PWD/.bin/go-mockgen -f github.com/sourcegraph/sourcegraph/enterprise/internal/insights/discovery -i SettingStore -o mock_setting_store.go diff --git a/enterprise/internal/insights/discovery/mock_setting_store.go b/enterprise/internal/insights/discovery/mock_setting_store.go new file mode 100644 index 000000000000..93ff14d05a72 --- /dev/null +++ b/enterprise/internal/insights/discovery/mock_setting_store.go @@ -0,0 +1,151 @@ +// Code generated by github.com/efritz/go-mockgen 0.1.0; DO NOT EDIT. + +package discovery + +import ( + "context" + api "github.com/sourcegraph/sourcegraph/internal/api" + "sync" +) + +// MockSettingStore is a mock implementation of the SettingStore interface +// (from the package +// github.com/sourcegraph/sourcegraph/enterprise/internal/insights/discovery) +// used for unit testing. +type MockSettingStore struct { + // GetLatestFunc is an instance of a mock function object controlling + // the behavior of the method GetLatest. + GetLatestFunc *SettingStoreGetLatestFunc +} + +// NewMockSettingStore creates a new mock of the SettingStore interface. All +// methods return zero values for all results, unless overwritten. +func NewMockSettingStore() *MockSettingStore { + return &MockSettingStore{ + GetLatestFunc: &SettingStoreGetLatestFunc{ + defaultHook: func(context.Context, api.SettingsSubject) (*api.Settings, error) { + return nil, nil + }, + }, + } +} + +// NewMockSettingStoreFrom creates a new mock of the MockSettingStore +// interface. All methods delegate to the given implementation, unless +// overwritten. +func NewMockSettingStoreFrom(i SettingStore) *MockSettingStore { + return &MockSettingStore{ + GetLatestFunc: &SettingStoreGetLatestFunc{ + defaultHook: i.GetLatest, + }, + } +} + +// SettingStoreGetLatestFunc describes the behavior when the GetLatest +// method of the parent MockSettingStore instance is invoked. +type SettingStoreGetLatestFunc struct { + defaultHook func(context.Context, api.SettingsSubject) (*api.Settings, error) + hooks []func(context.Context, api.SettingsSubject) (*api.Settings, error) + history []SettingStoreGetLatestFuncCall + mutex sync.Mutex +} + +// GetLatest delegates to the next hook function in the queue and stores the +// parameter and result values of this invocation. +func (m *MockSettingStore) GetLatest(v0 context.Context, v1 api.SettingsSubject) (*api.Settings, error) { + r0, r1 := m.GetLatestFunc.nextHook()(v0, v1) + m.GetLatestFunc.appendCall(SettingStoreGetLatestFuncCall{v0, v1, r0, r1}) + return r0, r1 +} + +// SetDefaultHook sets function that is called when the GetLatest method of +// the parent MockSettingStore instance is invoked and the hook queue is +// empty. +func (f *SettingStoreGetLatestFunc) SetDefaultHook(hook func(context.Context, api.SettingsSubject) (*api.Settings, error)) { + f.defaultHook = hook +} + +// PushHook adds a function to the end of hook queue. Each invocation of the +// GetLatest method of the parent MockSettingStore instance invokes the hook +// at the front of the queue and discards it. After the queue is empty, the +// default hook function is invoked for any future action. +func (f *SettingStoreGetLatestFunc) PushHook(hook func(context.Context, api.SettingsSubject) (*api.Settings, error)) { + f.mutex.Lock() + f.hooks = append(f.hooks, hook) + f.mutex.Unlock() +} + +// SetDefaultReturn calls SetDefaultDefaultHook with a function that returns +// the given values. +func (f *SettingStoreGetLatestFunc) SetDefaultReturn(r0 *api.Settings, r1 error) { + f.SetDefaultHook(func(context.Context, api.SettingsSubject) (*api.Settings, error) { + return r0, r1 + }) +} + +// PushReturn calls PushDefaultHook with a function that returns the given +// values. +func (f *SettingStoreGetLatestFunc) PushReturn(r0 *api.Settings, r1 error) { + f.PushHook(func(context.Context, api.SettingsSubject) (*api.Settings, error) { + return r0, r1 + }) +} + +func (f *SettingStoreGetLatestFunc) nextHook() func(context.Context, api.SettingsSubject) (*api.Settings, error) { + f.mutex.Lock() + defer f.mutex.Unlock() + + if len(f.hooks) == 0 { + return f.defaultHook + } + + hook := f.hooks[0] + f.hooks = f.hooks[1:] + return hook +} + +func (f *SettingStoreGetLatestFunc) appendCall(r0 SettingStoreGetLatestFuncCall) { + f.mutex.Lock() + f.history = append(f.history, r0) + f.mutex.Unlock() +} + +// History returns a sequence of SettingStoreGetLatestFuncCall objects +// describing the invocations of this function. +func (f *SettingStoreGetLatestFunc) History() []SettingStoreGetLatestFuncCall { + f.mutex.Lock() + history := make([]SettingStoreGetLatestFuncCall, len(f.history)) + copy(history, f.history) + f.mutex.Unlock() + + return history +} + +// SettingStoreGetLatestFuncCall is an object that describes an invocation +// of method GetLatest on an instance of MockSettingStore. +type SettingStoreGetLatestFuncCall struct { + // Arg0 is the value of the 1st argument passed to this method + // invocation. + Arg0 context.Context + // Arg1 is the value of the 2nd argument passed to this method + // invocation. + Arg1 api.SettingsSubject + // Result0 is the value of the 1st result returned from this method + // invocation. + Result0 *api.Settings + // Result1 is the value of the 2nd result returned from this method + // invocation. + Result1 error +} + +// Args returns an interface slice containing the arguments of this +// invocation. +func (c SettingStoreGetLatestFuncCall) Args() []interface{} { + return []interface{}{c.Arg0, c.Arg1} +} + +// Results returns an interface slice containing the results of this +// invocation. +func (c SettingStoreGetLatestFuncCall) Results() []interface{} { + return []interface{}{c.Result0, c.Result1} +} From c0b23b3fb4c1414cef2547e5ee3c6d919f9188b9 Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Fri, 12 Feb 2021 18:07:09 -0700 Subject: [PATCH 2/3] insights: resolvers: use the new discovery package Signed-off-by: Stephen Gutekanst --- .../resolvers/insight_connection_resolver.go | 39 +--------- .../insight_connection_resolver_test.go | 78 ++----------------- .../resolvers/insight_series_resolver_test.go | 14 ++-- 3 files changed, 16 insertions(+), 115 deletions(-) diff --git a/enterprise/internal/insights/resolvers/insight_connection_resolver.go b/enterprise/internal/insights/resolvers/insight_connection_resolver.go index 95460679e83a..e1df4f488193 100644 --- a/enterprise/internal/insights/resolvers/insight_connection_resolver.go +++ b/enterprise/internal/insights/resolvers/insight_connection_resolver.go @@ -7,10 +7,8 @@ import ( "github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend" "github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend/graphqlutil" + "github.com/sourcegraph/sourcegraph/enterprise/internal/insights/discovery" "github.com/sourcegraph/sourcegraph/enterprise/internal/insights/store" - "github.com/sourcegraph/sourcegraph/internal/api" - "github.com/sourcegraph/sourcegraph/internal/database" - "github.com/sourcegraph/sourcegraph/internal/jsonc" "github.com/sourcegraph/sourcegraph/schema" ) @@ -18,11 +16,7 @@ var _ graphqlbackend.InsightConnectionResolver = &insightConnectionResolver{} type insightConnectionResolver struct { store store.Interface - settingStore *database.SettingStore - - // We use our own mock here because database.Mocks.Settings.GetLatest is a global which means - // we could not run our tests in parallel. - mocksSettingsGetLatest func(ctx context.Context, subject api.SettingsSubject) (*api.Settings, error) + settingStore discovery.SettingStore // cache results because they are used by multiple fields once sync.Once @@ -61,38 +55,11 @@ func (r *insightConnectionResolver) PageInfo(ctx context.Context) (*graphqlutil. func (r *insightConnectionResolver) compute(ctx context.Context) ([]*schema.Insight, int64, error) { r.once.Do(func() { - settingsGetLatest := r.settingStore.GetLatest - if r.mocksSettingsGetLatest != nil { - settingsGetLatest = r.mocksSettingsGetLatest - } - - // Get latest Global user settings. - // - // FUTURE: include user/org settings. - subject := api.SettingsSubject{Site: true} - globalSettingsRaw, err := settingsGetLatest(ctx, subject) - if err != nil { - r.err = err - return - } - globalSettings, err := parseUserSettings(globalSettingsRaw) - r.insights = globalSettings.Insights + r.insights, r.err = discovery.Discover(ctx, r.settingStore) }) return r.insights, r.next, r.err } -func parseUserSettings(settings *api.Settings) (*schema.Settings, error) { - if settings == nil { - // Settings have never been saved for this subject; equivalent to `{}`. - return &schema.Settings{}, nil - } - var v schema.Settings - if err := jsonc.Unmarshal(settings.Contents, &v); err != nil { - return nil, err - } - return &v, nil -} - // InsightResolver is also defined here as it is covered by the same tests. var _ graphqlbackend.InsightResolver = &insightResolver{} diff --git a/enterprise/internal/insights/resolvers/insight_connection_resolver_test.go b/enterprise/internal/insights/resolvers/insight_connection_resolver_test.go index 81b06477474d..d2046aa9ba3c 100644 --- a/enterprise/internal/insights/resolvers/insight_connection_resolver_test.go +++ b/enterprise/internal/insights/resolvers/insight_connection_resolver_test.go @@ -11,9 +11,9 @@ import ( "github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend" "github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend/graphqlutil" insightsdbtesting "github.com/sourcegraph/sourcegraph/enterprise/internal/insights/dbtesting" + "github.com/sourcegraph/sourcegraph/enterprise/internal/insights/discovery" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/database/dbtesting" - "github.com/sourcegraph/sourcegraph/schema" ) // Note: You can `go test ./resolvers -update` to update the expected `want` values in these tests. @@ -75,12 +75,11 @@ func TestResolver_InsightConnection(t *testing.T) { if err != nil { t.Fatal(err) } - conn.(*insightConnectionResolver).mocksSettingsGetLatest = func(ctx context.Context, subject api.SettingsSubject) (*api.Settings, error) { - if !subject.Site { // TODO: future: site is an extremely poor name for "global settings", we should change this. - t.Fatal("expected only to request settings from global user settings") - } - return testRealGlobalSettings, nil - } + + // Mock the setting store to return the desired settings. + settingStore := discovery.NewMockSettingStore() + conn.(*insightConnectionResolver).settingStore = settingStore + settingStore.GetLatestFunc.SetDefaultReturn(testRealGlobalSettings, nil) return ctx, conn } @@ -129,68 +128,3 @@ func TestResolver_InsightConnection(t *testing.T) { autogold.Want("second insight: series length", int(2)).Equal(t, len(nodes[1].Series())) }) } - -func Test_parseUserSettings(t *testing.T) { - tests := []struct { - name string - input *api.Settings - want autogold.Value - }{ - { - name: "nil", - input: nil, - want: autogold.Want("nil", [2]interface{}{&schema.Settings{}, nil}), - }, - { - name: "empty", - input: &api.Settings{ - Contents: "{}", - }, - want: autogold.Want("empty", [2]interface{}{&schema.Settings{}, nil}), - }, - { - name: "real", - input: testRealGlobalSettings, - want: autogold.Want("real", [2]interface{}{ - &schema.Settings{Insights: []*schema.Insight{ - { - Description: "fmt.Errorf/fmt.Printf usage", - Series: []*schema.InsightSeries{ - { - Label: "fmt.Errorf", - Search: "errorf", - }, - { - Label: "printf", - Search: "fmt.Printf", - }, - }, - Title: "fmt usage", - }, - { - Description: "gitserver exec & close usage", - Series: []*schema.InsightSeries{ - { - Label: "exec", - Search: "gitserver.Exec", - }, - { - Label: "close", - Search: "gitserver.Close", - }, - }, - Title: "gitserver usage", - }, - }}, - nil, - }), - }, - } - for _, tst := range tests { - t.Run(tst.name, func(t *testing.T) { - got, err := parseUserSettings(tst.input) - tst.want.Equal(t, [2]interface{}{got, err}) - }) - } - -} diff --git a/enterprise/internal/insights/resolvers/insight_series_resolver_test.go b/enterprise/internal/insights/resolvers/insight_series_resolver_test.go index 2c143b9be85b..9831bcfa71bb 100644 --- a/enterprise/internal/insights/resolvers/insight_series_resolver_test.go +++ b/enterprise/internal/insights/resolvers/insight_series_resolver_test.go @@ -11,8 +11,8 @@ import ( "github.com/sourcegraph/sourcegraph/cmd/frontend/backend" "github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend" insightsdbtesting "github.com/sourcegraph/sourcegraph/enterprise/internal/insights/dbtesting" + "github.com/sourcegraph/sourcegraph/enterprise/internal/insights/discovery" "github.com/sourcegraph/sourcegraph/enterprise/internal/insights/store" - "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/database/dbtesting" ) @@ -43,12 +43,12 @@ func TestResolver_InsightSeries(t *testing.T) { cleanup() t.Fatal(err) } - conn.(*insightConnectionResolver).mocksSettingsGetLatest = func(ctx context.Context, subject api.SettingsSubject) (*api.Settings, error) { - if !subject.Site { // TODO: future: site is an extremely poor name for "global settings", we should change this. - t.Fatal("expected only to request settings from global user settings") - } - return testRealGlobalSettings, nil - } + + // Mock the setting store to return the desired settings. + settingStore := discovery.NewMockSettingStore() + conn.(*insightConnectionResolver).settingStore = settingStore + settingStore.GetLatestFunc.SetDefaultReturn(testRealGlobalSettings, nil) + nodes, err := conn.Nodes(ctx) if err != nil { cleanup() From bd4f45547a21a00127eec2e317f3f1ccbaebd7dc Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Fri, 12 Feb 2021 18:12:29 -0700 Subject: [PATCH 3/3] gofmt Signed-off-by: Stephen Gutekanst --- .../internal/insights/discovery/discovery_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/enterprise/internal/insights/discovery/discovery_test.go b/enterprise/internal/insights/discovery/discovery_test.go index e620ebdfd43d..ecce4e69e5c9 100644 --- a/enterprise/internal/insights/discovery/discovery_test.go +++ b/enterprise/internal/insights/discovery/discovery_test.go @@ -58,28 +58,28 @@ func TestDiscover(t *testing.T) { t.Fatal(err) } autogold.Want("insights", []*schema.Insight{ - &schema.Insight{ + { Description: "fmt.Errorf/fmt.Printf usage", Series: []*schema.InsightSeries{ - &schema.InsightSeries{ + { Label: "fmt.Errorf", Search: "errorf", }, - &schema.InsightSeries{ + { Label: "printf", Search: "fmt.Printf", }, }, Title: "fmt usage", }, - &schema.Insight{ + { Description: "gitserver exec & close usage", Series: []*schema.InsightSeries{ - &schema.InsightSeries{ + { Label: "exec", Search: "gitserver.Exec", }, - &schema.InsightSeries{ + { Label: "close", Search: "gitserver.Close", },