Skip to content
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.

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

Fix NPE in empty XPENDING summary #3792

Closed
wants to merge 1 commit into from

Conversation

gerzse
Copy link
Contributor

@gerzse gerzse commented Mar 28, 2024

When there are no pending messages in a stream, the summary response to XPENDING contains nulls, except the first position which is 0 (the total number of pending message). Return early from the builder, in this case, to avoid NPE.

When there are no pending messages in a stream, the summary response to
XPENDING contains nulls, except the first position which is 0 (the total
number of pending message). Return early from the builder, in this case,
to avoid NPE.
@gerzse gerzse requested a review from sazzad16 March 28, 2024 13:07
Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

Do you have the test where you got NPE? Could you add that?

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 79.45%. Comparing base (a02b2eb) to head (e0d41e8).

Files Patch % Lines
.../main/java/redis/clients/jedis/BuilderFactory.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3792      +/-   ##
============================================
- Coverage     79.50%   79.45%   -0.05%     
  Complexity     5695     5695              
============================================
  Files           301      301              
  Lines         15274    15276       +2     
  Branches       1190     1191       +1     
============================================
- Hits          12143    12138       -5     
- Misses         2554     2559       +5     
- Partials        577      579       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gerzse
Copy link
Contributor Author

gerzse commented Mar 28, 2024

Do you have the test where you got NPE? Could you add that?

I have it in a big PR only with unit tests. It would be some effort to retrofit it. Would it be ok to merge this separately, and the test later?

@@ -1813,6 +1813,11 @@ public StreamPendingSummary build(Object data) {

List<Object> objectList = (List<Object>) data;
long total = BuilderFactory.LONG.build(objectList.get(0));

if (total == 0) {
return new StreamPendingSummary(total, null, null, null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We just don't return null on our own.
We return null or whatever only after reading it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check #3793

@gerzse gerzse closed this Mar 29, 2024
@gerzse
Copy link
Contributor Author

gerzse commented Mar 29, 2024

Fixed more properly in 7393.

@gerzse gerzse deleted the xpending-empty-response branch March 29, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants