fix: preserve errored items through Filter to prevent silent error loss - #66
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the Filter processor to ensure that items already carrying an error are passed through unchanged, preventing their errors from being silently discarded before reaching the engine's error-reporting step. Comprehensive unit tests have been added to verify this behavior under various scenarios, including with the InvertMatch option. The reviewer suggested adding a defensive nil check on item within the processing loop to prevent potential nil pointer dereference panics if nil items are passed into the processor.
| if item.Error != nil { | ||
| result = append(result, item) | ||
| continue | ||
| } |
There was a problem hiding this comment.
To prevent potential nil pointer dereference panics, it is highly recommended to perform a defensive nil check on item before accessing its fields or passing it to the predicate function. If an upstream processor or external caller passes a slice containing nil elements, this check ensures the application remains stable and does not panic.
| if item.Error != nil { | |
| result = append(result, item) | |
| continue | |
| } | |
| if item == nil { | |
| continue | |
| } | |
| if item.Error != nil { | |
| result = append(result, item) | |
| continue | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66 +/- ##
==========================================
+ Coverage 96.73% 96.76% +0.02%
==========================================
Files 12 12
Lines 368 371 +3
==========================================
+ Hits 356 359 +3
Misses 9 9
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Filter.Process applied the predicate to every item, including items whose Error field was already set by an earlier processor. When the predicate excluded such an item it was dropped from the output slice, so the batch engine (which only surfaces item.Error for items present at end-of-chain) silently lost the error. Items carrying an error now pass through the filter unconditionally; the predicate (and InvertMatch) is applied only to error-free items. No exported API changed. Godoc updated to document the passthrough guarantee. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9db8781 to
ad8de09
Compare
|
Declining the nil-check suggestion: nil items can only arise from a processor violating the chain contract — the engine never creates them (batch/batch.go:374) — and they'd panic in Transform/Channel/Error and the engine's final error scan regardless, so a check in Filter alone adds inconsistency without real safety. Silently |
Summary
processor.Filterran its predicate on every item, including items a prior processor had already marked with anError. If the predicate excluded such an item, it was dropped from the batch and its error never reached the engine's error channel — silent error loss when aFilterfollows an error-producing stage.Fix
Items with
item.Error != nilnow pass through the filter unconditionally; the predicate (andInvertMatch) apply only to error-free items. Godoc onFilterandProcessdocuments the guarantee.Testing
InvertMatch, errored-item passthrough (including withInvertMatch: true), and nil-predicate passthrough.go test -race ./...green ·go vet/gofmtclean.Backwards compatibility
No exported API signatures changed. Behavior changes only for already-errored items (now preserved instead of possibly dropped) — a strict bug fix.
Part of a 5-PR set from a full-repo review; file-disjoint and independently mergeable in any order.
🤖 Generated with Claude Code