[ISSUE #10774] Reject empty producer message lists - #10775
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds early validation in the proxy producer send path to reject null/empty message lists (avoiding raw NullPointerException / IndexOutOfBoundsException) and adds regression tests to ensure the failure is surfaced as a controlled ProxyException.
Changes:
- Add a
null/empty guard at the start ofProducerProcessor#sendMessage. - Add regression tests covering both
Collections.emptyList()andnullmessage list inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| proxy/src/main/java/org/apache/rocketmq/proxy/processor/ProducerProcessor.java | Adds an early guard to reject null/empty messageList before accessing messageList.get(0). |
| proxy/src/test/java/org/apache/rocketmq/proxy/processor/ProducerProcessorTest.java | Adds tests asserting the returned future completes exceptionally for empty/null message lists. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (messageList == null || messageList.isEmpty()) { | ||
| throw new ProxyException(ProxyExceptionCode.FORBIDDEN, "message list is empty"); | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10775 +/- ##
=============================================
- Coverage 48.34% 48.27% -0.08%
+ Complexity 13527 13506 -21
=============================================
Files 1380 1380
Lines 101104 101143 +39
Branches 13107 13121 +14
=============================================
- Hits 48882 48825 -57
- Misses 46267 46337 +70
- Partials 5955 5981 +26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Adds null/empty guard for messageList in ProducerActivityManagerImpl.sendMessage() before accessing messageList.get(0), throwing ProxyException with FORBIDDEN instead of risking IndexOutOfBoundsException.
Findings
- [Info] The fix is correct and minimal. Throwing
ProxyExceptionwithFORBIDDENis appropriate since sending zero messages is a client error. - [Info] Test coverage covers both null and empty cases, verifying the correct exception code and message.
Suggestions
- Consider whether other entry points in
ProducerActivityManagerImplthat access list elements by index (e.g.,sendMessagevariants withList<MessageView>) have similar guards. Consistency across the API surface would reduce future NPE/IOOBE risk.
LGTM.
Automated review by github-manager-bot
What changed
Fixes #10774.
Verification
mvn -pl proxy -Dtest=ProducerProcessorTest testResult: BUILD SUCCESS.
ProducerProcessorTestran 8 tests with 0 failures, errors, or skips. Checkstyle and SpotBugs also passed in the Maven run.