Skip to content

Conversation

@mk868
Copy link
Contributor

@mk868 mk868 commented Oct 22, 2025

User description

🔗 Related Issues

Related #14291

💥 What does this PR do?

JSpecify annotations added to the:

  • org.openqa.selenium.remote.HttpSessionId
  • org.openqa.selenium.remote.Response
  • org.openqa.selenium.remote.SessionId

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

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

PR Type

Enhancement


Description

  • Add JSpecify @NullMarked annotation to three remote classes

  • Mark nullable fields and parameters with @Nullable annotation

  • Improve null-safety documentation for Response, SessionId, HttpSessionId


Diagram Walkthrough

flowchart LR
  A["Remote Classes<br/>Response, SessionId,<br/>HttpSessionId"] -->|"Add @NullMarked"| B["Class-level<br/>Null Safety"]
  A -->|"Add @Nullable"| C["Field & Parameter<br/>Annotations"]
  B --> D["Improved Type<br/>Safety & IDE Support"]
  C --> D
Loading

File Walkthrough

Relevant files
Enhancement
HttpSessionId.java
Add JSpecify null-safety annotation                                           

java/src/org/openqa/selenium/remote/HttpSessionId.java

  • Added @NullMarked class-level annotation
  • Imported org.jspecify.annotations.NullMarked
+2/-0     
Response.java
Add comprehensive null-safety annotations                               

java/src/org/openqa/selenium/remote/Response.java

  • Added @NullMarked class-level annotation
  • Marked four volatile fields as @Nullable: value, sessionId, status,
    state
  • Annotated method parameters and return types with @Nullable for
    getters, setters, and equals() method
  • Imported org.jspecify.annotations.NullMarked and Nullable
+16/-13 
SessionId.java
Add JSpecify null-safety annotations                                         

java/src/org/openqa/selenium/remote/SessionId.java

  • Added @NullMarked class-level annotation
  • Annotated equals() method parameter with @Nullable
  • Imported org.jspecify.annotations.NullMarked and Nullable
+4/-1     

@selenium-ci selenium-ci added the C-java Java Bindings label Oct 22, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 22, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Possible logic risk

Description: equals allows a null argument (annotated @nullable) but does not override hashCode
changes—this is fine—however, no direct security impact is introduced; marking as possible
only if external equality checks rely on nullable fields causing subtle logic; overall no
meaningful exploit evident.
Response.java [82-88]

Referred Code
public boolean equals(@Nullable Object o) {
  if (!(o instanceof Response)) {
    return false;
  }

  Response that = (Response) o;
  return Objects.equals(value, that.value)
Ticket Compliance
🟡
🎫 #14291
🟢 Add JSpecify Nullness annotations to Selenium’s Java code to indicate which parameters and
return values can be null.
Prefer class-level @NullMarked where appropriate and annotate nullable fields, parameters,
and return types with @nullable.
Improve IDE and static analyzer support for null-safety, helping avoid
NullPointerExceptions.
Maintain compatibility and improve interoperability with Kotlin through explicit nullness.
Verify project-wide build integration for JSpecify annotations (dependency presence,
module configurations) and ensure no annotation processing issues in all Java modules.
Validate that public API changes are acceptable to downstream users and tools (e.g., no
breaking behavior in generated docs or warning policies).
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 22, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Improve equals method with explicit checks

Refactor the equals method in SessionId to use a more robust implementation with
explicit null and class equality checks.

java/src/org/openqa/selenium/remote/SessionId.java [51-54]

 @Override
 public boolean equals(@Nullable Object obj) {
-  return obj instanceof SessionId && opaqueKey.equals(((SessionId) obj).opaqueKey);
+  if (this == obj) {
+    return true;
+  }
+  if (obj == null || getClass() != obj.getClass()) {
+    return false;
+  }
+  SessionId that = (SessionId) obj;
+  return opaqueKey.equals(that.opaqueKey);
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion provides a more robust and conventional implementation for the equals method, improving code quality and adherence to Java best practices, although the reasoning about a potential ClassCastException is incorrect.

Low
Learned
best practice
Guard against null constructor input

Since sessionId is now nullable, validate the constructor argument and set a
non-null value or throw if invalid to avoid propagating nulls.

java/src/org/openqa/selenium/remote/Response.java [37-39]

 public Response(SessionId sessionId) {
+  if (sessionId == null) {
+    throw new IllegalArgumentException("sessionId must not be null");
+  }
   this.sessionId = String.valueOf(sessionId);
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs and states early to prevent null-related logic errors by enforcing non-null where appropriate or guarding nullable fields when exposed or used.

Low
  • Update

@diemol diemol merged commit 792df95 into SeleniumHQ:trunk Oct 23, 2025
38 checks passed
@mk868 mk868 deleted the jspecify-remote branch October 23, 2025 15:21
This was referenced Oct 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants