fix(NATSRS-001): CU-86akbhh82 2 review findings in message.rs - #101
flamingo[bot] wants to merge 1 commit into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub async fn ack(&self) -> Result<(), Error> { | ||
| pub async fn ack(&self) -> Result<(), AckError> { | ||
| if let Some(ref reply) = self.reply { | ||
| self.context | ||
| .client | ||
| .publish(reply.clone(), "".into()) | ||
| .map_err(Error::from) | ||
| .map_err(|err| AckError::with_source(AckErrorKind::Other, err)) | ||
| .await | ||
| } else { | ||
| Err(Box::new(std::io::Error::other( | ||
| "No reply subject, not a JetStream message", | ||
| ))) | ||
| Err(AckError::new(AckErrorKind::MissingReplySubject)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🔴 *Message::ack/ack_with/double_ack/info return plain std::io::Error boxed as Error instead of a typed ErrorKind
Introduced AckErrorKind/AckError (pub type AckError = error::Error<AckErrorKind>) with variants MissingReplySubject, TimedOut, Dropped, Other, implementing Display. Changed Message::ack, Message::ack_with, and Message::double_ack to return Result<(), AckError> instead of Result<(), Error>, replacing the ad-hoc Box::new(std::io::Error::other(...))/Box::new(std::io::Error::new(...)) constructions with AckError::new(...)/AckError::with_source(...). Also updated the parallel Acker::ack, Acker::ack_with, Acker::double_ack methods (same struct, same pattern) for consistency, since they share the identical ad-hoc error construction. Note: this is a public API signature change (return type changed from Error alias to AckError), which is a breaking change for callers matching on the old boxed error type, though Result<(), AckError> still coerces via ? into Result<(), Error> at call sites since AckError: std::error::Error + Send + Sync + 'static. Reviewer should confirm this breaking change is acceptable per crate versioning policy.
🤖 Prompt for AI agents
In async-nats/src/jetstream/message.rs around line 191, review and complete this code-review fix: Message::ack/ack_with/double_ack/info return plain std::io::Error boxed as Error instead of a typed *ErrorKind.
What the draft fix changed: Introduced `AckErrorKind`/`AckError` (`pub type AckError = error::Error<AckErrorKind>`) with variants `MissingReplySubject`, `TimedOut`, `Dropped`, `Other`, implementing `Display`. Changed `Message::ack`, `Message::ack_with`, and `Message::double_ack` to return `Result<(), AckError>` instead of `Result<(), Error>`, replacing the ad-hoc `Box::new(std::io::Error::other(...))`/`Box::new(std::io::Error::new(...))` constructions with `AckError::new(...)`/`AckError::with_source(...)`. Also updated the parallel `Acker::ack`, `Acker::ack_with`, `Acker::double_ack` methods (same struct, same pattern) for consistency, since they share the identical ad-hoc error construction. Note: this is a public API signature change (return type changed from `Error` alias to `AckError`), which is a breaking change for callers matching on the old boxed error type, though `Result<(), AckError>` still coerces via `?` into `Result<(), Error>` at call sites since `AckError: std::error::Error + Send + Sync + 'static`. Reviewer should confirm this breaking change is acceptable per crate versioning policy.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 72 medium — react 👍/👎 to teach the reviewer
| std::io::ErrorKind::TimedOut, | ||
| "double ack response timed out", | ||
| ) | ||
| })? { |
There was a problem hiding this comment.
🦩 🔴 Message::info() returns generic boxed std::io::Error variants instead of a typed InfoError/InfoErrorKind
Introduced InfoErrorKind/InfoError (pub type InfoError = error::Error<InfoErrorKind>) with variants MissingReplySubject, MissingPrefix, TooFewTokens, BadTokenNumber, ParseError, implementing Display. Changed Message::info to return Result<Info<'_>, InfoError> instead of Result<Info<'_>, Error>, replacing all Box<std::io::Error> constructions (missing reply subject, missing prefix, too few tokens, bad token number) with the corresponding typed variant, and changed the try_parse! macro's parse-failure and OffsetDateTime::from_unix_timestamp_nanos error paths to wrap the underlying parse errors via InfoError::with_source(InfoErrorKind::ParseError, ...) instead of Box::new(e)/using ? with From<time::error::ComponentRange>. This is also a breaking public API signature change; verify no other file in the crate matches on Message::info()'s old boxed-error type (only visible within this file, so cross-file impact could not be checked).
🤖 Prompt for AI agents
In async-nats/src/jetstream/message.rs around line 271, review and complete this code-review fix: Message::info() returns generic boxed std::io::Error variants instead of a typed InfoError/InfoErrorKind.
What the draft fix changed: Introduced `InfoErrorKind`/`InfoError` (`pub type InfoError = error::Error<InfoErrorKind>`) with variants `MissingReplySubject`, `MissingPrefix`, `TooFewTokens`, `BadTokenNumber`, `ParseError`, implementing `Display`. Changed `Message::info` to return `Result<Info<'_>, InfoError>` instead of `Result<Info<'_>, Error>`, replacing all `Box<std::io::Error>` constructions (missing reply subject, missing prefix, too few tokens, bad token number) with the corresponding typed variant, and changed the `try_parse!` macro's parse-failure and `OffsetDateTime::from_unix_timestamp_nanos` error paths to wrap the underlying parse errors via `InfoError::with_source(InfoErrorKind::ParseError, ...)` instead of `Box::new(e)`/using `?` with `From<time::error::ComponentRange>`. This is also a breaking public API signature change; verify no other file in the crate matches on `Message::info()`'s old boxed-error type (only visible within this file, so cross-file impact could not be checked).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 68 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 2 review findings in
async-nats/src/jetstream/message.rs.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
async-nats/src/jetstream/message.rs:191async-nats/src/jetstream/message.rs:271What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
cfb21c27-56cf-4a3e-bbfe-02ce27b8b29bMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akbhh82 NATS async-nats and CI review findings (9 PRs)