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{ diff --git a/env.example b/env.example index 04e0ab0a5ed..b1de659826d 100755 --- a/env.example +++ b/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=