refactor: simplify NodeValue API to 2 methods, fix AttrParser JID bug - #386
Conversation
…dling NodeValue public API reduced from 4 methods to 2: - as_str() -> Cow<'_, str> (was Option<&str>) — works for both String and Jid variants; zero-copy for String, formats Jid on demand - to_jid() -> Option<Jid> — unchanged, already works for both variants Removed methods: - as_jid() — replaced by to_jid() which handles both variants - to_string_value() — replaced by as_str().into_owned() or Display AttrParser fixes: - optional_string() now returns Option<Cow<str>> instead of Option<&str>, fixing a latent bug where JID-typed attributes silently returned None - required_string() follows the same Cow<str> return type - Removed deprecated string() method All callers migrated to use PartialEq<str> for comparisons (zero-copy) and Cow<str> for string extraction.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRefactors how string attributes and node values are represented and accessed: optional/required string APIs now use Cow<> and NodeValue::as_str returns Cow; call sites updated to use as_deref(), into_owned(), is_some_and(), and related patterns across handlers, wacore, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Bencher Report![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@wacore/src/iq/groups.rs`:
- Around line 344-348: The current participant_type assignment in groups.rs
collapses missing and invalid "type" values into ParticipantType::Member; change
it to first check node.attrs().optional_string("type") and only default to
Member when that returns None, but when it returns Some(s) attempt
ParticipantType::try_from(s.as_ref()) and surface/return the parse error if
try_from fails (do not unwrap_or), updating the surrounding function's error
handling to propagate that parse error; look for the participant_type variable
assignment and calls to node.attrs().optional_string("type") and
ParticipantType::try_from to implement this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39438e7e-fff8-4f97-8a36-f038339743ea
📒 Files selected for processing (43)
src/client.rssrc/features/presence.rssrc/handlers/iq.rssrc/handlers/notification.rssrc/message.rssrc/pair.rssrc/receipt.rssrc/request.rssrc/retry.rssrc/spam_report.rssrc/types/enc_handler.rssrc/unified_session.rstests/e2e/tests/groups.rstests/e2e/tests/memory_soak.rstests/e2e/tests/offline_groups.rstests/e2e/tests/receipts.rswacore/appstate/src/patch_decode.rswacore/binary/src/attrs.rswacore/binary/src/node.rswacore/derive/src/lib.rswacore/src/ib.rswacore/src/iq/blocklist.rswacore/src/iq/chatstate.rswacore/src/iq/contacts.rswacore/src/iq/dirty.rswacore/src/iq/groups.rswacore/src/iq/mediaconn.rswacore/src/iq/mex.rswacore/src/iq/node.rswacore/src/iq/prekeys.rswacore/src/iq/privacy.rswacore/src/iq/props.rswacore/src/iq/spam_report.rswacore/src/iq/tctoken.rswacore/src/iq/usync.rswacore/src/pair.rswacore/src/reporting_token.rswacore/src/request.rswacore/src/stanza/business.rswacore/src/stanza/devices.rswacore/src/stanza/groups.rswacore/src/types/spam_report.rswacore/tests/binary_protocol_test.rs
| let participant_type = node | ||
| .attrs() | ||
| .optional_string("type") | ||
| .and_then(|s| ParticipantType::try_from(s.as_ref()).ok()) | ||
| .unwrap_or(ParticipantType::Member); |
There was a problem hiding this comment.
Handle invalid participant types separately from missing values.
This currently maps both missing and invalid type values to Member, which can silently misclassify server responses and hide protocol changes. Keep the default only for missing values, and surface invalid values as parse errors.
🔧 Proposed fix
- let participant_type = node
- .attrs()
- .optional_string("type")
- .and_then(|s| ParticipantType::try_from(s.as_ref()).ok())
- .unwrap_or(ParticipantType::Member);
+ let participant_type = match node.attrs().optional_string("type").as_deref() {
+ None => ParticipantType::Member,
+ Some(raw) => ParticipantType::try_from(Some(raw))?,
+ };📝 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.
| let participant_type = node | |
| .attrs() | |
| .optional_string("type") | |
| .and_then(|s| ParticipantType::try_from(s.as_ref()).ok()) | |
| .unwrap_or(ParticipantType::Member); | |
| let participant_type = match node.attrs().optional_string("type").as_deref() { | |
| None => ParticipantType::Member, | |
| Some(raw) => ParticipantType::try_from(raw)?, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/iq/groups.rs` around lines 344 - 348, The current participant_type
assignment in groups.rs collapses missing and invalid "type" values into
ParticipantType::Member; change it to first check
node.attrs().optional_string("type") and only default to Member when that
returns None, but when it returns Some(s) attempt
ParticipantType::try_from(s.as_ref()) and surface/return the parse error if
try_from fails (do not unwrap_or), updating the surrounding function's error
handling to propagate that parse error; look for the participant_type variable
assignment and calls to node.attrs().optional_string("type") and
ParticipantType::try_from to implement this behavior.
…trs.get() + PartialEq
Replace 19 instances of the verbose pattern:
node.attrs().optional_string("key").as_deref() == Some("value")
with the cleaner zero-copy pattern:
node.attrs.get("key").is_some_and(|v| v == "value")
This bypasses AttrParser entirely for simple comparisons, using
NodeValue's PartialEq<str> directly — zero-copy for both String
and Jid variants, no error tracking overhead.
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 43s —— View job Code Review: Simplify NodeValue API
Overall: This is a clean, well-executed refactoring. The API surface reduction is sound, the JID bug fix is correct, and the comparison migrations are consistently applied. CI is green with benchmarks showing no regressions (many benchmarks actually improved). A few observations below. Core API changes — cleanThe The Derive macro changes — correct
Minor observations1. let id_opt = node.attrs.get("id").map(|v| v.to_string());This now goes through 2. CodeRabbit's let participant_type = node
.attrs()
.optional_string("type")
.and_then(|s| ParticipantType::try_from(s.as_ref()).ok())
.unwrap_or(ParticipantType::Member);CodeRabbit flagged that invalid types silently become That said, note the 3. Consistent Several sites follow this pattern for extracting owned strings from optional attrs: node.attrs().optional_string("subject").as_deref().unwrap_or_default().to_string()This is correct but creates a temporary 4. Lines 437-441 use the new SummarySolid refactoring that reduces API surface, fixes a real bug (JID attrs silently returning |
…g comparisons - Use as_str().into_owned() instead of to_string() for NodeValue→String extraction (avoids format! machinery for String variant) - Migrate skmsg type check to attrs.get() + PartialEq pattern - Simplify PresenceHandler: use to_jid() directly instead of to_string() + parse(), use is_some_and for type comparison
1 participant
Summary
NodeValuepublic API from 4 methods to 2 — eliminates variant-specific footgunsAttrParser::optional_string()where JID-typed attributes silently returnedNoneoptional_string().as_deref() == Some("x")to zero-copyattrs.get("k").is_some_and(|v| v == "x")as_str().into_owned()instead ofto_string()for NodeValue→String extraction (avoidsformat!overhead)to_jid()directly instead ofto_string()+parse()roundtripNodeValue API (final: 2 methods + 2 traits)
as_str() -> Cow<str>to_jid() -> Option<Jid>PartialEq<str>DisplayRemoved:
as_jid(),to_string_value(), deprecatedstring()Bug fixed
AttrParser::optional_string("from")returnedNonewhen the attribute was JID-typed (which happens after binary decoding). Now returnsSome(Cow::Owned(formatted_jid)).Comparison migration
20 comparison sites migrated. Remaining
.as_deref()sites are value extractions (not comparisons) or AttrParser API tests.Test plan
cargo test --all --exclude e2e-tests— 874 tests passcargo clippy --all --tests— zero warningscargo fmt --all— cleanSummary by CodeRabbit