From 0e958d19d6547a544065cc434e40548ea0022381 Mon Sep 17 00:00:00 2001 From: Quinn Slack Date: Fri, 19 Jul 2024 02:44:14 -0700 Subject: [PATCH] misc improvements to graphqlbackend (#63943) - skip graphqlbackend tests that use the DB when using `go test -short` - fix race condition in (settingsResolver).Author - fix OrgMembers.GetByOrgIDAndUserID mocks: The actual function returns a non-nil error (`database.ErrOrgMemberNotFound` type) when the user is not an org member. Our mocks should do the same for correctness. ## Test plan CI --- .../graphqlbackend/default_settings.go | 2 +- cmd/frontend/graphqlbackend/org.go | 2 +- cmd/frontend/graphqlbackend/org_test.go | 2 +- .../preview_repository_comparison_test.go | 4 ++++ .../repository_comparison_test.go | 8 ++++++++ .../repository_text_search_index_test.go | 4 ++++ cmd/frontend/graphqlbackend/settings.go | 20 +++++++++++-------- cmd/frontend/graphqlbackend/site.go | 2 +- .../graphqlbackend/site_admin_test.go | 2 +- cmd/frontend/graphqlbackend/user.go | 2 +- .../prompts/resolvers/resolvers_test.go | 4 ++-- .../savedsearches/resolvers/resolvers_test.go | 4 ++-- internal/database/executor_secrets_test.go | 2 +- 13 files changed, 39 insertions(+), 19 deletions(-) diff --git a/cmd/frontend/graphqlbackend/default_settings.go b/cmd/frontend/graphqlbackend/default_settings.go index b635dbcfc12..08584f3d617 100644 --- a/cmd/frontend/graphqlbackend/default_settings.go +++ b/cmd/frontend/graphqlbackend/default_settings.go @@ -35,7 +35,7 @@ func (r *defaultSettingsResolver) LatestSettings(_ context.Context) (*settingsRe Subject: api.SettingsSubject{Default: true}, Contents: `{"experimentalFeatures": {}}`, } - return &settingsResolver{r.db, &settingsSubjectResolver{defaultSettings: r}, settings, nil}, nil + return &settingsResolver{db: r.db, subject: &settingsSubjectResolver{defaultSettings: r}, settings: settings}, nil } func (r *defaultSettingsResolver) SettingsURL() *string { return nil } diff --git a/cmd/frontend/graphqlbackend/org.go b/cmd/frontend/graphqlbackend/org.go index 48e5b931bdc..b3ee53d252f 100644 --- a/cmd/frontend/graphqlbackend/org.go +++ b/cmd/frontend/graphqlbackend/org.go @@ -253,7 +253,7 @@ func (o *OrgResolver) LatestSettings(ctx context.Context) (*settingsResolver, er return nil, nil } - return &settingsResolver{o.db, &settingsSubjectResolver{org: o}, settings, nil}, nil + return &settingsResolver{db: o.db, subject: &settingsSubjectResolver{org: o}, settings: settings}, nil } func (o *OrgResolver) SettingsCascade() *settingsCascade { diff --git a/cmd/frontend/graphqlbackend/org_test.go b/cmd/frontend/graphqlbackend/org_test.go index 60212f516cd..5daf3131bd0 100644 --- a/cmd/frontend/graphqlbackend/org_test.go +++ b/cmd/frontend/graphqlbackend/org_test.go @@ -31,7 +31,7 @@ func TestOrganization(t *testing.T) { users.GetByCurrentAuthUserFunc.SetDefaultReturn(&types.User{ID: 1}, nil) orgMembers := dbmocks.NewMockOrgMemberStore() - orgMembers.GetByOrgIDAndUserIDFunc.SetDefaultReturn(nil, nil) + orgMembers.GetByOrgIDAndUserIDFunc.SetDefaultReturn(nil, &database.ErrOrgMemberNotFound{}) orgs := dbmocks.NewMockOrgStore() mockedOrg := types.Org{ID: 1, Name: "acme"} diff --git a/cmd/frontend/graphqlbackend/preview_repository_comparison_test.go b/cmd/frontend/graphqlbackend/preview_repository_comparison_test.go index c151389b0fe..194c1e7d469 100644 --- a/cmd/frontend/graphqlbackend/preview_repository_comparison_test.go +++ b/cmd/frontend/graphqlbackend/preview_repository_comparison_test.go @@ -21,6 +21,10 @@ import ( ) func TestPreviewRepositoryComparisonResolver(t *testing.T) { + if testing.Short() { + t.Skip() + } + logger := logtest.Scoped(t) ctx := context.Background() db := database.NewDB(logger, nil) diff --git a/cmd/frontend/graphqlbackend/repository_comparison_test.go b/cmd/frontend/graphqlbackend/repository_comparison_test.go index eaa7e323a72..28a5979f255 100644 --- a/cmd/frontend/graphqlbackend/repository_comparison_test.go +++ b/cmd/frontend/graphqlbackend/repository_comparison_test.go @@ -61,6 +61,10 @@ func TestRepositoryComparisonNoMergeBase(t *testing.T) { } func TestRepositoryComparisonRootCommit(t *testing.T) { + if testing.Short() { + t.Skip() + } + logger := logtest.Scoped(t) ctx := context.Background() db := database.NewDB(logger, nil) @@ -94,6 +98,10 @@ func TestRepositoryComparisonRootCommit(t *testing.T) { } func TestRepositoryComparison(t *testing.T) { + if testing.Short() { + t.Skip() + } + logger := logtest.Scoped(t) ctx := context.Background() db := database.NewDB(logger, nil) diff --git a/cmd/frontend/graphqlbackend/repository_text_search_index_test.go b/cmd/frontend/graphqlbackend/repository_text_search_index_test.go index 466c2098cbd..0d1268e72c9 100644 --- a/cmd/frontend/graphqlbackend/repository_text_search_index_test.go +++ b/cmd/frontend/graphqlbackend/repository_text_search_index_test.go @@ -20,6 +20,10 @@ import ( ) func TestRetrievingAndDeduplicatingIndexedRefs(t *testing.T) { + if testing.Short() { + t.Skip() + } + logger := logtest.Scoped(t) db := database.NewDB(logger, nil) defaultBranchRef := "refs/heads/main" diff --git a/cmd/frontend/graphqlbackend/settings.go b/cmd/frontend/graphqlbackend/settings.go index 06ab6816758..e4bbf954312 100644 --- a/cmd/frontend/graphqlbackend/settings.go +++ b/cmd/frontend/graphqlbackend/settings.go @@ -4,6 +4,7 @@ import ( "context" "os" "strconv" + "sync" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/database" @@ -17,7 +18,10 @@ type settingsResolver struct { db database.DB subject *settingsSubjectResolver settings *api.Settings - user *types.User + + authorUserOnce sync.Once + authorUser *types.User + authorUserErr error } func (o *settingsResolver) ID() int32 { @@ -45,14 +49,14 @@ func (o *settingsResolver) Author(ctx context.Context) (*UserResolver, error) { if o.settings.AuthorUserID == nil { return nil, nil } - if o.user == nil { - var err error - o.user, err = o.db.Users().GetByID(ctx, *o.settings.AuthorUserID) - if err != nil { - return nil, err - } + + o.authorUserOnce.Do(func() { + o.authorUser, o.authorUserErr = o.db.Users().GetByID(ctx, *o.settings.AuthorUserID) + }) + if o.authorUserErr != nil { + return nil, o.authorUserErr } - return NewUserResolver(ctx, o.db, o.user), nil + return NewUserResolver(ctx, o.db, o.authorUser), nil } var globalSettingsAllowEdits, _ = strconv.ParseBool(env.Get("GLOBAL_SETTINGS_ALLOW_EDITS", "false", "When GLOBAL_SETTINGS_FILE is in use, allow edits in the application to be made which will be overwritten on next process restart")) diff --git a/cmd/frontend/graphqlbackend/site.go b/cmd/frontend/graphqlbackend/site.go index 6f1cf94a5b4..4952e235ed3 100644 --- a/cmd/frontend/graphqlbackend/site.go +++ b/cmd/frontend/graphqlbackend/site.go @@ -156,7 +156,7 @@ func (r *siteResolver) LatestSettings(ctx context.Context) (*settingsResolver, e if settings == nil { return nil, nil } - return &settingsResolver{r.db, &settingsSubjectResolver{site: r}, settings, nil}, nil + return &settingsResolver{db: r.db, subject: &settingsSubjectResolver{site: r}, settings: settings}, nil } func (r *siteResolver) SettingsCascade() *settingsCascade { diff --git a/cmd/frontend/graphqlbackend/site_admin_test.go b/cmd/frontend/graphqlbackend/site_admin_test.go index 2838ab37c16..4abe0805bda 100644 --- a/cmd/frontend/graphqlbackend/site_admin_test.go +++ b/cmd/frontend/graphqlbackend/site_admin_test.go @@ -307,7 +307,7 @@ func TestDeleteOrganization_OnPremise(t *testing.T) { users.GetByCurrentAuthUserFunc.SetDefaultReturn(&types.User{ID: 1}, nil) orgMembers := dbmocks.NewMockOrgMemberStore() - orgMembers.GetByOrgIDAndUserIDFunc.SetDefaultReturn(nil, nil) + orgMembers.GetByOrgIDAndUserIDFunc.SetDefaultReturn(nil, &database.ErrOrgMemberNotFound{}) orgs := dbmocks.NewMockOrgStore() diff --git a/cmd/frontend/graphqlbackend/user.go b/cmd/frontend/graphqlbackend/user.go index c18e3cf9e06..de4d2f0c00e 100644 --- a/cmd/frontend/graphqlbackend/user.go +++ b/cmd/frontend/graphqlbackend/user.go @@ -393,7 +393,7 @@ func (r *UserResolver) LatestSettings(ctx context.Context) (*settingsResolver, e if settings == nil { return nil, nil } - return &settingsResolver{r.db, &settingsSubjectResolver{user: r}, settings, nil}, nil + return &settingsResolver{db: r.db, subject: &settingsSubjectResolver{user: r}, settings: settings}, nil } func (r *UserResolver) SettingsCascade() *settingsCascade { diff --git a/cmd/frontend/internal/prompts/resolvers/resolvers_test.go b/cmd/frontend/internal/prompts/resolvers/resolvers_test.go index 68155e84b1b..dd6893013e8 100644 --- a/cmd/frontend/internal/prompts/resolvers/resolvers_test.go +++ b/cmd/frontend/internal/prompts/resolvers/resolvers_test.go @@ -103,7 +103,7 @@ func TestPrompts(t *testing.T) { orgID := int32(2) om := dbmocks.NewMockOrgMemberStore() om.GetByOrgIDAndUserIDFunc.SetDefaultHook(func(ctx context.Context, oid, uid int32) (*types.OrgMembership, error) { - return nil, nil + return nil, &database.ErrOrgMemberNotFound{} }) ss := dbmocks.NewMockPromptStore() @@ -551,7 +551,7 @@ func TestPromptPermissions(t *testing.T) { if orgID == userID { return &types.OrgMembership{}, nil } - return nil, nil + return nil, &database.ErrOrgMemberNotFound{} }) db := dbmocks.NewMockDB() diff --git a/cmd/frontend/internal/savedsearches/resolvers/resolvers_test.go b/cmd/frontend/internal/savedsearches/resolvers/resolvers_test.go index 0c229760bab..0527ad0ba5d 100644 --- a/cmd/frontend/internal/savedsearches/resolvers/resolvers_test.go +++ b/cmd/frontend/internal/savedsearches/resolvers/resolvers_test.go @@ -104,7 +104,7 @@ func TestSavedSearches(t *testing.T) { orgID := int32(2) om := dbmocks.NewMockOrgMemberStore() om.GetByOrgIDAndUserIDFunc.SetDefaultHook(func(ctx context.Context, oid, uid int32) (*types.OrgMembership, error) { - return nil, nil + return nil, &database.ErrOrgMemberNotFound{} }) ss := dbmocks.NewMockSavedSearchStore() @@ -582,7 +582,7 @@ func TestSavedSearchPermissions(t *testing.T) { if orgID == userID { return &types.OrgMembership{}, nil } - return nil, nil + return nil, &database.ErrOrgMemberNotFound{} }) db := dbmocks.NewMockDB() diff --git a/internal/database/executor_secrets_test.go b/internal/database/executor_secrets_test.go index 98f631c3c39..6acbb332e27 100644 --- a/internal/database/executor_secrets_test.go +++ b/internal/database/executor_secrets_test.go @@ -45,7 +45,7 @@ func TestEnsureActorHasNamespaceWriteAccess(t *testing.T) { // Is a member. return &types.OrgMembership{}, nil } - return nil, nil + return nil, &database.ErrOrgMemberNotFound{} }) db.OrgMembersFunc.SetDefaultReturn(om)