Merge pull request #1270 from jhrozek/pr_in_db

Store PRs in the database to avoid special-casing them during evaluation

database/migrations/000003_rule_evaluations_pr_num.down.sql

@@ -0,0 +1,33 @@
+-- Copyright 2023 Stacklok, Inc
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--      http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+-- Postgres can't remove a value for an enum type. So, we can't really
+-- do a down migration. Instead, we'll just leave this here as a
+-- reminder that we can't remove this value.
+
+-- Drop the existing unique index on rule_evaluations
+DROP INDEX IF EXISTS rule_evaluations_results_idx;
+
+-- Recreate the unique index without COALESCE on pull_request_id
+CREATE UNIQUE INDEX rule_evaluations_results_idx
+    ON rule_evaluations(profile_id, repository_id, COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID), entity, rule_type_id)
+
+-- Remove the pr reference from rule_evaluations
+ALTER TABLE rule_evaluations DROP COLUMN IF EXISTS pull_request_id;
+
+-- Drop the existing unique index on rule_evaluations
+DROP INDEX IF EXISTS pr_in_repo_unique;
+
+-- Drop the pull_requests table
+DROP TABLE IF EXISTS pull_requests;

database/migrations/000003_rule_evaluations_pr_num.up.sql

@@ -0,0 +1,37 @@
+-- Copyright 2023 Stacklok, Inc
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--      http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+-- pull_requests table
+CREATE TABLE pull_requests (
+                           id UUID NOT NULL DEFAULT gen_random_uuid() PRIMARY KEY,
+                           repository_id UUID NOT NULL REFERENCES repositories(id) ON DELETE CASCADE,
+                           pr_number BIGINT NOT NULL, -- BIGINT because GitHub PR numbers are 64-bit integers
+                           created_at TIMESTAMP NOT NULL DEFAULT NOW(),
+                           updated_at TIMESTAMP NOT NULL DEFAULT NOW()
+);
+
+-- Add unique constraint on repository_id and pr_number. Needed for upserts.
+CREATE UNIQUE INDEX pr_in_repo_unique ON pull_requests (repository_id, pr_number);
+
+-- Add pull_request_id to rule_evaluations
+ALTER TABLE rule_evaluations
+    ADD COLUMN pull_request_id UUID REFERENCES pull_requests(id) ON DELETE CASCADE;
+
+-- Drop the existing unique index on rule_evaluations
+DROP INDEX IF EXISTS rule_evaluations_results_idx;
+
+-- Recreate the unique index with COALESCE on pull_request_id
+CREATE UNIQUE INDEX rule_evaluations_results_idx
+    ON rule_evaluations(profile_id, repository_id, COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID), entity, rule_type_id, COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID));
+

database/query/profile_status.sql

@@ -1,9 +1,9 @@
 
 -- name: UpsertRuleEvaluations :one
 INSERT INTO rule_evaluations (
-    profile_id, repository_id, artifact_id, rule_type_id, entity
-) VALUES ($1, $2, $3, $4, $5)
-ON CONFLICT (profile_id, repository_id, COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID), entity, rule_type_id)
+    profile_id, repository_id, artifact_id, pull_request_id, rule_type_id, entity
+) VALUES ($1, $2, $3, $4, $5, $6)
+ON CONFLICT (profile_id, repository_id, COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID), COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID), entity, rule_type_id)
   DO UPDATE SET profile_id = $1
 RETURNING id;
 
@@ -129,6 +129,8 @@ WHERE res.profile_id = $1 AND
         CASE
             WHEN sqlc.narg(entity_type)::entities = 'repository' AND res.repository_id = sqlc.narg(entity_id)::UUID THEN true
             WHEN sqlc.narg(entity_type)::entities  = 'artifact' AND res.artifact_id = sqlc.narg(entity_id)::UUID THEN true
+            WHEN sqlc.narg(entity_type)::entities  = 'artifact' AND res.artifact_id = sqlc.narg(entity_id)::UUID THEN true
+            WHEN sqlc.narg(entity_type)::entities  = 'pull_request' AND res.pull_request_id = sqlc.narg(entity_id)::UUID THEN true
             WHEN sqlc.narg(entity_id)::UUID IS NULL THEN true
             ELSE false
             END

database/query/pull_requests.sql

@@ -0,0 +1,24 @@
+-- name: CreatePullRequest :one
+INSERT INTO pull_requests (
+    repository_id,
+    pr_number
+) VALUES ($1, $2) RETURNING *;
+
+-- name: UpsertPullRequest :one
+INSERT INTO pull_requests (
+    repository_id,
+    pr_number
+) VALUES ($1, $2)
+ON CONFLICT (repository_id, pr_number)
+DO UPDATE SET
+    updated_at = NOW()
+WHERE pull_requests.repository_id = $1 AND pull_requests.pr_number = $2
+RETURNING *;
+
+-- name: GetPullRequest :one
+SELECT * FROM pull_requests
+WHERE repository_id = $1 AND pr_number = $2;
+
+-- name: DeletePullRequest :exec
+DELETE FROM pull_requests
+WHERE repository_id = $1 AND pr_number = $2;

internal/controlplane/handlers_githubwebhooks.go

@@ -85,6 +85,9 @@ var ErrArtifactNotFound = errors.New("artifact not found")
 // ErrRepoIsPrivate is returned when a repository is private
 var ErrRepoIsPrivate = errors.New("repository is private")
 
+// ErrPrNotHandled is returned when a pull request is not handled
+var ErrPrNotHandled = errors.New("pull request not handled")
+
 // HandleGitHubWebHook handles incoming GitHub webhooks
 // See https://docs.github.com/en/developers/webhooks-and-events/webhooks/about-webhooks
 // for more information.
@@ -132,6 +135,10 @@ func (s *Server) HandleGitHubWebHook() http.HandlerFunc {
 				log.Printf("Skipped webhook event processing: %v", err)
 				w.WriteHeader(http.StatusOK)
 				return
+			} else if errors.Is(err, ErrPrNotHandled) {
+				log.Printf("skipped PR processing")
+				w.WriteHeader(http.StatusOK)
+				return
 			}
 			log.Printf("Error parsing github webhook message: %v", err)
 			w.WriteHeader(http.StatusInternalServerError)
@@ -322,10 +329,8 @@ func (s *Server) parseGithubEventForProcessing(
 				ctx, payload, msg, dbRepo, provBuilder)
 		}
 	} else if hook_type == "pull_request" {
-		if payload["action"] == "opened" || payload["action"] == "synchronize" {
-			return parsePullRequestModEvent(
-				ctx, payload, msg, dbRepo, provBuilder)
-		}
+		return parsePullRequestModEvent(
+			ctx, payload, msg, dbRepo, s.store, provBuilder)
 	}
 
 	return parseRepoEvent(msg, dbRepo, provBuilder.GetName())
@@ -412,6 +417,7 @@ func parsePullRequestModEvent(
 	whPayload map[string]any,
 	msg *message.Message,
 	dbrepo db.Repository,
+	store db.Store,
 	prov *providers.ProviderBuilder,
 ) error {
 	// NOTE(jaosorior): this webhook is very specific to github
@@ -431,6 +437,11 @@ func parsePullRequestModEvent(
 		return fmt.Errorf("error getting pull request information from payload: %w", err)
 	}
 
+	dbPr, err := reconcilePrWithDb(ctx, store, dbrepo, prEvalInfo)
+	if err != nil {
+		return fmt.Errorf("error reconciling PR with DB: %w", err)
+	}
+
 	err = updatePullRequestInfoFromProvider(ctx, cli, dbrepo, prEvalInfo)
 	if err != nil {
 		return fmt.Errorf("error updating pull request information from provider: %w", err)
@@ -440,7 +451,7 @@ func parsePullRequestModEvent(
 
 	eiw := engine.NewEntityInfoWrapper().
 		WithPullRequest(prEvalInfo).
-		WithPullRequestNumber(prEvalInfo.Number).
+		WithPullRequestID(dbPr.ID).
 		WithProvider(prov.GetName()).
 		WithProjectID(dbrepo.ProjectID).
 		WithRepositoryID(dbrepo.ID)
@@ -818,13 +829,60 @@ func getPullRequestInfoFromPayload(
 		return nil, fmt.Errorf("error getting pull request author ID from payload: %w", err)
 	}
 
+	action, err := util.JQReadFrom[string](ctx, ".action", payload)
+	if err != nil {
+		return nil, fmt.Errorf("error getting action from payload: %w", err)
+	}
+
 	return &pb.PullRequest{
 		Url:      prUrl,
 		Number:   int32(prNumber),
 		AuthorId: int64(prAuthorId),
+		Action:   action,
 	}, nil
 }
 
+func reconcilePrWithDb(
+	ctx context.Context,
+	store db.Store,
+	dbrepo db.Repository,
+	prEvalInfo *pb.PullRequest,
+) (*db.PullRequest, error) {
+	var retErr error
+	var retPr *db.PullRequest
+
+	switch prEvalInfo.Action {
+	case "opened", "synchronize":
+		dbPr, err := store.UpsertPullRequest(ctx, db.UpsertPullRequestParams{
+			RepositoryID: dbrepo.ID,
+			PrNumber:     int64(prEvalInfo.Number),
+		})
+		if err != nil {
+			return nil, fmt.Errorf(
+				"cannot upsert PR %d in repo %s/%s",
+				prEvalInfo.Number, dbrepo.RepoOwner, dbrepo.RepoName)
+		}
+		retPr = &dbPr
+		retErr = nil
+	case "closed":
+		err := store.DeletePullRequest(ctx, db.DeletePullRequestParams{
+			RepositoryID: dbrepo.ID,
+			PrNumber:     int64(prEvalInfo.Number),
+		})
+		if err != nil && !errors.Is(err, sql.ErrNoRows) {
+			return nil, fmt.Errorf("cannot delete PR record %d in repo %s/%s",
+				prEvalInfo.Number, dbrepo.RepoOwner, dbrepo.RepoName)
+		}
+		retPr = nil
+		retErr = ErrPrNotHandled
+	default:
+		log.Printf("action %s is not handled for pull requests", prEvalInfo.Action)
+		retPr = nil
+		retErr = ErrPrNotHandled
+	}
+
+	return retPr, retErr
+}
 func updatePullRequestInfoFromProvider(
 	ctx context.Context,
 	cli provifv1.GitHub,