feat(jira): add JIRA_SKIP_UNPARSEABLE_ISSUES to survive unparseable issue pages (#9026)

* feat(jira): add JIRA_SKIP_UNPARSEABLE_ISSUES to survive unparseable issue pages

A page that comes back with a successful status but a body that is not the
expected JSON - a truncated payload, or an HTML error page substituted by a
proxy - fails the whole collectIssues subtask. On a large Jira instance that
discards an otherwise complete sync because of a single bad page, typically
during deep pagination with changelog expansion.

Add JIRA_SKIP_UNPARSEABLE_ISSUES, read through the same
taskCtx.GetConfigReader().GetBool() pattern that JIRA_JQL_AUTO_FULL_REFRESH
already uses in this plugin. It defaults to false, preserving the current
behaviour of surfacing the error and failing the subtask. When enabled, the
offending page is logged with its URL and body size and skipped, and collection
continues.

Only parse failures on otherwise-successful responses are affected. Non-2xx
statuses never reach the response parser and keep their existing retry
behaviour.

The V2 (api/2/search) and V3 (api/3/search/jql) response parsers were byte
identical, so both now share a single parseIssuesResponse helper rather than
carrying two copies of the change. A skipped page yields an empty non-nil
slice, so the collector records a page with no rows instead of treating the
result as absent.

Signed-off-by: Bhanu Chander Vallabaneni <v.bhanuchander@gmail.com>

* docs(jira): document JIRA_SKIP_UNPARSEABLE_ISSUES in env.example

Requested in review on #9026.

Signed-off-by: Bhanu Chander Vallabaneni <v.bhanuchander@gmail.com>

---------

Signed-off-by: Bhanu Chander Vallabaneni <v.bhanuchander@gmail.com>

Author: vbhanuchander-lang

backend/plugins/jira/tasks/issue_collector.go

@@ -30,6 +30,7 @@ import (
 
 	"github.com/apache/incubator-devlake/core/dal"
 	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/log"
 	"github.com/apache/incubator-devlake/core/plugin"
 	"github.com/apache/incubator-devlake/helpers/pluginhelper/api"
 	"github.com/apache/incubator-devlake/plugins/jira/models"
@@ -95,12 +96,17 @@ func CollectIssues(taskCtx plugin.SubTaskContext) errors.Error {
 		pageSize = 100
 	}
 
+	skipUnparseable := taskCtx.GetConfigReader().GetBool("JIRA_SKIP_UNPARSEABLE_ISSUES")
+	if skipUnparseable {
+		logger.Info("JIRA_SKIP_UNPARSEABLE_ISSUES is enabled, unparseable issue pages will be skipped")
+	}
+
 	if strings.EqualFold(string(data.JiraServerInfo.DeploymentType), string(models.DeploymentServer)) {
 		logger.Info("Using api/2/search for JIRA Server issue collection")
-		err = setupIssueV2Collector(apiCollector, data, filterJql, pageSize)
+		err = setupIssueV2Collector(apiCollector, data, filterJql, pageSize, skipUnparseable, logger)
 	} else {
 		logger.Info("Using api/3/search/jql for JIRA Cloud issue collection")
-		err = setupIssueV3Collector(apiCollector, data, filterJql, pageSize)
+		err = setupIssueV3Collector(apiCollector, data, filterJql, pageSize, skipUnparseable, logger)
 	}
 	if err != nil {
 		return err
@@ -176,7 +182,45 @@ func buildFilterJQL(filterId string, extraJql string, incrementalJql string) str
 	return strings.Join(conditions, " AND ") + " " + orderBy
 }
 
-func setupIssueV2Collector(apiCollector *api.StatefulApiCollector, data *JiraTaskData, filterJql string, pageSize int) errors.Error {
+// parseIssuesResponse extracts the `issues` array from a Jira search response.
+//
+// A response that arrived with a successful status but carries a body which is not the
+// expected JSON - a truncated payload, or an HTML error page substituted by a proxy -
+// normally fails the whole collectIssues subtask, discarding an otherwise complete sync
+// because of a single page. When skipUnparseable is set the page is logged and skipped
+// instead. Non-2xx responses never reach this point; they are handled by the retry logic
+// in the API client.
+func parseIssuesResponse(res *http.Response, skipUnparseable bool, logger log.Logger) ([]json.RawMessage, errors.Error) {
+	blob, err := io.ReadAll(res.Body)
+	if err != nil {
+		return nil, errors.Convert(err)
+	}
+	var body struct {
+		Issues []json.RawMessage `json:"issues"`
+	}
+	if err := json.Unmarshal(blob, &body); err != nil {
+		if !skipUnparseable {
+			return nil, errors.Convert(err)
+		}
+		if logger != nil {
+			logger.Warn(err, "skipping unparseable issue page from %s (%d bytes)", responseUrl(res), len(blob))
+		}
+		return []json.RawMessage{}, nil
+	}
+	return body.Issues, nil
+}
+
+// responseUrl reports the request URL behind a response, for log messages. The request is
+// always populated on responses returned by the API client, but a hand-built response in a
+// test may not carry one.
+func responseUrl(res *http.Response) string {
+	if res == nil || res.Request == nil || res.Request.URL == nil {
+		return "unknown url"
+	}
+	return res.Request.URL.String()
+}
+
+func setupIssueV2Collector(apiCollector *api.StatefulApiCollector, data *JiraTaskData, filterJql string, pageSize int, skipUnparseable bool, logger log.Logger) errors.Error {
 	return apiCollector.InitCollector(api.ApiCollectorArgs{
 		ApiClient:   data.ApiClient,
 		PageSize:    pageSize,
@@ -192,23 +236,12 @@ func setupIssueV2Collector(apiCollector *api.StatefulApiCollector, data *JiraTas
 		GetTotalPages: GetTotalPagesFromResponse,
 		Concurrency:   10,
 		ResponseParser: func(res *http.Response) ([]json.RawMessage, errors.Error) {
-			var data struct {
-				Issues []json.RawMessage `json:"issues"`
-			}
-			blob, err := io.ReadAll(res.Body)
-			if err != nil {
-				return nil, errors.Convert(err)
-			}
-			err = json.Unmarshal(blob, &data)
-			if err != nil {
-				return nil, errors.Convert(err)
-			}
-			return data.Issues, nil
+			return parseIssuesResponse(res, skipUnparseable, logger)
 		},
 	})
 }
 
-func setupIssueV3Collector(apiCollector *api.StatefulApiCollector, data *JiraTaskData, filterJql string, pageSize int) errors.Error {
+func setupIssueV3Collector(apiCollector *api.StatefulApiCollector, data *JiraTaskData, filterJql string, pageSize int, skipUnparseable bool, logger log.Logger) errors.Error {
 	return apiCollector.InitCollector(api.ApiCollectorArgs{
 		ApiClient:             data.ApiClient,
 		PageSize:              pageSize,
@@ -226,18 +259,7 @@ func setupIssueV3Collector(apiCollector *api.StatefulApiCollector, data *JiraTas
 			return query, nil
 		},
 		ResponseParser: func(res *http.Response) ([]json.RawMessage, errors.Error) {
-			var data struct {
-				Issues []json.RawMessage `json:"issues"`
-			}
-			blob, err := io.ReadAll(res.Body)
-			if err != nil {
-				return nil, errors.Convert(err)
-			}
-			err = json.Unmarshal(blob, &data)
-			if err != nil {
-				return nil, errors.Convert(err)
-			}
-			return data.Issues, nil
+			return parseIssuesResponse(res, skipUnparseable, logger)
 		},
 	})
 }

backend/plugins/jira/tasks/issue_collector_test.go

@@ -18,10 +18,17 @@ limitations under the License.
 package tasks
 
 import (
+	"io"
+	"net/http"
+	"net/url"
+	"strings"
 	"testing"
 	"time"
 
+	"github.com/apache/incubator-devlake/helpers/unithelper"
+	mocklog "github.com/apache/incubator-devlake/mocks/core/log"
 	"github.com/apache/incubator-devlake/plugins/jira/models"
+	"github.com/stretchr/testify/mock"
 )
 
 func Test_buildJQL(t *testing.T) {
@@ -142,6 +149,117 @@ func Test_buildFilterJQL(t *testing.T) {
 	}
 }
 
+func Test_parseIssuesResponse(t *testing.T) {
+	const twoIssues = `{"issues":[{"id":"1"},{"id":"2"}]}`
+	// A proxy returning an HTML error page under a 200, the case reported in #8949.
+	const htmlPage = `<html><head><title>502 Bad Gateway</title></head></html>`
+	// A response truncated mid-flight, so the JSON never terminates.
+	const truncated = `{"issues":[{"id":"1"},{"id":`
+
+	makeResponse := func(body string) *http.Response {
+		return &http.Response{
+			Body:    io.NopCloser(strings.NewReader(body)),
+			Request: &http.Request{URL: &url.URL{Scheme: "https", Host: "jira.example.com", Path: "/rest/api/2/search"}},
+		}
+	}
+
+	tests := []struct {
+		name            string
+		body            string
+		skipUnparseable bool
+		wantCount       int
+		wantErr         bool
+		// wantSkipped marks the cases that took the skip path, which must yield an
+		// empty non-nil slice so the collector records "this page had no rows" rather
+		// than treating the result as absent.
+		wantSkipped bool
+	}{
+		{
+			name:      "valid page is parsed",
+			body:      twoIssues,
+			wantCount: 2,
+		},
+		{
+			name:            "valid page is parsed when skipping is enabled",
+			body:            twoIssues,
+			skipUnparseable: true,
+			wantCount:       2,
+		},
+		{
+			name:    "html body fails by default",
+			body:    htmlPage,
+			wantErr: true,
+		},
+		{
+			name:            "html body is skipped when enabled",
+			body:            htmlPage,
+			skipUnparseable: true,
+			wantCount:       0,
+			wantSkipped:     true,
+		},
+		{
+			name:    "truncated body fails by default",
+			body:    truncated,
+			wantErr: true,
+		},
+		{
+			name:            "truncated body is skipped when enabled",
+			body:            truncated,
+			skipUnparseable: true,
+			wantCount:       0,
+			wantSkipped:     true,
+		},
+		{
+			name:      "valid page without an issues key yields no issues",
+			body:      `{"total":0}`,
+			wantCount: 0,
+		},
+	}
+
+	// unithelper.DummyLogger stubs Warn with two arguments, but the real signature is
+	// Warn(err, format, a ...interface{}), which mockery records as three. Add the
+	// variadic form so the skip path can log.
+	newLogger := func() *mocklog.Logger {
+		logger := unithelper.DummyLogger()
+		logger.On("Warn", mock.Anything, mock.Anything, mock.Anything).Maybe()
+		return logger
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := parseIssuesResponse(makeResponse(tt.body), tt.skipUnparseable, newLogger())
+			if (err != nil) != tt.wantErr {
+				t.Errorf("parseIssuesResponse() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if tt.wantErr {
+				return
+			}
+			if len(got) != tt.wantCount {
+				t.Errorf("parseIssuesResponse() returned %d issues, want %d", len(got), tt.wantCount)
+			}
+			if tt.wantSkipped && got == nil {
+				t.Error("parseIssuesResponse() returned nil for a skipped page, want an empty slice")
+			}
+		})
+	}
+}
+
+func Test_responseUrl(t *testing.T) {
+	withUrl := &http.Response{
+		Request: &http.Request{URL: &url.URL{Scheme: "https", Host: "jira.example.com", Path: "/rest/api/2/search"}},
+	}
+	if got, want := responseUrl(withUrl), "https://jira.example.com/rest/api/2/search"; got != want {
+		t.Errorf("responseUrl() = %v, want %v", got, want)
+	}
+	if got, want := responseUrl(&http.Response{}), "unknown url"; got != want {
+		t.Errorf("responseUrl() with no request = %v, want %v", got, want)
+	}
+	if got, want := responseUrl(nil), "unknown url"; got != want {
+		t.Errorf("responseUrl(nil) = %v, want %v", got, want)
+	}
+}
+
 func Test_renderExtraJQL(t *testing.T) {
 	makeData := func(boardId uint64, boardName string, _ string) *JiraTaskData {
 		return &JiraTaskData{

env.example

@@ -87,6 +87,16 @@ USE_GO_GIT_IN_GIT_EXTRACTOR=false
 SKIP_COMMIT_STAT=false
 SKIP_COMMIT_FILES=true
 
+# In plugin jira, continue collecting when a page of issues cannot be parsed.
+# A page that returns a successful HTTP status but a body that is not the
+# expected JSON - a truncated payload, or an HTML error page substituted by a
+# proxy - normally fails the whole collectIssues subtask, discarding an
+# otherwise complete sync because of a single page. Set to true to log and skip
+# that page instead. Only parse failures on successful responses are affected;
+# HTTP error statuses keep their existing retry behaviour.
+##########################
+JIRA_SKIP_UNPARSEABLE_ISSUES=false
+
 # Set if response error when requesting /connections/{connection_id}/test should be wrapped or not
 ##########################
 WRAP_RESPONSE_ERROR=