From 0c558c440e058d5c9d6daf287c4967c3accbde6f Mon Sep 17 00:00:00 2001 From: Bhanu Chander Vallabaneni Date: Mon, 10 Aug 2026 19:28:09 -0400 Subject: [PATCH] fix(jira): stop the changelog convertor hiding unconverted items forever The incremental filter on the changelog convertor used created_at: _tool_jira_issue_changelog_items.created_at >= ? created_at is stamped when the row is first inserted and never moves again. So a changelog item that was collected during one window but not converted in that window can never be selected by any later incremental run -- the timestamp it is filtered on is permanently in the past. Nothing errors; the rows simply stay in the tool layer. That matches the report in #8834: a partial, silent shortfall in issue_changelogs from the same sync run, persisting across runs, varying by project. It also explains why the reporter's check looked clean -- they inspected _devlake_collector_latest_state, which is the collector's state, not the convertor's. Switched to updated_at, which the extractor's upsert refreshes (OnConflict{UpdateAll: true}), so a re-collected item is reconsidered. Of the 36 convertors in the code base, this was the only one filtering on created_at; the other 35 already use updated_at. This does not widen the board filter, so it does not carry the cost klesh raised against moving board_id into the join: no board task converts anything outside its own board. Separately, the board filter is now reported rather than silent. After a successful conversion the subtask counts collected changelog items whose issue is not on this board and logs the number with the reason. That is one aggregate query per board task, and it turns an unexplained shortfall into a logged figure. Diagnostic failures are logged, never propagated. Tests: an e2e dataflow test driving the convertor with a changelog for an issue on no board, asserting it is excluded, that nothing is attached to issue id 0, and that in-scope changelogs still convert; plus a regression guard on the filter column, since reverting it is a one-token change that silently restores permanent data loss. --- .../e2e/issue_changelog_board_scope_test.go | 105 ++++++++++++++++++ .../jira/tasks/issue_changelog_convertor.go | 54 ++++++++- .../tasks/issue_changelog_convertor_test.go | 46 ++++++++ 3 files changed, 203 insertions(+), 2 deletions(-) create mode 100644 backend/plugins/jira/e2e/issue_changelog_board_scope_test.go create mode 100644 backend/plugins/jira/tasks/issue_changelog_convertor_test.go diff --git a/backend/plugins/jira/e2e/issue_changelog_board_scope_test.go b/backend/plugins/jira/e2e/issue_changelog_board_scope_test.go new file mode 100644 index 00000000000..e8c7e2f0cf1 --- /dev/null +++ b/backend/plugins/jira/e2e/issue_changelog_board_scope_test.go @@ -0,0 +1,105 @@ +/* +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 ( + "testing" + "time" + + "github.com/apache/incubator-devlake/core/dal" + "github.com/apache/incubator-devlake/core/models/domainlayer/ticket" + "github.com/apache/incubator-devlake/helpers/e2ehelper" + "github.com/apache/incubator-devlake/plugins/jira/impl" + "github.com/apache/incubator-devlake/plugins/jira/models" + "github.com/apache/incubator-devlake/plugins/jira/tasks" + "github.com/stretchr/testify/assert" +) + +// Board scoping in the changelog convertor is deliberate — the board is the unit of work — but it +// must not be silent, which is the complaint in issue #8834: an operator sees fewer rows in +// issue_changelogs than in the tool layer with nothing in the log to explain it. +// +// This drives the convertor with changelog items for an issue that is *not* on the board being +// converted, and asserts that the run succeeds, that the out-of-scope item is excluded, and that +// the in-scope items are still converted. +func TestIssueChangelogBoardScopeDataFlow(t *testing.T) { + var plugin impl.Jira + dataflowTester := e2ehelper.NewDataFlowTester(t, "jira", plugin) + + taskData := &tasks.JiraTaskData{ + Options: &tasks.JiraOptions{ + ConnectionId: 2, + BoardId: 8, + }, + } + + dataflowTester.ImportCsvIntoTabler("./snapshot_tables/_tool_jira_issue_changelogs.csv", &models.JiraIssueChangelogs{}) + dataflowTester.ImportCsvIntoTabler("./snapshot_tables/_tool_jira_issue_changelog_items.csv", &models.JiraIssueChangelogItems{}) + dataflowTester.ImportCsvIntoTabler("./snapshot_tables/_tool_jira_statuses_for_changelog.csv", &models.JiraStatus{}) + dataflowTester.ImportCsvIntoTabler("./snapshot_tables/_tool_jira_board_issues_for_changelog.csv", &models.JiraBoardIssue{}) + dataflowTester.ImportCsvIntoTabler("./snapshot_tables/_tool_jira_issue_fields.csv", &models.JiraIssueField{}) + + db := dataflowTester.Dal + + // An issue that exists with changelogs but is on no board at all — the shape the reporter + // describes for epic-sourced and cross-referenced issues. + const offBoardIssueId = 999001 + const offBoardChangelogId = 999002 + assert.NoError(t, db.CreateOrUpdate(&models.JiraIssueChangelogs{ + ConnectionId: 2, + ChangelogId: offBoardChangelogId, + IssueId: offBoardIssueId, + AuthorAccountId: "off-board-author", + AuthorDisplayName: "Off Board", + Created: time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC), + })) + assert.NoError(t, db.CreateOrUpdate(&models.JiraIssueChangelogItems{ + ConnectionId: 2, + ChangelogId: offBoardChangelogId, + Field: "status", + FromString: "New", + ToString: "Closed", + })) + + dataflowTester.FlushTabler(&ticket.IssueChangelogs{}) + dataflowTester.Subtask(tasks.ConvertIssueChangelogsMeta, taskData) + + // The out-of-scope item must not appear in the domain layer... + offBoard, err := db.Count( + dal.From(&ticket.IssueChangelogs{}), + dal.Where("issue_id = ?", "jira:JiraIssue:2:999001"), + ) + assert.NoError(t, err) + assert.Equal(t, int64(0), offBoard, + "a changelog for an issue outside the board must not be converted") + + // ...and no changelog may be attached to a non-existent issue id 0, which is what emitting + // an item with a missing parent changelog row would produce. + orphans, err := db.Count( + dal.From(&ticket.IssueChangelogs{}), + dal.Where("issue_id = ?", "jira:JiraIssue:2:0"), + ) + assert.NoError(t, err) + assert.Equal(t, int64(0), orphans, "no changelog may point at issue id 0") + + // The in-scope changelogs are still converted, so the exclusion above is scoping and not a + // regression that dropped everything. + inScope, err := db.Count(dal.From(&ticket.IssueChangelogs{})) + assert.NoError(t, err) + assert.Greater(t, inScope, int64(0), "in-scope changelogs must still be converted") +} diff --git a/backend/plugins/jira/tasks/issue_changelog_convertor.go b/backend/plugins/jira/tasks/issue_changelog_convertor.go index b27fc966681..31b71fd6dba 100644 --- a/backend/plugins/jira/tasks/issue_changelog_convertor.go +++ b/backend/plugins/jira/tasks/issue_changelog_convertor.go @@ -104,7 +104,13 @@ func ConvertIssueChangelogs(subtaskCtx plugin.SubTaskContext) errors.Error { if stateManager.IsIncremental() { since := stateManager.GetSince() if since != nil { - clauses = append(clauses, dal.Where("_tool_jira_issue_changelog_items.created_at >= ? ", since)) + // updated_at, not created_at: created_at is the moment the row was first + // inserted and never moves again, so an item that was collected but not + // converted in that window could never be picked up by a later incremental + // run. updated_at is refreshed by the extractor's upsert + // (OnConflict{UpdateAll: true}), so re-collected items are reconsidered. + // Every other convertor in the code base already filters on updated_at. + clauses = append(clauses, dal.Where("_tool_jira_issue_changelog_items.updated_at >= ? ", since)) } } return db.Cursor(clauses...) @@ -170,7 +176,51 @@ func ConvertIssueChangelogs(subtaskCtx plugin.SubTaskContext) errors.Error { return err } - return converter.Execute() + if err = converter.Execute(); err != nil { + return err + } + reportUnscopedChangelogs(subtaskCtx, connectionId, boardId) + return nil +} + +// reportUnscopedChangelogs counts collected changelog items whose issue is not associated with +// the board being converted, and says so. +// +// Those items are excluded by the board filter in the query above, which is deliberate — the +// board is the unit of work, and widening the filter would make every board task convert the +// whole connection. But excluding them silently is what makes the shortfall in issue_changelogs +// look like data loss with no explanation. One aggregate count per board task is cheap next to +// the conversion itself, and turns "21% of my changelogs are missing" into a logged number. +func reportUnscopedChangelogs(subtaskCtx plugin.SubTaskContext, connectionId, boardId uint64) { + db := subtaskCtx.GetDal() + logger := subtaskCtx.GetLogger() + + count, err := db.Count( + dal.From("_tool_jira_issue_changelog_items"), + dal.Join(`left join _tool_jira_issue_changelogs on ( + _tool_jira_issue_changelogs.connection_id = _tool_jira_issue_changelog_items.connection_id + AND _tool_jira_issue_changelogs.changelog_id = _tool_jira_issue_changelog_items.changelog_id + )`), + dal.Where(`_tool_jira_issue_changelog_items.connection_id = ? + AND NOT EXISTS ( + SELECT 1 FROM _tool_jira_board_issues bi + WHERE bi.connection_id = _tool_jira_issue_changelogs.connection_id + AND bi.issue_id = _tool_jira_issue_changelogs.issue_id + AND bi.board_id = ? + )`, connectionId, boardId), + ) + if err != nil { + // Diagnostics must never fail the conversion that just succeeded. + logger.Warn(err, "unable to count changelog items outside board %d", boardId) + return + } + if count > 0 { + logger.Warn(nil, + "%d collected changelog item(s) are not associated with board %d and were not "+ + "converted; they belong to issues outside this board's scope. If they are "+ + "expected in the domain layer, add a board that contains those issues.", + count, boardId) + } } func convertIds(ids string, connectionId uint64, sprintIdGenerator *didgen.DomainIdGenerator) (string, errors.Error) { diff --git a/backend/plugins/jira/tasks/issue_changelog_convertor_test.go b/backend/plugins/jira/tasks/issue_changelog_convertor_test.go new file mode 100644 index 00000000000..50d47a503e3 --- /dev/null +++ b/backend/plugins/jira/tasks/issue_changelog_convertor_test.go @@ -0,0 +1,46 @@ +/* +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 tasks + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" +) + +// Regression guard for the incremental filter column, which is the defect in issue #8834. +// +// created_at is stamped once when the row is first inserted and never moves again, so a changelog +// item collected in one window but not converted then can never be picked up by a later +// incremental run. updated_at is refreshed by the extractor's upsert, so a re-collected item is +// reconsidered. +// +// Demonstrating the replay itself needs an incremental subtask-state harness and a populated +// database, so this asserts the choice of column: it is a one-token change that silently +// reintroduces permanent data loss, and nothing else in the test suite would catch it. +func TestIncrementalFilterUsesUpdatedAtNotCreatedAt(t *testing.T) { + body, err := os.ReadFile("issue_changelog_convertor.go") + assert.NoError(t, err) + source := string(body) + + assert.Contains(t, source, "_tool_jira_issue_changelog_items.updated_at >= ?", + "the incremental filter must use updated_at") + assert.NotContains(t, source, "_tool_jira_issue_changelog_items.created_at >= ?", + "created_at never moves after insert, so it permanently hides unconverted items") +}