Skip to content

Conversation

@ttypic
Copy link
Contributor

@ttypic ttypic commented Sep 29, 2025

Resolves #1167

Bug only shown in the log, but still annoying

Summary by CodeRabbit

  • Bug Fixes
    • Fixed a rare crash when processing messages with no payload, preventing unnecessary exceptions and improving stability in edge cases.
    • Improved error handling for message payload types to avoid false positives.
    • Previously, some operations could throw an error if a message payload was absent; these cases are now handled gracefully, reducing log noise and avoiding interruptions for end users.

@ttypic ttypic requested a review from sacOO7 September 29, 2025 11:59
@coderabbitai
Copy link

coderabbitai bot commented Sep 29, 2025

Walkthrough

Adjusted conditional handling in BaseMessage to avoid throwing when lastPayload is null during data decoding. Previously, a non-String/byte[] payload triggered an exception unconditionally; now the exception is thrown only if lastPayload is non-null. Minor formatting tweak in an if condition.

Changes

Cohort / File(s) Summary of Changes
Message decoding logic
lib/src/main/java/io/ably/lib/types/BaseMessage.java
Modified final else branch to throw only when lastPayload != null; prevents exception on null data. Minor formatting change in an if condition.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant BaseMessage

  Client->>BaseMessage: decode(data)
  alt data is String or byte[]
    BaseMessage-->>Client: proceed with decoded payload
  else data is null
    Note right of BaseMessage: No exception thrown (new behavior)
    BaseMessage-->>Client: proceed without payload
  else data is other type
    BaseMessage-->>Client: throw decoding exception
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A hush where data used to be,
No bytes, no strings—still bug-free.
I twitch my ears: no errors sound,
The nulls now hop and pass around.
Carrots up! The logs stay clear—
A gentle fix, a happy cheer. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title precisely communicates the primary change by stating that the pull request fixes the null‐check behavior for empty message data during decoding, using a standard “fix:” prefix and clear, specific language that aligns with the implemented code modifications.
Linked Issues Check ✅ Passed The updated BaseMessage decoding logic now only throws an exception when lastPayload is non-null, thereby preventing errors and error logs for valid empty payloads and directly satisfying the requirements of issues #1167 and ECO-5570.
Out of Scope Changes Check ✅ Passed All code changes are confined to the Message decoding logic in BaseMessage.java and relate solely to handling empty data payloads, with no modifications outside the scope of the linked issues.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ECO-5570/fix-decoding-empty-data

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 13b8e2e and 09ca74d.

📒 Files selected for processing (1)
  • lib/src/main/java/io/ably/lib/types/BaseMessage.java (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
  • GitHub Check: check (29)
  • GitHub Check: build
  • GitHub Check: check-liveobjects
  • GitHub Check: check-rest
  • GitHub Check: check-realtime-okhttp
  • GitHub Check: check-rest-okhttp
  • GitHub Check: check (29)
  • GitHub Check: check
  • GitHub Check: check-realtime
  • GitHub Check: check (24)
  • GitHub Check: check (19)
  • GitHub Check: check (21)
🔇 Additional comments (1)
lib/src/main/java/io/ably/lib/types/BaseMessage.java (1)

174-179: Good catch: avoid throwing for absent payloads.

Guarding the exception behind a lastPayload != null check stops us from logging spurious decode errors when a message legitimately contains no payload, while preserving the safety net for unexpected types. Looks solid.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Collaborator

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

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

Lgtm

@ttypic ttypic merged commit 96fbe1b into main Sep 30, 2025
14 checks passed
@ttypic ttypic deleted the ECO-5570/fix-decoding-empty-data branch September 30, 2025 07:44
@ttypic ttypic mentioned this pull request Oct 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Decoding error in the logs when data field is empty

3 participants