Make Either.stream() and iterator() empty for a left value

[vasiliy-mikhailov]

Problem

Either.stream() returned Stream.of(right()) and Either.iterator() returned Collections.singleton(right()).iterator() unconditionally, regardless of which side the Either holds. For a left-valued Either, right() is null, so:

• stream() yields a single null element (and throws NullPointerException in downstream operations such as toList() on some pipelines), and
• iterator() yields one null element instead of being empty.

A left-valued Either has no right value, so both should be empty — consistent with treating Either as a (possibly empty) collection of the right value.

Fix

Return an empty stream / iterator when the Either holds a left value.

Test

Adds EitherTest covering stream() and iterator() for both left and right values. The left-value cases fail on trunk (non-empty, containing null) and pass with this change.

[CLAassistant]

All committers have signed the CLA.

[qodo-code-review]

PR Summary by Qodo

Fix Either left-side stream/iterator and treat UrlTemplate prefixes literally
🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Description

• Make Either.stream() and iterator() empty when Either holds a left value.
• Prevent UrlTemplate.match(url, prefix) from treating prefix as a regex.
• Add regression tests for Either collection semantics and UrlTemplate prefix stripping.

Diagram

flowchart TD
A["Callers Tests"] --> C{"Either Right?"}
C -->|yes| D["Stream of Right"] --> E{{"Stream API"}}
C -->|no| F["Empty Stream"] --> E
A --> G["Template Match"] --> H["Strip Prefix"] --> I{{"Pattern API"}}

subgraph Legend
direction LR
_mod["Module Class"] ~~~ _dec{"Decision"} ~~~ _ext{{"JDK API"}}
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Avoid regex in UrlTemplate prefix stripping

• ➕ Eliminates regex engine overhead and all regex edge cases
• ➕ More obviously correct for path-prefix stripping (startsWith + substring)
• ➖ Requires careful handling of edge cases (prefix=/, empty prefix, encoding) currently handled implicitly
• ➖ More code than the minimal Pattern.quote fix

2. Model Either’s right projection as Optional/Iterable wrapper

• ➕ Makes collection-like semantics explicit (e.g., right().stream())
• ➕ Could reduce future ambiguity about left-valued behavior
• ➖ Larger API surface / potential compatibility concerns
• ➖ Overkill for a targeted behavioral bug fix

Recommendation: The PR’s approach is the best minimal, compatibility-preserving fix: it corrects Either’s collection semantics without changing the public API, and it hardens UrlTemplate.match(prefix) by quoting the prefix while keeping the existing replaceFirst-based implementation. Consider the substring-based alternative only if UrlTemplate performance/clarity becomes a priority.

Files changed (4) +71 / -3

Bug fix (2) +3 / -3

Either.javaMake iterator()/stream() empty for left-valued Either +2/-2

Make iterator()/stream() empty for left-valued Either

• Changes iterator() and stream() to return empty results when the Either is left-valued. Prevents exposing a single null element (and downstream NPEs) when no right value exists.

java/src/org/openqa/selenium/internal/Either.java

UrlTemplate.javaQuote prefix in match(url, prefix) before replaceFirst +1/-1

Quote prefix in match(url, prefix) before replaceFirst

• Wraps the prefix with Pattern.quote when stripping it via replaceFirst. This ensures prefixes containing regex metacharacters are treated literally and avoids PatternSyntaxException or incorrect stripping.

java/src/org/openqa/selenium/remote/http/UrlTemplate.java

Tests (2) +68 / -0

EitherTest.javaAdd tests for Either.stream()/iterator() left vs right behavior +57/-0

Add tests for Either.stream()/iterator() left vs right behavior

• Introduces JUnit tests asserting that left-valued Eithers produce empty streams/iterators and right-valued Eithers yield exactly the right value. Provides regression coverage for the null-element behavior.

java/test/org/openqa/selenium/internal/EitherTest.java

UrlTemplateTest.javaAdd regression test for regex-metacharacter prefix handling +11/-0

Add regression test for regex-metacharacter prefix handling

• Adds a test ensuring match(url, prefix) does not throw when the prefix contains regex metacharacters (e.g., '('). Verifies parameters are still extracted correctly after literal prefix stripping.

java/test/org/openqa/selenium/remote/http/UrlTemplateTest.java

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 18 rules

1. Java 11 incompatible test ✓ Resolved 🐞 Bug ≡ Correctness

Description

EitherTest calls Stream.toList(), but the repo’s default Bazel compilation target is `--release
11, where Stream.toList()` does not exist. This will fail compilation for the new test under the
default build configuration.

Code

java/test/org/openqa/selenium/internal/EitherTest.java[R31-38]

+    List<Integer> result = either.stream().toList();
+    assertThat(result).isEmpty();
+  }
+
+  @Test
+  void streamOnRightValueShouldContainRight() {
+    Either<String, Integer> either = Either.right(42);
+    List<Integer> result = either.stream().toList();

Evidence

The new test uses Stream.toList() (Java 16+), while the repository’s Bazel configuration
explicitly targets Java 11 by default via --release 11, making this a compile-time incompatibility
in the default build.

java/test/org/openqa/selenium/internal/EitherTest.java[28-40]
.bazelrc[23-35]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`java/test/org/openqa/selenium/internal/EitherTest.java` uses `either.stream().toList()`. `Stream.toList()` was introduced in Java 16, but this repo’s Bazel defaults to `--release 11`, so the test will not compile under the default toolchain/CI settings.

### Issue Context
Bazel is configured to target Java 11 by default (`--javacopt="--release 11"`). New tests should avoid APIs introduced after Java 11 unless the test target explicitly compiles with a higher `--release`.

### Fix Focus Areas
- java/test/org/openqa/selenium/internal/EitherTest.java[22-40]
- .bazelrc[23-35]

### Suggested fix
Update the test to avoid `Stream.toList()` (e.g., `either.stream().collect(Collectors.toList())`) and add the necessary `Collectors` import, or use `Collectors.toUnmodifiableList()` if you want closer behavior to `Stream.toList()` while still staying within Java 11.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. iterator()/stream() missing Javadoc 📘 Rule violation ✧ Quality

Description

The modified public methods iterator() and stream() have no Javadoc blocks, so they do not
document purpose or include required @return tags. This can lead to undocumented public behavior
and violates the project’s public-method Javadoc requirements.

Code

java/src/org/openqa/selenium/internal/Either.java[R71-78]

  @Override
  public Iterator<B> iterator() {
-    return Collections.singleton(right()).iterator();
+    return isRight() ? Collections.singleton(right()).iterator() : Collections.emptyIterator();
  }

  public Stream<B> stream() {
-    return Stream.of(right());
+    return isRight() ? Stream.of(right()) : Stream.empty();
  }

Evidence

PR Compliance ID 330201 requires complete Javadoc on changed public methods (including a descriptive
sentence and @return for non-void methods). The changed methods at Either.iterator() and
Either.stream() have no preceding /** ... */ Javadoc blocks.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/internal/Either.java[71-78]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The modified public methods `iterator()` and `stream()` lack Javadoc. Per compliance, public methods must have Javadoc including at least one descriptive sentence and a non-empty `@return` tag.

## Issue Context
These methods are `public` and were changed in this PR, so they must meet the public-method Javadoc completeness requirements.

## Fix Focus Areas
- java/src/org/openqa/selenium/internal/Either.java[71-78]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Test not in Bazel suite 🐞 Bug ☼ Reliability

Description

EitherTest is added under java/test/org/openqa/selenium/internal, but the Bazel test suites in
java/test/org/openqa/selenium/BUILD.bazel only include tests from that directory (explicit list +
non-recursive glob), so this new test file is not executed under those suites.

Code

java/test/org/openqa/selenium/internal/EitherTest.java[R18-57]

+package org.openqa.selenium.internal;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Iterator;
+import java.util.List;
+import org.junit.jupiter.api.Test;
+
+class EitherTest {
+
+  @Test
+  void streamOnLeftValueShouldBeEmpty() {
+    Either<String, Integer> either = Either.left("error");
+    List<Integer> result = either.stream().toList();
+    assertThat(result).isEmpty();
+  }
+
+  @Test
+  void streamOnRightValueShouldContainRight() {
+    Either<String, Integer> either = Either.right(42);
+    List<Integer> result = either.stream().toList();
+    assertThat(result).containsExactly(42);
+  }
+
+  @Test
+  void iteratorOnLeftValueShouldBeEmpty() {
+    Either<String, Integer> either = Either.left("error");
+    Iterator<Integer> it = either.iterator();
+    assertThat(it.hasNext()).isFalse();
+  }
+
+  @Test
+  void iteratorOnRightValueShouldContainRight() {
+    Either<String, Integer> either = Either.right(42);
+    Iterator<Integer> it = either.iterator();
+    assertThat(it.hasNext()).isTrue();
+    assertThat(it.next()).isEqualTo(42);
+    assertThat(it.hasNext()).isFalse();
+  }
+}

Evidence

The new test class lives in the internal/ subdirectory, but the Bazel suites only include explicitly
listed files and glob(["*Test.java"]) from the parent directory, which does not recurse into
internal/; therefore EitherTest is not part of these suites.

java/test/org/openqa/selenium/internal/EitherTest.java[18-57]
java/test/org/openqa/selenium/BUILD.bazel[5-40]
java/test/org/openqa/selenium/BUILD.bazel[60-67]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`java/test/org/openqa/selenium/internal/EitherTest.java` is added, but it is not included in any Bazel test target, so it will not be compiled/executed when running the existing `SmallTests`/`LargeTests` suites.

## Issue Context
`java/test/org/openqa/selenium/BUILD.bazel` only enumerates root-level tests (explicit `SMALL_TESTS` list and `glob(["*Test.java"])`), which does not match tests in the `internal/` subdirectory.

## Fix Focus Areas
- java/test/org/openqa/selenium/BUILD.bazel[5-67]
- java/test/org/openqa/selenium/internal/EitherTest.java[18-57]

## Suggested fix
Choose one:
1) Add a new `java/test/org/openqa/selenium/internal/BUILD.bazel` defining a `java_test_suite` (or equivalent macro used elsewhere) for `*Test.java` in that package.
2) Alternatively, extend an existing suite to include `internal/*Test.java` (e.g., add those files to `SMALL_TESTS`, or switch the suite glob to include subdirectories where appropriate, such as `glob(["**/*Test.java"])` with careful exclusions to avoid pulling in unintended tests).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Null right misclassified 🐞 Bug ≡ Correctness

Description

Either.iterator() and Either.stream() now return empty whenever isRight() is false, but
isRight() is defined as right != null while Either.right(B) accepts a nullable B. As a
result, Either.right(null) becomes indistinguishable from a left-valued Either for
iteration/streaming, which is a behavioral regression vs the previous Stream.of(right()) /
singleton(right()) behavior.

Code

java/src/org/openqa/selenium/internal/Either.java[R73-77]

+    return isRight() ? Collections.singleton(right()).iterator() : Collections.emptyIterator();
  }

  public Stream<B> stream() {
-    return Stream.of(right());
+    return isRight() ? Stream.of(right()) : Stream.empty();

Evidence

Either.right(B) stores the provided value without null validation, isRight() uses `right !=
null to determine the side, and the updated iterator()/stream() use isRight()` to decide
emptiness—so a constructed Either.right(null) will now produce an empty iterator/stream and report
isRight()==false.

java/src/org/openqa/selenium/internal/Either.java[28-52]
java/src/org/openqa/selenium/internal/Either.java[71-78]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`iterator()`/`stream()` now rely on `isRight()` (implemented as `right != null`). Because `Either.right(B)` currently allows `b` to be null (and the type parameter is declared as `B extends @Nullable Object`), `Either.right(null)` is treated as not-right and yields an empty iterator/stream.

## Issue Context
Decide and enforce one of these contracts:
1) **Null right/left values are not allowed** (simplest): validate inputs in `Either.left(...)` / `Either.right(...)` with `Require.nonNull(...)` and consider removing the `extends @Nullable Object` bounds.
2) **Null right/left values are allowed**: store an explicit discriminator (e.g., `boolean isRight`) rather than inferring the side from nullness, and update `isLeft()/isRight()/toString()/iterator()/stream()` to use it.

## Fix Focus Areas
- java/src/org/openqa/selenium/internal/Either.java[28-83]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 6e84dee

[diemol]

Thanks for the fix. The Either changes are correct, and the tests are well-written.

But I believe these tests are not running in our CI, I could not find a BUILD.bazel that has them configured. Would it be possible for you to add a new BUILD.bazel or to add the tests to the BUILD.bazel that lives in the parent directory?

[vasiliy-mikhailov]

Thanks @diemol! Added java/test/org/openqa/selenium/internal/BUILD.bazel — a small java_test_suite that globs the *Test.java files in that package. That wires up EitherTest, and it turns out MapsTest/MultimapTest/SetsTest weren't running either (the directory had no BUILD.bazel), so this picks those up too. I mirrored the pattern used in sibling test packages; happy to move it into the parent BUILD.bazel instead if you'd prefer.

[qodo-code-review]

Code review by qodo was updated up to the latest commit c93811d

[qodo-code-review]

Code review by qodo was updated up to the latest commit f6410d8

[qodo-code-review]

Code review by qodo was updated up to the latest commit 266131d

[qodo-code-review]

Code review by qodo was updated up to the latest commit fb8ed8b

[diemol]

Thank you