Skip to content

Conversation

@mk868
Copy link
Contributor

@mk868 mk868 commented Oct 21, 2025

User description

🔗 Related Issues

Related #14291

💥 What does this PR do?

JSpecify annotations added to the:

  • org.openqa.selenium.ScriptKey
  • org.openqa.selenium.UnpinnedScriptKey

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

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

PR Type

Enhancement


Description

  • Add JSpecify null-safety annotations to ScriptKey class

  • Add JSpecify null-safety annotations to UnpinnedScriptKey class

  • Mark classes with @NullMarked for comprehensive null-checking

  • Annotate nullable parameters and return types appropriately


Diagram Walkthrough

flowchart LR
  A["ScriptKey and UnpinnedScriptKey"] -->|Add @NullMarked| B["Class-level null safety"]
  A -->|Add @Nullable annotations| C["Method parameters and fields"]
  B --> D["Enhanced type safety"]
  C --> D
Loading

File Walkthrough

Relevant files
Enhancement
ScriptKey.java
Add JSpecify null-safety annotations                                         

java/src/org/openqa/selenium/ScriptKey.java

  • Import JSpecify annotations (@NullMarked and @Nullable)
  • Add @NullMarked class-level annotation for comprehensive null-checking
  • Annotate equals() method parameter with @Nullable
+4/-1     
UnpinnedScriptKey.java
Add JSpecify null-safety annotations                                         

java/src/org/openqa/selenium/UnpinnedScriptKey.java

  • Import JSpecify annotations (@NullMarked and @Nullable)
  • Add @NullMarked class-level annotation for comprehensive null-checking
  • Annotate scriptId field as @Nullable
  • Annotate setScriptId() parameter and getScriptId() return type as
    @Nullable
  • Annotate equals() method parameter with @Nullable
+7/-4     

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

qodo-merge-pro bot commented Oct 21, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Null dereference risk

Description: Marking the field and accessor scriptId as nullable introduces potential null dereference
risks for existing code paths that assumed non-null, which could cause NPEs or logic
errors if not audited where getScriptId() is consumed.
UnpinnedScriptKey.java [67-73]

Referred Code
public void setScriptId(@Nullable String id) {
  this.scriptId = id;
}

public @Nullable String getScriptId() {
  return this.scriptId;
}
Ticket Compliance
🟢
🎫 #14291
🟢 Add JSpecify Nullness annotations across Selenium code to indicate which parameters and
return values can be null.
Ensure classes are marked for null-safety so IDEs and static analyzers can detect
potential nullability issues.
Annotate nullable parameters and return values (e.g., getters that may return null)
appropriately.
Improve Kotlin/IDE interoperability by providing explicit nullness annotations.
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 21, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix incorrect object equality comparison

Fix the equals method in UnpinnedScriptKey by removing the super.equals(o) call
and comparing the script and scriptId fields for equality.

java/src/org/openqa/selenium/UnpinnedScriptKey.java [105-110]

-if (!super.equals(o)) {
-  return false;
-}
+UnpinnedScriptKey that = (UnpinnedScriptKey) o;
+return Objects.equals(this.script, that.script) && Objects.equals(this.scriptId, that.scriptId);
 
-UnpinnedScriptKey that = (UnpinnedScriptKey) o;
-return Objects.equals(this.script, that.script);
-

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical bug in the equals method where super.equals() is called on an identifier initialized with a random UUID, making equality checks for different instances always fail. The fix is crucial for the correct behavior of the class.

High
Ensure hashCode is consistent with equals

Update the hashCode method in UnpinnedScriptKey to be consistent with the equals
method by using the script and scriptId fields, and removing super.hashCode().

java/src/org/openqa/selenium/UnpinnedScriptKey.java [113-116]

 @Override
 public int hashCode() {
-  return Objects.hash(super.hashCode(), script);
+  return Objects.hash(script, scriptId);
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: This suggestion correctly points out that the hashCode implementation is inconsistent with a correct equals method, as it depends on a random identifier. Fixing this is essential to uphold the equals/hashCode contract, which is critical for using these objects in collections.

High
Learned
best practice
Document and validate nullable state

Add explicit null-state validation or documentation to clarify when scriptId may
be null and guard access sites; at minimum, document the nullability contract
here.

java/src/org/openqa/selenium/UnpinnedScriptKey.java [35-73]

+/**
+ * May be null until the script is pinned; once set, may be cleared on unpin.
+ */
 private @Nullable String scriptId;
 
+/**
+ * Sets the script id; pass null to clear when unpinning.
+ */
 public void setScriptId(@Nullable String id) {
   this.scriptId = id;
 }
 
+/**
+ * Returns the script id if pinned, otherwise null.
+ */
 public @Nullable String getScriptId() {
   return this.scriptId;
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs and states early to prevent null-related logic errors when introducing nullable fields and parameters.

Low
  • Update

@diemol diemol merged commit bd434fb into SeleniumHQ:trunk Oct 22, 2025
38 checks passed
@mk868 mk868 deleted the jspecify-ScriptKey-UnpinnedScriptKey branch October 22, 2025 13:00
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