Terminate stream with error on `null` values returned by `RedisElementReader` for top-level elements #2672

mp911de · 2023-08-14T09:30:37Z

We now use mapNotNull to map Redis response values to avoid NullPointerExceptions caused by RedisElementReader returning null.

This effectively turns RedisElementReader into a filter function that can suppress elements from being emitted.

Closes #2655

If merged, we should bring these changes to all maintained 3.x branches.

jxblum

I think all these changes and Javadoc edits look completely fine to me.

I'd be curious what @christophstrobl would have to say about this, though.

christophstrobl · 2023-08-16T10:42:53Z

I'm a bit torn on this one, since the map operator on Flux accepts a plain Function which may return null with all the consequences when used in a reactive environment. The outlined snippet demonstrates this quite well, and as @mp911de mentioned in the original ticket, if used that way then this is more of an application concern. While this way of filtering will do the job, others may use exactly that behaviour to raise errors in the flow.

mp911de · 2023-08-16T12:24:02Z

So what do we want to do here? Our serializer clearly allow for null values. A serializer cannot know whether it is being used in the specific arrangement of top-level values (e.g. JDK serializer that serializes a null value).

…tReader` for top-level elements. We now emit InvalidDataAccessApiUsageException when a RedisElementReader returns null in the context of a top-level stream to indicate invalid API usage although RedisElementReader.read can generally return null values if these are being collected in a container or value wrapper or parent complex object.

Consistent operations documentation wording.

mp911de · 2023-08-17T10:06:08Z

I rewrote this pull request to document the behavior in the context of top-level stream values. We also now terminate the stream in a controlled manner by emitting InvalidDataAccessApiUsageException.

jxblum · 2023-09-14T21:18:57Z

I am reviewing this PR now and I generally like the direction the changes took.

However, I don't necessarily agree with this:

If merged, we should bring these changes to all maintained 3.x branches.

I think this is a significant change in the contract and behavior of SD Redis that will potentially and adversely affect the older GA'd branches in the 3.x line, mainly 3.0.x and 3.1.x now, both of which are GA.

Honestly, with regard to #2655, I really think this is a user problem and application concern more than it is a framework issue that needs to be addressed and handled by the framework.

While these changes are an improvement overall (i.e. failing fast when Reactive streams are misused as the user has done in #2655, along with employing the Spring Framework DAO Exception Hierarchy (Javadoc)), it is also a clear change in the contract and behavior that diverges from the Reactive Streams specification, which is very clear on the matter (Rule 2.13) of null elements in a stream (thereby causing and throwing a NullPointerException).

I also agree with and reiterate @christophstrobl comments above, earlier.

jxblum · 2023-09-14T21:58:00Z

src/main/java/org/springframework/data/redis/core/DefaultReactiveListOperations.java

@@ -285,7 +288,8 @@ public Mono<V> rightPopAndLeftPush(K sourceKey, K destinationKey, Duration timeo
 		Assert.isTrue(isZeroOrGreater1Second(timeout), "Duration must be either zero or greater or equal to 1 second");

 		return createMono(
-				connection -> connection.bRPopLPush(rawKey(sourceKey), rawKey(destinationKey), timeout).map(this::readValue));
+				connection -> connection.bRPopLPush(rawKey(sourceKey), rawKey(destinationKey), timeout)
+						.map(this::readRequiredValue));


Shouldn't this be .mapNotNull(..) in this case given the Duration parameter?

The previous variant was mapNotNull that effectively filters null values turning RedisElementReader into something that has an ability of filtering. That's why we chose to use readRequiredValue throwing a proper exception.

This previous variant was .map(this::readValue), line 288.

I was thinking more along the lines of the rightPop(key, :Duration) overloaded method above on line 262.

But, perhaps the choice of simply .map(this:readRequiredValue) vs. .mapNotNull(this:readRequiredValue) is due to the right pop followed by left push compound operation??

See spring-projects#2655 Original pull request: spring-projects#2672

jxblum · 2023-09-14T23:07:37Z

src/main/java/org/springframework/data/redis/core/DefaultReactiveGeoOperations.java

@@ -40,6 +39,7 @@
 import org.springframework.data.redis.domain.geo.GeoReference.GeoMemberReference;
 import org.springframework.data.redis.domain.geo.GeoShape;
 import org.springframework.data.redis.serializer.RedisSerializationContext;
+import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;



It seems to me the geoDist(..) methods in ReactiveGeoOperations for the GEODIST command variations should be protected in the case of returning null.

While the arguments passed to the ReactiveGeoOperations.distance(..) methods used in the computation of the geo distance calculation may not be null, the member argument may also not have been geocoded in Redis with the geoAdd(..) command properly (which I think is a prerequisite based on the docs).

Therefore, in the case of a "missing" member (or element) the GEODIST command returns null as documented.

I think, perhaps, ReactiveGeoOperations needs further refinements down the road, particularly with guards around possible null values.

jxblum · 2023-10-11T01:53:07Z

src/main/java/org/springframework/data/redis/core/DefaultReactiveListOperations.java

@@ -285,7 +288,8 @@ public Mono<V> rightPopAndLeftPush(K sourceKey, K destinationKey, Duration timeo
 		Assert.isTrue(isZeroOrGreater1Second(timeout), "Duration must be either zero or greater or equal to 1 second");

 		return createMono(
-				connection -> connection.bRPopLPush(rawKey(sourceKey), rawKey(destinationKey), timeout).map(this::readValue));
+				connection -> connection.bRPopLPush(rawKey(sourceKey), rawKey(destinationKey), timeout)
+						.map(this::readRequiredValue));


This previous variant was .map(this::readValue), line 288.

I was thinking more along the lines of the rightPop(key, :Duration) overloaded method above on line 262.

But, perhaps the choice of simply .map(this:readRequiredValue) vs. .mapNotNull(this:readRequiredValue) is due to the right pop followed by left push compound operation??

jxblum · 2023-10-11T01:55:46Z

src/main/java/org/springframework/data/redis/core/ReactiveStreamOperations.java

-import org.springframework.data.redis.connection.stream.PendingMessages;
-import org.springframework.data.redis.connection.stream.PendingMessagesSummary;
-import org.springframework.data.redis.connection.stream.ReadOffset;
+import org.springframework.data.redis.connection.stream.*;


We should avoid star imports.

See spring-projects#2655 Original pull request: spring-projects#2672

jxblum · 2023-10-11T02:29:08Z

The changes have been merged to main (only, due to contractual/behavioral changes) for 3.2, Spring Data 2023.1.0-RC1.

mp911de · 2023-10-11T06:32:21Z

Please close pull requests and remove their branches after merge.

mp911de linked an issue Aug 14, 2023 that may be closed by this pull request

Nullability of RedisElementReader.read(…) contradicts non-nullability of Flux.map(…) #2655

Closed

mp911de added type: bug A general bug for: team-attention An issue we need to discuss as a team to make progress labels Aug 14, 2023

mp911de requested review from jxblum and christophstrobl and removed request for jxblum August 14, 2023 09:30

jxblum reviewed Aug 15, 2023

View reviewed changes

mp911de changed the title ~~Filter null values from top-level Flux and Mono responses~~ Cancel stream on null values returned by RedisElementReader for top-level elements Aug 17, 2023

mp911de changed the title ~~Cancel stream on null values returned by RedisElementReader for top-level elements~~ Terminate stream with error on null values returned by RedisElementReader for top-level elements Aug 17, 2023

mp911de added 3 commits August 17, 2023 12:02

Prepare issue branch.

7d3e805

Polishing.

0b123d2

Consistent operations documentation wording.

mp911de force-pushed the issue/3.0.x/2655 branch from 23c6ecf to 0b123d2 Compare August 17, 2023 10:05

mp911de assigned jxblum Sep 12, 2023

mp911de removed the for: team-attention An issue we need to discuss as a team to make progress label Sep 12, 2023

jxblum added type: enhancement A general enhancement and removed type: bug A general bug labels Sep 14, 2023

jxblum reviewed Sep 14, 2023

View reviewed changes

jxblum added a commit to jxblum/spring-data-redis that referenced this pull request Sep 14, 2023

Polish for Issue spring-projects#2655 and PR spring-projects#2672.

4cb843d

See spring-projects#2655 Original pull request: spring-projects#2672

jxblum added a commit to jxblum/spring-data-redis that referenced this pull request Oct 11, 2023

Polish for Issue spring-projects#2655 and PR spring-projects#2672.

420d314

See spring-projects#2655 Original pull request: spring-projects#2672

jxblum reviewed Oct 11, 2023

View reviewed changes

jxblum added a commit to jxblum/spring-data-redis that referenced this pull request Oct 11, 2023

Polish for Issue spring-projects#2655 and PR spring-projects#2672.

04dedcd

See spring-projects#2655 Original pull request: spring-projects#2672

jxblum added a commit to jxblum/spring-data-redis that referenced this pull request Oct 11, 2023

Polish for Issue spring-projects#2655 and PR spring-projects#2672.

dc0756f

See spring-projects#2655 Original pull request: spring-projects#2672

jxblum added this to the 3.2 RC1 (2023.1.0) milestone Oct 11, 2023

mp911de closed this Oct 11, 2023

mp911de deleted the issue/3.0.x/2655 branch October 11, 2023 06:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Terminate stream with error on `null` values returned by `RedisElementReader` for top-level elements #2672

Terminate stream with error on `null` values returned by `RedisElementReader` for top-level elements #2672

mp911de commented Aug 14, 2023

jxblum left a comment •

edited

Loading

christophstrobl commented Aug 16, 2023

mp911de commented Aug 16, 2023

mp911de commented Aug 17, 2023

jxblum commented Sep 14, 2023 •

edited

Loading

jxblum Sep 14, 2023 •

edited

Loading

mp911de Sep 15, 2023

jxblum Oct 11, 2023

jxblum Sep 14, 2023

jxblum Oct 11, 2023

jxblum Oct 11, 2023

jxblum Oct 11, 2023

jxblum commented Oct 11, 2023 •

edited

Loading

mp911de commented Oct 11, 2023

Terminate stream with error on null values returned by RedisElementReader for top-level elements #2672

Terminate stream with error on null values returned by RedisElementReader for top-level elements #2672

Conversation

mp911de commented Aug 14, 2023

jxblum left a comment • edited Loading

Choose a reason for hiding this comment

christophstrobl commented Aug 16, 2023

mp911de commented Aug 16, 2023

mp911de commented Aug 17, 2023

jxblum commented Sep 14, 2023 • edited Loading

jxblum Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

mp911de Sep 15, 2023

Choose a reason for hiding this comment

jxblum Oct 11, 2023

Choose a reason for hiding this comment

jxblum Sep 14, 2023

Choose a reason for hiding this comment

jxblum Oct 11, 2023

Choose a reason for hiding this comment

jxblum Oct 11, 2023

Choose a reason for hiding this comment

jxblum Oct 11, 2023

Choose a reason for hiding this comment

jxblum commented Oct 11, 2023 • edited Loading

mp911de commented Oct 11, 2023

Terminate stream with error on `null` values returned by `RedisElementReader` for top-level elements #2672

Terminate stream with error on `null` values returned by `RedisElementReader` for top-level elements #2672

jxblum left a comment •

edited

Loading

jxblum commented Sep 14, 2023 •

edited

Loading

jxblum Sep 14, 2023 •

edited

Loading

jxblum commented Oct 11, 2023 •

edited

Loading