remove protobuf dependency with prost-codec feature#579
Conversation
ggirol-rc
force-pushed
the
pure_prost
branch
from
f6fb460 to
acccd2b
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a protocompat compatibility module unifying prost/protobuf message APIs behind shared types/traits, makes protobuf an optional feature, updates proto build configuration, removes direct protobuf dependency from the root, and changes imports across the crate and tests to use the new protocompat surface. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant App as Client / Test
participant Raft as raft crate
participant ProtoCompat as protocompat
participant PbCrate as pbcrate (prost / protobuf)
App->>Raft: request serialize / build message
Raft->>ProtoCompat: call PbMessageExt::write_to_bytes()
ProtoCompat->>PbCrate: delegate to prost::Message / protobuf::Message
PbCrate-->>ProtoCompat: bytes or PbError
ProtoCompat-->>Raft: bytes or PbError
Raft-->>App: return bytes or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
proto/src/lib.rs (3)
58-65: Protobuf-codecPbMessageExtrelies on method name overlap — this is clever but worth a doc comment.In
protobuf-codecmode,PbMessageExtis empty becauseprotobuf::Messagealready providescompute_size,merge_from_bytes, andwrite_to_bytesnatively. Call sites work uniformly because the same method names resolve through either the trait (prost) or inherent methods (protobuf). This is subtle — a brief comment here explaining the design would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/src/lib.rs` around lines 58 - 65, Add a brief doc comment above the protobuf-codec module explaining that protoimpl::PbMessageExt is intentionally empty and relies on the inherent methods of protobuf::Message (compute_size, merge_from_bytes, write_to_bytes) so call sites can use the same method names as the prost-based PbMessageExt; update the comment near mod protoimpl, the trait PbMessageExt, and its impl<T: PbMessage> super::protocompat::PbMessageExt for T {} to clarify this subtle name-resolution design for future maintainers.
36-65: Simultaneous activation ofprost-codecandprotobuf-codecwill produce a confusing "duplicate module" error.Both feature gates define
mod protoimpl. If a user accidentally enables both features, the compiler error will be cryptic. A compile-time check at the crate root would give a much better developer experience.Add a compile_error for mutual exclusion
Add this near the top of the file (e.g., after the
#![allow(...)]lines):#[cfg(all(feature = "prost-codec", feature = "protobuf-codec"))] compile_error!("Features `prost-codec` and `protobuf-codec` are mutually exclusive. Please enable only one."); #[cfg(not(any(feature = "prost-codec", feature = "protobuf-codec")))] compile_error!("Either `prost-codec` or `protobuf-codec` feature must be enabled.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/src/lib.rs` around lines 36 - 65, Add a compile-time feature guard to avoid the confusing duplicate module error by detecting invalid feature combinations and missing features: insert two compile_error checks near the top of the file (before the conditional mod protoimpl blocks) that 1) trigger when both "prost-codec" and "protobuf-codec" are enabled, and 2) trigger when neither is enabled; reference the feature names ("prost-codec", "protobuf-codec") and the module name protoimpl in the error messages so users get a clear, actionable message instead of the duplicate-mod compile error.
36-57: Potentialu32truncation incompute_sizeand redundant.as_ref().Line 48:
self.encoded_len() as u32silently truncates if encoded length exceedsu32::MAX. While raft messages are realistically small, this differs from theu32return thatprotobuf::Message::compute_sizeguarantees by design. Consider usingu32::try_from(...).expect(...)for a clear panic if this assumption is ever violated.Line 51:
bytes.as_ref()is a no-op on&[u8]—self.merge(bytes)suffices.Proposed cleanup
fn compute_size(&self) -> u32 { - self.encoded_len() as u32 + u32::try_from(self.encoded_len()).expect("message too large for u32") } fn merge_from_bytes(&mut self, bytes: &[u8]) -> Result<(), PbError> { - self.merge(bytes.as_ref()) + self.merge(bytes) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/src/lib.rs` around lines 36 - 57, The compute_size implementation on trait PbMessageExt silently truncates with self.encoded_len() as u32 — change it to convert explicitly (e.g., u32::try_from(self.encoded_len()).expect("encoded message too large")) so it panics clearly if the length exceeds u32::MAX; also simplify merge_from_bytes by removing the redundant bytes.as_ref() and call self.merge(bytes) directly. Refer to PbMessageExt::compute_size and PbMessageExt::merge_from_bytes (and the use of encoded_len and merge) when making the edits.proto/Cargo.toml (1)
15-27: Feature gating is correct; document the security trade-off for default users.The
protobuf-codecfeature properly activates theprotobufdependency and itsbytessub-feature. When onlyprost-codecis enabled,protobufwon't be pulled in.However, users on the default feature set still pull in
protobuf = "2", which contains RUSTSEC-2024-0437 (stack overflow DoS via uncontrolled recursion when parsing untrusted protobuf data, fixed inprotobuf >= 3.7.2). Consider adding a doc comment or CHANGELOG entry clarifying that users concerned about this advisory should switch toprost-codec, or document any mitigations in place (e.g., trusted input sources only).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/Cargo.toml` around lines 15 - 27, The default feature enables the old protobuf crate (protobuf = "2") which is affected by RUSTSEC-2024-0437; add a short doc comment (e.g., in proto/Cargo.toml or the crate-level README) and a CHANGELOG entry stating that the default feature pulls in protobuf v2 with the known advisory, advising security-conscious users to enable the prost-codec feature instead (or only enable protobuf-codec if they accept the risk), and note any intended mitigation (trusted inputs only) so users know how to opt out or mitigate the issue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@proto/Cargo.toml`:
- Around line 15-27: The default feature enables the old protobuf crate
(protobuf = "2") which is affected by RUSTSEC-2024-0437; add a short doc comment
(e.g., in proto/Cargo.toml or the crate-level README) and a CHANGELOG entry
stating that the default feature pulls in protobuf v2 with the known advisory,
advising security-conscious users to enable the prost-codec feature instead (or
only enable protobuf-codec if they accept the risk), and note any intended
mitigation (trusted inputs only) so users know how to opt out or mitigate the
issue.
In `@proto/src/lib.rs`:
- Around line 58-65: Add a brief doc comment above the protobuf-codec module
explaining that protoimpl::PbMessageExt is intentionally empty and relies on the
inherent methods of protobuf::Message (compute_size, merge_from_bytes,
write_to_bytes) so call sites can use the same method names as the prost-based
PbMessageExt; update the comment near mod protoimpl, the trait PbMessageExt, and
its impl<T: PbMessage> super::protocompat::PbMessageExt for T {} to clarify this
subtle name-resolution design for future maintainers.
- Around line 36-65: Add a compile-time feature guard to avoid the confusing
duplicate module error by detecting invalid feature combinations and missing
features: insert two compile_error checks near the top of the file (before the
conditional mod protoimpl blocks) that 1) trigger when both "prost-codec" and
"protobuf-codec" are enabled, and 2) trigger when neither is enabled; reference
the feature names ("prost-codec", "protobuf-codec") and the module name
protoimpl in the error messages so users get a clear, actionable message instead
of the duplicate-mod compile error.
- Around line 36-57: The compute_size implementation on trait PbMessageExt
silently truncates with self.encoded_len() as u32 — change it to convert
explicitly (e.g., u32::try_from(self.encoded_len()).expect("encoded message too
large")) so it panics clearly if the length exceeds u32::MAX; also simplify
merge_from_bytes by removing the redundant bytes.as_ref() and call
self.merge(bytes) directly. Refer to PbMessageExt::compute_size and
PbMessageExt::merge_from_bytes (and the use of encoded_len and merge) when
making the edits.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Cargo.tomlexamples/five_mem_node/main.rsharness/tests/integration_cases/test_raft.rsharness/tests/integration_cases/test_raw_node.rsharness/tests/test_util/mod.rsproto/Cargo.tomlproto/build.rsproto/src/lib.rssrc/errors.rssrc/lib.rssrc/raft.rssrc/raft_log.rssrc/raw_node.rssrc/storage.rssrc/util.rs
ggirol-rc
force-pushed
the
pure_prost
branch
from
acccd2b to
54c9f2f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
harness/tests/integration_cases/test_raft.rs (1)
400-400: Consider usingget_msg_type()accessor for consistency.The changes use direct field access with
.into()conversion (ms[0].msg_type, MessageType::MsgAppend.into()), while other parts of the file use theget_msg_type()accessor method (e.g., lines 801, 1386, 1416, 2728). The accessor method provides a cleaner interface that returns the enum type directly.♻️ Optional: Use accessor method for consistency
- assert_eq!(ms[0].msg_type, MessageType::MsgAppend.into()); + assert_eq!(ms[0].get_msg_type(), MessageType::MsgAppend);- if m.msg_type != MessageType::MsgAppend.into() { + if m.get_msg_type() != MessageType::MsgAppend {Also applies to: 413-413, 429-429
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harness/tests/integration_cases/test_raft.rs` at line 400, Replace direct field access of the message type (ms[0].msg_type == MessageType::MsgAppend.into()) with the accessor method ms[0].get_msg_type() so the comparison reads ms[0].get_msg_type() == MessageType::MsgAppend; do the same for the other occurrences around the same block (the checks at the similar locations) to match the file's existing usage of get_msg_type() and avoid manual .into() conversions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@harness/tests/integration_cases/test_raft.rs`:
- Line 400: Replace direct field access of the message type (ms[0].msg_type ==
MessageType::MsgAppend.into()) with the accessor method ms[0].get_msg_type() so
the comparison reads ms[0].get_msg_type() == MessageType::MsgAppend; do the same
for the other occurrences around the same block (the checks at the similar
locations) to match the file's existing usage of get_msg_type() and avoid manual
.into() conversions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Cargo.tomlexamples/five_mem_node/main.rsharness/tests/integration_cases/test_raft.rsharness/tests/integration_cases/test_raw_node.rsharness/tests/test_util/mod.rsproto/Cargo.tomlproto/build.rsproto/src/lib.rssrc/errors.rssrc/lib.rssrc/raft.rssrc/raft_log.rssrc/raw_node.rssrc/storage.rssrc/util.rs
✅ Files skipped from review due to trivial changes (1)
- src/raw_node.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- proto/Cargo.toml
- src/util.rs
- src/errors.rs
- proto/build.rs
- harness/tests/test_util/mod.rs
- examples/five_mem_node/main.rs
- src/raft_log.rs
- src/storage.rs
- proto/src/lib.rs
- Cargo.toml
|
I had removed an unused import just before pushing but apparently it was only unused with one of the backends. I re-added it and CI for windows should now pass. However the CI for ubuntu is strict for clippy and this will fail due to preexisting issues (see https://github.com/tikv/raft-rs/actions/runs/20450487487/job/59202734774?pr=573 for example). What are your expectations in this regard? |
|
You can send a different PR to fix those issues first if you would like. |
|
will you review and merge it? |
|
Yes, as long as CI is green. |
ggirol-rc
force-pushed
the
pure_prost
branch
from
54c9f2f to
05d049c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
harness/tests/integration_cases/test_raft.rs (1)
413-415: Use accessor in panic output for backend-agnostic message typing.Lines 414 and 430 check messages using
m.get_msg_type()but printm.msg_typedirectly in panic messages. Use the accessor consistently for codec-agnostic diagnostics.♻️ Proposed fix
- panic!("{}: expected MsgAppend, got {:?}", i, m.msg_type); + panic!("{}: expected MsgAppend, got {:?}", i, m.get_msg_type());Also applies to: 429-431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harness/tests/integration_cases/test_raft.rs` around lines 413 - 415, The panic messages use the field m.msg_type directly which breaks backend-agnostic diagnostics; update both occurrences (the check for MsgAppend around MessageType::MsgAppend and the similar check at lines 429-431) to print the accessor m.get_msg_type() instead of m.msg_type so the panic output uses the codec-agnostic getter (i.e., where code compares m.get_msg_type() != MessageType::MsgAppend, change the panic formatting to show m.get_msg_type()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proto/src/lib.rs`:
- Around line 48-79: Add compile-time guards to prevent both codec features from
being enabled or neither being enabled: before the first #[cfg(feature =
"prost-codec")] block, add two compile_error! checks—one using #[cfg(all(feature
= "prost-codec", feature = "protobuf-codec"))] with a message that the features
are mutually exclusive, and one using #[cfg(not(any(feature = "prost-codec",
feature = "protobuf-codec")))] with a message that one codec must be enabled;
this ensures the protoimpl module (mod protoimpl) from prost-codec and
protobuf-codec cannot both be defined or both omitted.
---
Nitpick comments:
In `@harness/tests/integration_cases/test_raft.rs`:
- Around line 413-415: The panic messages use the field m.msg_type directly
which breaks backend-agnostic diagnostics; update both occurrences (the check
for MsgAppend around MessageType::MsgAppend and the similar check at lines
429-431) to print the accessor m.get_msg_type() instead of m.msg_type so the
panic output uses the codec-agnostic getter (i.e., where code compares
m.get_msg_type() != MessageType::MsgAppend, change the panic formatting to show
m.get_msg_type()).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Cargo.tomlexamples/five_mem_node/main.rsharness/tests/integration_cases/test_raft.rsharness/tests/integration_cases/test_raw_node.rsharness/tests/test_util/mod.rsproto/Cargo.tomlproto/build.rsproto/src/lib.rssrc/errors.rssrc/lib.rssrc/raft.rssrc/raft_log.rssrc/raw_node.rssrc/storage.rssrc/util.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/errors.rs
- src/raft.rs
- src/raw_node.rs
- proto/Cargo.toml
- src/raft_log.rs
- src/util.rs
- src/lib.rs
ggirol-rc
force-pushed
the
pure_prost
branch
from
05d049c to
2b84b8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/errors.rs`:
- Around line 26-27: The #[error(...)] string for the CodecError variant is
still hardcoded to "protobuf" even though CodecError now wraps
crate::protocompat::PbError (a backend-agnostic compat error); update the
display string on the CodecError enum variant to remove or generalize "protobuf"
(e.g., "codec error {0:?}" or "codec error: {0}") so it accurately reflects the
wrapped compat error (variant name: CodecError, type:
crate::protocompat::PbError).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Cargo.tomlexamples/five_mem_node/main.rsharness/tests/integration_cases/test_raft.rsharness/tests/integration_cases/test_raw_node.rsharness/tests/test_util/mod.rsproto/Cargo.tomlproto/build.rsproto/src/lib.rssrc/errors.rssrc/lib.rssrc/raft.rssrc/raft_log.rssrc/raw_node.rssrc/storage.rssrc/util.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- src/raft_log.rs
- proto/build.rs
- src/raft.rs
- proto/Cargo.toml
- harness/tests/integration_cases/test_raw_node.rs
- proto/src/lib.rs
- src/raw_node.rs
- Cargo.toml
- src/storage.rs
| #[error("protobuf codec error {0:?}")] | ||
| CodecError(#[from] protobuf::ProtobufError), | ||
| CodecError(#[from] crate::protocompat::PbError), |
There was a problem hiding this comment.
Update error text to match backend-agnostic codec error type.
CodecError now wraps a compat error type, but the display message still hardcodes “protobuf”.
Proposed fix
- #[error("protobuf codec error {0:?}")]
+ #[error("codec error {0:?}")]
CodecError(#[from] crate::protocompat::PbError),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[error("protobuf codec error {0:?}")] | |
| CodecError(#[from] protobuf::ProtobufError), | |
| CodecError(#[from] crate::protocompat::PbError), | |
| #[error("codec error {0:?}")] | |
| CodecError(#[from] crate::protocompat::PbError), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/errors.rs` around lines 26 - 27, The #[error(...)] string for the
CodecError variant is still hardcoded to "protobuf" even though CodecError now
wraps crate::protocompat::PbError (a backend-agnostic compat error); update the
display string on the CodecError enum variant to remove or generalize "protobuf"
(e.g., "codec error {0:?}" or "codec error: {0}") so it accurately reflects the
wrapped compat error (variant name: CodecError, type:
crate::protocompat::PbError).
There was a problem hiding this comment.
the error message mentions protobuf as a serialisation format not as a crate name.
There was a problem hiding this comment.
@ggirol-rc, you're absolutely right! The error message correctly refers to the Protocol Buffers serialization format, which both prost and protobuf crates implement. Keeping "protobuf" in the message is appropriate and informative. My suggestion to change it was mistaken. Thank you for the clarification!
|
ci is green and this should be ready for review |
| fn write_to_bytes(&self) -> Result<Vec<u8>, PbError>; | ||
| } | ||
| impl<T: PbMessage> PbMessageExt for T { | ||
| fn compute_size(&self) -> u32 { |
ggirol-rc
force-pushed
the
pure_prost
branch
from
2b84b8a to
001eb91
Compare
| [features] | ||
| default = ["protobuf-codec"] | ||
| protobuf-codec = ["protobuf-build/protobuf-codec", "bytes", "protobuf/bytes"] | ||
| protobuf-codec = ["protobuf-build/protobuf-codec", "bytes", "protobuf/bytes", "protobuf"] |
There was a problem hiding this comment.
"protobuf/bytes" activates protobuf already.
The motivation is that protobuf v2 is vulnerable to RUSTSEC-2024-0437. `prost::Message` is not 100% the same as `protobuf::Message` but it's not impossible to present a common interface for the needs of this crate. This change is a breaking change. Signed-off-by: Guillaume Girol <guillaume.girol@rubycat.eu>
Signed-off-by: Guillaume Girol <guillaume.girol@rubycat.eu>
|
ci was failing because of a missing apt-get update, I took the freedom of adding it to this PR. Feel free to direct me to do it differently if preferred. |
|
Thank you! |
|
Thank you! Would you mind also reviewing #578 ? Along with this PR, it makes it possible to depend on raft-rs without upsetting cargo-audit. |
2 participants
The motivation is that protobuf v2 is vulnerable to RUSTSEC-2024-0437.
prost::Messageis not 100% the same asprotobuf::Messagebut it's not impossible to present a common interface for the needs of this crate.This change is a breaking change.
Summary by CodeRabbit
Refactor
Chores