vbhanuchander-lang opened a new pull request, #9172:
URL: https://github.com/apache/devlake/pull/9172
### Summary
The stateful collector's `ResponseParser` wrapper discarded the records a
plugin parser returned whenever that parser signalled `ErrFinishCollect`:
```go
items, err := args.CollectNewRecordsByList.ResponseParser(res)
if err != nil {
return nil, err // drops `items` even when err is ErrFinishCollect
}
```
Plugin parsers return the valid prefix of the page they stopped on *together
with* that signal — `circleci/tasks/pipeline_collector.go:70` is `return
filteredItems, api.ErrFinishCollect` — and this is the same contract the file's
own time filter uses a few lines below (`return validItems, ErrFinishCollect`).
`fetchAsync` persists whatever comes back and sets `handler = nil` to stop
paginating, so returning `nil` means `count == 0`, the early return fires, and
the page is never written.
The effect is a silent, page-aligned loss at the `timeAfter` boundary: the
last page of a time-bounded collection is dropped in full, including the
records on it that are inside the requested window. Nothing errors, the task
reports `TASK_COMPLETED` with zero failed subtasks, and the tool and domain
tables agree with each other because both derive from the same truncated raw
data — so it is only visible against the upstream API.
**The sibling collectors already get this right**, which is what convinced
me this wrapper is the outlier rather than the contract being ambiguous:
- `api_collector.go` — `fetchAsync` logs `"a fetch stop by parser"`, sets
`handler = nil`, then **falls through to the save**
- `graphql_collector.go` — saves its rows *before* checking for the signal
The stateful wrapper sits between the plugin parser and `fetchAsync`, and
was the only place that threw the page away.
### The change
`ResponseParser` is extracted to `newStatefulResponseParser` so the
behaviour can be tested without a collector, an API client or a database. The
move is mechanical; the only behaviour change is preserving the prefix:
```go
items, err := parseResponse(res)
parserFinished := errors.Is(err, ErrFinishCollect)
if err != nil && !parserFinished {
return nil, err
}
```
One deliberate difference from the fix suggested in the issue: rather than
`return items, err` immediately, the prefix is passed on through the time
filter below, and the stop signal reaches the final `return items, err` because
the filter's inner `err` values are block-scoped. Returning early would skip
the `createdAfter` filtering and could re-collect records the previous run
already had, so this keeps the incremental dedup guarantee for the records that
are kept.
### Does this close any open issues?
Closes #9167. Thanks to @michaelgriskified for the report — the root-cause
analysis and the in-band detector in it were accurate, and I was able to
confirm both halves against `main` directly.
### Tests
`api_collector_stateful_test.go`, six cases. **Two fail on `main`** with
`actual: []json.RawMessage(nil)` where the records are expected:
- `KeepsPartialPageOnFinish` — the prefix survives and the stop signal still
reaches `fetchAsync`
- `KeepsPartialPageWhileFiltering` — the same with the incremental filter
active
The other four pin behaviour that must not change, so they pass either way
and guard the extraction:
- `PropagatesRealErrors` — a genuine parse failure still discards the page
and is not mistaken for a stop signal
- `StopsOnEmptyFinishedPage` — an empty page plus the signal still stops
- `PassesThroughWhenNotFinished` — an ordinary page is untouched
- `TrimsRecordsBeforeCreatedAfter` — the time filter still trims a page
whose tail predates the last successful collection
```
ok github.com/apache/devlake/helpers/pluginhelper/api 6 passed
```
One note for whoever runs these locally: `backend/mocks/` is generated and
gitignored, and a clone made before the `incubator-devlake` → `devlake` rename
still has mocks importing the old module path, which fails the whole package at
setup (`no required module provides package
github.com/apache/incubator-devlake/core/config`) before any test runs. That is
unrelated to this change — it reproduces on pristine `main` — and regenerating
the mocks clears it. It may be the same root cause as #9088.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
