From 4859aa605072f08e38b7ef3028587f323ef7245b Mon Sep 17 00:00:00 2001 From: Bhanu Chander Vallabaneni Date: Sat, 1 Aug 2026 11:14:04 -0400 Subject: [PATCH] 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 --- backend/plugins/jira/tasks/issue_collector.go | 78 +++++++----- .../jira/tasks/issue_collector_test.go | 118 ++++++++++++++++++ 2 files changed, 168 insertions(+), 28 deletions(-) diff --git a/backend/plugins/jira/tasks/issue_collector.go b/backend/plugins/jira/tasks/issue_collector.go index fe1939b721b..997a7368598 100644 --- a/backend/plugins/jira/tasks/issue_collector.go +++ b/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) }, }) } diff --git a/backend/plugins/jira/tasks/issue_collector_test.go b/backend/plugins/jira/tasks/issue_collector_test.go index 50c66947c59..098196f9354 100644 --- a/backend/plugins/jira/tasks/issue_collector_test.go +++ b/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 = `502 Bad Gateway` + // 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{