You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains 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
Return Type Change The method onAuthRequired has changed its return type from void to long. Ensure that this change is documented and that all calling code is updated to handle the new return type correctly.
Error Handling The method removeAuthenticationHandler and clearAuthenticationHandlers lack error handling for cases where the id or intercept might not be valid or already removed. Consider adding error handling to prevent runtime exceptions.
public long addAuthenticationHandler(UsernameAndPassword usernameAndPassword) {
+ if (usernameAndPassword == null) {+ throw new IllegalArgumentException("Username and password should not be null.");+ }
String intercept =
network.addIntercept(new AddInterceptParameters(InterceptPhase.AUTH_REQUIRED));
Apply this suggestion
Suggestion importance[1-10]: 10
Why: Adding a null check for usernameAndPassword in addAuthenticationHandler prevents potential null pointer exceptions, ensuring the method handles invalid inputs gracefully.
10
Enhancement
Use ConcurrentHashMap to ensure thread safety
Ensure thread safety for operations on authCallbackIdMap since it might be accessed by multiple threads. Consider using ConcurrentHashMap instead of HashMap for authCallbackIdMap.
-private final Map<Long, String> authCallbackIdMap = new HashMap<>();+private final Map<Long, String> authCallbackIdMap = new ConcurrentHashMap<>();
Apply this suggestion
Suggestion importance[1-10]: 9
Why: Ensuring thread safety by using ConcurrentHashMap is crucial for preventing concurrency issues, especially if authCallbackIdMap is accessed by multiple threads.
9
Add logging for removing intercepts and listeners
When removing intercepts and listeners, consider logging these operations to help with debugging and maintaining the system.
+logger.info("Removing intercept with ID: " + intercept);
network.removeIntercept(intercept);
+logger.info("Removing listener with ID: " + id);
this.biDi.removeListener(id);
Apply this suggestion
Suggestion importance[1-10]: 7
Why: Adding logging for removing intercepts and listeners can aid in debugging and maintaining the system, though it is a minor enhancement compared to functional changes.
7
Possible issue
Add exception handling around addListener calls
Consider handling the case where addListener might throw an exception due to invalid parameters or other issues. You can wrap the addListener calls in a try-catch block to manage such exceptions gracefully.
Why: Adding exception handling around addListener calls is a good practice to manage potential runtime exceptions, improving the robustness of the code.
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.
User description
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
Related to #13993
Motivation and Context
Highi-level APIs for W3C BiDi for authentication
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
onAuthRequired
method inNetwork
module to return a listener ID.Network
interface for managing authentication handlers.Network
interface in theRemoteNetwork
class.RemoteWebDriver
class.WebNetworkTest
.Changes walkthrough 📝
Network.java
Update `onAuthRequired` method to return listener ID
java/src/org/openqa/selenium/bidi/module/Network.java
onAuthRequired
method to return a long value.Network.java
Introduce `Network` interface for authentication handlers
java/src/org/openqa/selenium/remote/Network.java
Network
with methods for authentication handlers.RemoteNetwork.java
Implement `Network` interface in `RemoteNetwork` class
java/src/org/openqa/selenium/remote/RemoteNetwork.java
Network
interface inRemoteNetwork
class.RemoteWebDriver.java
Add network management to `RemoteWebDriver`
java/src/org/openqa/selenium/remote/RemoteWebDriver.java
remoteNetwork
field andnetwork
method toRemoteWebDriver
.WebNetworkTest.java
Add tests for network authentication handlers
java/test/org/openqa/selenium/WebNetworkTest.java
WebNetworkTest
.BUILD.bazel
Update Bazel build file for network package
java/src/org/openqa/selenium/remote/BUILD.bazel