Skip to content

Conversation

@mk868
Copy link
Contributor

@mk868 mk868 commented Oct 18, 2025

User description

🔗 Related Issues

Related #14291

💥 What does this PR do?

JSpecify annotations added to the:

  • org.openqa.selenium.remote.ExecuteMethod
  • org.openqa.selenium.remote.RemoteExecuteMethod

🔧 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 ExecuteMethod interface

  • Add JSpecify null-safety annotations to RemoteExecuteMethod implementation

  • Mark parameters parameter as @Nullable in both classes

  • Apply @NullMarked class-level annotation for default non-null semantics


Diagram Walkthrough

flowchart LR
  A["ExecuteMethod interface"] -->|"add @NullMarked"| B["Null-safety annotations"]
  A -->|"mark parameters @Nullable"| B
  C["RemoteExecuteMethod class"] -->|"add @NullMarked"| B
  C -->|"mark parameters @Nullable"| B
Loading

File Walkthrough

Relevant files
Enhancement
ExecuteMethod.java
Add JSpecify null-safety annotations to interface               

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

  • Import JSpecify annotations (@NullMarked and @Nullable)
  • Add @NullMarked class-level annotation to interface
  • Mark parameters parameter as @Nullable in execute() method signature
+4/-1     
RemoteExecuteMethod.java
Add JSpecify null-safety annotations to implementation     

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

  • Import JSpecify annotations (@NullMarked and @Nullable)
  • Add @NullMarked class-level annotation to implementation class
  • Mark parameters parameter as @Nullable in execute() method override
+4/-1     

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

qodo-merge-pro bot commented Oct 18, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Null parameter handling

Description: Passing a null map to lower-level driver.execute could cause unexpected behavior if
downstream assumptions change; however current code guards null before calling, so this is
only a potential risk if future edits remove the guard.
RemoteExecuteMethod.java [36-41]

Referred Code
public Object execute(String commandName, @Nullable Map<String, ?> parameters) {
  Response response;

  if (parameters == null || parameters.isEmpty()) {
    response = driver.execute(commandName);
  } else {
Ticket Compliance
🟡
🎫 #14291
🟢 Add JSpecify Nullness annotations to Selenium Java code to indicate nullable parameters
and return values.
Ensure annotations are IDE and static analyzer friendly by using org.jspecify annotations.
🔴 Example intent: methods like getAttribute may return @nullable and code should apply
@NullMarked for non-null-by-default semantics where appropriate.
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

@mk868 mk868 force-pushed the jspecify-ExecuteMethod branch from 89ba46e to bf71403 Compare October 18, 2025 11:25
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 18, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Annotate method return type as nullable

Annotate the return type of the execute method with @Nullable because its
implementation can return a null value.

java/src/org/openqa/selenium/remote/ExecuteMethod.java [38]

-Object execute(String commandName, @Nullable Map<String, ?> parameters);
+@Nullable Object execute(String commandName, @Nullable Map<String, ?> parameters);
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out that the @NullMarked annotation introduces an incorrect non-null contract for the method's return type, as the implementation can return null. Adding @Nullable is crucial for API correctness and to prevent potential runtime errors for clients.

Medium
Learned
best practice
Clarify Javadoc for nullability

Update the Javadoc to document that parameters may be null and what null/empty
imply. Clarify return type/value expectations.

java/src/org/openqa/selenium/remote/ExecuteMethod.java [30-38]

 /**
- * Execute the given command on the remote webdriver server. Any exceptions will be thrown by the
+ * Execute the given command on the remote WebDriver server. Any exceptions will be thrown by the
  * underlying execute method.
  *
- * @param commandName The remote command to execute
- * @param parameters The parameters to execute that command with
- * @return The result of {@link Response#getValue()}.
+ * @param commandName the remote command to execute
+ * @param parameters the parameters to execute that command with; may be {@code null} or empty to indicate no parameters
+ * @return the value from {@link Response#getValue()}
  */
 Object execute(String commandName, @Nullable Map<String, ?> parameters);
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Keep API and documentation accurate and consistent by updating Javadoc to reflect nullability and parameter semantics.

Low
  • Update

@mk868 mk868 force-pushed the jspecify-ExecuteMethod branch from bf71403 to 1db3916 Compare October 18, 2025 11:34
@mk868 mk868 force-pushed the jspecify-ExecuteMethod branch from 1db3916 to faa0dbd Compare October 18, 2025 12:41
@diemol diemol merged commit 912f35e into SeleniumHQ:trunk Oct 19, 2025
38 checks passed
@mk868 mk868 deleted the jspecify-ExecuteMethod branch October 19, 2025 17:16
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