fix(batch): derive has_errors from errors - #2120
Conversation
BatchObjectReturn.has_errors is a stored field set by hand at each construction site. The server-side-batching receive loop never sets it, and __add__ only ORs the flags together, so accumulating partial results could never flip it: data.ingest() and batch.stream() returned a populated errors dict alongside has_errors=False. Derive the flag in __post_init__ on BatchObjectReturn and BatchReferenceReturn so an instance cannot be constructed claiming success while carrying errors. Fixes #2107
![orca-security-eu[bot]](https://avatars.githubusercontent.com/in/276244?s=60&v=4){kind=small}
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
 Passed |
Infrastructure as Code |  0  0  0  0 |
View in Orca |
| Passed |
SAST | 0 0 0 0 |
View in Orca |
| Passed |
Secrets | 0 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
There was a problem hiding this comment.
Pull request overview
Ensures batch result objects can’t report has_errors=False while carrying populated errors, fixing the inconsistent behavior observed on the server-side batching ingest path (data.ingest()), and adding regression coverage for both sync/async and streaming paths.
Changes:
- Derive
has_errorsfromerrorsinBatchObjectReturnandBatchReferenceReturnvia__post_init__. - Add unit tests validating
has_errorsderivation and accumulation behavior for both return types. - Extend mock-server tests to cover partial-failure behavior for
data.ingest()(sync/async) andbatch.stream().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| weaviate/collections/classes/batch.py | Derives has_errors from errors during dataclass initialization for object/reference batch return types. |
| test/collection/test_batch.py | Adds unit tests for has_errors behavior on construction and via __add__. |
| mock_tests/test_batch.py | Adds mock gRPC service + tests to validate partial failures set has_errors on ingest/stream (sync + async). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| err = _error_object(1) | ||
| result = BatchObjectReturn() | ||
| result += BatchObjectReturn(_all_responses=[uuid.uuid4()], uuids={0: uuid.uuid4()}) |
| def __post_init__(self) -> None: | ||
| self.has_errors = self.has_errors or len(self.errors) > 0 | ||
|
|
| def __post_init__(self) -> None: | ||
| self.has_errors = self.has_errors or len(self.errors) > 0 | ||
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2120 +/- ##
==========================================
+ Coverage 86.64% 88.41% +1.76%
==========================================
Files 300 302 +2
Lines 23172 23360 +188
==========================================
+ Hits 20077 20653 +576
+ Misses 3095 2707 -388 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Closing in favour of #2115, which was opened first by @vaibzde for the same issue. The implementation from this PR has been pushed onto that branch, so nothing is lost — see #2115 (comment). |
has_errorsis a stored field onBatchObjectReturn, set by hand at each construction site. The server-side-batching receive loop never sets it, and__add__only ORs the flags together, so accumulating partial results could never flip it:data.ingest()andbatch.stream()returned a populatederrorsdict alongsidehas_errors=False.insert_manywas never affected, as it already passes the flag correctly.__post_init__now derives the flag fromerrorson bothBatchObjectReturnandBatchReferenceReturn, so an instance cannot be constructed claiming success while carrying errors. Theorkeeps the change widening-only, so an explicitly passedTrueis preserved.Tests cover both classes plus the ingest, async ingest and stream paths against a mock server. All seven new assertions fail without the change.
Fixes #2107
@vaibzde reported the same trace on the issue and proposed a per-call-site patch. This fixes it at the shared seam instead, so a future construction site cannot reintroduce it.