From c4d0030a84a9f375e75539fd53d72329fa0ca60e Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Mon, 29 Jun 2020 16:35:49 +0200 Subject: [PATCH 1/8] Implement support to preview unsupported patches --- cmd/frontend/graphqlbackend/campaigns.go | 1 + cmd/frontend/graphqlbackend/schema.go | 3 + cmd/frontend/graphqlbackend/schema.graphql | 3 + .../campaigns/resolvers/apitest/types.go | 1 + .../campaigns/resolvers/patch_sets.go | 8 + .../campaigns/resolvers/patch_sets_test.go | 12 +- enterprise/internal/campaigns/service.go | 19 +- enterprise/internal/campaigns/service_test.go | 16 ++ enterprise/internal/campaigns/store.go | 21 +- enterprise/internal/campaigns/store_test.go | 87 ++++++++- .../campaigns/detail/CampaignDetails.tsx | 2 - .../CampaignDetails.test.tsx.snap | 1 - .../enterprise/campaigns/detail/backend.ts | 2 + .../detail/patches/PatchNode.test.tsx | 39 ++-- .../campaigns/detail/patches/PatchNode.tsx | 11 +- .../detail/patches/PatchSetPatches.test.tsx | 1 - .../detail/patches/PatchSetPatches.tsx | 4 +- .../__snapshots__/PatchNode.test.tsx.snap | 181 +++++++++++++++++- 18 files changed, 380 insertions(+), 32 deletions(-) diff --git a/cmd/frontend/graphqlbackend/campaigns.go b/cmd/frontend/graphqlbackend/campaigns.go index 59001170dba8..984adf3aeaa9 100644 --- a/cmd/frontend/graphqlbackend/campaigns.go +++ b/cmd/frontend/graphqlbackend/campaigns.go @@ -319,6 +319,7 @@ type PatchResolver interface { Diff() PatchResolver FileDiffs(ctx context.Context, args *FileDiffsConnectionArgs) (FileDiffConnection, error) PublicationEnqueued(ctx context.Context) (bool, error) + Publishable(ctx context.Context) (bool, error) } type ChangesetEventsConnectionResolver interface { diff --git a/cmd/frontend/graphqlbackend/schema.go b/cmd/frontend/graphqlbackend/schema.go index 2a21022d9148..ffc9494e7d2d 100644 --- a/cmd/frontend/graphqlbackend/schema.go +++ b/cmd/frontend/graphqlbackend/schema.go @@ -732,6 +732,9 @@ type Patch implements PatchInterface & Node { # - A campaign has been created with the patchset to which this patch belongs. # - The patch has been individually published through the publishChangeset mutation. publicationEnqueued: Boolean! + + # True, when the code host of the associated repository is supported by campaigns. + publishable: Boolean! } # A hidden patch is a patch in a repository that the user does NOT have diff --git a/cmd/frontend/graphqlbackend/schema.graphql b/cmd/frontend/graphqlbackend/schema.graphql index 47d017c90c64..50cafc10e15c 100755 --- a/cmd/frontend/graphqlbackend/schema.graphql +++ b/cmd/frontend/graphqlbackend/schema.graphql @@ -739,6 +739,9 @@ type Patch implements PatchInterface & Node { # - A campaign has been created with the patchset to which this patch belongs. # - The patch has been individually published through the publishChangeset mutation. publicationEnqueued: Boolean! + + # True, when the code host of the associated repository is supported by campaigns. + publishable: Boolean! } # A hidden patch is a patch in a repository that the user does NOT have diff --git a/enterprise/internal/campaigns/resolvers/apitest/types.go b/enterprise/internal/campaigns/resolvers/apitest/types.go index ee7ac5787f2a..b6a994ed4c99 100644 --- a/enterprise/internal/campaigns/resolvers/apitest/types.go +++ b/enterprise/internal/campaigns/resolvers/apitest/types.go @@ -70,6 +70,7 @@ type Patch struct { Typename string `json:"__typename"` ID string PublicationEnqueued bool + Publishable bool Repository struct{ Name, URL string } Diff struct { FileDiffs FileDiffs diff --git a/enterprise/internal/campaigns/resolvers/patch_sets.go b/enterprise/internal/campaigns/resolvers/patch_sets.go index dfd725764099..764b2617d55a 100644 --- a/enterprise/internal/campaigns/resolvers/patch_sets.go +++ b/enterprise/internal/campaigns/resolvers/patch_sets.go @@ -316,6 +316,14 @@ func (r *patchResolver) PublicationEnqueued(ctx context.Context) (bool, error) { return cj.FinishedAt.IsZero(), nil } +func (r *patchResolver) Publishable(ctx context.Context) (bool, error) { + repo, _, err := r.computeRepoCommit(ctx) + if err != nil { + return false, err + } + return campaigns.IsRepoSupported(repo.ExternalRepo()), nil +} + func (r *patchResolver) Diff() graphqlbackend.PatchResolver { return r } diff --git a/enterprise/internal/campaigns/resolvers/patch_sets_test.go b/enterprise/internal/campaigns/resolvers/patch_sets_test.go index 94c189d46038..8f02869a6c30 100644 --- a/enterprise/internal/campaigns/resolvers/patch_sets_test.go +++ b/enterprise/internal/campaigns/resolvers/patch_sets_test.go @@ -119,7 +119,9 @@ func TestPatchSetResolver(t *testing.T) { ... on Patch { repository { name - } + } + publicationEnqueued + publishable diff { fileDiffs(first: %d, after: %s) { rawDiff @@ -179,6 +181,14 @@ func TestPatchSetResolver(t *testing.T) { t.Fatalf("wrong Repository Name %q. want=%q", have, want) } + if have, want := patch.PublicationEnqueued, false; have != want { + t.Fatalf("wrong publication enqueued status %t. want=%t", have, want) + } + + if have, want := patch.Publishable, true; have != want { + t.Fatalf("wrong publishable status %t. want=%t", have, want) + } + if have, want := patch.Diff.FileDiffs.RawDiff, testDiff; have != want { t.Fatalf("wrong RawDiff. diff=%s", cmp.Diff(have, want)) } diff --git a/enterprise/internal/campaigns/service.go b/enterprise/internal/campaigns/service.go index 5cd083c606d7..610d64579f56 100644 --- a/enterprise/internal/campaigns/service.go +++ b/enterprise/internal/campaigns/service.go @@ -54,7 +54,7 @@ func (s *Service) CreatePatchSetFromPatches(ctx context.Context, patches []*camp repoIDs := make([]api.RepoID, len(patches)) for i, patch := range patches { - repoIDs[i] = api.RepoID(patch.RepoID) + repoIDs[i] = patch.RepoID } // 🚨 SECURITY: We use db.Repos.GetByIDs to check for which the user has access. repos, err := db.Repos.GetByIDs(ctx, repoIDs...) @@ -79,13 +79,9 @@ func (s *Service) CreatePatchSetFromPatches(ctx context.Context, patches []*camp } for _, patch := range patches { - repo, ok := reposByID[patch.RepoID] - if !ok { + if _, ok := reposByID[patch.RepoID]; !ok { return nil, &db.RepoNotFoundErr{ID: patch.RepoID} } - if !campaigns.IsRepoSupported(&repo.ExternalRepo) { - continue - } patch.PatchSetID = patchSet.ID if err := tx.CreatePatch(ctx, patch); err != nil { @@ -166,6 +162,9 @@ var ErrNoPatches = errors.New("cannot create or update a Campaign without any ch // finished execution. var ErrCloseProcessingCampaign = errors.New("cannot close a Campaign while changesets are being created on codehosts") +// ErrUnsupportedCodehost is returned by EnqueueChangesetJobForPatch if the target repo of a patch is an unsupported repo. +var ErrUnsupportedCodehost = errors.New("cannot publish patch for unsupported codehost") + // CloseCampaign closes the Campaign with the given ID if it has not been closed yet. func (s *Service) CloseCampaign(ctx context.Context, id int64, closeChangesets bool) (campaign *campaigns.Campaign, err error) { traceTitle := fmt.Sprintf("campaign: %d, closeChangesets: %t", id, closeChangesets) @@ -586,6 +585,7 @@ func (s *Service) EnqueueChangesetJobs(ctx context.Context, campaignID int64) (_ Limit: -1, OnlyWithDiff: true, OnlyWithoutChangesetJob: campaign.ID, + OnlySupportedCodehosts: true, NoDiff: true, }) if err != nil { @@ -648,6 +648,13 @@ func (s *Service) EnqueueChangesetJobForPatch(ctx context.Context, patchID int64 if err != nil { return err } + repo, err := db.Repos.Get(ctx, job.RepoID) + if err != nil { + return err + } + if !campaigns.IsRepoSupported(&repo.ExternalRepo) { + return ErrUnsupportedCodehost + } campaign, err := s.store.GetCampaign(ctx, GetCampaignOpts{PatchSetID: job.PatchSetID}) if err != nil { diff --git a/enterprise/internal/campaigns/service_test.go b/enterprise/internal/campaigns/service_test.go index a1d5001d8720..f58a5260adcd 100644 --- a/enterprise/internal/campaigns/service_test.go +++ b/enterprise/internal/campaigns/service_test.go @@ -257,6 +257,13 @@ func TestService(t *testing.T) { rs = append(rs, r) } + awsCodeCommitRepoID := 4 + { + r := testRepo(awsCodeCommitRepoID, extsvc.TypeAWSCodeCommit) + r.Sources = map[string]*repos.SourceInfo{ext.URN(): {ID: ext.URN()}} + rs = append(rs, r) + } + err := reposStore.UpsertRepos(ctx, rs...) if err != nil { t.Fatal(err) @@ -615,6 +622,15 @@ func TestService(t *testing.T) { if !haveJob3.FinishedAt.IsZero() { t.Errorf("unexpected changesetJob FinishedAt value: %v. want=%v", haveJob3.FinishedAt, time.Time{}) } + + // Update repo to unsupported codehost type. + awsPatch := testPatch(patchSet.ID, rs[awsCodeCommitRepoID].ID, now) + if err := store.CreatePatch(ctx, awsPatch); err != nil { + t.Fatal(err) + } + if wantErr, haveErr := ErrUnsupportedCodehost, svc.EnqueueChangesetJobForPatch(ctx, awsPatch.ID); wantErr != haveErr { + t.Fatalf("got invalid error, want=%v have=%v", wantErr, haveErr) + } }) t.Run("EnqueueChangesetJobs", func(t *testing.T) { diff --git a/enterprise/internal/campaigns/store.go b/enterprise/internal/campaigns/store.go index 61940af4dd12..a97c53cd7123 100644 --- a/enterprise/internal/campaigns/store.go +++ b/enterprise/internal/campaigns/store.go @@ -78,7 +78,11 @@ func (s *Store) ProcessPendingChangesetJobs(ctx context.Context, process func(ct return false, errors.Wrap(err, "starting transaction") } defer tx.Done(&err) - q := sqlf.Sprintf(getPendingChangesetJobQuery) + supportedTypes := make([]*sqlf.Query, 0) + for t := range campaigns.SupportedExternalServices { + supportedTypes = append(supportedTypes, sqlf.Sprintf("%s", t)) + } + q := sqlf.Sprintf(getPendingChangesetJobQuery, sqlf.Join(supportedTypes, ",")) var job campaigns.ChangesetJob _, count, err := tx.query(ctx, q, func(sc scanner) (last, count int64, err error) { err = scanChangesetJob(&job, sc) @@ -101,7 +105,9 @@ const getPendingChangesetJobQuery = ` UPDATE changeset_jobs j SET started_at = now() WHERE id = ( SELECT j.id FROM changeset_jobs j JOIN campaigns c ON c.id = j.campaign_id - WHERE j.started_at IS NULL AND c.patch_set_id IS NOT NULL + INNER JOIN patches p ON p.id = j.patch_id + INNER JOIN repo r ON r.id = p.repo_id + WHERE j.started_at IS NULL AND c.patch_set_id IS NOT NULL AND r.external_service_type IN (%s) ORDER BY j.updated_at ASC FOR UPDATE SKIP LOCKED LIMIT 1 ) @@ -2246,6 +2252,9 @@ type ListPatchesOpts struct { // memory allocations that can be unnecessary if only the other columns are // used. NoDiff bool + + // If this is set, only patches on supported codehosts are returned. + OnlySupportedCodehosts bool } // ListPatches lists Patches with the given filters. @@ -2326,6 +2335,14 @@ func listPatchesQuery(opts *ListPatchesOpts) *sqlf.Query { preds = append(preds, sqlf.Sprintf("(patches.diff_stat_added IS NULL OR patches.diff_stat_deleted IS NULL OR patches.diff_stat_changed IS NULL)")) } + if opts.OnlySupportedCodehosts { + supportedTypes := make([]*sqlf.Query, 0) + for t := range campaigns.SupportedExternalServices { + supportedTypes = append(supportedTypes, sqlf.Sprintf("%s", t)) + } + preds = append(preds, sqlf.Sprintf("repo.external_service_type IN (%s)", sqlf.Join(supportedTypes, ","))) + } + // To replace a field within a SELECT, we need to avoid extra escaping, // which we can do by ensuring it's a sqlf.Query already by using // sqlf.Sprintf, even though there's no actual formatting to be done. diff --git a/enterprise/internal/campaigns/store_test.go b/enterprise/internal/campaigns/store_test.go index 4188e405cfe0..c245aceedd2f 100644 --- a/enterprise/internal/campaigns/store_test.go +++ b/enterprise/internal/campaigns/store_test.go @@ -1706,7 +1706,8 @@ func testStorePatches(t *testing.T, ctx context.Context, s *Store, reposStore re repo := testRepo(1, extsvc.TypeGitHub) deletedRepo := testRepo(2, extsvc.TypeGitHub).With(repos.Opt.RepoDeletedAt(clock.now())) - if err := reposStore.UpsertRepos(ctx, deletedRepo, repo); err != nil { + unsupportedRepo := testRepo(3, extsvc.TypeAWSCodeCommit) + if err := reposStore.UpsertRepos(ctx, deletedRepo, repo, unsupportedRepo); err != nil { t.Fatal(err) } @@ -2013,6 +2014,39 @@ func testStorePatches(t *testing.T, ctx context.Context, s *Store, reposStore re } }) + t.Run("Listing OnlySupportedCodehosts", func(t *testing.T) { + // Create patch to unsupported repo. + unsupportedRepoPatch := &cmpgn.Patch{ + PatchSetID: 1000, + RepoID: unsupportedRepo.ID, + Rev: api.CommitID("deadbeef"), + BaseRef: "master", + Diff: "+ foobar - barfoo", + } + err = s.CreatePatch(ctx, unsupportedRepoPatch) + if err != nil { + t.Fatal(err) + } + // List the patches and see what we get back. + listOpts := ListPatchesOpts{OnlySupportedCodehosts: true} + have, _, err := s.ListPatches(ctx, listOpts) + if err != nil { + t.Fatal(err) + } + + want := patches + if len(have) != len(want) { + t.Fatalf("listed %d patches, want: %d", len(have), len(want)) + } + + if diff := cmp.Diff(have, want); diff != "" { + t.Fatalf("opts: %+v, diff: %s", listOpts, diff) + } + if err := s.DeletePatch(ctx, unsupportedRepoPatch.ID); err != nil { + t.Fatal(err) + } + }) + t.Run("Listing OnlyWithoutDiffStats", func(t *testing.T) { listOpts := ListPatchesOpts{OnlyWithoutDiffStats: true} have, _, err := s.ListPatches(ctx, listOpts) @@ -3342,6 +3376,57 @@ func testProcessChangesetJob(db *sql.DB, userID int32) func(*testing.T) { if rc != 1 { t.Errorf("Want %d, got %d", 1, rc) } + if err := s.DeleteChangesetJob(ctx, job.ID); err != nil { + t.Fatal(err) + } + }) + + t.Run("GetPendingChangesetJobUnsupportedCodehost", func(t *testing.T) { + tx := dbtest.NewTx(t, db) + s := NewStoreWithClock(tx, clock) + + process := func(ctx context.Context, s *Store, job cmpgn.ChangesetJob) error { + return errors.New("rollback") + } + + // Set repo type to AWS code commit, an unsupported codehost of campaigns. + awsRepo := &repos.Repo{ + Name: "codecommit.aws/sourcegraph/changeset-job-test", + ExternalRepo: api.ExternalRepoSpec{ + ID: "external-id", + ServiceType: extsvc.TypeAWSCodeCommit, + ServiceID: "https://github.com/", + }, + Sources: map[string]*repos.SourceInfo{ + "extsvc:github:4": { + ID: "extsvc:awscodeCommit:7", + CloneURL: "https://secrettoken@codecommit.aws/sourcegraph/sourcegraph", + }, + }, + } + if err := reposStore.UpsertRepos(ctx, awsRepo); err != nil { + t.Fatal(err) + } + patch.RepoID = awsRepo.ID + if err := s.UpdatePatch(ctx, patch); err != nil { + t.Fatal(err) + } + job := &cmpgn.ChangesetJob{ + CampaignID: campaign.ID, + PatchID: patch.ID, + } + if err := s.CreateChangesetJob(ctx, job); err != nil { + t.Fatal(err) + } + + ran, err := s.ProcessPendingChangesetJobs(ctx, process) + if err != nil { + t.Fatal(err) + } + if ran { + // We shouldn't have any pending, supported job. + t.Fatalf("process function should not have run") + } }) } } diff --git a/web/src/enterprise/campaigns/detail/CampaignDetails.tsx b/web/src/enterprise/campaigns/detail/CampaignDetails.tsx index 74a77e971fdb..ed0d78b3d633 100644 --- a/web/src/enterprise/campaigns/detail/CampaignDetails.tsx +++ b/web/src/enterprise/campaigns/detail/CampaignDetails.tsx @@ -583,8 +583,6 @@ export const CampaignDetails: React.FunctionComponent = ({ patchSet={patchSet!} campaignUpdates={campaignUpdates} changesetUpdates={changesetUpdates} - // No publishing allowed in create view. - enablePublishing={false} history={history} location={location} isLightTheme={isLightTheme} diff --git a/web/src/enterprise/campaigns/detail/__snapshots__/CampaignDetails.test.tsx.snap b/web/src/enterprise/campaigns/detail/__snapshots__/CampaignDetails.test.tsx.snap index ba890ca8548d..6015569d5aa3 100644 --- a/web/src/enterprise/campaigns/detail/__snapshots__/CampaignDetails.test.tsx.snap +++ b/web/src/enterprise/campaigns/detail/__snapshots__/CampaignDetails.test.tsx.snap @@ -290,7 +290,6 @@ exports[`CampaignDetails creation form given existing patch set 1`] = ` { const history = H.createMemoryHistory({ keyLength: 0 }) const location = H.createLocation('/campaigns') - const renderPatch = ({ enablePublishing, fileDiff }: { enablePublishing: boolean; fileDiff: boolean }): void => { + const renderPatch = ({ + enablePublishing, + publishable, + fileDiff, + }: { + enablePublishing: boolean + publishable: boolean + fileDiff: boolean + }): void => { expect( shallow( { node={ { __typename: 'Patch', + id: 'something', + publishable, + publicationEnqueued: false, diff: fileDiff - ? { + ? ({ fileDiffs: { __typename: 'FileDiffConnection', diffStat: { @@ -28,13 +39,13 @@ describe('PatchNode', () => { deleted: 100, }, }, - } + } as IPreviewRepositoryComparison) : null, repository: { __typename: 'Repository', name: 'sourcegraph', url: 'github.com/sourcegraph/sourcegraph', - }, + } as IRepository, } as IPatch } campaignUpdates={new Subject()} @@ -43,13 +54,17 @@ describe('PatchNode', () => { ) ).toMatchSnapshot() } - test('renders a patch with publishing disabled', () => { - renderPatch({ enablePublishing: false, fileDiff: true }) - }) - test('renders a patch with publishing enabled', () => { - renderPatch({ enablePublishing: true, fileDiff: true }) - }) + for (const publishable of [false, true]) { + test(`renders a patch with publishing disabled and publishable: ${ + publishable ? 'enabled' : 'disabled' + }`, () => { + renderPatch({ enablePublishing: false, publishable, fileDiff: true }) + }) + test(`renders a patch with publishing enabled and publishable: ${publishable ? 'enabled' : 'disabled'}`, () => { + renderPatch({ enablePublishing: true, publishable, fileDiff: true }) + }) + } test('renders a patch without a filediff', () => { - renderPatch({ enablePublishing: true, fileDiff: false }) + renderPatch({ enablePublishing: true, publishable: true, fileDiff: false }) }) }) diff --git a/web/src/enterprise/campaigns/detail/patches/PatchNode.tsx b/web/src/enterprise/campaigns/detail/patches/PatchNode.tsx index 1cf408ab0c97..5f3df09225f8 100644 --- a/web/src/enterprise/campaigns/detail/patches/PatchNode.tsx +++ b/web/src/enterprise/campaigns/detail/patches/PatchNode.tsx @@ -14,6 +14,7 @@ import { asError, isErrorLike } from '../../../../../../shared/src/util/errors' import { FileDiffConnection } from '../../../../components/diff/FileDiffConnection' import { FilteredConnectionQueryArgs } from '../../../../components/FilteredConnection' import { Observer } from 'rxjs' +import InfoCircleOutlineIcon from 'mdi-react/InfoCircleOutlineIcon' export interface PatchNodeProps extends ThemeProps { node: IPatch @@ -64,6 +65,14 @@ export const PatchNode: React.FunctionComponent = ({ {node.repository.name} + {!node.publishable && ( + + + + )} @@ -77,7 +86,7 @@ export const PatchNode: React.FunctionComponent = ({ + + + } + titleClassName="changeset-node__content flex-fill" + wholeTitleClickable={false} + > + + + +`; + +exports[`PatchNode renders a patch with publishing enabled and publishable: enabled 1`] = `
  • From 1b6988d563cdda56e087900020b3cfe469c963e8 Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Tue, 30 Jun 2020 17:10:58 +0200 Subject: [PATCH 2/8] Simplify implementation --- enterprise/internal/campaigns/service.go | 16 +++- enterprise/internal/campaigns/store.go | 21 +---- enterprise/internal/campaigns/store_test.go | 87 +-------------------- 3 files changed, 18 insertions(+), 106 deletions(-) diff --git a/enterprise/internal/campaigns/service.go b/enterprise/internal/campaigns/service.go index 610d64579f56..3ee09a07091e 100644 --- a/enterprise/internal/campaigns/service.go +++ b/enterprise/internal/campaigns/service.go @@ -585,7 +585,6 @@ func (s *Service) EnqueueChangesetJobs(ctx context.Context, campaignID int64) (_ Limit: -1, OnlyWithDiff: true, OnlyWithoutChangesetJob: campaign.ID, - OnlySupportedCodehosts: true, NoDiff: true, }) if err != nil { @@ -615,6 +614,8 @@ func (s *Service) EnqueueChangesetJobs(ctx context.Context, campaignID int64) (_ jobsByPatchID[j.PatchID] = j } + reposByID := make(map[api.RepoID]*types.Repo) + for _, p := range patches { if _, ok := jobsByPatchID[p.ID]; ok { continue @@ -624,6 +625,19 @@ func (s *Service) EnqueueChangesetJobs(ctx context.Context, campaignID int64) (_ continue } + if _, ok := reposByID[p.RepoID]; !ok { + repo, err := db.Repos.Get(ctx, p.RepoID) + if err != nil { + return nil, err + } + reposByID[p.RepoID] = repo + } + + // Check if the repo is on a supported codehost. + if !campaigns.IsRepoSupported(&reposByID[p.RepoID].ExternalRepo) { + continue + } + j := &campaigns.ChangesetJob{CampaignID: campaign.ID, PatchID: p.ID} if err := tx.CreateChangesetJob(ctx, j); err != nil { return nil, err diff --git a/enterprise/internal/campaigns/store.go b/enterprise/internal/campaigns/store.go index a97c53cd7123..61940af4dd12 100644 --- a/enterprise/internal/campaigns/store.go +++ b/enterprise/internal/campaigns/store.go @@ -78,11 +78,7 @@ func (s *Store) ProcessPendingChangesetJobs(ctx context.Context, process func(ct return false, errors.Wrap(err, "starting transaction") } defer tx.Done(&err) - supportedTypes := make([]*sqlf.Query, 0) - for t := range campaigns.SupportedExternalServices { - supportedTypes = append(supportedTypes, sqlf.Sprintf("%s", t)) - } - q := sqlf.Sprintf(getPendingChangesetJobQuery, sqlf.Join(supportedTypes, ",")) + q := sqlf.Sprintf(getPendingChangesetJobQuery) var job campaigns.ChangesetJob _, count, err := tx.query(ctx, q, func(sc scanner) (last, count int64, err error) { err = scanChangesetJob(&job, sc) @@ -105,9 +101,7 @@ const getPendingChangesetJobQuery = ` UPDATE changeset_jobs j SET started_at = now() WHERE id = ( SELECT j.id FROM changeset_jobs j JOIN campaigns c ON c.id = j.campaign_id - INNER JOIN patches p ON p.id = j.patch_id - INNER JOIN repo r ON r.id = p.repo_id - WHERE j.started_at IS NULL AND c.patch_set_id IS NOT NULL AND r.external_service_type IN (%s) + WHERE j.started_at IS NULL AND c.patch_set_id IS NOT NULL ORDER BY j.updated_at ASC FOR UPDATE SKIP LOCKED LIMIT 1 ) @@ -2252,9 +2246,6 @@ type ListPatchesOpts struct { // memory allocations that can be unnecessary if only the other columns are // used. NoDiff bool - - // If this is set, only patches on supported codehosts are returned. - OnlySupportedCodehosts bool } // ListPatches lists Patches with the given filters. @@ -2335,14 +2326,6 @@ func listPatchesQuery(opts *ListPatchesOpts) *sqlf.Query { preds = append(preds, sqlf.Sprintf("(patches.diff_stat_added IS NULL OR patches.diff_stat_deleted IS NULL OR patches.diff_stat_changed IS NULL)")) } - if opts.OnlySupportedCodehosts { - supportedTypes := make([]*sqlf.Query, 0) - for t := range campaigns.SupportedExternalServices { - supportedTypes = append(supportedTypes, sqlf.Sprintf("%s", t)) - } - preds = append(preds, sqlf.Sprintf("repo.external_service_type IN (%s)", sqlf.Join(supportedTypes, ","))) - } - // To replace a field within a SELECT, we need to avoid extra escaping, // which we can do by ensuring it's a sqlf.Query already by using // sqlf.Sprintf, even though there's no actual formatting to be done. diff --git a/enterprise/internal/campaigns/store_test.go b/enterprise/internal/campaigns/store_test.go index c245aceedd2f..4188e405cfe0 100644 --- a/enterprise/internal/campaigns/store_test.go +++ b/enterprise/internal/campaigns/store_test.go @@ -1706,8 +1706,7 @@ func testStorePatches(t *testing.T, ctx context.Context, s *Store, reposStore re repo := testRepo(1, extsvc.TypeGitHub) deletedRepo := testRepo(2, extsvc.TypeGitHub).With(repos.Opt.RepoDeletedAt(clock.now())) - unsupportedRepo := testRepo(3, extsvc.TypeAWSCodeCommit) - if err := reposStore.UpsertRepos(ctx, deletedRepo, repo, unsupportedRepo); err != nil { + if err := reposStore.UpsertRepos(ctx, deletedRepo, repo); err != nil { t.Fatal(err) } @@ -2014,39 +2013,6 @@ func testStorePatches(t *testing.T, ctx context.Context, s *Store, reposStore re } }) - t.Run("Listing OnlySupportedCodehosts", func(t *testing.T) { - // Create patch to unsupported repo. - unsupportedRepoPatch := &cmpgn.Patch{ - PatchSetID: 1000, - RepoID: unsupportedRepo.ID, - Rev: api.CommitID("deadbeef"), - BaseRef: "master", - Diff: "+ foobar - barfoo", - } - err = s.CreatePatch(ctx, unsupportedRepoPatch) - if err != nil { - t.Fatal(err) - } - // List the patches and see what we get back. - listOpts := ListPatchesOpts{OnlySupportedCodehosts: true} - have, _, err := s.ListPatches(ctx, listOpts) - if err != nil { - t.Fatal(err) - } - - want := patches - if len(have) != len(want) { - t.Fatalf("listed %d patches, want: %d", len(have), len(want)) - } - - if diff := cmp.Diff(have, want); diff != "" { - t.Fatalf("opts: %+v, diff: %s", listOpts, diff) - } - if err := s.DeletePatch(ctx, unsupportedRepoPatch.ID); err != nil { - t.Fatal(err) - } - }) - t.Run("Listing OnlyWithoutDiffStats", func(t *testing.T) { listOpts := ListPatchesOpts{OnlyWithoutDiffStats: true} have, _, err := s.ListPatches(ctx, listOpts) @@ -3376,57 +3342,6 @@ func testProcessChangesetJob(db *sql.DB, userID int32) func(*testing.T) { if rc != 1 { t.Errorf("Want %d, got %d", 1, rc) } - if err := s.DeleteChangesetJob(ctx, job.ID); err != nil { - t.Fatal(err) - } - }) - - t.Run("GetPendingChangesetJobUnsupportedCodehost", func(t *testing.T) { - tx := dbtest.NewTx(t, db) - s := NewStoreWithClock(tx, clock) - - process := func(ctx context.Context, s *Store, job cmpgn.ChangesetJob) error { - return errors.New("rollback") - } - - // Set repo type to AWS code commit, an unsupported codehost of campaigns. - awsRepo := &repos.Repo{ - Name: "codecommit.aws/sourcegraph/changeset-job-test", - ExternalRepo: api.ExternalRepoSpec{ - ID: "external-id", - ServiceType: extsvc.TypeAWSCodeCommit, - ServiceID: "https://github.com/", - }, - Sources: map[string]*repos.SourceInfo{ - "extsvc:github:4": { - ID: "extsvc:awscodeCommit:7", - CloneURL: "https://secrettoken@codecommit.aws/sourcegraph/sourcegraph", - }, - }, - } - if err := reposStore.UpsertRepos(ctx, awsRepo); err != nil { - t.Fatal(err) - } - patch.RepoID = awsRepo.ID - if err := s.UpdatePatch(ctx, patch); err != nil { - t.Fatal(err) - } - job := &cmpgn.ChangesetJob{ - CampaignID: campaign.ID, - PatchID: patch.ID, - } - if err := s.CreateChangesetJob(ctx, job); err != nil { - t.Fatal(err) - } - - ran, err := s.ProcessPendingChangesetJobs(ctx, process) - if err != nil { - t.Fatal(err) - } - if ran { - // We shouldn't have any pending, supported job. - t.Fatalf("process function should not have run") - } }) } } From f728554f28dcbb830acd7bc3281fe8216eabc1dc Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Tue, 30 Jun 2020 17:23:52 +0200 Subject: [PATCH 3/8] Add check for update --- enterprise/internal/campaigns/service.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/enterprise/internal/campaigns/service.go b/enterprise/internal/campaigns/service.go index 3ee09a07091e..801be3096ae1 100644 --- a/enterprise/internal/campaigns/service.go +++ b/enterprise/internal/campaigns/service.go @@ -1149,7 +1149,13 @@ func computeCampaignUpdateDiff( if group, ok := byRepoID[j.RepoID]; ok { group.newPatch = j } else { - + repo, err := db.Repos.Get(ctx, j.RepoID) + if err != nil { + return nil, err + } + if !campaigns.IsRepoSupported(&repo.ExternalRepo) { + continue + } // If we have new Patches that don't match an existing // ChangesetJob we need to create new ChangesetJobs. diff.Create = append(diff.Create, &campaigns.ChangesetJob{ From 83c8eaec5bbcee8a7b250fa8937bbb7309679cd9 Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Tue, 30 Jun 2020 17:28:26 +0200 Subject: [PATCH 4/8] Improve performance --- enterprise/internal/campaigns/service.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/enterprise/internal/campaigns/service.go b/enterprise/internal/campaigns/service.go index 801be3096ae1..38b720475c5f 100644 --- a/enterprise/internal/campaigns/service.go +++ b/enterprise/internal/campaigns/service.go @@ -615,6 +615,15 @@ func (s *Service) EnqueueChangesetJobs(ctx context.Context, campaignID int64) (_ } reposByID := make(map[api.RepoID]*types.Repo) + { + repos, err := db.Repos.GetByIDs(ctx, repoIDs...) + if err != nil { + return nil, err + } + for _, r := range repos { + reposByID[r.ID] = r + } + } for _, p := range patches { if _, ok := jobsByPatchID[p.ID]; ok { @@ -626,11 +635,7 @@ func (s *Service) EnqueueChangesetJobs(ctx context.Context, campaignID int64) (_ } if _, ok := reposByID[p.RepoID]; !ok { - repo, err := db.Repos.Get(ctx, p.RepoID) - if err != nil { - return nil, err - } - reposByID[p.RepoID] = repo + return nil, errors.Errorf("cannot find repo %d", p.RepoID) } // Check if the repo is on a supported codehost. From 90804d84559d4d954fd6aee11d5fac133c783b8d Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Tue, 30 Jun 2020 18:02:03 +0200 Subject: [PATCH 5/8] Add test --- enterprise/internal/campaigns/service_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/enterprise/internal/campaigns/service_test.go b/enterprise/internal/campaigns/service_test.go index f58a5260adcd..baea6babecd5 100644 --- a/enterprise/internal/campaigns/service_test.go +++ b/enterprise/internal/campaigns/service_test.go @@ -1301,6 +1301,7 @@ func TestService_UpdateCampaignWithNewPatchSetID(t *testing.T) { for i := 0; i < 4; i++ { rs = append(rs, testRepo(i, extsvc.TypeGitHub)) } + rs = append(rs, testRepo(len(rs), extsvc.TypeAWSCodeCommit)) reposStore := repos.NewDBStore(dbconn.Global, sql.TxOptions{}) err := reposStore.UpsertRepos(ctx, rs...) @@ -1537,6 +1538,17 @@ func TestService_UpdateCampaignWithNewPatchSetID(t *testing.T) { wantUnmodified: repoNames{"repo-0"}, wantCreated: repoNames{"repo-1"}, }, + { + name: "1 added on unsupported codehost", + updatePatchSet: true, + oldPatches: repoNames{"repo-0"}, + newPatches: []newPatchSpec{ + {repo: "repo-0"}, + {repo: "repo-4"}, + }, + wantUnmodified: repoNames{"repo-0"}, + wantCreated: []string{}, + }, } for _, tt := range tests { From 991873adea370e10e8759b3d03fb921ed2063135 Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Tue, 30 Jun 2020 18:15:56 +0200 Subject: [PATCH 6/8] Strengthen test --- enterprise/internal/campaigns/service_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/enterprise/internal/campaigns/service_test.go b/enterprise/internal/campaigns/service_test.go index baea6babecd5..65c8d0ba13dc 100644 --- a/enterprise/internal/campaigns/service_test.go +++ b/enterprise/internal/campaigns/service_test.go @@ -656,7 +656,7 @@ func TestService(t *testing.T) { } // Filter out one repository to make sure it's skipped - filteredOutPatch := patches[len(patches)-1] + filteredOutPatch := patches[len(patches)-2] db.MockAuthzFilter = func(ctx context.Context, repos []*types.Repo, p authz.Perms) ([]*types.Repo, error) { var filtered []*types.Repo for _, r := range repos { @@ -681,7 +681,7 @@ func TestService(t *testing.T) { t.Fatal(err) } - wantJobsCount := len(patches) - 1 // We filtered out one repository + wantJobsCount := len(patches) - 2 // We filtered out one repository, and one is unsupported. if have, want := len(haveJobs), wantJobsCount; have != want { t.Fatal("wrong number of changeset jobs created") } From 44ca9dafb14270fd938891c5d541065518c366f4 Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Wed, 1 Jul 2020 13:01:53 +0200 Subject: [PATCH 7/8] Improve performance --- enterprise/internal/campaigns/service.go | 40 ++++++++---------------- 1 file changed, 13 insertions(+), 27 deletions(-) diff --git a/enterprise/internal/campaigns/service.go b/enterprise/internal/campaigns/service.go index 38b720475c5f..a9ded149c039 100644 --- a/enterprise/internal/campaigns/service.go +++ b/enterprise/internal/campaigns/service.go @@ -404,13 +404,13 @@ func (s *Service) AddChangesetsToCampaign(ctx context.Context, campaignID int64, return nil, err } - accessibleRepoIDs, err := accessibleRepos(ctx, changesets.RepoIDs()) + accessibleRepos, err := accessibleRepos(ctx, changesets.RepoIDs()) if err != nil { return nil, err } for _, c := range changesets { - if _, ok := accessibleRepoIDs[c.RepoID]; !ok { + if _, ok := accessibleRepos[c.RepoID]; !ok { return nil, &db.RepoNotFoundErr{ID: c.RepoID} } @@ -523,14 +523,14 @@ func (s *Service) RetryPublishCampaign(ctx context.Context, id int64) (campaign return nil, err } - accessibleRepoIDs, err := accessibleRepos(ctx, repoIDs) + accessibleRepos, err := accessibleRepos(ctx, repoIDs) if err != nil { return nil, err } var resetPatchIDs []int64 for _, p := range patches { - if _, ok := accessibleRepoIDs[p.RepoID]; !ok { + if _, ok := accessibleRepos[p.RepoID]; !ok { continue } @@ -596,7 +596,7 @@ func (s *Service) EnqueueChangesetJobs(ctx context.Context, campaignID int64) (_ repoIDs = append(repoIDs, p.RepoID) } - accessibleRepoIDs, err := accessibleRepos(ctx, repoIDs) + accessibleRepos, err := accessibleRepos(ctx, repoIDs) if err != nil { return nil, err } @@ -614,32 +614,18 @@ func (s *Service) EnqueueChangesetJobs(ctx context.Context, campaignID int64) (_ jobsByPatchID[j.PatchID] = j } - reposByID := make(map[api.RepoID]*types.Repo) - { - repos, err := db.Repos.GetByIDs(ctx, repoIDs...) - if err != nil { - return nil, err - } - for _, r := range repos { - reposByID[r.ID] = r - } - } - for _, p := range patches { if _, ok := jobsByPatchID[p.ID]; ok { continue } - if _, ok := accessibleRepoIDs[p.RepoID]; !ok { + r, ok := accessibleRepos[p.RepoID] + if !ok { continue } - if _, ok := reposByID[p.RepoID]; !ok { - return nil, errors.Errorf("cannot find repo %d", p.RepoID) - } - // Check if the repo is on a supported codehost. - if !campaigns.IsRepoSupported(&reposByID[p.RepoID].ExternalRepo) { + if !campaigns.IsRepoSupported(&r.ExternalRepo) { continue } @@ -1046,14 +1032,14 @@ func resetAccessibleChangesetJobs(ctx context.Context, tx *Store, campaign *camp repoIDs = append(repoIDs, p.RepoID) } - accessibleRepoIDs, err := accessibleRepos(ctx, repoIDs) + accessibleRepos, err := accessibleRepos(ctx, repoIDs) if err != nil { return err } var resetPatchIDs []int64 for _, p := range patches { - if _, ok := accessibleRepoIDs[p.RepoID]; !ok { + if _, ok := accessibleRepos[p.RepoID]; !ok { continue } @@ -1314,7 +1300,7 @@ func hasCampaignAdminPermissions(ctx context.Context, c *campaigns.Campaign) (bo // accessibleRepos collects the RepoIDs of the changesets and returns a set of // the api.RepoID for which the subset of repositories for which the actor in // ctx has read permissions. -func accessibleRepos(ctx context.Context, ids []api.RepoID) (map[api.RepoID]struct{}, error) { +func accessibleRepos(ctx context.Context, ids []api.RepoID) (map[api.RepoID]*types.Repo, error) { // 🚨 SECURITY: We use db.Repos.GetByIDs to filter out repositories the // user doesn't have access to. accessibleRepos, err := db.Repos.GetByIDs(ctx, ids...) @@ -1322,9 +1308,9 @@ func accessibleRepos(ctx context.Context, ids []api.RepoID) (map[api.RepoID]stru return nil, err } - accessibleRepoIDs := make(map[api.RepoID]struct{}, len(accessibleRepos)) + accessibleRepoIDs := make(map[api.RepoID]*types.Repo, len(accessibleRepos)) for _, r := range accessibleRepos { - accessibleRepoIDs[r.ID] = struct{}{} + accessibleRepoIDs[r.ID] = r } return accessibleRepoIDs, nil From 1f73503b770d7f28e2524d592762946b12a6ef7e Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Wed, 1 Jul 2020 13:12:33 +0200 Subject: [PATCH 8/8] Reduce redundancy --- enterprise/internal/campaigns/service.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/enterprise/internal/campaigns/service.go b/enterprise/internal/campaigns/service.go index a9ded149c039..2c418abed598 100644 --- a/enterprise/internal/campaigns/service.go +++ b/enterprise/internal/campaigns/service.go @@ -1123,7 +1123,7 @@ func computeCampaignUpdateDiff( } // 🚨 SECURITY: Check which repositories the user has access to. If the // user doesn't have access, don't create/delete/update anything. - accessibleRepoIDs, err := accessibleRepos(ctx, repoIDs) + accessibleRepos, err := accessibleRepos(ctx, repoIDs) if err != nil { return nil, err } @@ -1133,20 +1133,17 @@ func computeCampaignUpdateDiff( // return an error instead of skipping patches, so we don't end up with // an unfixable state (i.e. unpublished patch + changeset for same // repo). - if _, ok := accessibleRepoIDs[j.RepoID]; !ok { + repo, ok := accessibleRepos[j.RepoID] + if !ok { return nil, &db.RepoNotFoundErr{ID: j.RepoID} } + if !campaigns.IsRepoSupported(&repo.ExternalRepo) { + continue + } if group, ok := byRepoID[j.RepoID]; ok { group.newPatch = j } else { - repo, err := db.Repos.Get(ctx, j.RepoID) - if err != nil { - return nil, err - } - if !campaigns.IsRepoSupported(&repo.ExternalRepo) { - continue - } // If we have new Patches that don't match an existing // ChangesetJob we need to create new ChangesetJobs. diff.Create = append(diff.Create, &campaigns.ChangesetJob{ @@ -1159,7 +1156,7 @@ func computeCampaignUpdateDiff( for repoID, group := range byRepoID { // If the user is lacking permissions for this repository we don't // delete/update the changeset. - if _, ok := accessibleRepoIDs[repoID]; !ok { + if _, ok := accessibleRepos[repoID]; !ok { continue }