You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "3pacccccc (via GitHub)" <gi...@apache.org> on 2025/12/03 16:19:46 UTC
[PR] [improve][client] Add null checks for MessageAcknowledger methods to prevent NullPointerException [pulsar]
3pacccccc opened a new pull request, #25036:
URL: https://github.com/apache/pulsar/pull/25036
### Motivation
Currently, several methods in the ```MessageAcknowledger``` interface implementation do not validate null parameters for messages, message IDs, or message lists. Passing null values to methods such as ```acknowledge(List<MessageId>)```, ```acknowledge(Messages)```, or ```acknowledgeAsync(MessageId, Transaction)``` can lead to unexpected ```NullPointerException``` errors deep in the call stack. This PR adds explicit null validation at the entry points of these methods to provide clear, early feedback to developers.
### Modifications
1. Added ```validateMessageIds(List<MessageId>)``` and ```validateMessages(Messages)``` helper methods in ```ConsumerBase.java``` to check for null inputs and throw a descriptive ```PulsarClientException.InvalidMessageException```.
2. Integrated these validation checks into:
- ```acknowledge(List<MessageId>)```
- ```acknowledge(Messages)```
- ```acknowledgeAsync(MessageId, Transaction)```
- ```acknowledgeCumulativeAsync(MessageId, Transaction)```
- ```doAcknowledgeWithTxn(...)```
3. Added corresponding unit tests in multiple test classes to verify that null inputs are properly rejected with appropriate exception messages:
- ```TransactionTest.testAckTnxWithNullMessageId()```
- ```ConsumerAckListTest.testAckNullMessageIdList()```
- ```ConsumerBatchReceiveTest.testBatchReceiveAckWithNullMessages()```
- ```TransactionEndToEndTest.testAcknowledgeCumulativeAsyncWithNullMessageId()```
### Verifying this change
- [x] Make sure that the change passes the CI checks.
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [x] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: https://github.com/3pacccccc/pulsar/pull/31
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] Add null checks for MessageAcknowledger methods to prevent NullPointerException [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #25036:
URL: https://github.com/apache/pulsar/pull/25036#discussion_r2587257600
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java:
##########
@@ -330,6 +331,21 @@ public void testCreateTransactionSystemTopic() throws Exception {
assertEquals(e.getMessage(), "Cannot create topic in system topic format!");
}
}
+ @Test(timeOut = 10000)
+ public void testAckTnxWithNullMessageId() throws Exception {
+ final String topic = BrokerTestUtil.newUniqueName("persistent://tnx/ns1/testAckTnxWithNullMessageId");
+ @Cleanup final Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING)
+ .topic(topic)
+ .subscriptionName("sub1")
+ .subscribe();
+ List<MessageId> messageIdList = null;
+ assertThatThrownBy(
+ () -> consumer.acknowledgeAsync(messageIdList, null).get()
Review Comment:
You added this test to `TransactionTest`, but it's nothing related to transaction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] Add null checks for MessageAcknowledger methods to prevent NullPointerException [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #25036:
URL: https://github.com/apache/pulsar/pull/25036#discussion_r2587256921
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java:
##########
@@ -330,6 +331,21 @@ public void testCreateTransactionSystemTopic() throws Exception {
assertEquals(e.getMessage(), "Cannot create topic in system topic format!");
}
}
+ @Test(timeOut = 10000)
+ public void testAckTnxWithNullMessageId() throws Exception {
+ final String topic = BrokerTestUtil.newUniqueName("persistent://tnx/ns1/testAckTnxWithNullMessageId");
+ @Cleanup final Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING)
+ .topic(topic)
+ .subscriptionName("sub1")
+ .subscribe();
+ List<MessageId> messageIdList = null;
+ assertThatThrownBy(
+ () -> consumer.acknowledgeAsync(messageIdList, null).get()
Review Comment:
Oh I see the 1st argument is also null.
I think you can just put all `acknowledge` tests into a single test method because there are many overloads.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] Add null checks for MessageAcknowledger methods to prevent NullPointerException [pulsar]
Posted by "3pacccccc (via GitHub)" <gi...@apache.org>.
3pacccccc commented on PR #25036:
URL: https://github.com/apache/pulsar/pull/25036#issuecomment-3609853527
> Please also document this behavior to `MessageAcknowledger` so that the behavior will be clear if a null message id or list is passed to `acknowledge` family
OK
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] Add null checks for MessageAcknowledger methods to prevent NullPointerException [pulsar]
Posted by "3pacccccc (via GitHub)" <gi...@apache.org>.
3pacccccc commented on code in PR #25036:
URL: https://github.com/apache/pulsar/pull/25036#discussion_r2587281307
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java:
##########
@@ -330,6 +331,21 @@ public void testCreateTransactionSystemTopic() throws Exception {
assertEquals(e.getMessage(), "Cannot create topic in system topic format!");
}
}
+ @Test(timeOut = 10000)
+ public void testAckTnxWithNullMessageId() throws Exception {
+ final String topic = BrokerTestUtil.newUniqueName("persistent://tnx/ns1/testAckTnxWithNullMessageId");
+ @Cleanup final Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING)
+ .topic(topic)
+ .subscriptionName("sub1")
+ .subscribe();
+ List<MessageId> messageIdList = null;
+ assertThatThrownBy(
+ () -> consumer.acknowledgeAsync(messageIdList, null).get()
Review Comment:
> Oh I see the 1st argument is also null.
>
> I think you can just put all `acknowledge` tests into a single test method because there are many overloads.
OK, thanks for the suggestion, that's make sense, I'll do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] Add null checks for MessageAcknowledger methods to prevent NullPointerException [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25036:
URL: https://github.com/apache/pulsar/pull/25036#issuecomment-3613547684
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/25036?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `92.30769%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 74.36%. Comparing base ([`b37b5b3`](https://app.codecov.io/gh/apache/pulsar/commit/b37b5b3fa01494fd6541f4d460f3586443c221b6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`64410e7`](https://app.codecov.io/gh/apache/pulsar/commit/64410e73147e025530a0f60e51f7a2edfeae01ec?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/25036?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...va/org/apache/pulsar/client/impl/ConsumerBase.java](https://app.codecov.io/gh/apache/pulsar/pull/25036?src=pr&el=tree&filepath=pulsar-client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fclient%2Fimpl%2FConsumerBase.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0NvbnN1bWVyQmFzZS5qYXZh) | 92.30% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/25036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/25036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #25036 +/- ##
============================================
- Coverage 74.47% 74.36% -0.11%
+ Complexity 34149 33756 -393
============================================
Files 1921 1921
Lines 150350 150378 +28
Branches 17463 17469 +6
============================================
- Hits 111972 111833 -139
- Misses 29496 29645 +149
- Partials 8882 8900 +18
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/25036/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/25036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.29% <15.38%> (-0.36%)` | :arrow_down: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/25036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `22.85% <15.38%> (-0.09%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/25036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.91% <92.30%> (-0.10%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/25036?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../apache/pulsar/client/api/MessageAcknowledger.java](https://app.codecov.io/gh/apache/pulsar/pull/25036?src=pr&el=tree&filepath=pulsar-client-api%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fclient%2Fapi%2FMessageAcknowledger.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9jbGllbnQvYXBpL01lc3NhZ2VBY2tub3dsZWRnZXIuamF2YQ==) | `0.00% <ø> (ø)` | |
| [...va/org/apache/pulsar/client/impl/ConsumerBase.java](https://app.codecov.io/gh/apache/pulsar/pull/25036?src=pr&el=tree&filepath=pulsar-client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fclient%2Fimpl%2FConsumerBase.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0NvbnN1bWVyQmFzZS5qYXZh) | `75.00% <92.30%> (+0.57%)` | :arrow_up: |
... and [75 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/25036/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] Add null checks for MessageAcknowledger methods to prevent NullPointerException [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- merged PR #25036:
URL: https://github.com/apache/pulsar/pull/25036
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] Add null checks for MessageAcknowledger methods to prevent NullPointerException [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #25036:
URL: https://github.com/apache/pulsar/pull/25036#discussion_r2587168773
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java:
##########
@@ -330,6 +331,21 @@ public void testCreateTransactionSystemTopic() throws Exception {
assertEquals(e.getMessage(), "Cannot create topic in system topic format!");
}
}
+ @Test(timeOut = 10000)
+ public void testAckTnxWithNullMessageId() throws Exception {
+ final String topic = BrokerTestUtil.newUniqueName("persistent://tnx/ns1/testAckTnxWithNullMessageId");
+ @Cleanup final Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING)
+ .topic(topic)
+ .subscriptionName("sub1")
+ .subscribe();
+ List<MessageId> messageIdList = null;
+ assertThatThrownBy(
+ () -> consumer.acknowledgeAsync(messageIdList, null).get()
Review Comment:
This is a null `Transaction`, not message id. It seems that the test name should change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] Add null checks for MessageAcknowledger methods to prevent NullPointerException [pulsar]
Posted by "3pacccccc (via GitHub)" <gi...@apache.org>.
3pacccccc closed pull request #25036: [improve][client] Add null checks for MessageAcknowledger methods to prevent NullPointerException
URL: https://github.com/apache/pulsar/pull/25036
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] Add null checks for MessageAcknowledger methods to prevent NullPointerException [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #25036:
URL: https://github.com/apache/pulsar/pull/25036#discussion_r2587197081
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java:
##########
@@ -330,6 +331,21 @@ public void testCreateTransactionSystemTopic() throws Exception {
assertEquals(e.getMessage(), "Cannot create topic in system topic format!");
}
}
+ @Test(timeOut = 10000)
+ public void testAckTnxWithNullMessageId() throws Exception {
+ final String topic = BrokerTestUtil.newUniqueName("persistent://tnx/ns1/testAckTnxWithNullMessageId");
+ @Cleanup final Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING)
+ .topic(topic)
+ .subscriptionName("sub1")
+ .subscribe();
+ List<MessageId> messageIdList = null;
+ assertThatThrownBy(
+ () -> consumer.acknowledgeAsync(messageIdList, null).get()
Review Comment:
and I think a null transaction is accepted, though it's not formally documented. In this case, it will fall back to `acknowledgeAsync(messageIdList)`
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java:
##########
@@ -330,6 +331,21 @@ public void testCreateTransactionSystemTopic() throws Exception {
assertEquals(e.getMessage(), "Cannot create topic in system topic format!");
}
}
+ @Test(timeOut = 10000)
+ public void testAckTnxWithNullMessageId() throws Exception {
+ final String topic = BrokerTestUtil.newUniqueName("persistent://tnx/ns1/testAckTnxWithNullMessageId");
+ @Cleanup final Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING)
+ .topic(topic)
+ .subscriptionName("sub1")
+ .subscribe();
+ List<MessageId> messageIdList = null;
+ assertThatThrownBy(
+ () -> consumer.acknowledgeAsync(messageIdList, null).get()
Review Comment:
and I think a null transaction should be accepted, though it's not formally documented. In this case, it will fall back to `acknowledgeAsync(messageIdList)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] Add null checks for MessageAcknowledger methods to prevent NullPointerException [pulsar]
Posted by "3pacccccc (via GitHub)" <gi...@apache.org>.
3pacccccc commented on code in PR #25036:
URL: https://github.com/apache/pulsar/pull/25036#discussion_r2587250457
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java:
##########
@@ -330,6 +331,21 @@ public void testCreateTransactionSystemTopic() throws Exception {
assertEquals(e.getMessage(), "Cannot create topic in system topic format!");
}
}
+ @Test(timeOut = 10000)
+ public void testAckTnxWithNullMessageId() throws Exception {
+ final String topic = BrokerTestUtil.newUniqueName("persistent://tnx/ns1/testAckTnxWithNullMessageId");
+ @Cleanup final Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING)
+ .topic(topic)
+ .subscriptionName("sub1")
+ .subscribe();
+ List<MessageId> messageIdList = null;
+ assertThatThrownBy(
+ () -> consumer.acknowledgeAsync(messageIdList, null).get()
Review Comment:
Thanks for the feedback @BewareMyPower
Actually, in this test, the main point is that the ```messageIdList``` is null. The test is designed to verify that passing a null ```messageIdList``` to ```acknowledgeAsync()``` triggers the expected ```InvalidMessageException```, regardless of whether the ```Transaction``` parameter is null or not.
I named the test ```testAckTnxWithNullMessageId``` because the method signature includes both ```messageIdList``` and ```Transaction```, but the focus is really on the ```messageIdList``` validation.
To make it clearer, I’m happy to rename the test to something like:
```testAcknowledgeAsyncWithNullMessageIdList()``` – would that help avoid confusion?
Let me know if you'd prefer that update, or if you have other suggestions!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org