fix(github): stop re-collecting closed and unmerged pull requests

backend/plugins/github/e2e/pr_unfinished_test.go

@@ -0,0 +1,70 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements.  See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You 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.
+*/
+
+package e2e
+
+import (
+	"sort"
+	"testing"
+
+	"github.com/apache/devlake/helpers/e2ehelper"
+	"github.com/apache/devlake/plugins/github/impl"
+	"github.com/apache/devlake/plugins/github/models"
+	"github.com/apache/devlake/plugins/github/tasks"
+	"github.com/stretchr/testify/assert"
+)
+
+// TestGithubUnfinishedPullRequests pins the set of pull requests the "Collect Pull Requests"
+// detail collector re-collects on an incremental run. The seeded table covers every
+// combination that decides it, and the repo/connection of another scope.
+//
+// Pull request 6 is the regression guard: it is closed, was never merged and its details
+// were already collected, so there is nothing left to collect for it. The previous
+// condition (`state != 'closed' OR merged = false`) kept it in the set on every run,
+// forever, which cost one request per closed and unmerged pull request per run (#9240).
+func TestGithubUnfinishedPullRequests(t *testing.T) {
+	var githubPlugin impl.Github
+	dataflowTester := e2ehelper.NewDataFlowTester(t, "github", githubPlugin)
+
+	dataflowTester.ImportCsvIntoTabler(
+		"./raw_tables/_tool_github_pull_requests_unfinished.csv",
+		&models.GithubPullRequest{},
+	)
+
+	var pullRequests []models.GithubPullRequest
+	assert.Nil(t, dataflowTester.Dal.All(
+		&pullRequests,
+		tasks.UnfinishedPullRequestsClauses(134018330, 1)...,
+	))
+
+	numbers := make([]int, len(pullRequests))
+	for i, pullRequest := range pullRequests {
+		numbers[i] = pullRequest.Number
+	}
+	sort.Ints(numbers)
+
+	assert.Equal(t, []int{
+		1, // open, details never collected
+		2, // open, details already collected
+		4, // closed and merged on GitHub, `merged` not collected yet
+		5, // closed, never merged, details never collected
+	}, numbers)
+	assert.NotContains(t, numbers, 6, "closed and unmerged pull requests with collected details must not be re-collected")
+	assert.NotContains(t, numbers, 3, "closed and merged pull requests must not be re-collected")
+	assert.NotContains(t, numbers, 7, "pull requests of another repo must not be collected")
+	assert.NotContains(t, numbers, 8, "pull requests of another connection must not be collected")
+}

backend/plugins/github/e2e/raw_tables/_tool_github_pull_requests_unfinished.csv

@@ -0,0 +1,9 @@
+connection_id,github_id,repo_id,number,state,commits,merged,merged_at,github_created_at,github_updated_at,closed_at
+1,101,134018330,1,open,0,0,,2018-07-25T08:19:30.000+00:00,2018-07-25T08:19:30.000+00:00,
+1,102,134018330,2,open,3,0,,2018-07-25T08:19:30.000+00:00,2018-07-26T08:19:30.000+00:00,
+1,103,134018330,3,closed,7,1,2018-07-26T02:32:41.000+00:00,2018-07-25T08:19:30.000+00:00,2018-07-26T02:32:41.000+00:00,2018-07-26T02:32:41.000+00:00
+1,104,134018330,4,closed,0,0,2018-07-26T02:32:41.000+00:00,2018-07-25T08:19:30.000+00:00,2018-07-26T02:32:41.000+00:00,2018-07-26T02:32:41.000+00:00
+1,105,134018330,5,closed,0,0,,2018-07-25T08:19:30.000+00:00,2018-07-26T02:32:41.000+00:00,2018-07-26T02:32:41.000+00:00
+1,106,134018330,6,closed,2,0,,2018-07-25T08:19:30.000+00:00,2018-07-26T02:32:41.000+00:00,2018-07-26T02:32:41.000+00:00
+1,107,999999999,7,open,0,0,,2018-07-25T08:19:30.000+00:00,2018-07-25T08:19:30.000+00:00,
+2,108,134018330,8,open,0,0,,2018-07-25T08:19:30.000+00:00,2018-07-25T08:19:30.000+00:00,

backend/plugins/github/tasks/pr_collector.go

@@ -105,14 +105,8 @@ func CollectApiPullRequests(taskCtx plugin.SubTaskContext) errors.Error {
 		},
 		CollectUnfinishedDetails: &helper.FinalizableApiCollectorDetailArgs{
 			BuildInputIterator: func() (helper.Iterator, errors.Error) {
-				// select pull id from database
 				cursor, err := db.Cursor(
-					dal.Select("number"),
-					dal.From(&models.GithubPullRequest{}),
-					dal.Where(
-						"repo_id = ? AND connection_id = ? AND (state != 'closed' OR merged = false)",
-						data.Options.GithubId, data.Options.ConnectionId,
-					),
+					UnfinishedPullRequestsClauses(data.Options.GithubId, data.Options.ConnectionId)...,
 				)
 				if err != nil {
 					return nil, err
@@ -140,3 +134,23 @@ func CollectApiPullRequests(taskCtx plugin.SubTaskContext) errors.Error {
 
 	return collector.Execute()
 }
+
+// UnfinishedPullRequestsClauses selects the pull requests whose details the "Collect Pull
+// Requests" subtask re-collects one by one on an incremental run: the ones that are still
+// open, plus the ones whose detail-only fields have never been collected.
+//
+// `repos/{owner}/{repo}/pulls` does not return `merged`, `commits`, `additions`,
+// `deletions`, `review_comments` or `merged_by`, so a pull request that was only ever seen
+// in a list page is stored with `commits = 0`. Every pull request has at least one commit,
+// which makes `commits = 0` the marker for "details not collected yet", and the condition
+// stops matching as soon as the details are in.
+func UnfinishedPullRequestsClauses(repoId int, connectionId uint64) []dal.Clause {
+	return []dal.Clause{
+		dal.Select("number"),
+		dal.From(&models.GithubPullRequest{}),
+		dal.Where(
+			"repo_id = ? AND connection_id = ? AND (state != 'closed' OR commits = 0)",
+			repoId, connectionId,
+		),
+	}
+}