Introduce `ReversedRedisListView` for Java 21 compatibility #2608

jxblum · 2023-06-14T17:15:19Z

Do not merge yet.

mp911de · 2023-06-15T07:52:01Z

src/main/java/org/springframework/data/redis/support/collections/DefaultRedisList.java

+
+		DefaultRedisList<E> redisList = new DefaultRedisList<>(getKey(), getOperations(), this.maxSize);
+
+		StreamSupport.stream(this.spliterator(), false)


We should honor that reversed is a view of the underlying list. Here, we duplicate the contents of the list by inserting everything that is stored at getKey().

I suggest introducing a package-private ReversedRedisList type where we delegate to this. As our baseline remains Java 17, I think we will have to set up our own implementation without being able to reuse much from the Java libraries.

Looking at the JDK libs, their classes are delegations.

Yes, this change was intentional and only a temporary hack. I will change this into a proper, view-based abstraction.

mp911de · 2023-06-15T07:53:08Z

src/main/java/org/springframework/data/redis/support/collections/RedisList.java

+
+	@Override
+	default RedisList<E> reversed() {
+		throw new UnsupportedOperationException("Not Implemented");


Let's turn that into a proper implementation using a package-private delegation type. Users looking at the code and finding UOE will have a hard time understanding why this is and whether reversed is supported at all.

Well, methods : [ addFirst(:E), addLast(:E), getFirst(), getLast(), removeFirst(:E), removeLast(:E) ] must be redefined in RedisList<E> since RedisList is an "interface", unlike Java's own LinkedList, which is a "class".

And, since the declarations/definitions of these methods differ between Java's List interface and the Deque interface, where List provides default implementations, and Deque does not, leads to the following compilation error:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (java-compile) on project spring-data-redis: Compilation failure [ERROR] /Users/jblum/pivdev/spring-data-redis/src/main/java/org/springframework/data/redis/support/collections/RedisList.java:[40,7] error: types List<E#1> and BlockingDeque<E#2> are incompatible; [ERROR] interface RedisList<E#3> inherits abstract and default for addFirst(E#3) from types List and BlockingDeque [ERROR] where E#1,E#2,E#3 are type-variables: [ERROR] E#1 extends Object declared in interface List [ERROR] E#2 extends Object declared in interface BlockingDeque [ERROR] E#3 extends Object declared in interface RedisList

reversed(), on the other hand, can indeed be encapsulated in a proper type with a supporting implementation.

Closes spring-projects#2602

…erse order. Closed spring-projects#2602

…versed(). * Add delegate methods to avoid using Java 21 API. * Rename ReversedRedisList to ReversedRedisListView to express its view aspect. * Refine tests. Closes spring-projects#2602

mp911de

This looks pretty decent. I left a few thoughts, other than that, I will go ahead and merge this one for Friday.

mp911de · 2023-07-12T07:00:37Z

src/main/java/org/springframework/data/redis/support/collections/DefaultRedisList.java

+				"Next or previous must be called before set");
+
+			try {
+				DefaultRedisList.this.set(indexOf(this.lastReturnedElement), element);


Let's resort to using the cursor index. Using the index of the last returned element can hit an equal object in the list (e.g. if the list contains [a a a b c c c] and the last returned element is c).

mp911de · 2023-07-12T07:02:10Z

src/main/java/org/springframework/data/redis/support/collections/ReversedRedisListView.java

+	public boolean addAll(Collection<? extends E> collection) {
+
+		@SuppressWarnings("unchecked")
+		E[] adds = (E[]) collection.toArray();


We could have the toArray conversion even after the isEmpty() check

jxblum · 2023-07-12T20:13:18Z

This looks pretty decent. I left a few thoughts, other than that, I will go ahead and merge this one for Friday.

FYI, I am still in favor of removing all this (copy/pasted) code after we move to a Java baseline >= 21, with preference for the provided Java library & API facilities. I had this working reliably on Java 21 in this commit, and I am not interested in maintaining their code. As such, I see this only as a temporary solution.

Additionally, though I did not try it, Google Guava would provide what we need in this case (for example: here), even though it would add a library dependency.

jxblum · 2023-07-12T22:49:53Z

Once more, I have made some modifications based on your feedback. SD Redis still builds reliably on Java 17 and Java 21.

…ctions.reverse(). Closes spring-projects#2602

See #2602 Original pull request: #2608

Closes #2602 Original pull request: #2608

Add documentation for generics. Fix remove() and element() methods to adhere to their contract. See #2602 Original pull request: #2608

mp911de · 2023-07-13T08:06:09Z

That's merged and polished now.

See #2602 Original pull request: #2608

Closes #2602 Original pull request: #2608

Add documentation for generics. Fix remove() and element() methods to adhere to their contract. See #2602 Original pull request: #2608

jxblum added type: dependency-upgrade A dependency upgrade type: task A general task labels Jun 14, 2023

jxblum requested review from mp911de and christophstrobl June 14, 2023 17:15

jxblum force-pushed the issue/2602/java21 branch from 0dfe613 to ea8c303 Compare June 14, 2023 17:34

jxblum force-pushed the main branch from 6afe1e0 to f1492e1 Compare June 14, 2023 19:55

mp911de reviewed Jun 15, 2023

View reviewed changes

Prepare issue branch 'issue/2602' with support for JDK 21.

251135a

jxblum force-pushed the issue/2602/java21 branch from ea8c303 to c4d99d7 Compare July 6, 2023 18:14

jxblum added 2 commits July 6, 2023 11:42

Test against Java 21 in CI.

011af7c

Closes spring-projects#2602

Adapt RedisList to API changes in java.util.List and java.util.Deque.

0a96738

Closes spring-projects#2602

jxblum force-pushed the issue/2602/java21 branch 2 times, most recently from ce6fa3a to 38a42cc Compare July 7, 2023 02:00

Create ReversedRedisList class and view of a wrapped RedisList in rev…

4c9e591

…erse order. Closed spring-projects#2602

jxblum force-pushed the issue/2602/java21 branch from 38a42cc to 4c9e591 Compare July 7, 2023 02:49

mp911de added this to the 3.2 M1 (2023.1.0) milestone Jul 7, 2023

Add delegate reverse operations without using SequencedCollections.re…

d7ea5fc

…versed(). * Add delegate methods to avoid using Java 21 API. * Rename ReversedRedisList to ReversedRedisListView to express its view aspect. * Refine tests. Closes spring-projects#2602

jxblum force-pushed the issue/2602/java21 branch from 3274747 to 98336a0 Compare July 11, 2023 23:18

mp911de approved these changes Jul 12, 2023

View reviewed changes

mp911de changed the title ~~Test against Java 21~~ Introduce ReversedRedisListView for Java 21 compatibility Jul 12, 2023

jxblum force-pushed the issue/2602/java21 branch from 98336a0 to 8944101 Compare July 12, 2023 22:45

Polish for delegating reverse operations without using SequencedColle…

652f916

…ctions.reverse(). Closes spring-projects#2602

jxblum force-pushed the issue/2602/java21 branch from 8944101 to 652f916 Compare July 12, 2023 22:55

mp911de pushed a commit that referenced this pull request Jul 13, 2023

Test against Java 21 in CI.

bb2d053

See #2602 Original pull request: #2608

mp911de pushed a commit that referenced this pull request Jul 13, 2023

Adapt RedisList to API changes in Java 21 SequencedCollections.

dd81aba

Closes #2602 Original pull request: #2608

mp911de added a commit that referenced this pull request Jul 13, 2023

Polishing.

917cc21

Add documentation for generics. Fix remove() and element() methods to adhere to their contract. See #2602 Original pull request: #2608

mp911de closed this Jul 13, 2023

mp911de linked an issue Jul 13, 2023 that may be closed by this pull request

Introduce ReversedRedisListView for Java 21 compatibility #2602

Closed

mp911de pushed a commit that referenced this pull request Jul 13, 2023

Test against Java 21 in CI.

5996078

See #2602 Original pull request: #2608

mp911de pushed a commit that referenced this pull request Jul 13, 2023

Adapt RedisList to API changes in Java 21 SequencedCollections.

a05b25a

Closes #2602 Original pull request: #2608

mp911de added a commit that referenced this pull request Jul 13, 2023

Polishing.

2e5aed4

Add documentation for generics. Fix remove() and element() methods to adhere to their contract. See #2602 Original pull request: #2608

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Introduce `ReversedRedisListView` for Java 21 compatibility #2608

Introduce `ReversedRedisListView` for Java 21 compatibility #2608

Uh oh!

jxblum commented Jun 14, 2023 •

edited

Loading

Uh oh!

mp911de Jun 15, 2023

Uh oh!

jxblum Jul 6, 2023 •

edited

Loading

Uh oh!

mp911de Jun 15, 2023

Uh oh!

jxblum Jul 6, 2023 •

edited

Loading

Uh oh!

jxblum Jul 6, 2023 •

edited

Loading

Uh oh!

mp911de left a comment

Uh oh!

mp911de Jul 12, 2023

Uh oh!

mp911de Jul 12, 2023

Uh oh!

jxblum commented Jul 12, 2023 •

edited

Loading

Uh oh!

jxblum commented Jul 12, 2023 •

edited

Loading

Uh oh!

mp911de commented Jul 13, 2023

Uh oh!

Uh oh!


		DefaultRedisList<E> redisList = new DefaultRedisList<>(getKey(), getOperations(), this.maxSize);

		StreamSupport.stream(this.spliterator(), false)

Introduce ReversedRedisListView for Java 21 compatibility #2608

Introduce ReversedRedisListView for Java 21 compatibility #2608

Uh oh!

Conversation

jxblum commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mp911de Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

jxblum Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mp911de Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

jxblum Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jxblum Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mp911de left a comment

Choose a reason for hiding this comment

Uh oh!

mp911de Jul 12, 2023

Choose a reason for hiding this comment

Uh oh!

mp911de Jul 12, 2023

Choose a reason for hiding this comment

Uh oh!

jxblum commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jxblum commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mp911de commented Jul 13, 2023

Uh oh!

Uh oh!

Introduce `ReversedRedisListView` for Java 21 compatibility #2608

Introduce `ReversedRedisListView` for Java 21 compatibility #2608

jxblum commented Jun 14, 2023 •

edited

Loading

jxblum Jul 6, 2023 •

edited

Loading

jxblum Jul 6, 2023 •

edited

Loading

jxblum Jul 6, 2023 •

edited

Loading

jxblum commented Jul 12, 2023 •

edited

Loading

jxblum commented Jul 12, 2023 •

edited

Loading