artin/sourcegraph
1
0
Fork 0
mirror of https://github.com/sourcegraph/sourcegraph.git synced 2026-02-06 17:51:57 +00:00
Code Issues Packages Projects Releases Wiki Activity

Policy patch - leave fields unchanged if they're missing from request (#64174)

Browse Source 
Fixes GRAPH-782

Specifically this allows us avoid updating syntactic_indexing_enabled
when we don't have the value available.

This is done to solve the problem where not providing that field crashes
the resolver. I confirmed it by both via tests and via manual testing of
the API
![CleanShot 2024-07-31 at 15 51
54](https://github.com/user-attachments/assets/2e82ff20-5541-4a7d-9dd6-17196274d59a)

## Test plan

- Added a new test for resolvers specifically to test the extra logic
around this field
This commit is contained in:
Anton Sviridov 2024-08-01 12:01:26 +01:00 committed by GitHub
parent a64832ab44
commit 7aba4732ee
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 200 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
internal/codeintel/policies/internal/store/configurations.go
View File

@ -292,22 +292,22 @@ var (
errIllegalConfigurationPolicyDelete = errors.New("protected configuration policies cannot be deleted")
)
func (s *store) UpdateConfigurationPolicy(ctx context.Context, policy shared.ConfigurationPolicy) (err error) {
func (s *store) UpdateConfigurationPolicy(ctx context.Context, policyPatch shared.ConfigurationPolicyPatch) (err error) {
ctx, _, endObservation := s.operations.updateConfigurationPolicy.With(ctx, &err, observation.Args{Attrs: []attribute.KeyValue{
attribute.Int("id", policy.ID),
attribute.Int("id", policyPatch.ID),
}})
defer endObservation(1, observation.Args{})
retentionDuration := optionalNumHours(policy.RetentionDuration)
indexCommitMaxAge := optionalNumHours(policy.IndexCommitMaxAge)
repositoryPatterns := optionalArray(policy.RepositoryPatterns)
retentionDuration := optionalNumHours(policyPatch.RetentionDuration)
indexCommitMaxAge := optionalNumHours(policyPatch.IndexCommitMaxAge)
repositoryPatterns := optionalArray(policyPatch.RepositoryPatterns)
return s.db.WithTransact(ctx, func(tx *basestore.Store) error {
// First, pull current policy to see if it's protected, and if so whether or not the
// fields that must remain stable (names, types, patterns, and retention enabled) have
// the same current and target values.
currentPolicy, ok, err := scanFirstConfigurationPolicy(tx.Query(ctx, sqlf.Sprintf(updateConfigurationPolicySelectQuery, policy.ID)))
currentPolicy, ok, err := scanFirstConfigurationPolicy(tx.Query(ctx, sqlf.Sprintf(updateConfigurationPolicySelectQuery, policyPatch.ID)))
if err != nil {
return err
}
@ -315,25 +315,33 @@ func (s *store) UpdateConfigurationPolicy(ctx context.Context, policy shared.Con
return errUnknownConfigurationPolicy
}
if currentPolicy.Protected {
if policy.Name != currentPolicy.Name || policy.Type != currentPolicy.Type || policy.Pattern != currentPolicy.Pattern || policy.RetentionEnabled != currentPolicy.RetentionEnabled || policy.RetainIntermediateCommits != currentPolicy.RetainIntermediateCommits {
if policyPatch.Name != currentPolicy.Name || policyPatch.Type != currentPolicy.Type || policyPatch.Pattern != currentPolicy.Pattern || policyPatch.RetentionEnabled != currentPolicy.RetentionEnabled || policyPatch.RetainIntermediateCommits != currentPolicy.RetainIntermediateCommits {
return errIllegalConfigurationPolicyUpdate
}
}
var syntaticIndexingEnabled bool
if policyPatch.SyntacticIndexingEnabled != nil {
syntaticIndexingEnabled = *policyPatch.SyntacticIndexingEnabled
} else {
syntaticIndexingEnabled = currentPolicy.SyntacticIndexingEnabled
}
return tx.Exec(ctx, sqlf.Sprintf(updateConfigurationPolicyQuery,
policy.Name,
policyPatch.Name,
repositoryPatterns,
policy.Type,
policy.Pattern,
policy.RetentionEnabled,
policyPatch.Type,
policyPatch.Pattern,
policyPatch.RetentionEnabled,
retentionDuration,
policy.RetainIntermediateCommits,
policy.PreciseIndexingEnabled,
policy.SyntacticIndexingEnabled,
policyPatch.RetainIntermediateCommits,
policyPatch.PreciseIndexingEnabled,
syntaticIndexingEnabled,
indexCommitMaxAge,
policy.IndexIntermediateCommits,
policy.EmbeddingEnabled,
policy.ID,
policyPatch.IndexIntermediateCommits,
policyPatch.EmbeddingEnabled,
policyPatch.ID,
))
})
}
@ -361,7 +369,7 @@ FOR UPDATE
`
const updateConfigurationPolicyQuery = `
UPDATE lsif_configuration_policies SET
UPDATE lsif_configuration_policies p SET
name = %s,
repository_patterns = %s,
type = %s,

2
internal/codeintel/policies/internal/store/store.go
View File

@ -20,7 +20,7 @@ type Store interface {
GetConfigurationPolicies(ctx context.Context, opts policiesshared.GetConfigurationPoliciesOptions) ([]shared.ConfigurationPolicy, int, error)
GetConfigurationPolicyByID(ctx context.Context, id int) (shared.ConfigurationPolicy, bool, error)
CreateConfigurationPolicy(ctx context.Context, configurationPolicy shared.ConfigurationPolicy) (shared.ConfigurationPolicy, error)
UpdateConfigurationPolicy(ctx context.Context, policy shared.ConfigurationPolicy) error
UpdateConfigurationPolicy(ctx context.Context, policy shared.ConfigurationPolicyPatch) error
DeleteConfigurationPolicyByID(ctx context.Context, id int) error
// Repository matches

22
internal/codeintel/policies/mocks_test.go generated
View File

@ -94,7 +94,7 @@ func NewMockStore() *MockStore {
},
},
UpdateConfigurationPolicyFunc: &StoreUpdateConfigurationPolicyFunc{
defaultHook: func(context.Context, shared.ConfigurationPolicy) (r0 error) {
defaultHook: func(context.Context, shared.ConfigurationPolicyPatch) (r0 error) {
return
},
},
@ -146,7 +146,7 @@ func NewStrictMockStore() *MockStore {
},
},
UpdateConfigurationPolicyFunc: &StoreUpdateConfigurationPolicyFunc{
defaultHook: func(context.Context, shared.ConfigurationPolicy) error {
defaultHook: func(context.Context, shared.ConfigurationPolicyPatch) error {
panic("unexpected invocation of MockStore.UpdateConfigurationPolicy")
},
},
@ -977,15 +977,15 @@ func (c StoreSelectPoliciesForRepositoryMembershipUpdateFuncCall) Results() []in
// UpdateConfigurationPolicy method of the parent MockStore instance is
// invoked.
type StoreUpdateConfigurationPolicyFunc struct {
defaultHook func(context.Context, shared.ConfigurationPolicy) error
hooks []func(context.Context, shared.ConfigurationPolicy) error
defaultHook func(context.Context, shared.ConfigurationPolicyPatch) error
hooks []func(context.Context, shared.ConfigurationPolicyPatch) error
history []StoreUpdateConfigurationPolicyFuncCall
mutex sync.Mutex
}
// UpdateConfigurationPolicy delegates to the next hook function in the
// queue and stores the parameter and result values of this invocation.
func (m *MockStore) UpdateConfigurationPolicy(v0 context.Context, v1 shared.ConfigurationPolicy) error {
func (m *MockStore) UpdateConfigurationPolicy(v0 context.Context, v1 shared.ConfigurationPolicyPatch) error {
r0 := m.UpdateConfigurationPolicyFunc.nextHook()(v0, v1)
m.UpdateConfigurationPolicyFunc.appendCall(StoreUpdateConfigurationPolicyFuncCall{v0, v1, r0})
return r0
@ -994,7 +994,7 @@ func (m *MockStore) UpdateConfigurationPolicy(v0 context.Context, v1 shared.Conf
// SetDefaultHook sets function that is called when the
// UpdateConfigurationPolicy method of the parent MockStore instance is
// invoked and the hook queue is empty.
func (f *StoreUpdateConfigurationPolicyFunc) SetDefaultHook(hook func(context.Context, shared.ConfigurationPolicy) error) {
func (f *StoreUpdateConfigurationPolicyFunc) SetDefaultHook(hook func(context.Context, shared.ConfigurationPolicyPatch) error) {
f.defaultHook = hook
}
@ -1002,7 +1002,7 @@ func (f *StoreUpdateConfigurationPolicyFunc) SetDefaultHook(hook func(context.Co
// UpdateConfigurationPolicy method of the parent MockStore 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 *StoreUpdateConfigurationPolicyFunc) PushHook(hook func(context.Context, shared.ConfigurationPolicy) error) {
func (f *StoreUpdateConfigurationPolicyFunc) PushHook(hook func(context.Context, shared.ConfigurationPolicyPatch) error) {
f.mutex.Lock()
f.hooks = append(f.hooks, hook)
f.mutex.Unlock()
@ -1011,19 +1011,19 @@ func (f *StoreUpdateConfigurationPolicyFunc) PushHook(hook func(context.Context,
// SetDefaultReturn calls SetDefaultHook with a function that returns the
// given values.
func (f *StoreUpdateConfigurationPolicyFunc) SetDefaultReturn(r0 error) {
f.SetDefaultHook(func(context.Context, shared.ConfigurationPolicy) error {
f.SetDefaultHook(func(context.Context, shared.ConfigurationPolicyPatch) error {
return r0
})
}
// PushReturn calls PushHook with a function that returns the given values.
func (f *StoreUpdateConfigurationPolicyFunc) PushReturn(r0 error) {
f.PushHook(func(context.Context, shared.ConfigurationPolicy) error {
f.PushHook(func(context.Context, shared.ConfigurationPolicyPatch) error {
return r0
})
}
func (f *StoreUpdateConfigurationPolicyFunc) nextHook() func(context.Context, shared.ConfigurationPolicy) error {
func (f *StoreUpdateConfigurationPolicyFunc) nextHook() func(context.Context, shared.ConfigurationPolicyPatch) error {
f.mutex.Lock()
defer f.mutex.Unlock()
@ -1062,7 +1062,7 @@ type StoreUpdateConfigurationPolicyFuncCall struct {
Arg0 context.Context
// Arg1 is the value of the 2nd argument passed to this method
// invocation.
Arg1 shared.ConfigurationPolicy
Arg1 shared.ConfigurationPolicyPatch
// Result0 is the value of the 1st result returned from this method
// invocation.
Result0 error

14
internal/codeintel/policies/service.go
View File

@ -59,17 +59,17 @@ func (s *Service) CreateConfigurationPolicy(ctx context.Context, configurationPo
return policy, err
}
if err := s.UpdateReposMatchingPolicyPatterns(ctx, policy); err != nil {
if err := s.UpdateReposMatchingPolicyPatterns(ctx, policy.RepositoryPatterns, policy.ID); err != nil {
return policy, err
}
return policy, nil
}
func (s *Service) UpdateReposMatchingPolicyPatterns(ctx context.Context, policy policiesshared.ConfigurationPolicy) error {
func (s *Service) UpdateReposMatchingPolicyPatterns(ctx context.Context, policyPatterns *[]string, policyID int) error {
var patterns []string
if policy.RepositoryPatterns != nil {
patterns = *policy.RepositoryPatterns
if policyPatterns != nil {
patterns = *policyPatterns
}
if len(patterns) == 0 {
@ -81,14 +81,14 @@ func (s *Service) UpdateReposMatchingPolicyPatterns(ctx context.Context, policy
repositoryMatchLimit = &val
}
if err := s.store.UpdateReposMatchingPatterns(ctx, patterns, policy.ID, repositoryMatchLimit); err != nil {
if err := s.store.UpdateReposMatchingPatterns(ctx, patterns, policyID, repositoryMatchLimit); err != nil {
return err
}
return nil
}
func (s *Service) UpdateConfigurationPolicy(ctx context.Context, policy policiesshared.ConfigurationPolicy) (err error) {
func (s *Service) UpdateConfigurationPolicy(ctx context.Context, policy policiesshared.ConfigurationPolicyPatch) (err error) {
ctx, _, endObservation := s.operations.updateConfigurationPolicy.With(ctx, &err, observation.Args{})
defer endObservation(1, observation.Args{})
@ -96,7 +96,7 @@ func (s *Service) UpdateConfigurationPolicy(ctx context.Context, policy policies
return err
}
return s.UpdateReposMatchingPolicyPatterns(ctx, policy)
return s.UpdateReposMatchingPolicyPatterns(ctx, policy.RepositoryPatterns, policy.ID)
}
func (s *Service) DeleteConfigurationPolicyByID(ctx context.Context, id int) error {

22
internal/codeintel/policies/shared/types.go
View File

@ -1,6 +1,8 @@
package shared
import "time"
import (
"time"
)
type ConfigurationPolicy struct {
ID int
@ -20,6 +22,24 @@ type ConfigurationPolicy struct {
EmbeddingEnabled bool
}
type ConfigurationPolicyPatch struct {
ID int
RepositoryID *int
RepositoryPatterns *[]string
Name string
Type GitObjectType
Pattern string
Protected bool
RetentionEnabled bool
RetentionDuration *time.Duration
RetainIntermediateCommits bool
PreciseIndexingEnabled bool
SyntacticIndexingEnabled *bool
IndexCommitMaxAge *time.Duration
IndexIntermediateCommits bool
EmbeddingEnabled bool
}
type GitObjectType string
const (

19
internal/codeintel/policies/transport/graphql/BUILD.bazel
View File

@ -1,3 +1,4 @@
load("//dev:go_defs.bzl", "go_test")
load("@io_bazel_rules_go//go:def.bzl", "go_library")
go_library(
@ -30,3 +31,21 @@ go_library(
"@io_opentelemetry_go_otel//attribute",
],
)
go_test(
name = "graphql_test",
srcs = ["root_resolver_test.go"],
embed = [":graphql"],
tags = ["requires-network"],
deps = [
"//internal/codeintel/policies",
"//internal/codeintel/resolvers",
"//internal/codeintel/shared/resolvers",
"//internal/database",
"//internal/database/dbtest",
"//internal/gitserver",
"//internal/observation",
"@com_github_sourcegraph_log//logtest",
"@com_github_stretchr_testify//require",
],
)

2
internal/codeintel/policies/transport/graphql/iface.go
View File

@ -15,7 +15,7 @@ type PoliciesService interface {
// Modify policies
CreateConfigurationPolicy(ctx context.Context, configurationPolicy shared.ConfigurationPolicy) (shared.ConfigurationPolicy, error)
UpdateConfigurationPolicy(ctx context.Context, policy shared.ConfigurationPolicy) (err error)
UpdateConfigurationPolicy(ctx context.Context, policy shared.ConfigurationPolicyPatch) (err error)
DeleteConfigurationPolicyByID(ctx context.Context, id int) error
// Filter previews

14
internal/codeintel/policies/transport/graphql/root_resolver_policy_mutations.go
View File

@ -49,10 +49,12 @@ func (r *rootResolver) CreateCodeIntelligenceConfigurationPolicy(ctx context.Con
RetentionDuration: toDuration(args.RetentionDurationHours),
RetainIntermediateCommits: args.RetainIntermediateCommits,
PreciseIndexingEnabled: args.IndexingEnabled,
SyntacticIndexingEnabled: *args.SyntacticIndexingEnabled,
IndexCommitMaxAge: toDuration(args.IndexCommitMaxAgeHours),
IndexIntermediateCommits: args.IndexIntermediateCommits,
EmbeddingEnabled: args.EmbeddingsEnabled != nil && *args.EmbeddingsEnabled,
// Tools using the old API will not pass syntactic indexing flag
// so we default to false while the feature is still in experimental mode.
SyntacticIndexingEnabled: args.SyntacticIndexingEnabled != nil && *args.SyntacticIndexingEnabled,
IndexCommitMaxAge: toDuration(args.IndexCommitMaxAgeHours),
IndexIntermediateCommits: args.IndexIntermediateCommits,
EmbeddingEnabled: args.EmbeddingsEnabled != nil && *args.EmbeddingsEnabled,
}
configurationPolicy, err := r.policySvc.CreateConfigurationPolicy(ctx, opts)
if err != nil {
@ -82,7 +84,7 @@ func (r *rootResolver) UpdateCodeIntelligenceConfigurationPolicy(ctx context.Con
return nil, err
}
opts := shared.ConfigurationPolicy{
opts := shared.ConfigurationPolicyPatch{
ID: id,
Name: args.Name,
RepositoryPatterns: args.RepositoryPatterns,
@ -92,7 +94,7 @@ func (r *rootResolver) UpdateCodeIntelligenceConfigurationPolicy(ctx context.Con
RetentionDuration: toDuration(args.RetentionDurationHours),
RetainIntermediateCommits: args.RetainIntermediateCommits,
PreciseIndexingEnabled: args.IndexingEnabled,
SyntacticIndexingEnabled: *args.SyntacticIndexingEnabled,
SyntacticIndexingEnabled: args.SyntacticIndexingEnabled,
IndexCommitMaxAge: toDuration(args.IndexCommitMaxAgeHours),
IndexIntermediateCommits: args.IndexIntermediateCommits,
EmbeddingEnabled: args.EmbeddingsEnabled != nil && *args.EmbeddingsEnabled,

105
internal/codeintel/policies/transport/graphql/root_resolver_test.go Normal file
View File

@ -0,0 +1,105 @@
package graphql
import (
"context"
"fmt"
"testing"
"github.com/sourcegraph/log/logtest"
"github.com/sourcegraph/sourcegraph/internal/codeintel/policies"
"github.com/sourcegraph/sourcegraph/internal/codeintel/resolvers"
sharedresolvers "github.com/sourcegraph/sourcegraph/internal/codeintel/shared/resolvers"
"github.com/sourcegraph/sourcegraph/internal/database"
"github.com/sourcegraph/sourcegraph/internal/database/dbtest"
"github.com/sourcegraph/sourcegraph/internal/gitserver"
"github.com/sourcegraph/sourcegraph/internal/observation"
"github.com/stretchr/testify/require"
)
func TestOmittingSyntacticField(t *testing.T) {
logger := logtest.Scoped(t)
db := database.NewDB(logger, dbtest.NewDB(t))
ctx := context.Background()
observationCtx := observation.TestContextTB(t)
service := policies.NewService(observationCtx, db, &mockUploadService{}, gitserver.NewMockClient())
resolver := NewRootResolver(observationCtx, service, db.Repos(), lenientSiteadminchecker{})
policyArgs :=
func(syntacticIndexingEnabled *bool) resolvers.CodeIntelConfigurationPolicy {
return resolvers.CodeIntelConfigurationPolicy{
Name: "test",
Type: "GIT_COMMIT",
Pattern: "HEAD",
RetentionEnabled: false,
RetainIntermediateCommits: true,
RetentionDurationHours: intptr(0),
IndexingEnabled: true,
SyntacticIndexingEnabled: syntacticIndexingEnabled,
IndexCommitMaxAgeHours: intptr(8064),
IndexIntermediateCommits: true,
}
}
// By default the syntactic indexing is disabled if request doesn't send an explicit value
created, err := resolver.CreateCodeIntelligenceConfigurationPolicy(ctx, &resolvers.CreateCodeIntelligenceConfigurationPolicyArgs{
CodeIntelConfigurationPolicy: policyArgs(nil),
})
require.NoError(t, err)
require.False(t, *created.SyntacticIndexingEnabled())
// Explicitly setting syntactic indexing field to true
_, err = resolver.UpdateCodeIntelligenceConfigurationPolicy(ctx, &resolvers.UpdateCodeIntelligenceConfigurationPolicyArgs{
ID: created.ID(),
CodeIntelConfigurationPolicy: policyArgs(boolptr(true)),
})
require.NoError(t, err)
retrieved, err := resolver.ConfigurationPolicyByID(ctx, created.ID())
require.NoError(t, err)
require.True(t, *retrieved.SyntacticIndexingEnabled())
// Omitting the syntactic indexing field during update should lead to no changes
_, err = resolver.UpdateCodeIntelligenceConfigurationPolicy(ctx, &resolvers.UpdateCodeIntelligenceConfigurationPolicyArgs{
ID: created.ID(),
CodeIntelConfigurationPolicy: policyArgs(nil),
})
require.NoError(t, err)
retrieved, err = resolver.ConfigurationPolicyByID(ctx, created.ID())
require.NoError(t, err)
require.True(t, *retrieved.SyntacticIndexingEnabled())
// Setting syntactic indexing field explicitly propagates the value into the policy
createdAnother, err := resolver.CreateCodeIntelligenceConfigurationPolicy(ctx, &resolvers.CreateCodeIntelligenceConfigurationPolicyArgs{
CodeIntelConfigurationPolicy: policyArgs(boolptr(true)),
})
fmt.Println(createdAnother)
require.NoError(t, err)
require.NotEqual(t, created.ID(), createdAnother.ID())
require.True(t, *createdAnother.SyntacticIndexingEnabled())
}
type lenientSiteadminchecker struct{}
// CheckCurrentUserIsSiteAdmin implements sharedresolvers.SiteAdminChecker.
func (l lenientSiteadminchecker) CheckCurrentUserIsSiteAdmin(ctx context.Context) error {
return nil
}
var _ sharedresolvers.SiteAdminChecker = lenientSiteadminchecker{}
func intptr(i int32) *int32 {
return &i
}
func boolptr(i bool) *bool {
return &i
}
type mockUploadService struct{}
// GetCommitsVisibleToUpload implements policies.UploadService.
func (m *mockUploadService) GetCommitsVisibleToUpload(ctx context.Context, uploadID int, limit int, token *string) (_ []string, nextToken *string, err error) {
panic("unimplemented")
}
var _ policies.UploadService = &mockUploadService{}

2
internal/codeintel/syntactic_indexing/scheduler_test.go
View File

@ -198,6 +198,6 @@ func setupRepoPolicies(t *testing.T, ctx context.Context, db database.DB, polici
for _, policyID := range []int{1100} {
policy, _, err := policies.GetConfigurationPolicyByID(ctx, policyID)
require.NoError(t, err)
require.NoError(t, policies.UpdateReposMatchingPolicyPatterns(ctx, policy))
require.NoError(t, policies.UpdateReposMatchingPolicyPatterns(ctx, policy.RepositoryPatterns, policy.ID))
}
}