[PR] Recheck Unchecked PR Merge Status (#434)

cmd/gitness/wire_gen.go

@@ -22,7 +22,7 @@ import (
 	"github.com/harness/gitness/internal/api/controller/pipeline"
 	"github.com/harness/gitness/internal/api/controller/plugin"
 	"github.com/harness/gitness/internal/api/controller/principal"
-	"github.com/harness/gitness/internal/api/controller/pullreq"
+	pullreq2 "github.com/harness/gitness/internal/api/controller/pullreq"
 	"github.com/harness/gitness/internal/api/controller/repo"
 	"github.com/harness/gitness/internal/api/controller/secret"
 	"github.com/harness/gitness/internal/api/controller/service"
@@ -51,7 +51,7 @@ import (
 	"github.com/harness/gitness/internal/services/codecomments"
 	"github.com/harness/gitness/internal/services/importer"
 	"github.com/harness/gitness/internal/services/job"
-	pullreq2 "github.com/harness/gitness/internal/services/pullreq"
+	"github.com/harness/gitness/internal/services/pullreq"
 	trigger2 "github.com/harness/gitness/internal/services/trigger"
 	"github.com/harness/gitness/internal/services/webhook"
 	"github.com/harness/gitness/internal/store"
@@ -174,10 +174,6 @@ func initSystem(ctx context.Context, config *types.Config) (*server.System, erro
 		return nil, err
 	}
 	migrator := codecomments.ProvideMigrator(gitrpcInterface)
-	pullreqController := pullreq.ProvideController(db, provider, authorizer, pullReqStore, pullReqActivityStore, codeCommentView, pullReqReviewStore, pullReqReviewerStore, repoStore, principalStore, gitrpcInterface, reporter, mutexManager, migrator)
-	webhookConfig := server.ProvideWebhookConfig(config)
-	webhookStore := database.ProvideWebhookStore(db)
-	webhookExecutionStore := database.ProvideWebhookExecutionStore(db)
 	readerFactory, err := events4.ProvideReaderFactory(eventsSystem)
 	if err != nil {
 		return nil, err
@@ -186,6 +182,16 @@ func initSystem(ctx context.Context, config *types.Config) (*server.System, erro
 	if err != nil {
 		return nil, err
 	}
+	repoGitInfoView := database.ProvideRepoGitInfoView(db)
+	repoGitInfoCache := cache.ProvideRepoGitInfoCache(repoGitInfoView)
+	pullreqService, err := pullreq.ProvideService(ctx, config, readerFactory, eventsReaderFactory, reporter, gitrpcInterface, db, repoGitInfoCache, repoStore, pullReqStore, pullReqActivityStore, codeCommentView, migrator, pubSub, provider)
+	if err != nil {
+		return nil, err
+	}
+	pullreqController := pullreq2.ProvideController(db, provider, authorizer, pullReqStore, pullReqActivityStore, codeCommentView, pullReqReviewStore, pullReqReviewerStore, repoStore, principalStore, gitrpcInterface, reporter, mutexManager, migrator, pullreqService)
+	webhookConfig := server.ProvideWebhookConfig(config)
+	webhookStore := database.ProvideWebhookStore(db)
+	webhookExecutionStore := database.ProvideWebhookExecutionStore(db)
 	webhookService, err := webhook.ProvideService(ctx, webhookConfig, readerFactory, eventsReaderFactory, webhookStore, webhookExecutionStore, repoStore, pullReqStore, provider, principalStore, gitrpcInterface)
 	if err != nil {
 		return nil, err
@@ -228,12 +234,6 @@ func initSystem(ctx context.Context, config *types.Config) (*server.System, erro
 		return nil, err
 	}
 	cronManager := cron.ProvideManager(serverConfig)
-	repoGitInfoView := database.ProvideRepoGitInfoView(db)
-	repoGitInfoCache := cache.ProvideRepoGitInfoCache(repoGitInfoView)
-	pullreqService, err := pullreq2.ProvideService(ctx, config, readerFactory, eventsReaderFactory, reporter, gitrpcInterface, db, repoGitInfoCache, repoStore, pullReqStore, pullReqActivityStore, codeCommentView, migrator, pubSub, provider)
-	if err != nil {
-		return nil, err
-	}
 	triggerConfig := server.ProvideTriggerConfig(config)
 	triggerService, err := trigger2.ProvideService(ctx, triggerConfig, triggerStore, commitService, pullReqStore, repoStore, pipelineStore, triggererTriggerer, readerFactory, eventsReaderFactory)
 	if err != nil {

internal/api/controller/pullreq/controller.go

@@ -16,6 +16,7 @@ import (
 	"github.com/harness/gitness/internal/auth/authz"
 	pullreqevents "github.com/harness/gitness/internal/events/pullreq"
 	"github.com/harness/gitness/internal/services/codecomments"
+	"github.com/harness/gitness/internal/services/pullreq"
 	"github.com/harness/gitness/internal/store"
 	"github.com/harness/gitness/internal/url"
 	"github.com/harness/gitness/lock"
@@ -40,6 +41,7 @@ type Controller struct {
 	eventReporter       *pullreqevents.Reporter
 	mtxManager          lock.MutexManager
 	codeCommentMigrator *codecomments.Migrator
+	pullreqService      *pullreq.Service
 }
 
 func NewController(
@@ -57,6 +59,7 @@ func NewController(
 	eventReporter *pullreqevents.Reporter,
 	mtxManager lock.MutexManager,
 	codeCommentMigrator *codecomments.Migrator,
+	pullreqService *pullreq.Service,
 ) *Controller {
 	return &Controller{
 		db:                  db,
@@ -73,6 +76,7 @@ func NewController(
 		codeCommentMigrator: codeCommentMigrator,
 		eventReporter:       eventReporter,
 		mtxManager:          mtxManager,
+		pullreqService:      pullreqService,
 	}
 }
 

internal/api/controller/pullreq/pr_recheck.go

@@ -0,0 +1,33 @@
+// Copyright 2022 Harness Inc. All rights reserved.
+// Use of this source code is governed by the Polyform Free Trial License
+// that can be found in the LICENSE.md file for this repository.
+
+package pullreq
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/harness/gitness/internal/auth"
+	"github.com/harness/gitness/types/enum"
+)
+
+// Recheck re-checks all system PR checks (mergeability check, ...).
+func (c *Controller) Recheck(
+	ctx context.Context,
+	session *auth.Session,
+	repoRef string,
+	prNum int64,
+) error {
+	repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoPush)
+	if err != nil {
+		return fmt.Errorf("failed to acquire access to repo: %w", err)
+	}
+
+	err = c.pullreqService.UpdateMergeDataIfRequired(ctx, repo.ID, prNum)
+	if err != nil {
+		return fmt.Errorf("failed to refresh merge data: %w", err)
+	}
+
+	return nil
+}

internal/api/controller/pullreq/wire.go

@@ -9,6 +9,7 @@ import (
 	"github.com/harness/gitness/internal/auth/authz"
 	pullreqevents "github.com/harness/gitness/internal/events/pullreq"
 	"github.com/harness/gitness/internal/services/codecomments"
+	"github.com/harness/gitness/internal/services/pullreq"
 	"github.com/harness/gitness/internal/store"
 	"github.com/harness/gitness/internal/url"
 	"github.com/harness/gitness/lock"
@@ -28,12 +29,14 @@ func ProvideController(db *sqlx.DB, urlProvider *url.Provider, authorizer authz.
 	pullReqReviewStore store.PullReqReviewStore, pullReqReviewerStore store.PullReqReviewerStore,
 	repoStore store.RepoStore, principalStore store.PrincipalStore,
 	rpcClient gitrpc.Interface, eventReporter *pullreqevents.Reporter,
-	mtxManager lock.MutexManager, codeCommentMigrator *codecomments.Migrator) *Controller {
+	mtxManager lock.MutexManager, codeCommentMigrator *codecomments.Migrator,
+	pullreqService *pullreq.Service,
+) *Controller {
 	return NewController(db, urlProvider, authorizer,
 		pullReqStore, pullReqActivityStore,
 		codeCommentsView,
 		pullReqReviewStore, pullReqReviewerStore,
 		repoStore, principalStore,
 		rpcClient, eventReporter,
-		mtxManager, codeCommentMigrator)
+		mtxManager, codeCommentMigrator, pullreqService)
 }

internal/api/handler/pullreq/pr_recheck.go

@@ -0,0 +1,41 @@
+// Copyright 2022 Harness Inc. All rights reserved.
+// Use of this source code is governed by the Polyform Free Trial License
+// that can be found in the LICENSE.md file for this repository.
+
+package pullreq
+
+import (
+	"net/http"
+
+	"github.com/harness/gitness/internal/api/controller/pullreq"
+	"github.com/harness/gitness/internal/api/render"
+	"github.com/harness/gitness/internal/api/request"
+)
+
+// HandleRecheck handles API that re-checks all system PR checks (mergeability check, ...).
+func HandleRecheck(pullreqCtrl *pullreq.Controller) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		ctx := r.Context()
+		session, _ := request.AuthSessionFrom(ctx)
+
+		repoRef, err := request.GetRepoRefFromPath(r)
+		if err != nil {
+			render.TranslatedUserError(w, err)
+			return
+		}
+
+		pullreqNumber, err := request.GetPullReqNumberFromPath(r)
+		if err != nil {
+			render.TranslatedUserError(w, err)
+			return
+		}
+
+		err = pullreqCtrl.Recheck(ctx, session, repoRef, pullreqNumber)
+		if err != nil {
+			render.TranslatedUserError(w, err)
+			return
+		}
+
+		w.WriteHeader(http.StatusNoContent)
+	}
+}

internal/api/openapi/pullreq.go

@@ -469,4 +469,15 @@ func pullReqOperations(reflector *openapi3.Reflector) {
 	_ = reflector.SetJSONResponse(&opMetaData, new(usererror.Error), http.StatusForbidden)
 	_ = reflector.SetJSONResponse(&opMetaData, new(usererror.Error), http.StatusNotFound)
 	_ = reflector.Spec.AddOperation(http.MethodGet, "/repos/{repo_ref}/pullreq/{pullreq_number}/metadata", opMetaData)
+
+	recheckPullReq := openapi3.Operation{}
+	recheckPullReq.WithTags("pullreq")
+	recheckPullReq.WithMapOfAnything(map[string]interface{}{"operationId": "recheckPullReq"})
+	_ = reflector.SetRequest(&recheckPullReq, nil, http.MethodPost)
+	_ = reflector.SetJSONResponse(&recheckPullReq, nil, http.StatusNoContent)
+	_ = reflector.SetJSONResponse(&recheckPullReq, new(usererror.Error), http.StatusBadRequest)
+	_ = reflector.SetJSONResponse(&recheckPullReq, new(usererror.Error), http.StatusInternalServerError)
+	_ = reflector.SetJSONResponse(&recheckPullReq, new(usererror.Error), http.StatusUnauthorized)
+	_ = reflector.SetJSONResponse(&recheckPullReq, new(usererror.Error), http.StatusForbidden)
+	_ = reflector.Spec.AddOperation(http.MethodPost, "/repos/{repo_ref}/pullreq/{pullreq_number}/recheck", recheckPullReq)
 }

internal/router/api.go

@@ -461,6 +461,7 @@ func SetupPullReq(r chi.Router, pullreqCtrl *pullreq.Controller) {
 			r.Get("/", handlerpullreq.HandleFind(pullreqCtrl))
 			r.Patch("/", handlerpullreq.HandleUpdate(pullreqCtrl))
 			r.Post("/state", handlerpullreq.HandleState(pullreqCtrl))
+			r.Post("/recheck", handlerpullreq.HandleRecheck(pullreqCtrl))
 			r.Get("/activities", handlerpullreq.HandleListActivities(pullreqCtrl))
 			r.Route("/comments", func(r chi.Router) {
 				r.Post("/", handlerpullreq.HandleCommentCreate(pullreqCtrl))

internal/services/pullreq/handlers_mergeable.go

@@ -106,6 +106,28 @@ func (s *Service) deleteMergeRef(ctx context.Context, repoID int64, prNum int64)
 	return nil
 }
 
+// UpdateMergeDataIfRequired rechecks the merge data of a PR.
+// TODO: This is a temporary solution - doesn't fix changed merge-base or other things.
+func (s *Service) UpdateMergeDataIfRequired(
+	ctx context.Context,
+	repoID int64,
+	prNum int64,
+) error {
+	pr, err := s.pullreqStore.FindByNumber(ctx, repoID, prNum)
+	if err != nil {
+		return fmt.Errorf("failed to get pull request number %d: %w", prNum, err)
+	}
+
+	// nothing to-do if check was already performed
+	if pr.MergeCheckStatus != enum.MergeCheckStatusUnchecked {
+		return nil
+	}
+
+	// WARNING: This CAN lead to two (or more) merge-checks on the same SHA
+	// running on different machines at the same time.
+	return s.updateMergeDataInner(ctx, pr, "", pr.SourceSHA)
+}
+
 //nolint:funlen // refactor if required.
 func (s *Service) updateMergeData(
 	ctx context.Context,
@@ -114,21 +136,31 @@ func (s *Service) updateMergeData(
 	oldSHA string,
 	newSHA string,
 ) error {
-	// TODO: Merge check should not update the merge base.
-	// TODO: Instead it should accept it as an argument and fail if it doesn't match.
-	// Then is would not longer be necessary to cancel already active mergeability checks.
-
 	pr, err := s.pullreqStore.FindByNumber(ctx, repoID, prNum)
 	if err != nil {
 		return fmt.Errorf("failed to get pull request number %d: %w", prNum, err)
 	}
 
+	return s.updateMergeDataInner(ctx, pr, oldSHA, newSHA)
+}
+
+//nolint:funlen // refactor if required.
+func (s *Service) updateMergeDataInner(
+	ctx context.Context,
+	pr *types.PullReq,
+	oldSHA string,
+	newSHA string,
+) error {
+	// TODO: Merge check should not update the merge base.
+	// TODO: Instead it should accept it as an argument and fail if it doesn't match.
+	// Then is would not longer be necessary to cancel already active mergeability checks.
+
 	if pr.State != enum.PullReqStateOpen {
-		return fmt.Errorf("cannot do mergability check on closed PR %d", prNum)
+		return fmt.Errorf("cannot do mergability check on closed PR %d", pr.Number)
 	}
 
 	// cancel all previous mergability work for this PR based on oldSHA
-	if err = s.pubsub.Publish(ctx, cancelMergeCheckKey, []byte(oldSHA),
+	if err := s.pubsub.Publish(ctx, cancelMergeCheckKey, []byte(oldSHA),
 		pubsub.WithPublishNamespace("pullreq")); err != nil {
 		return err
 	}
@@ -137,6 +169,13 @@ func (s *Service) updateMergeData(
 	ctx, cancel = context.WithCancel(ctx)
 
 	s.cancelMutex.Lock()
+	// NOTE: Temporary workaround to avoid overwriting existing cancel method on same machine.
+	// This doesn't avoid same SHA running on multiple machines
+	if _, ok := s.cancelMergeability[newSHA]; ok {
+		s.cancelMutex.Unlock()
+		cancel()
+		return nil
+	}
 	s.cancelMergeability[newSHA] = cancel
 	s.cancelMutex.Unlock()
 
@@ -175,7 +214,7 @@ func (s *Service) updateMergeData(
 		HeadRepoUID:     sourceRepo.GitUID,
 		HeadBranch:      pr.SourceBranch,
 		RefType:         gitrpcenum.RefTypePullReqMerge,
-		RefName:         strconv.Itoa(int(prNum)),
+		RefName:         strconv.Itoa(int(pr.Number)),
 		HeadExpectedSHA: newSHA,
 		Force:           true,
 

web/src/pages/PullRequest/PullRequest.tsx

@@ -1,7 +1,7 @@
 import React, { useCallback, useEffect, useMemo, useState } from 'react'
 import { Container, Layout, PageBody, Tabs, Text } from '@harnessio/uicore'
 import { FontVariation } from '@harnessio/design-system'
-import { useGet } from 'restful-react'
+import { useGet, useMutate } from 'restful-react'
 import { Render } from 'react-jsx-match'
 import { useHistory } from 'react-router-dom'
 import { compact } from 'lodash-es'
@@ -9,7 +9,7 @@ import { useAppContext } from 'AppContext'
 import { useGetRepositoryMetadata } from 'hooks/useGetRepositoryMetadata'
 import { useStrings } from 'framework/strings'
 import { RepositoryPageHeader } from 'components/RepositoryPageHeader/RepositoryPageHeader'
-import { voidFn, getErrorMessage, PullRequestSection } from 'utils/Utils'
+import { voidFn, getErrorMessage, PullRequestSection, MergeCheckStatus } from 'utils/Utils'
 import { CodeIcon } from 'utils/GitUtils'
 import type { TypesPullReq, TypesPullReqStats, TypesRepository } from 'services/code'
 import { LoadingSpinner } from 'components/LoadingSpinner/LoadingSpinner'
@@ -83,6 +83,14 @@ export default function PullRequest() {
       })
     )
   }, [history, routes, repoMetadata?.path, pullRequestId])
+  const recheckPath = useMemo(
+    () => `/api/v1/repos/${repoMetadata?.path}/+/pullreq/${pullRequestId}/recheck`,
+    [repoMetadata?.path, pullRequestId]
+  )
+  const { mutate : recheckPR, loading: loadingRecheckPR } = useMutate({
+    verb: 'POST',
+    path: recheckPath,
+  })
 
   useEffect(
     function setStatsIfNotSet() {
@@ -98,6 +106,27 @@ export default function PullRequest() {
   useEffect(
     function setPrDataIfNotSet() {
       if (pullRequestData) {
+        // recheck pr (merge-check, ...) in case it's unavailable
+        // Approximation of identifying target branch update:
+        //   1. branch got updated before page was loaded (status is unchecked and prData is empty)
+        //      NOTE: This doesn't guarantee the status is UNCHECKED due to target branch update and can cause duplicate
+        //      PR merge checks being run on PR creation or source branch update.
+        //   2. branch got updated while we are on the page (same source_sha but status changed to UNCHECKED)
+        //      NOTE: This doesn't cover the case in which the status changed back to UNCHECKED before the PR is refetched.
+        //      In that case, the user will have to re-open the PR - better than us spamming the backend with rechecks.
+        // This is a TEMPORARY SOLUTION and will most likely change in the future (more so on backend side)
+        if (pullRequestData.state == 'open' &&
+            pullRequestData.merge_check_status == MergeCheckStatus.UNCHECKED &&
+            (
+              // case 1:
+              !prData || 
+              // case 2:
+              (prData?.merge_check_status != MergeCheckStatus.UNCHECKED && prData?.source_sha == pullRequestData.source_sha)
+            ) && !loadingRecheckPR) {
+              // best effort attempt to recheck PR - fail silently
+              recheckPR({})
+        }
+
         setPrData(pullRequestData)
       }
     },

web/src/services/code/index.tsx

@@ -3398,6 +3398,40 @@ export const usePullReqMetaData = ({ repo_ref, pullreq_number, ...props }: UsePu
     { base: getConfig('code/api/v1'), pathParams: { repo_ref, pullreq_number }, ...props }
   )
 
+export interface RecheckPullReqPathParams {
+  repo_ref: string
+  pullreq_number: number
+}
+
+export type RecheckPullReqProps = Omit<
+  MutateProps<void, UsererrorError, void, OpenapiUpdatePullReqRequest, RecheckPullReqPathParams>,
+  'path' | 'verb'
+> &
+  RecheckPullReqPathParams
+
+export const RecheckPullReq = ({ repo_ref, pullreq_number, ...props }: RecheckPullReqProps) => (
+  <Mutate<void, UsererrorError, void, OpenapiUpdatePullReqRequest, RecheckPullReqPathParams>
+    verb="POST"
+    path={`/repos/${repo_ref}/pullreq/${pullreq_number}/recheck`}
+    base={getConfig('code/api/v1')}
+    {...props}
+  />
+)
+
+export type UseRecheckPullReqProps = Omit<
+  UseMutateProps<void, UsererrorError, void, OpenapiUpdatePullReqRequest, RecheckPullReqPathParams>,
+  'path' | 'verb'
+> &
+  RecheckPullReqPathParams
+
+export const useRecheckPullReq = ({ repo_ref, pullreq_number, ...props }: UseRecheckPullReqProps) =>
+  useMutate<void, UsererrorError, void, OpenapiUpdatePullReqRequest, RecheckPullReqPathParams>(
+    'POST',
+    (paramsInPath: RecheckPullReqPathParams) =>
+      `/repos/${paramsInPath.repo_ref}/pullreq/${paramsInPath.pullreq_number}/recheck`,
+    { base: getConfig('code/api/v1'), pathParams: { repo_ref, pullreq_number }, ...props }
+  )
+
 export interface ReviewerListPullReqPathParams {
   repo_ref: string
   pullreq_number: number

web/src/services/code/swagger.yaml

@@ -3622,6 +3622,54 @@ paths:
           description: Internal Server Error
       tags:
         - pullreq
+  /repos/{repo_ref}/pullreq/{pullreq_number}/recheck:
+    post:
+      operationId: recheckPullReq
+      parameters:
+        - in: path
+          name: repo_ref
+          required: true
+          schema:
+            type: string
+        - in: path
+          name: pullreq_number
+          required: true
+          schema:
+            type: integer
+      requestBody:
+        content:
+          application/json:
+            schema:
+              $ref: '#/components/schemas/OpenapiUpdatePullReqRequest'
+      responses:
+        "204":
+          description: No Content
+        "400":
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/UsererrorError'
+          description: Bad Request
+        "401":
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/UsererrorError'
+          description: Unauthorized
+        "403":
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/UsererrorError'
+          description: Forbidden
+        "500":
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/UsererrorError'
+          description: Internal Server Error
+      tags:
+        - pullreq
   /repos/{repo_ref}/pullreq/{pullreq_number}/reviewers:
     get:
       operationId: reviewerListPullReq