Skip to content

mark JsonTypeCoercer.coerce as nullable - #17802

Draft
asolntsev wants to merge 2 commits into
SeleniumHQ:trunkfrom
asolntsev:nullability
Draft

mark JsonTypeCoercer.coerce as nullable#17802
asolntsev wants to merge 2 commits into
SeleniumHQ:trunkfrom
asolntsev:nullability

Conversation

@asolntsev

Copy link
Copy Markdown
Contributor

de-facto it can return null. Now IDEA will know it, and show warning for unsafe usages.

🔗 Related Issues

Fixes #14291

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s):
    • What was generated:
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Bug fix (backwards compatible)

@selenium-ci selenium-ci added the C-java Java Bindings label Jul 20, 2026
@asolntsev asolntsev self-assigned this Jul 20, 2026
@asolntsev asolntsev added this to the 4.47.0 milestone Jul 20, 2026
@qodo-code-review

qodo-code-review Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Mark JsonTypeCoercer.coerce() as @nullable and harden callers

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Annotate JSON coercion APIs as @Nullable to match runtime null handling.
• Update collection/map/object coercers to accept nullable values and assert non-null keys.
• Add tests for top-level and array-element JSON null deserialization behavior.
Diagram

graph TD
  A["JsonInput"] --> B["JsonTypeCoercer.coerce()"] --> C["TypeCoercer.apply()"]
  C --> D["CollectionCoercer"] --> A
  C --> E["MapCoercer"] --> A
  C --> F["ObjectCoercer"] --> B
  C --> G["Constructor/Instance"] --> B
  E --> H[("Map instance")]
  D --> I[("Collection instance")]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Add a coerceNonNull(...) wrapper API
  • ➕ Keeps most call sites non-null by default; fewer new warnings for existing code.
  • ➕ Encodes intent at call sites (nullable vs non-null) explicitly.
  • ➖ Still requires auditing call sites to choose the right variant.
  • ➖ Adds another public-ish API surface to maintain and document.
2. Return Optional from coerce(...)
  • ➕ Forces callers to handle missing/null values explicitly without annotations.
  • ➖ High churn across all coercers and callers; not idiomatic for container element nulls.
  • ➖ Awkward for performance-sensitive streaming code; lots of Optional allocations.

Recommendation: The chosen approach (jspecify @nullable on coerce() and propagation through container/writer types) is the best incremental fix: it matches existing runtime behavior (JSON null → Java null) while improving IDE/static-analysis feedback. Adding coerceNonNull() could be a follow-up convenience, but isn’t necessary for correctness given readNonNull()/Require.nonNull usages already exist.

Files changed (11) +56 / -29

Bug fix (8) +33 / -24
CollectionCoercer.javaAllow nullable array elements when coercing collections +5/-4

Allow nullable array elements when coercing collections

• Updates generics to use Collection<?> and propagates @Nullable through the consumer factory and local consumer variable. This reflects that element coercion can produce null when the JSON contains null.

java/src/org/openqa/selenium/json/CollectionCoercer.java

ConstructorCoercer.javaAssert non-null constructor property map; mark coerceValue nullable +4/-1

Assert non-null constructor property map; mark coerceValue nullable

• Wraps map coercion with Require.nonNull to avoid propagating a null properties map into constructor selection. Annotates coerceValue(...) as @Nullable to match the underlying coercer behavior.

java/src/org/openqa/selenium/json/ConstructorCoercer.java

InstanceCoercer.javaAllow nullable values when writing fields/setters during deserialization +7/-6

Allow nullable values when writing fields/setters during deserialization

• Propagates @Nullable through TypeAndWriter and writer implementations (FieldWriter and SimplePropertyWriter). This matches that coerced JSON values may be null and avoids unsafe nullness assumptions.

java/src/org/openqa/selenium/json/InstanceCoercer.java

JsonInput.javaModel readArray() results as possibly containing null elements +2/-2

Model readArray() results as possibly containing null elements

• Updates readArray(Type) to return List<@Nullable T> and store @Nullable elements, matching actual behavior when encountering JSON nulls. Keeps readNonNull/readMap helpers for non-null guarantees at call sites.

java/src/org/openqa/selenium/json/JsonInput.java

JsonTypeCoercer.javaAnnotate coerce() as @Nullable and remove obsolete unchecked suppressions +2/-4

Annotate coerce() as @nullable and remove obsolete unchecked suppressions

• Marks JsonTypeCoercer.coerce(...) as @Nullable since it can return null for JSON null (outside Optional). Cleans up container coercer registration now that generics are tightened elsewhere.

java/src/org/openqa/selenium/json/JsonTypeCoercer.java

MapCoercer.javaAllow nullable map values and enforce non-null map keys when coerced +7/-4

Allow nullable map values and enforce non-null map keys when coerced

• Propagates @Nullable through the consumer factory for map values. Adds requireNonNull around coerced non-string keys to prevent inserting null keys into the target map.

java/src/org/openqa/selenium/json/MapCoercer.java

ObjectCoercer.javaMake ObjectCoercer’s apply() nullable-aware +2/-1

Make ObjectCoercer’s apply() nullable-aware

• Changes the produced BiFunction to return @Nullable Object, aligning with JsonTypeCoercer.coerce(...) and the possibility of JSON null at runtime.

java/src/org/openqa/selenium/json/ObjectCoercer.java

TypeCoercer.javaAllow TypeCoercer.apply() to produce nullable results +4/-2

Allow TypeCoercer.apply() to produce nullable results

• Updates the TypeCoercer functional signature to return BiFunction<..., @Nullable T>, enabling coercers to correctly model JSON null handling in their types.

java/src/org/openqa/selenium/json/TypeCoercer.java

Refactor (1) +1 / -1
EnumCoercer.javaTighten enum generic bound for type safety +1/-1

Tighten enum generic bound for type safety

• Changes the type parameter to T extends Enum<T>, aligning with standard Enum generic constraints and reducing raw-type warnings.

java/src/org/openqa/selenium/json/EnumCoercer.java

Tests (2) +22 / -4
JsonInputTest.javaAdjust tests to new non-null helpers; add null array element coverage +15/-4

Adjust tests to new non-null helpers; add null array element coverage

• Switches map/object reads to readMap() and readNonNull(...) where tests require non-null. Adds a new test asserting readArray(Integer.class) can return a list containing nulls.

java/test/org/openqa/selenium/json/JsonInputTest.java

JsonTest.javaAdd regression test: toType returns null for top-level JSON null +7/-0

Add regression test: toType returns null for top-level JSON null

• Adds a test ensuring Json.toType("null", String.class) returns null, validating and documenting the nullable coercion contract.

java/test/org/openqa/selenium/json/JsonTest.java

@qodo-code-review

qodo-code-review Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

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

Grey Divider


Action required

1. TypeCoercer nullness contract ✓ Resolved 🐞 Bug ≡ Correctness
Description
TypeCoercer still implements Function<Type, BiFunction<..., T>> (non-null result), but its
apply now returns BiFunction<..., @Nullable T>, creating a contradictory nullness contract for
JSpecify/IDE tooling. This undermines the PR’s goal by letting callers treat coercers as producing
non-null values when they are explicitly nullable.
Code

java/src/org/openqa/selenium/json/TypeCoercer.java[R26-27]

public abstract class TypeCoercer<T>
    implements Predicate<Class<?>>, Function<Type, BiFunction<JsonInput, PropertySetting, T>> {
Evidence
TypeCoercer’s declared supertype promises a BiFunction producing non-null T, while the
abstract apply now explicitly produces @Nullable T, which is inconsistent under nullness-aware
tooling.

java/src/org/openqa/selenium/json/TypeCoercer.java[26-34]

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

### Issue description
`TypeCoercer` implements `Function<Type, BiFunction<JsonInput, PropertySetting, T>>` but its `apply` method now returns `BiFunction<JsonInput, PropertySetting, @Nullable T>`. With JSpecify-aware nullness tools, this is an incompatible/contradictory contract.

### Issue Context
The PR is introducing nullability metadata; leaving `TypeCoercer`’s implemented `Function` signature non-null defeats or breaks the intended propagation.

### Fix Focus Areas
- java/src/org/openqa/selenium/json/TypeCoercer.java[26-34]

### Expected fix
Update the implemented `Function` type argument to match the new nullable return:
- `Function<Type, BiFunction<JsonInput, PropertySetting, @Nullable T>>`
and adjust any resulting type-checker fallout accordingly.

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


2. EnumCoercer missing class Javadoc ✗ Dismissed 📘 Rule violation ✧ Quality
Description
The modified public class EnumCoercer has no Javadoc comment immediately preceding its
declaration. This violates the requirement that modified public API types include Javadoc and makes
the public API harder to understand and maintain.
Code

java/src/org/openqa/selenium/json/EnumCoercer.java[25]

+public class EnumCoercer<T extends Enum<T>> extends TypeCoercer<T> {
Evidence
The compliance rule requires Javadoc for all modified public API types. EnumCoercer is declared
public and its declaration was changed in this PR, but there is no /** ... */ Javadoc block
immediately above the class declaration.

Rule 330200: Require Javadoc for all public API types and methods
java/src/org/openqa/selenium/json/EnumCoercer.java[20-26]

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

## Issue description
`EnumCoercer` is a public API type modified in this PR, but it has no class-level Javadoc immediately above the class declaration.

## Issue Context
This PR changes the generic bound to `T extends Enum<T>`, which is a user-visible API refinement and should be documented.

## Fix Focus Areas
- java/src/org/openqa/selenium/json/EnumCoercer.java[25-25]

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


3. Nullable coerce returned as nonnull ✓ Resolved 🐞 Bug ≡ Correctness
Description
JsonTypeCoercer.coerce is now @Nullable, but core APIs still return it as non-null (e.g.,
Json#toType) or dereference it without a non-null assertion (e.g., ConstructorCoercer uses
properties.keySet()), which will trigger nullness-checker errors and hides real nullable behavior
(JSON null -> Java null).
Code

java/src/org/openqa/selenium/json/JsonTypeCoercer.java[R140-143]

+  @Nullable <T> T coerce(JsonInput json, Type typeOfT, PropertySetting setter) {
    BiFunction<JsonInput, PropertySetting, Object> coercer =
        knownCoercers.computeIfAbsent(typeOfT, this::buildCoercer);
Evidence
coerce is explicitly nullable and returns null on JsonType.NULL; Json#toType returns that
value as non-null T, and ConstructorCoercer stores it into a non-null Map and immediately
dereferences it, both of which become invalid under the new nullability contract.

java/src/org/openqa/selenium/json/JsonTypeCoercer.java[140-155]
java/src/org/openqa/selenium/json/Json.java[209-214]
java/src/org/openqa/selenium/json/ConstructorCoercer.java[57-66]

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

### Issue description
`JsonTypeCoercer.coerce` is now annotated `@Nullable`, but callers like `Json#toType` return the result as non-null and `ConstructorCoercer` dereferences the result without a non-null assertion. This creates inconsistent nullness contracts and will cause IDE/JSpecify warnings (and can propagate unexpected nulls at API boundaries).

### Issue Context
Runtime behavior already allows `coerce` to return null when the JSON token is `null` (for non-Optional targets). The PR now exposes this via annotations, so callers must either:
- accept/annotate nullable returns, or
- enforce non-null with `Require.nonNull(...)` / `Objects.requireNonNull(...)` and throw a domain exception.

### Fix Focus Areas
- java/src/org/openqa/selenium/json/JsonTypeCoercer.java[140-155]
- java/src/org/openqa/selenium/json/Json.java[209-214]
- java/src/org/openqa/selenium/json/ConstructorCoercer.java[57-66]

### Expected fix
Pick one consistent policy per API:
- For `Json#toType(...)`: either change return type to `@Nullable <T> T` (most accurate), or wrap with `Require.nonNull` and throw `JsonException` when JSON is `null`.
- For `ConstructorCoercer`: assert non-null for `properties` (e.g., `Require.nonNull("properties", ...)`) before `properties.keySet()` since this coercer is only invoked for non-null JSON objects.

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


View high (1)
4. TypeCoercer.apply missing Javadoc ✗ Dismissed 📘 Rule violation ✧ Quality
Description
The modified public API method TypeCoercer.apply(Type) has no Javadoc block, including required
free-text description and @param/@return tags. This violates the requirement for complete
Javadoc on changed public methods and reduces API clarity for users and implementers.
Code

java/src/org/openqa/selenium/json/TypeCoercer.java[33]

+  public abstract BiFunction<JsonInput, PropertySetting, @Nullable T> apply(Type type);
Evidence
The compliance rule requires complete Javadoc for each changed public method. In TypeCoercer.java,
the modified public abstract BiFunction<JsonInput, PropertySetting, @Nullable T> apply(Type type);
has no preceding Javadoc block and therefore lacks the required descriptive sentence and tags.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/json/TypeCoercer.java[26-34]

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

## Issue description
`TypeCoercer.apply(Type)` is a changed public method but has no Javadoc. The compliance rule requires a Javadoc block with at least one descriptive sentence plus complete `@param` and `@return` tags.

## Issue Context
This PR introduces `@Nullable` in the return type of `apply`, making correct documentation more important for API consumers and implementers.

## Fix Focus Areas
- java/src/org/openqa/selenium/json/TypeCoercer.java[26-34]

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



Remediation recommended

5. Nullable assigned to nonnull 🐞 Bug ⚙ Maintainability ⭐ New
Description
In a @NullMarked package, the new test assigns a value it expects to be null into a non-null
String, contradicting the nullness contract and triggering nullness-checker/IDE warnings in the
test itself.
Code

java/test/org/openqa/selenium/json/JsonTest.java[R171-174]

+  void toTypeReturnsNullForTopLevelJsonNull() {
+    String text = new Json().toType("null", String.class);
+
+    assertThat(text).isNull();
Evidence
The package is explicitly @NullMarked (so unannotated String is non-null by default), yet the
test stores and asserts a null value in that variable.

java/src/org/openqa/selenium/json/package-info.java[18-21]
java/test/org/openqa/selenium/json/JsonTest.java[170-175]

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

### Issue description
`JsonTest.toTypeReturnsNullForTopLevelJsonNull` assigns a value it asserts is `null` into a non-null `String` within the `org.openqa.selenium.json` package, which is `@NullMarked`.

### Issue Context
This PR’s goal is to surface nullability correctly; tests should follow the same contract to avoid introducing warnings/errors into the build.

### Fix Focus Areas
- java/test/org/openqa/selenium/json/JsonTest.java[170-175]
- java/src/org/openqa/selenium/json/package-info.java[18-21]

### Suggested change
Change `String text = ...` to `@Nullable String text = ...` (or equivalent) so the test matches the intended nullability.

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


6. Nullable param typed nonnull 🐞 Bug ≡ Correctness ⭐ New
Description
ConstructorCoercer.coerceValue is now annotated as returning @Nullable, but its value
parameter remains non-null even though callers pass properties.get(...) which can be null,
creating a nullness-contract violation and warnings under @NullMarked.
Code

java/src/org/openqa/selenium/json/ConstructorCoercer.java[R233-236]

+  @Nullable
  private Object coerceValue(Object value, Type type, PropertySetting setting) {
    StringWriter rawJson = new StringWriter();
    try (JsonOutput output = new JsonOutput(rawJson)) {
Evidence
The method is declared @Nullable on return but still takes a non-null Object value; the caller
passes properties.get(...), and the subsequent logic explicitly allows value == null, proving
null is a valid input path.

java/src/org/openqa/selenium/json/ConstructorCoercer.java[233-242]
java/src/org/openqa/selenium/json/ConstructorCoercer.java[274-293]
java/src/org/openqa/selenium/json/package-info.java[18-21]

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

### Issue description
`coerceValue` can be invoked with `null` (when the JSON map contains a key with a `null` value), but its signature requires a non-null `Object value`.

### Issue Context
This is in `org.openqa.selenium.json` which is `@NullMarked`, so unannotated parameters are non-null by default. The body already handles `null` correctly by writing it as JSON and re-coercing.

### Fix Focus Areas
- java/src/org/openqa/selenium/json/ConstructorCoercer.java[233-242]
- java/src/org/openqa/selenium/json/ConstructorCoercer.java[285-293]

### Suggested change
Update the signature to `private Object coerceValue(@Nullable Object value, Type type, PropertySetting setting)` (keeping the existing `@Nullable` return annotation).

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


7. Tests use nonnull List ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
JsonInput.readArray now returns List<@Nullable T>, but JsonInputTest assigns it to
List<Integer> and even asserts null elements, which conflicts with the new nullness contract and
will be flagged by IDE/nullness tooling.
Code

java/test/org/openqa/selenium/json/JsonInputTest.java[R266-275]

+  @Test
+  void canReadListOfType_null() {
+    String raw = "[null, null]";
+
+    try (JsonInput in = new JsonInput(new StringReader(raw), new JsonTypeCoercer(), BY_NAME)) {
+      List<Integer> array = in.readArray(Integer.class);
+
+      assertThat(array).containsExactly(null, null);
+    }
+  }
Evidence
The production signature now explicitly allows null elements in the returned list, while the
modified test code still uses a non-null element type and verifies nulls, which is precisely the
unsafe usage the PR aims to surface.

java/src/org/openqa/selenium/json/JsonInput.java[543-552]
java/test/org/openqa/selenium/json/JsonInputTest.java[255-275]

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

### Issue description
`JsonInput.readArray(Type)` now returns a list whose elements may be null (`List<@Nullable T>`). Tests currently store that into `List<Integer>` and assert nulls, which is inconsistent with the new type contract.

### Issue Context
This PR’s goal is to make IDEA warn on unsafe usages; test code should model the correct (nullable) types to avoid nullness-checker failures.

### Fix Focus Areas
- java/src/org/openqa/selenium/json/JsonInput.java[543-552]
- java/test/org/openqa/selenium/json/JsonInputTest.java[255-275]

### Expected fix
Update tests to use nullable element types, e.g.:
- `List<@Nullable Integer> array = in.readArray(Integer.class);`
(and add `import org.jspecify.annotations.Nullable;`). Consider doing this for both `canReadListOfType` and `canReadListOfType_null`.

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


Grey Divider

Context sources
✅ Compliance rules (platform): 18 rules

Grey Divider

Tip of the day
💡 Did you know, you can turn these tips off under Display preferences

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit 50d4a85 ⚖️ Balanced

Results up to commit 43226dc ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. EnumCoercer missing class Javadoc ✗ Dismissed 📘 Rule violation ✧ Quality
Description
The modified public class EnumCoercer has no Javadoc comment immediately preceding its
declaration. This violates the requirement that modified public API types include Javadoc and makes
the public API harder to understand and maintain.
Code

java/src/org/openqa/selenium/json/EnumCoercer.java[25]

+public class EnumCoercer<T extends Enum<T>> extends TypeCoercer<T> {
Evidence
The compliance rule requires Javadoc for all modified public API types. EnumCoercer is declared
public and its declaration was changed in this PR, but there is no /** ... */ Javadoc block
immediately above the class declaration.

Rule 330200: Require Javadoc for all public API types and methods
java/src/org/openqa/selenium/json/EnumCoercer.java[20-26]

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

## Issue description
`EnumCoercer` is a public API type modified in this PR, but it has no class-level Javadoc immediately above the class declaration.

## Issue Context
This PR changes the generic bound to `T extends Enum<T>`, which is a user-visible API refinement and should be documented.

## Fix Focus Areas
- java/src/org/openqa/selenium/json/EnumCoercer.java[25-25]

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


2. TypeCoercer.apply missing Javadoc ✗ Dismissed 📘 Rule violation ✧ Quality
Description
The modified public API method TypeCoercer.apply(Type) has no Javadoc block, including required
free-text description and @param/@return tags. This violates the requirement for complete
Javadoc on changed public methods and reduces API clarity for users and implementers.
Code

java/src/org/openqa/selenium/json/TypeCoercer.java[33]

+  public abstract BiFunction<JsonInput, PropertySetting, @Nullable T> apply(Type type);
Evidence
The compliance rule requires complete Javadoc for each changed public method. In TypeCoercer.java,
the modified public abstract BiFunction<JsonInput, PropertySetting, @Nullable T> apply(Type type);
has no preceding Javadoc block and therefore lacks the required descriptive sentence and tags.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/json/TypeCoercer.java[26-34]

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

## Issue description
`TypeCoercer.apply(Type)` is a changed public method but has no Javadoc. The compliance rule requires a Javadoc block with at least one descriptive sentence plus complete `@param` and `@return` tags.

## Issue Context
This PR introduces `@Nullable` in the return type of `apply`, making correct documentation more important for API consumers and implementers.

## Fix Focus Areas
- java/src/org/openqa/selenium/json/TypeCoercer.java[26-34]

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


3. Nullable coerce returned as nonnull ✓ Resolved 🐞 Bug ≡ Correctness
Description
JsonTypeCoercer.coerce is now @Nullable, but core APIs still return it as non-null (e.g.,
Json#toType) or dereference it without a non-null assertion (e.g., ConstructorCoercer uses
properties.keySet()), which will trigger nullness-checker errors and hides real nullable behavior
(JSON null -> Java null).
Code

java/src/org/openqa/selenium/json/JsonTypeCoercer.java[R140-143]

+  @Nullable <T> T coerce(JsonInput json, Type typeOfT, PropertySetting setter) {
    BiFunction<JsonInput, PropertySetting, Object> coercer =
        knownCoercers.computeIfAbsent(typeOfT, this::buildCoercer);
Evidence
coerce is explicitly nullable and returns null on JsonType.NULL; Json#toType returns that
value as non-null T, and ConstructorCoercer stores it into a non-null Map and immediately
dereferences it, both of which become invalid under the new nullability contract.

java/src/org/openqa/selenium/json/JsonTypeCoercer.java[140-155]
java/src/org/openqa/selenium/json/Json.java[209-214]
java/src/org/openqa/selenium/json/ConstructorCoercer.java[57-66]

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

### Issue description
`JsonTypeCoercer.coerce` is now annotated `@Nullable`, but callers like `Json#toType` return the result as non-null and `ConstructorCoercer` dereferences the result without a non-null assertion. This creates inconsistent nullness contracts and will cause IDE/JSpecify warnings (and can propagate unexpected nulls at API boundaries).

### Issue Context
Runtime behavior already allows `coerce` to return null when the JSON token is `null` (for non-Optional targets). The PR now exposes this via annotations, so callers must either:
- accept/annotate nullable returns, or
- enforce non-null with `Require.nonNull(...)` / `Objects.requireNonNull(...)` and throw a domain exception.

### Fix Focus Areas
- java/src/org/openqa/selenium/json/JsonTypeCoercer.java[140-155]
- java/src/org/openqa/selenium/json/Json.java[209-214]
- java/src/org/openqa/selenium/json/ConstructorCoercer.java[57-66]

### Expected fix
Pick one consistent policy per API:
- For `Json#toType(...)`: either change return type to `@Nullable <T> T` (most accurate), or wrap with `Require.nonNull` and throw `JsonException` when JSON is `null`.
- For `ConstructorCoercer`: assert non-null for `properties` (e.g., `Require.nonNull("properties", ...)`) before `properties.keySet()` since this coercer is only invoked for non-null JSON objects.

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


View high (1)
4. TypeCoercer nullness contract ✓ Resolved 🐞 Bug ≡ Correctness
Description
TypeCoercer still implements Function<Type, BiFunction<..., T>> (non-null result), but its
apply now returns BiFunction<..., @Nullable T>, creating a contradictory nullness contract for
JSpecify/IDE tooling. This undermines the PR’s goal by letting callers treat coercers as producing
non-null values when they are explicitly nullable.
Code

java/src/org/openqa/selenium/json/TypeCoercer.java[R26-27]

public abstract class TypeCoercer<T>
    implements Predicate<Class<?>>, Function<Type, BiFunction<JsonInput, PropertySetting, T>> {
Evidence
TypeCoercer’s declared supertype promises a BiFunction producing non-null T, while the
abstract apply now explicitly produces @Nullable T, which is inconsistent under nullness-aware
tooling.

java/src/org/openqa/selenium/json/TypeCoercer.java[26-34]

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

### Issue description
`TypeCoercer` implements `Function<Type, BiFunction<JsonInput, PropertySetting, T>>` but its `apply` method now returns `BiFunction<JsonInput, PropertySetting, @Nullable T>`. With JSpecify-aware nullness tools, this is an incompatible/contradictory contract.

### Issue Context
The PR is introducing nullability metadata; leaving `TypeCoercer`’s implemented `Function` signature non-null defeats or breaks the intended propagation.

### Fix Focus Areas
- java/src/org/openqa/selenium/json/TypeCoercer.java[26-34]

### Expected fix
Update the implemented `Function` type argument to match the new nullable return:
- `Function<Type, BiFunction<JsonInput, PropertySetting, @Nullable T>>`
and adjust any resulting type-checker fallout accordingly.

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



Remediation recommended
5. Tests use nonnull List ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
JsonInput.readArray now returns List<@Nullable T>, but JsonInputTest assigns it to
List<Integer> and even asserts null elements, which conflicts with the new nullness contract and
will be flagged by IDE/nullness tooling.
Code

java/test/org/openqa/selenium/json/JsonInputTest.java[R266-275]

+  @Test
+  void canReadListOfType_null() {
+    String raw = "[null, null]";
+
+    try (JsonInput in = new JsonInput(new StringReader(raw), new JsonTypeCoercer(), BY_NAME)) {
+      List<Integer> array = in.readArray(Integer.class);
+
+      assertThat(array).containsExactly(null, null);
+    }
+  }
Evidence
The production signature now explicitly allows null elements in the returned list, while the
modified test code still uses a non-null element type and verifies nulls, which is precisely the
unsafe usage the PR aims to surface.

java/src/org/openqa/selenium/json/JsonInput.java[543-552]
java/test/org/openqa/selenium/json/JsonInputTest.java[255-275]

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

### Issue description
`JsonInput.readArray(Type)` now returns a list whose elements may be null (`List<@Nullable T>`). Tests currently store that into `List<Integer>` and assert nulls, which is inconsistent with the new type contract.

### Issue Context
This PR’s goal is to make IDEA warn on unsafe usages; test code should model the correct (nullable) types to avoid nullness-checker failures.

### Fix Focus Areas
- java/src/org/openqa/selenium/json/JsonInput.java[543-552]
- java/test/org/openqa/selenium/json/JsonInputTest.java[255-275]

### Expected fix
Update tests to use nullable element types, e.g.:
- `List<@Nullable Integer> array = in.readArray(Integer.class);`
(and add `import org.jspecify.annotations.Nullable;`). Consider doing this for both `canReadListOfType` and `canReadListOfType_null`.

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


Grey Divider

Qodo Logo

Comment thread java/src/org/openqa/selenium/json/TypeCoercer.java
Comment thread java/src/org/openqa/selenium/json/EnumCoercer.java
Comment thread java/src/org/openqa/selenium/json/TypeCoercer.java
Comment thread java/src/org/openqa/selenium/json/JsonTypeCoercer.java Outdated
Comment thread java/test/org/openqa/selenium/json/JsonInputTest.java
@asolntsev
asolntsev marked this pull request as draft July 20, 2026 09:28
de-facto it can return null. Now IDEA will know it, and show warning for unsafe usages.
@asolntsev
asolntsev marked this pull request as ready for review August 23, 2026 13:37
Comment on lines +171 to +174
void toTypeReturnsNullForTopLevelJsonNull() {
String text = new Json().toType("null", String.class);

assertThat(text).isNull();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remediation recommended

1. Nullable assigned to nonnull 🐞 Bug ⚙ Maintainability

In a @NullMarked package, the new test assigns a value it expects to be null into a non-null
String, contradicting the nullness contract and triggering nullness-checker/IDE warnings in the
test itself.
Agent Prompt
### Issue description
`JsonTest.toTypeReturnsNullForTopLevelJsonNull` assigns a value it asserts is `null` into a non-null `String` within the `org.openqa.selenium.json` package, which is `@NullMarked`.

### Issue Context
This PR’s goal is to surface nullability correctly; tests should follow the same contract to avoid introducing warnings/errors into the build.

### Fix Focus Areas
- java/test/org/openqa/selenium/json/JsonTest.java[170-175]
- java/src/org/openqa/selenium/json/package-info.java[18-21]

### Suggested change
Change `String text = ...` to `@Nullable String text = ...` (or equivalent) so the test matches the intended nullability.

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

Comment on lines +233 to 236
@Nullable
private Object coerceValue(Object value, Type type, PropertySetting setting) {
StringWriter rawJson = new StringWriter();
try (JsonOutput output = new JsonOutput(rawJson)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remediation recommended

2. Nullable param typed nonnull 🐞 Bug ≡ Correctness

ConstructorCoercer.coerceValue is now annotated as returning @Nullable, but its value
parameter remains non-null even though callers pass properties.get(...) which can be null,
creating a nullness-contract violation and warnings under @NullMarked.
Agent Prompt
### Issue description
`coerceValue` can be invoked with `null` (when the JSON map contains a key with a `null` value), but its signature requires a non-null `Object value`.

### Issue Context
This is in `org.openqa.selenium.json` which is `@NullMarked`, so unannotated parameters are non-null by default. The body already handles `null` correctly by writing it as JSON and re-coercing.

### Fix Focus Areas
- java/src/org/openqa/selenium/json/ConstructorCoercer.java[233-242]
- java/src/org/openqa/selenium/json/ConstructorCoercer.java[285-293]

### Suggested change
Update the signature to `private Object coerceValue(@Nullable Object value, Type type, PropertySetting setting)` (keeping the existing `@Nullable` return annotation).

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

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 50d4a85

@asolntsev
asolntsev marked this pull request as draft August 23, 2026 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-java Java Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🚀 Feature]: JSpecify Nullness annotations for Java

2 participants