fix(error): use status.code instead of error_code for Milvus > 2.3 compatibility#105
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: marlon-costa-dc The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@marlon-costa-dc Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco. |
|
@marlon-costa-dc Please associate the related issue to the body of your Pull Request. (eg. “issue: #187”) |
| "unknown error code {}", | ||
| status.error_code | ||
| ))), | ||
| match ErrorCode::try_from(status.code) { |
There was a problem hiding this comment.
This PR completely switches from status.error_code to status.code, but Milvus servers < 2.3 never populate status.code (protobuf default = 0 = Success). Any real error from an older server will be misclassified as Success and silently swallowed — the exact inverse of the bug this PR fixes.
The vendored proto confirms code is field tag 3 (new) vs error_code tag 1 (deprecated but present). pymilvus handles this by checking both: if status.code != 0 or status.error_code != 0.
Fix: read status.code first; when it is 0, fall back to status.error_code. Apply the same logic in error.rs From<Status>.
| impl From<Status> for Error { | ||
| fn from(s: Status) -> Self { | ||
| Error::Server(ErrorCode::from_i32(s.error_code).unwrap(), s.reason) | ||
| let code = ErrorCode::try_from(s.code).unwrap_or(ErrorCode::UnexpectedError); |
There was a problem hiding this comment.
The From<Status> impl reads only s.code, so for Milvus < 2.3 servers it will produce Error::Server(Success, …) for genuine errors. This is the same compatibility regression as in utils.rs and needs the same code-then-error_code fallback.
| impl From<Status> for Error { | ||
| fn from(s: Status) -> Self { | ||
| Error::Server(ErrorCode::from_i32(s.error_code).unwrap(), s.reason) | ||
| let code = ErrorCode::try_from(s.code).unwrap_or(ErrorCode::UnexpectedError); |
There was a problem hiding this comment.
Milvus >= 2.3 writes new-framework error codes into status.code (e.g. 100 = collection not found, 700 = index not found). These values do not match the old ErrorCode enum variants. ErrorCode::try_from(status.code) therefore maps to the wrong variant or falls through to UnexpectedError, losing the specific error type. Impact is limited because reason still carries the correct text, but any user code that pattern-matches on ErrorCode will hit the wrong arm.
| status.error_code | ||
| ))), | ||
| match ErrorCode::try_from(status.code) { | ||
| Ok(ErrorCode::Success) => Ok(()), |
There was a problem hiding this comment.
status_to_result() and From<Status> for Error are the global error gateway for every RPC call, yet there are no unit tests covering them. At minimum, tests should cover: code = 0 (success), code = known non-zero, code = unknown value (try_from failure), and the compatibility branch code = 0 && error_code != 0.
| match ErrorCode::try_from(status.code) { | ||
| Ok(ErrorCode::Success) => Ok(()), | ||
| Ok(_) => Err(Error::from(status)), | ||
| Err(_) => Err(Error::Server( |
There was a problem hiding this comment.
The Err(_) branch in status_to_result() (lines 32-35) manually constructs Error::Server(UnexpectedError, …), duplicating the same fallback already in From<Status> (error.rs lines 78-83). This creates two independent maintenance points. The Err(_) branch can simply delegate to Err(Error::from(status)).
yhmo
force-pushed
the
fix/error-handling-status-code
branch
from
53637b8 to
add40bc
Compare
yhmo
force-pushed
the
fix/error-handling-status-code
branch
from
add40bc to
12fda7f
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates Milvus Status error handling to be compatible with Milvus server versions > 2.3 by interpreting status.code (instead of the deprecated status.error_code) when mapping server responses into the SDK’s Error type.
Changes:
- Updated
status_to_resultto interpretStatus.codeviaErrorCodeconversion and preserve unknown numeric codes in the error message. - Updated
From<Status> for Errorto deriveErrorCodefromStatus.code, falling back toUnexpectedErrorfor unknown codes while retaining the original numeric code in the reason string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/utils.rs | Switches status success/error detection to use Status.code and formats unknown codes into the returned error. |
| src/error.rs | Switches Status -> Error conversion to map from Status.code and preserve unknown numeric codes in the reason text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match ErrorCode::try_from(status.code) { | ||
| Ok(ErrorCode::Success) => Ok(()), | ||
| Ok(_) => Err(Error::from(status)), | ||
| Err(_) => Err(Error::Server( | ||
| ErrorCode::UnexpectedError, | ||
| format!("{} (code {})", status.reason, status.code), | ||
| )), |
| let code = ErrorCode::try_from(s.code).unwrap_or(ErrorCode::UnexpectedError); | ||
| let reason = if code == ErrorCode::UnexpectedError && s.code != code as i32 { | ||
| format!("{} (code {})", s.reason, s.code) | ||
| } else { | ||
| s.reason | ||
| }; | ||
| Error::Server(code, reason) | ||
| } |
Summary
status.codeinstead ofstatus.error_code.error.rsandutils.rsto mapstatus.codetoErrorCodeproperly.UnexpectedErrorwith the original code and reason if the code is unknown.