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

DATAREDIS 852 - Various NullPointerException using Lettuce and Range.Bound.unbounded() #353

Closed
wants to merge 3 commits into from

Conversation

@mmanciop
Copy link
Contributor

mmanciop commented Jul 19, 2018

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).
@pivotal-issuemaster

This comment has been minimized.

Copy link

pivotal-issuemaster commented Jul 19, 2018

@mmanciop Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@mmanciop

This comment has been minimized.

Copy link
Contributor Author

mmanciop commented Jul 19, 2018

I am making separate changes for the different steps for ease of review. If this PR is accepted, please consider squashing the changes.

@mmanciop mmanciop force-pushed the mmanciop:DATAREDIS-852 branch from d276e26 to e69417d Jul 19, 2018
@mmanciop mmanciop changed the title Dataredis 852 - Various NullPointerException using Lettuce and Range.Bound.unbounded() DATAREDIS 852 - Various NullPointerException using Lettuce and Range.Bound.unbounded() Jul 19, 2018
Copy link
Member

mp911de left a comment

Thanks a lot for submitting a pull request. I added a few comments regarding the starting indexes ad documented at redis.io/commands. Care to have a look?

@@ -323,7 +324,7 @@
Range<Long> range = command.getRange();

return (!Range.unbounded().equals(range) ? cmd.bitcount(command.getKey(),
range.getLowerBound().getValue().orElse(null), range.getUpperBound().getValue().orElse(null))
range.getLowerBound().getValue().orElse(Long.MIN_VALUE), range.getUpperBound().getValue().orElse(Long.MAX_VALUE))

This comment has been minimized.

Copy link
@mp911de

mp911de Jul 19, 2018

Member

The starting index (unbounded) should be 0L and not -263.

@@ -189,28 +191,28 @@
if (command.isWithScores()) {

result = cmd
.zrangeWithScores(command.getKey(), command.getRange().getLowerBound().getValue().orElse(null),
command.getRange().getUpperBound().getValue().orElse(null))
.zrangeWithScores(command.getKey(), command.getRange().getLowerBound().getValue().orElse(Long.MIN_VALUE),

This comment has been minimized.

Copy link
@mp911de

mp911de Jul 19, 2018

Member

The starting index (unbounded) should be 0L and not -263.

This comment has been minimized.

Copy link
@mmanciop

mmanciop Jul 19, 2018

Author Contributor

I am actually not 100% sure about this one. The range is based on scores, and scores can be negative, right?

This comment has been minimized.

Copy link
@mp911de

mp911de Jul 19, 2018

Member

This one is https://redis.io/commands/zrange. You're mentioning https://redis.io/commands/zrangebyscore where min/max are based on scores and use -inf/+inf to denote min and max.

This comment has been minimized.

Copy link
@mmanciop

mmanciop Jul 19, 2018

Author Contributor

Oh, OK then. The "with scores" threw me way off.

.map(sc -> (Tuple) new DefaultTuple(getBytes(sc), sc.getScore()));
} else {

result = cmd
.zrange(command.getKey(), command.getRange().getLowerBound().getValue().orElse(null),
command.getRange().getUpperBound().getValue().orElse(null))
.zrange(command.getKey(), command.getRange().getLowerBound().getValue().orElse(Long.MIN_VALUE),

This comment has been minimized.

Copy link
@mp911de

mp911de Jul 19, 2018

Member

The starting index (unbounded) should be 0L and not -263.

.map(value -> (Tuple) new DefaultTuple(ByteUtils.getBytes(value), Double.NaN));
}
} else {
if (command.isWithScores()) {

result = cmd
.zrevrangeWithScores(command.getKey(), command.getRange().getLowerBound().getValue().orElse(null),
command.getRange().getUpperBound().getValue().orElse(null))
.zrevrangeWithScores(command.getKey(), command.getRange().getLowerBound().getValue().orElse(Long.MIN_VALUE),

This comment has been minimized.

Copy link
@mp911de

mp911de Jul 19, 2018

Member

The starting index (unbounded) should be 0L and not -263.

.map(sc -> (Tuple) new DefaultTuple(getBytes(sc), sc.getScore()));
} else {

result = cmd
.zrevrange(command.getKey(), command.getRange().getLowerBound().getValue().orElse(null),
command.getRange().getUpperBound().getValue().orElse(null))
.zrevrange(command.getKey(), command.getRange().getLowerBound().getValue().orElse(Long.MIN_VALUE),

This comment has been minimized.

Copy link
@mp911de

mp911de Jul 19, 2018

Member

The starting index (unbounded) should be 0L and not -263.

@@ -377,8 +378,8 @@
Assert.notNull(command.getRange(), "Range must not be null!");

return cmd
.zremrangebyrank(command.getKey(), command.getRange().getLowerBound().getValue().orElse(null),
command.getRange().getUpperBound().getValue().orElse(null))
.zremrangebyrank(command.getKey(), command.getRange().getLowerBound().getValue().orElse(Long.MIN_VALUE),

This comment has been minimized.

Copy link
@mp911de

mp911de Jul 19, 2018

Member

The starting index (unbounded) should be 0L and not -263.

@pivotal-issuemaster

This comment has been minimized.

Copy link

pivotal-issuemaster commented Jul 19, 2018

@mmanciop Thank you for signing the Contributor License Agreement!

@mmanciop mmanciop force-pushed the mmanciop:DATAREDIS-852 branch from 8354e58 to 25ea292 Jul 19, 2018
@mmanciop

This comment has been minimized.

Copy link
Contributor Author

mmanciop commented Jul 19, 2018

@mp911de I think I have addressed your remarks with the last push :-)

@mmanciop

This comment has been minimized.

Copy link
Contributor Author

mmanciop commented Jul 19, 2018

Looks like it timed out before running the ZSet and String tests.

mp911de added a commit that referenced this pull request Jul 24, 2018
…ange.Bound.unbounded() with Lettuce.

We now return in List, String, and ZSet commands a lower/upper bound range index to prevent NullPointerException. Previously, unbounded ranges could render null values that were attempted to cast to primitives.

Original pull request: #353.
mp911de added a commit that referenced this pull request Jul 24, 2018
Refactor lower/upper bound retrieval into LettuceConverters for a consistent lower/upper boundary return value. Use minus one (-1) as the upper bound index to always retrieve the last element if the upper boundary is not bounded.

Original pull request: #353.
mp911de added a commit that referenced this pull request Jul 24, 2018
…ange.Bound.unbounded() with Lettuce.

We now return in List, String, and ZSet commands a lower/upper bound range index to prevent NullPointerException. Previously, unbounded ranges could render null values that were attempted to cast to primitives.

Original pull request: #353.
mp911de added a commit that referenced this pull request Jul 24, 2018
Refactor lower/upper bound retrieval into LettuceConverters for a consistent lower/upper boundary return value. Use minus one (-1) as the upper bound index to always retrieve the last element if the upper boundary is not bounded.

Original pull request: #353.
@mp911de

This comment has been minimized.

Copy link
Member

mp911de commented Jul 24, 2018

That's merged, polished, and backported now.

@mp911de mp911de closed this Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.