fix(conversion): keep an absent UpdateResult.operation_id as None over gRPC - #1408
fix(conversion): keep an absent UpdateResult.operation_id as None over gRPC#14082sumtech wants to merge 1 commit into
Conversation
…r gRPC `UpdateResult.operation_id` is declared `optional uint64` in points.proto, so it carries explicit presence. The server leaves it unset for updates that were never assigned a sequence number - a delete-by-filter that matched no points, a clock-rejected update, or a write to a custom-sharded collection that has no shard keys yet. `GrpcToRest.convert_update_result` read the field unconditionally, so the proto default surfaced as `operation_id=0` while the same update over REST (where the key is omitted from the body) yields `operation_id=None`. Since 0 is also a valid operation id, callers could not tell the two apart. Guard the read with `HasField`, matching the presence handling already used for the neighbouring optional fields in this module.
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to gRPC update results now preserve an omitted operation ID as None while retaining explicit zero values. The conversion and round-trip coverage support the intended behavior, with no remaining merge-blocking risk. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
All Submissions:
devbranch. Did you create your branch fromdev?Changes to Core Features:
What
UpdateResult.operation_idis declared with explicit presence inpoints.proto:GrpcToRest.convert_update_resultread it unconditionally, so an absent fieldcame back as the proto default
0instead ofNone:Every neighbouring optional field in this module is already presence-guarded
(
indexed_vectors_count,params,update_queue, …); this one was the odd one out.Why it matters
The server really does leave
operation_idunset. Inqdrant,UpdateResult.operation_idis
Option<SeqNumberType>and is returned asNonefrom at least three paths:lib/collection/src/shards/replica_set/mod.rs— delete-by-filter that matched no pointslib/collection/src/shards/local_shard/shard_ops.rs—SubmitOutcome::ClockRejectedlib/storage/src/content_manager/toc/point_ops.rs— custom sharding with no shard keys yetOn the REST side the field is annotated
#[serde(skip_serializing_if = "Option::is_none")],so the key is simply missing from the body and pydantic yields
None. Over gRPC the sameupdate yielded
operation_id=0. Because0is itself a valid operation id (the firstoperation in a fresh collection), callers had no way to tell "no operation id" from
"operation 0", and
prefer_grpc=Truesilently disagreed withprefer_grpc=False.Reproduction and evidence (no server required)
repro.py— the two shapes the client receives for an update that got no sequence number:Before the fix:
After the fix:
Tests fail before, pass after. Two independent checks cover it: the new
update_result_no_operation_idfixture (which flows through the existingtest_conversion_completenessgrpc→rest→grpc harness) and a focused unit test.On the unpatched tree, with the new tests in place:
With the fix:
Surrounding server-free suites are unaffected:
(The rest of
tests/needs a live Qdrant instance, which was not available in thisenvironment; nothing outside
conversions/is touched by this change.)ruff-format --line-length=99is clean on all three touched files.How it was found
A static probe over
GrpcToRest: for every converter whose first argument is annotatedgrpc.<Message>, cross-reference the proto descriptor's explicit-presence scalar fieldsagainst
HasField(...)guards in the function body. Seven converters read anexplicit-presence scalar unguarded; five were false positives (
oneofvariants read viaWhichOneof, or fields whose REST counterpart is a requiredintthat cannot holdNone).UpdateResult.operation_idwas the one where the REST counterpart isOptional[int]andthe server has real code paths that omit the field.
Disclosure: prepared with AI assistance (Claude Code); I reviewed the change and take responsibility for it.