[java] Prevent accessing BiDi internals from driver#17745
Closed
pujagani wants to merge 1 commit into
Closed
Conversation
selenium-ci
added
C-java
Contributor
PR Summary by QodoJava: add BiDi.from(WebDriver) and deprecate HasBiDi#getBiDi access
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Contributor
Code Review by Qodo
Context used✅ Tickets:
🎫 Instance ChroneDriver Error: ConnectFailure 🎫 2.48 doesn't trigger javascript in link's href on click()✅ Compliance rules (platform):
18 rules 1. BiDi.from() missing Javadoc
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+44
to
+52
| public static BiDi from(WebDriver driver) { | ||
| Require.nonNull("WebDriver", driver); | ||
| if (!(driver instanceof HasBiDi)) { | ||
| throw new BiDiException("WebDriver does not support BiDi protocol"); | ||
| } | ||
| return ((HasBiDi) driver) | ||
| .maybeGetBiDi() | ||
| .orElseThrow(() -> new BiDiException("Unable to create a BiDi connection")); | ||
| } |
Contributor
There was a problem hiding this comment.
1. bidi.from() missing javadoc 📘 Rule violation ✧ Quality
The new public API method BiDi.from(WebDriver) lacks a Javadoc block, and therefore does not document its parameter and return value as required. This reduces API clarity and violates the project’s public API documentation requirements.
Agent Prompt
## Issue description
`BiDi.from(WebDriver)` is a newly added public API method but has no Javadoc, so it does not document purpose, `@param driver`, and `@return`.
## Issue Context
Public API methods must have Javadoc and include tags for parameters and non-void returns.
## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[44-52]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+27
to
33
| /** | ||
| * @deprecated Use {@link BiDi#from(WebDriver)} instead. | ||
| */ | ||
| @Deprecated(since = "4.24", forRemoval = true) | ||
| default BiDi getBiDi() { | ||
| return maybeGetBiDi() | ||
| .orElseThrow(() -> new BiDiException("Unable to create a BiDi connection")); |
Contributor
There was a problem hiding this comment.
2. hasbidi.getbidi() javadoc incomplete 📘 Rule violation ✧ Quality
The modified (deprecated) public method HasBiDi.getBiDi() has Javadoc consisting only of an @deprecated tag and omits an @return tag. This violates the requirement for complete Javadoc on touched public methods.
Agent Prompt
## Issue description
The touched public method `HasBiDi.getBiDi()` has tag-only Javadoc (`@deprecated` only) and does not include an `@return` tag even though it returns `BiDi`.
## Issue Context
Public methods changed in this PR must have complete Javadoc including at least one free-text sentence and `@return` for non-void returns.
## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/HasBiDi.java[27-34]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+44
to
+52
| public static BiDi from(WebDriver driver) { | ||
| Require.nonNull("WebDriver", driver); | ||
| if (!(driver instanceof HasBiDi)) { | ||
| throw new BiDiException("WebDriver does not support BiDi protocol"); | ||
| } | ||
| return ((HasBiDi) driver) | ||
| .maybeGetBiDi() | ||
| .orElseThrow(() -> new BiDiException("Unable to create a BiDi connection")); | ||
| } |
Contributor
There was a problem hiding this comment.
3. Bidi.from skips initialization 🐞 Bug ≡ Correctness
BiDi.from(WebDriver) calls HasBiDi.maybeGetBiDi() and throws when it’s empty, but the BiDi augmenter (BiDiProvider) intentionally keeps BiDi lazy and returns Optional.empty until getBiDi() is called. This makes BiDi.from() fail on freshly augmented RemoteWebDriver instances, breaking RemoteWebDriver.script()/RemoteScript and the updated RemoteWebDriverBiDiTest path.
Agent Prompt
### Issue description
`BiDi.from(WebDriver)` currently uses `HasBiDi.maybeGetBiDi()` which may legitimately be empty for augmented drivers (BiDi is lazily created). This causes `BiDi.from()` to throw even when BiDi is supported, breaking BiDi-based features on `Augmenter`-enhanced `RemoteWebDriver`.
### Issue Context
- The `BiDiProvider` augmentation returns `Optional.empty()` from `maybeGetBiDi()` until `getBiDi()` is invoked (lazy initialization).
- The PR updates production code (`RemoteScript`) and tests to call `BiDi.from(driver)`, so this is a real runtime path.
### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[44-52]
### Suggested fix approach
Update `BiDi.from(WebDriver)` to *initialize* the BiDi instance when needed, e.g.:
- Prefer calling `((HasBiDi) driver).getBiDi()` (with a local `@SuppressWarnings("deprecation")` if necessary) so lazy augmenters initialize.
- Or: `hasBiDi.maybeGetBiDi().orElseGet(() -> hasBiDi.getBiDi())` to keep the “already initialized” fast path.
This ensures `BiDi.from()` works with `Augmenter`-provided `HasBiDi` implementations and fixes the failing `RemoteScript`/remote BiDi flows.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
|
Resolving the AI bot comments |
Contributor
Author
|
This approach needs a rethink. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
Aligning the code to the ADR #17670.
💥 What does this PR do?
It deprecates the method that allow public access to BiDi internals and adds a method that be used by passing a driver object.
🔧 Implementation Notes
🤖 AI assistance
💡 Additional Considerations
Follow-up
Considerations
🔄 Types of changes