[java] specify nullability in package org.openqa.selenium.remote#17325
[java] specify nullability in package org.openqa.selenium.remote#17325asolntsev wants to merge 1 commit intoSeleniumHQ:trunkfrom
org.openqa.selenium.remote#17325Conversation
selenium-ci
added
B-grid
|
Thank you, @asolntsev for this code suggestion. The support packages contain example code that many users find helpful, but they do not necessarily represent After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks. |
Review Summary by QodoSpecify nullability in Java bindings using JSpecify annotations
WalkthroughsDescriptionSpecify nullability in the Java bindings using JSpecify annotations * Mark everything as non-nullable by default using @NullMarked at package level across multiple packages * Add selective @Nullable annotations to specific fields, parameters, and return types that can be null * Replace manual null checks with utility methods like Require.nonNull() and requireNonNullElse() * Make DriverService abstract and refine nullability of its fields and methods * Extract PasswordAuthenticator class from JdkHttpClient for better code organization * Add query parameter utility methods to HttpRequest (getQueryString(), forEachQueryParameter(), getQueryParameters()) * Add new getHeader() method to HttpMessage with default value support * Make inner classes final where appropriate (e.g., in Route, UrlTemplate) * Update driver service implementations (Chrome, Firefox, Safari, Edge, IE) to make timeout parameter non-nullable * Add comprehensive test coverage for new functionality (PasswordAuthenticatorTest, HttpRequestTest) * Update multiple test classes with nullability annotations and improve test robustness * Add package-info.java files with @NullMarked annotation to 16 packages * Update BUILD.bazel files to include org.jspecify:jspecify dependency * Minor code cleanups and refactorings to improve null-safety analysis Partially implements #14291 File Changes1. java/src/org/openqa/selenium/remote/RemoteWebDriver.java
|
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Review Summary by QodoSpecify nullability annotations in package
WalkthroughsDescriptionComprehensive nullability annotation implementation across the Java codebase in the org.openqa.selenium.remote package and related modules. **Key Changes:** • Added @NullMarked package-level annotations to establish non-nullable-by-default policy across multiple packages • Systematically added @Nullable annotations to fields, parameters, and return types that can be null • Replaced manual null checks with utility methods (Require.nonNull(), requireNonNull(), requireNonNullElse()) • Made DriverService abstract class with improved nullability handling • Extracted helper classes (PasswordAuthenticator) and methods for better code organization • Refactored query parameter handling with new utility methods (forEachQueryParameter(), getQueryString(), getQueryParameters()) • Added new test files for HttpRequest and PasswordAuthenticator classes • Updated multiple driver service classes to make timeout parameter non-nullable • Changed exception classes to have non-nullable parameters • Added jspecify dependency to build configurations • Minor code cleanups: removed unused imports, made inner classes static/final, fixed JavaDoc typos File Changes1. java/src/org/openqa/selenium/remote/RemoteWebDriver.java
|
Code Review by Qodo
|
| @@ -111,18 +112,7 @@ public class JdkHttpClient implements HttpClient { | |||
| Credentials credentials = config.credentials(); | |||
| String info = config.baseUri().getUserInfo(); | |||
There was a problem hiding this comment.
2. Null baseuri dereference 🐞 Bug ≡ Correctness
JdkHttpClient/JdkHttpMessages dereference config.baseUri() even though ClientConfig.defaultConfig() initializes baseUri to null and baseUri() is annotated @Nullable. Creating an HTTP client with a ClientConfig that hasn’t had baseUri/baseUrl set can throw NullPointerException during client construction or request URI resolution.
Agent Prompt
### Issue description
`ClientConfig.baseUri()` is documented/annotated as nullable and `ClientConfig.defaultConfig()` sets it to `null`, but `JdkHttpClient` and `JdkHttpMessages` dereference it without validation. This creates a runtime NPE hazard for callers that construct an `HttpClient` from a `ClientConfig` that hasn’t been finalized with `.baseUri(...)`/`.baseUrl(...)`.
### Issue Context
The PR is making nullness contracts explicit via `@NullMarked` + `@Nullable`. Once `baseUri()` is nullable, all code paths that dereference it must either:
- enforce that it’s set (fail fast with a clear error), or
- implement a meaningful fallback behavior.
### Fix Focus Areas
- java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java[112-116]
- java/src/org/openqa/selenium/remote/http/jdk/JdkHttpMessages.java[123-140]
- (optional hardening) java/src/org/openqa/selenium/remote/HttpCommandExecutor.java[111-120]
### Suggested fix
1. In `JdkHttpClient` constructor, fetch and validate once, then use the validated value:
- `URI baseUri = Require.nonNull("Base URI", config.baseUri());`
- use `baseUri.getUserInfo()` instead of `config.baseUri().getUserInfo()`.
2. In `JdkHttpMessages.getRawUri`, validate `config.baseUri()` before calling `toString()`:
- `URI baseUrl = Require.nonNull("Base URI", config.baseUri());`
3. (Optional) In `HttpCommandExecutor`’s `(ClientConfig, Factory)` constructor, validate `config.baseUrl()` / `config.baseUri()` so the executor cannot be created in an unusable state, and the failure mode is a clear `IllegalArgumentException` rather than a later NPE.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
asolntsev
force-pushed
the
nullability
branch
2 times, most recently
from
a9b5e20 to
cf1965a
Compare
* Mark everything as non-nullable by default, and * Mark only the needed methods/parameters as nullable. Partially implements SeleniumHQ#14291
2 participants
🔗 Related Issues
Partially implements #14291
🔄 Types of changes