Skip to content

Revert "[java] make the signature change in ExecuteMethod backward compatible"#17184

Open
vaibhav09mbm wants to merge 1 commit intoSeleniumHQ:trunkfrom
vaibhav09mbm:revert-17183-make-signature-change-backward-compatible
Open

Revert "[java] make the signature change in ExecuteMethod backward compatible"#17184
vaibhav09mbm wants to merge 1 commit intoSeleniumHQ:trunkfrom
vaibhav09mbm:revert-17183-make-signature-change-backward-compatible

Conversation

@vaibhav09mbm
Copy link

Reverts #17183

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@selenium-ci selenium-ci added the C-java Java Bindings label Mar 7, 2026
@qodo-code-review
Copy link
Contributor

Review Summary by Qodo

Revert ExecuteMethod backward compatibility changes and restore generic signatures

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Reverts backward compatibility changes to ExecuteMethod interface
• Restores generic return type to execute() method signature
• Removes three convenience methods: execute(String, Map, T), executeAs(), execute(String)
• Introduces new executeRequired() method for non-null result handling
• Updates all call sites to use executeRequired() where null-safety needed

Grey Divider

File Changes

1. java/src/org/openqa/selenium/remote/ExecuteMethod.java 🐞 Bug fix +3/-30

Restore generic return type and simplify method signatures

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


2. java/src/org/openqa/selenium/remote/RemoteExecuteMethod.java 🐞 Bug fix +3/-2

Update execute() to return generic type with cast

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


3. java/src/org/openqa/selenium/remote/LocalExecuteMethod.java 🐞 Bug fix +1/-1

Update execute() signature to return generic type

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


View more (9)
4. java/src/org/openqa/selenium/chromium/AddHasCasting.java 🐞 Bug fix +3/-2

Replace execute() calls with executeRequired() or requireNonNullElse()

java/src/org/openqa/selenium/chromium/AddHasCasting.java


5. java/src/org/openqa/selenium/chromium/AddHasCdp.java 🐞 Bug fix +2/-1

Replace executeAs() with executeRequired() method call

java/src/org/openqa/selenium/chromium/AddHasCdp.java


6. java/src/org/openqa/selenium/chromium/AddHasNetworkConditions.java 🐞 Bug fix +2/-1

Replace execute() with executeRequired() and add suppression

java/src/org/openqa/selenium/chromium/AddHasNetworkConditions.java


7. java/src/org/openqa/selenium/firefox/AddHasContext.java 🐞 Bug fix +1/-1

Replace execute() with executeRequired() for context retrieval

java/src/org/openqa/selenium/firefox/AddHasContext.java


8. java/src/org/openqa/selenium/firefox/AddHasExtensions.java 🐞 Bug fix +1/-1

Replace executeAs() with executeRequired() method call

java/src/org/openqa/selenium/firefox/AddHasExtensions.java


9. java/src/org/openqa/selenium/remote/FedCmDialogImpl.java 🐞 Bug fix +5/-4

Replace execute() and executeAs() with executeRequired() calls

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


10. java/src/org/openqa/selenium/remote/RemoteLogs.java 🐞 Bug fix +2/-1

Replace execute() with executeRequired() for log types

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


11. java/src/org/openqa/selenium/safari/AddHasPermissions.java 🐞 Bug fix +1/-1

Replace execute() with executeRequired() for permissions

java/src/org/openqa/selenium/safari/AddHasPermissions.java


12. java/test/org/openqa/selenium/remote/RemoteLogsTest.java 🧪 Tests +1/-1

Update mock to use executeRequired() instead of execute()

java/test/org/openqa/selenium/remote/RemoteLogsTest.java


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Mar 7, 2026

Code Review by Qodo

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

Grey Divider


Action required

1. ExecuteMethod helpers removed 📘 Rule violation ✓ Correctness
Description
The public ExecuteMethod interface no longer exposes previously available convenience/default
methods and changes the execute API shape, forcing downstream users to update code when upgrading
Selenium. This is a breaking public API change and removes public functionality without a
deprecation/migration period.
Code

java/src/org/openqa/selenium/remote/ExecuteMethod.java[R40-44]

+  @Nullable <T> T execute(String commandName, @Nullable Map<String, ?> parameters);

-  /**
-   * Execute the given command and return the default value if the command return null.
-   *
-   * @return non-nullable value of type T.
-   */
-  @SuppressWarnings("unchecked")
-  default <T> T execute(String commandName, @Nullable Map<String, ?> parameters, T defaultValue) {
-    return (T) requireNonNullElse(execute(commandName, parameters), defaultValue);
-  }
-
-  /**
-   * Execute the given command and cast the returned value to T.
-   *
-   * @return non-nullable value of type T.
-   */
-  @SuppressWarnings("unchecked")
-  default <T> T executeAs(String commandName, @Nullable Map<String, ?> parameters) {
-    return (T) requireNonNull(execute(commandName, parameters));
-  }
-
-  /**
-   * Execute the given command without parameters and cast the returned value to T.
-   *
-   * @return non-nullable value of type T.
-   */
-  @SuppressWarnings("unchecked")
-  default <T> T execute(String commandName) {
-    return (T) requireNonNull(execute(commandName, null));
+  default <T> T executeRequired(String commandName, @Nullable Map<String, ?> parameters) {
+    return requireNonNull(execute(commandName, parameters));
  }
Evidence
PR Compliance ID 1 requires maintaining backward compatibility for user-facing APIs, and PR
Compliance ID 5 requires deprecating public functionality before removal. The updated
ExecuteMethod interface now only contains execute and the new executeRequired, indicating
prior convenience/default APIs are no longer present and consumers must adapt (as shown by new
usages of executeRequired).

AGENTS.md
AGENTS.md
java/src/org/openqa/selenium/remote/ExecuteMethod.java[40-44]
java/src/org/openqa/selenium/chromium/AddHasCdp.java[54-56]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ExecuteMethod` has changed in a way that breaks external consumers by removing previously available public convenience/default methods and forcing call sites to migrate immediately.

## Issue Context
This PR is required to maintain API/ABI compatibility for user-facing changes and to deprecate public functionality before removing it.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/ExecuteMethod.java[40-44]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Bare NPE in executeRequired 🐞 Bug ✧ Quality
Description
ExecuteMethod.executeRequired throws a context-free NullPointerException when the remote command
returns null, making failures hard to diagnose. This impacts multiple new call sites that now rely
on executeRequired for non-null responses.
Code

java/src/org/openqa/selenium/remote/ExecuteMethod.java[R42-44]

+  default <T> T executeRequired(String commandName, @Nullable Map<String, ?> parameters) {
+    return requireNonNull(execute(commandName, parameters));
  }
Evidence
executeRequired wraps execute() with requireNonNull but provides no message (or command name), so
null responses become opaque NPEs with no indication of which command returned null.

java/src/org/openqa/selenium/remote/ExecuteMethod.java[40-44]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ExecuteMethod.executeRequired` currently throws a bare `NullPointerException` when a remote command returns `null`, with no message identifying the command.

### Issue Context
Many call sites were updated to use `executeRequired(command, null)`; when a server returns `null` unexpectedly, users will get an unhelpful NPE.

### Fix Focus Areas
- java/src/org/openqa/selenium/remote/ExecuteMethod.java[40-44]

### Suggested change
Use the message overload, e.g.:
```java
default &lt;T&gt; T executeRequired(String commandName, @Nullable Map&lt;String, ?&gt; parameters) {
 return requireNonNull(execute(commandName, parameters),
     () -&gt; &quot;Remote command &#x27;&quot; + commandName + &quot;&#x27; returned null&quot;);
}
```
(If `requireNonNull` overload with `Supplier&lt;String&gt;` is unavailable in the target JDK, use the `String` overload.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

3. Unchecked cast hidden in execute 🐞 Bug ⛯ Reliability
Description
RemoteExecuteMethod.execute performs an unchecked cast from Response.getValue() to the inferred
generic type, so type mismatches will fail at runtime with ClassCastException. This is likely
unavoidable but should be treated as an intentionally unsafe boundary and documented accordingly.
Code

java/src/org/openqa/selenium/remote/RemoteExecuteMethod.java[R34-39]

+  @SuppressWarnings("unchecked")
  @Override
-  public @Nullable Object execute(String commandName, @Nullable Map<String, ?> parameters) {
+  public @Nullable <T> T execute(String commandName, @Nullable Map<String, ?> parameters) {
    Response response;

    if (parameters == null || parameters.isEmpty()) {
Evidence
The interface exposes a generic return type, but the implementation must cast the untyped remote
value into that generic type without runtime validation, meaning incorrect caller expectations can
manifest as runtime ClassCastExceptions.

java/src/org/openqa/selenium/remote/RemoteExecuteMethod.java[34-46]
java/src/org/openqa/selenium/remote/ExecuteMethod.java[40-44]
java/src/org/openqa/selenium/firefox/AddHasContext.java[67-73]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`RemoteExecuteMethod.execute` must cast an untyped `Response#getValue()` into the generic return type `T` without runtime validation.

### Issue Context
This is a normal boundary when bridging untyped remote protocol values into typed Java APIs, but the generic signature can imply more type safety than is actually enforced.

### Fix Focus Areas
- java/src/org/openqa/selenium/remote/ExecuteMethod.java[32-44]
- java/src/org/openqa/selenium/remote/RemoteExecuteMethod.java[34-46]

### Suggested improvements (pick one)
1) Add Javadoc to `ExecuteMethod#execute` explaining that the generic type is unchecked and may throw `ClassCastException` if the server returns an unexpected type.
2) Add an overload such as `executeAs(String commandName, @Nullable Map&lt;String, ?&gt; params, Class&lt;T&gt; type)` that validates `type.isInstance(value)` and throws a `WebDriverException` with command/type details on mismatch.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment on lines +40 to 44
@Nullable <T> T execute(String commandName, @Nullable Map<String, ?> parameters);

/**
* Execute the given command and return the default value if the command return null.
*
* @return non-nullable value of type T.
*/
@SuppressWarnings("unchecked")
default <T> T execute(String commandName, @Nullable Map<String, ?> parameters, T defaultValue) {
return (T) requireNonNullElse(execute(commandName, parameters), defaultValue);
}

/**
* Execute the given command and cast the returned value to T.
*
* @return non-nullable value of type T.
*/
@SuppressWarnings("unchecked")
default <T> T executeAs(String commandName, @Nullable Map<String, ?> parameters) {
return (T) requireNonNull(execute(commandName, parameters));
}

/**
* Execute the given command without parameters and cast the returned value to T.
*
* @return non-nullable value of type T.
*/
@SuppressWarnings("unchecked")
default <T> T execute(String commandName) {
return (T) requireNonNull(execute(commandName, null));
default <T> T executeRequired(String commandName, @Nullable Map<String, ?> parameters) {
return requireNonNull(execute(commandName, parameters));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. executemethod helpers removed 📘 Rule violation ✓ Correctness

The public ExecuteMethod interface no longer exposes previously available convenience/default
methods and changes the execute API shape, forcing downstream users to update code when upgrading
Selenium. This is a breaking public API change and removes public functionality without a
deprecation/migration period.
Agent Prompt
## Issue description
`ExecuteMethod` has changed in a way that breaks external consumers by removing previously available public convenience/default methods and forcing call sites to migrate immediately.

## Issue Context
This PR is required to maintain API/ABI compatibility for user-facing changes and to deprecate public functionality before removing it.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/ExecuteMethod.java[40-44]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-java Java Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants