Skip to content

[java] Support @Nullable constructor parameters in ConstructorCoercer - #17770

Merged
pujagani merged 1 commit into
SeleniumHQ:trunkfrom
pujagani:add-nullable-json-check
Jul 13, 2026
Merged

pujagani merged 1 commit into
SeleniumHQ:trunkfrom
pujagani:add-nullable-json-check

Conversation

@pujagani

@pujagani pujagani commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

🔗 Related Issues

💥 What does this PR do?

ConstructorCoercer previously only permitted a JSON null for constructor parameters typed Optional; any other parameter threw a JsonException when the value was null, even if the key was present and the field is legitimately nullable by whatever it models.

This adds support for marking a required parameter nullable via jspecify's @nullable type annotation (org.jspecify.annotations.Nullable), independent of whether the key itself is required.

🔧 Implementation Notes

🤖 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

💡 Additional Considerations

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

@selenium-ci selenium-ci added C-java Java Bindings B-build Includes scripting, bazel and CI integrations labels Jul 13, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Java: Accept JSON null for @nullable constructor parameters in ConstructorCoercer

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 10-20 Minutes

Grey Divider

AI Description

• Allow JSON null for non-Optional constructor parameters annotated with jspecify @Nullable.
• Preserve existing "required key" behavior: nullable values may be null but must be present.
• Add unit coverage for required+nullable parameters and wire jspecify into the test target.
Diagram

graph TD
  A["Json.toType(...)" ] --> B["ConstructorCoercer" ] --> C["coerceValue(...)" ] --> D{"value == null?"}
  D -- "no" --> E["constructor.newInstance" ]
  D -- "yes" --> F{"isNullable?"}
  F -- "yes" --> E
  F -- "no" --> G["throw JsonException" ]
  H["@Nullable (jspecify)" ] --> F
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Treat all reference-typed parameters as nullable
  • ➕ No dependency on a specific nullability annotation library
  • ➕ Simpler runtime reflection check
  • ➖ Would silently accept unexpected nulls and weaken JSON contract validation
  • ➖ Harder to catch data issues early (current behavior intentionally throws)
2. Support multiple @nullable annotations (e.g., javax/jetbrains)
  • ➕ More flexible for downstream users with different annotation ecosystems
  • ➕ Reduces coupling to jspecify specifically
  • ➖ Adds branching/maintenance for multiple annotation types
  • ➖ Potential ambiguity around TYPE_USE vs PARAMETER targeting semantics

Recommendation: The PR’s approach is the best trade-off: it preserves strictness by default while allowing an explicit, standards-aligned opt-in via jspecify @nullable. Checking both Parameter annotations and the AnnotatedType is appropriate given TYPE_USE targeting, and the added tests cover the key behavioral contract (null allowed when present; still missing-key error when absent).

Files changed (3) +53 / -1

Bug fix (1) +15 / -1
ConstructorCoercer.javaAdd @Nullable-aware null acceptance for constructor parameters +15/-1

Add @Nullable-aware null acceptance for constructor parameters

• Introduces an isNullable(Parameter) helper that treats Optional<T> and jspecify @Nullable parameters as accepting JSON null. Updates instance creation to throw only when a null value is provided for a non-nullable parameter, without changing the existing missing-key validation logic.

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

Tests (1) +37 / -0
ConstructorCoercerTest.javaAdd tests for required @Nullable constructor parameters +37/-0

Add tests for required @nullable constructor parameters

• Adds a new test fixture class with a required @Nullable parameter and verifies JSON null is accepted when the key is present. Adds a regression test ensuring the nullable parameter is still required to be present (missing key continues to fail).

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

Other (1) +1 / -0
BUILD.bazelAdd jspecify dependency to ConstructorCoercer test suite +1/-0

Add jspecify dependency to ConstructorCoercer test suite

• Adds org.jspecify:jspecify to the ConstructorCoercerTests Bazel deps so the new @Nullable-based test compiles.

java/test/org/openqa/selenium/json/BUILD.bazel

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Qodo Logo

@pujagani
pujagani merged commit af2267d into SeleniumHQ:trunk Jul 13, 2026
46 checks passed
This was referenced Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations C-java Java Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants