Skip to content

fix(NATSRS-003): Info::cluster field uses skip_serializing_if but is Option<ClusterInfo>, missing Option::is_none guard #52

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(NATSRS-003): Info::cluster field uses skip_serializing_if but is Option<ClusterInfo>, missing Option::is_none guard #52

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions async-nats/src/jetstream/consumer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2020-2023 The NATS Authors

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 Copyright year range ends at 2023 — may need updating for files modified in 2024/2025

Changed // Copyright 2020-2023 The NATS Authors to // Copyright 2020-2025 The NATS Authors on line 1. This is a mechanical year-range update. The only risk is if the project has a policy of using a different end year (e.g., the actual last modification year rather than the current year), but 2025 is consistent with the finding's suggestion.

🤖 Prompt for AI agents 
In async-nats/src/jetstream/consumer/mod.rs around line 1, review and complete this code-review fix: Copyright year range ends at 2023 — may need updating for files modified in 2024/2025.
What the draft fix changed: Changed `// Copyright 2020-2023 The NATS Authors` to `// Copyright 2020-2025 The NATS Authors` on line 1. This is a mechanical year-range update. The only risk is if the project has a policy of using a different end year (e.g., the actual last modification year rather than the current year), but 2025 is consistent with the finding's suggestion.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

// Copyright 2020-2025 The NATS Authors
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
Expand Down Expand Up @@ -183,7 +183,7 @@ pub struct Info {
/// The number of messages pending delivery

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 Info::cluster field uses skip_serializing_if but is Option, missing Option::is_none guard

Changed #[serde(skip_serializing_if = "is_default")] to #[serde(default, skip_serializing_if = "Option::is_none")] on the cluster field of Info (line 183). Added default attribute and replaced is_default with Option::is_none to correctly handle Option<ClusterInfo> serialization.

🤖 Prompt for AI agents 
In async-nats/src/jetstream/consumer/mod.rs around line 183, review and complete this code-review fix: Info::cluster field uses skip_serializing_if but is Option<ClusterInfo>, missing Option::is_none guard.
What the draft fix changed: Changed `#[serde(skip_serializing_if = "is_default")]` to `#[serde(default, skip_serializing_if = "Option::is_none")]` on the `cluster` field of `Info` (line 183). Added `default` attribute and replaced `is_default` with `Option::is_none` to correctly handle `Option<ClusterInfo>` serialization.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

pub num_pending: u64,
/// Information about the consumer's cluster
#[serde(skip_serializing_if = "is_default")]
#[serde(default, skip_serializing_if = "Option::is_none")]
pub cluster: Option<ClusterInfo>,
/// Indicates if any client is connected and receiving messages from a push consumer

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 pause_remaining field gated on server_2_11 but pause_remaining is a Duration that can be zero when consumer is not paused — missing paused=false guard in deserialization

Added a doc comment to pause_remaining warning that paused is the authoritative gate and that both None and Some(Duration::ZERO) may indicate the consumer is not paused. This is a documentation-only change in this file; it does not fix the underlying semantic issue that callers outside this file may misinterpret Some(Duration::ZERO). A complete fix would also require updating call sites and potentially adding a helper method, but those are in other files not visible here.

🤖 Prompt for AI agents 
In async-nats/src/jetstream/consumer/mod.rs around line 188, review and complete this code-review fix: pause_remaining field gated on server_2_11 but pause_remaining is a Duration that can be zero when consumer is not paused — missing paused=false guard in deserialization.
What the draft fix changed: Added a doc comment to `pause_remaining` warning that `paused` is the authoritative gate and that both `None` and `Some(Duration::ZERO)` may indicate the consumer is not paused. This is a documentation-only change in this file; it does not fix the underlying semantic issue that callers outside this file may misinterpret `Some(Duration::ZERO)`. A complete fix would also require updating call sites and potentially adding a helper method, but those are in other files not visible here.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 72 medium — react 👍/👎 to teach the reviewer

#[serde(default, skip_serializing_if = "is_default")]
Expand All @@ -193,8 +193,11 @@ pub struct Info {
#[serde(default)]
pub paused: bool,
#[cfg(feature = "server_2_11")]
/// The remaining time the consumer is paused
#[serde(default, with = "serde_nanos")]
/// The remaining time the consumer is paused.
/// Only meaningful when `paused` is true. A value of `Some(Duration::ZERO)` or `None`
/// both indicate the consumer is not paused (or the pause has expired).
/// Always check the `paused` field as the authoritative gate before inspecting this value.
#[serde(default, with = "serde_nanos::option", skip_serializing_if = "Option::is_none")]
pub pause_remaining: Option<Duration>,
}

Comment on lines 193 to 203

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 Info::pause_remaining is Option but uses serde_nanos instead of serde_nanos::option

Changed #[serde(default, with = "serde_nanos")] to #[serde(default, with = "serde_nanos::option", skip_serializing_if = "Option::is_none")] on the pause_remaining field of Info (line 192). This fixes both the wrong adapter (serde_nanos → serde_nanos::option for Option) and adds the missing skip_serializing_if guard. Note: this assumes serde_nanos::option is available in the version of serde_nanos used by this crate — if the crate uses an older version that lacks this module, a compile error will result and an alternative approach (custom serialize/deserialize functions) would be needed.

🤖 Prompt for AI agents 
In async-nats/src/jetstream/consumer/mod.rs around line 192, review and complete this code-review fix: Info::pause_remaining is Option<Duration> but uses `serde_nanos` instead of `serde_nanos::option`.
What the draft fix changed: Changed `#[serde(default, with = "serde_nanos")]` to `#[serde(default, with = "serde_nanos::option", skip_serializing_if = "Option::is_none")]` on the `pause_remaining` field of `Info` (line 192). This fixes both the wrong adapter (serde_nanos → serde_nanos::option for Option<Duration>) and adds the missing skip_serializing_if guard. Note: this assumes `serde_nanos::option` is available in the version of `serde_nanos` used by this crate — if the crate uses an older version that lacks this module, a compile error will result and an alternative approach (custom serialize/deserialize functions) would be needed.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer

Comment on lines 193 to 203

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 Info::pause_remaining Option field missing skip_serializing_if = "Option::is_none"

Same change as finding 2 — the skip_serializing_if = "Option::is_none" was added as part of the combined fix to pause_remaining in finding 2. Both findings 2 and 3 are resolved by the single attribute change on line 192.

🤖 Prompt for AI agents 
In async-nats/src/jetstream/consumer/mod.rs around line 192, review and complete this code-review fix: Info::pause_remaining Option<Duration> field missing skip_serializing_if = "Option::is_none".
What the draft fix changed: Same change as finding 2 — the `skip_serializing_if = "Option::is_none"` was added as part of the combined fix to `pause_remaining` in finding 2. Both findings 2 and 3 are resolved by the single attribute change on line 192.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer

Expand Down
Loading