Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bidi][java] Add network request handler APIs #14424

Merged
merged 6 commits into from
Nov 13, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Aug 22, 2024

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.

Related to #13993

Description

Adding network request handler APIs to allow users to modify/intercept network requests using BiDi.

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Tests


Description

  • Added network request handler APIs to allow users to modify/intercept network requests using BiDi.
  • Implemented request interception logic in RemoteNetwork class, including methods for adding, removing, and clearing request handlers.
  • Changed status type from long to int in ResponseData class and updated relevant methods.
  • Added tests for network request handler functionality, including adding, modifying, and removing handlers.
  • Updated NetworkEventsTest to reflect changes in ResponseData status type.

Changes walkthrough 📝

Relevant files
Enhancement
Header.java
Modify constructor visibility in `Header` class                   

java/src/org/openqa/selenium/bidi/network/Header.java

  • Changed constructor visibility from private to public.
+1/-1     
ResponseData.java
Change `status` type in `ResponseData` class                         

java/src/org/openqa/selenium/bidi/network/ResponseData.java

  • Changed status type from long to int.
  • Updated fromJson method to read status as Integer.
  • +5/-5     
    Network.java
    Add request handler management methods in `Network` interface

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

    • Added methods for adding, removing, and clearing request handlers.
    +8/-0     
    RemoteNetwork.java
    Implement request interception in `RemoteNetwork` class   

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

  • Implemented request interception and modification logic.
  • Added RequestDetails class for handling request filters and handlers.
  • +111/-0 
    HttpMethod.java
    Add method to convert string to `HttpMethod`                         

    java/src/org/openqa/selenium/remote/http/HttpMethod.java

    • Added getHttpMethod method to retrieve HttpMethod from string.
    +13/-1   
    Tests
    WebNetworkTest.java
    Add tests for network request handler functionality           

    java/test/org/openqa/selenium/WebNetworkTest.java

  • Added multiple tests for request handler functionality.
  • Tests include adding, modifying, and removing request handlers.
  • +268/-0 
    NetworkEventsTest.java
    Update tests for `ResponseData` status type change             

    java/test/org/openqa/selenium/bidi/network/NetworkEventsTest.java

  • Updated assertions to reflect status type change from long to int.
  • +3/-3     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Performance Issue
    The interceptRequest method creates a new ContinueRequestParameters object for every request, even if no modifications are made. This could be optimized to only create the object when needed.

    Error Handling
    The getHttpMethod method throws an IllegalArgumentException with a generic message when an invalid method is provided. It might be more helpful to include the invalid method in the error message.

    Test Coverage
    Several test methods are ignored for Chrome and Edge browsers. It's important to ensure these tests are eventually implemented for all supported browsers to maintain consistent behavior across platforms.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Use a Map for faster HTTP method lookup instead of valueOf

    Consider using a Map for faster lookup of HTTP methods instead of using valueOf,
    which may throw an exception for invalid methods.

    java/src/org/openqa/selenium/remote/http/HttpMethod.java [27-37]

    +private static final Map<String, HttpMethod> METHOD_MAP = Arrays.stream(HttpMethod.values())
    +    .collect(Collectors.toMap(Enum::name, Function.identity()));
    +
     public static HttpMethod getHttpMethod(String method) {
       if (method == null) {
         throw new IllegalArgumentException("Method cannot be null");
       }
     
    -  try {
    -    return HttpMethod.valueOf(method.toUpperCase());
    -  } catch (IllegalArgumentException e) {
    +  HttpMethod httpMethod = METHOD_MAP.get(method.toUpperCase());
    +  if (httpMethod == null) {
         throw new IllegalArgumentException("No enum constant for method: " + method);
       }
    +  return httpMethod;
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using a Map for HTTP method lookup can significantly improve performance by avoiding exceptions and providing constant-time complexity for lookups. This is a meaningful optimization for frequently called methods.

    8
    Use a more efficient data structure for storing and retrieving request handlers

    Consider using a more efficient data structure for storing and retrieving request
    handlers. A TreeMap with URI paths as keys could provide faster lookups compared to
    iterating through a list of predicates.

    java/src/org/openqa/selenium/remote/RemoteNetwork.java [138-143]

    +private TreeMap<String, List<RequestDetails>> requestHandlerMap = new TreeMap<>();
    +
     private Optional<UnaryOperator<HttpRequest>> getRequestHandler(URI uri) {
    -  return requestHandlers.values().stream()
    +  String path = uri.getPath();
    +  return requestHandlerMap.floorEntry(path).getValue().stream()
           .filter(requestDetails -> requestDetails.getFilter().test(uri))
           .map(RequestDetails::getHandler)
           .findFirst();
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use a TreeMap for storing request handlers could improve performance by providing faster lookups. However, the current implementation using a stream may be sufficient for the expected use cases, and the change might introduce unnecessary complexity.

    7
    Best practice
    Use a more specific exception type for invalid HTTP methods

    Consider using a more specific exception type instead of IllegalArgumentException
    when handling invalid HTTP methods in the getHttpMethod method.

    java/src/org/openqa/selenium/remote/RemoteNetwork.java [34-36]

     } catch (IllegalArgumentException e) {
    -  throw new IllegalArgumentException("No enum constant for method: " + method);
    +  throw new UnsupportedOperationException("Unsupported HTTP method: " + method, e);
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a more specific exception type like UnsupportedOperationException provides clearer semantics and better communicates the nature of the error. However, this change is not critical and mainly improves code readability and error handling clarity.

    6
    Possible issue
    Add a null check before setting headers in the continue request parameters

    Consider adding a check for null or empty headerList before setting headers in the
    continueRequestParameters to avoid unnecessary method calls.

    java/src/org/openqa/selenium/remote/RemoteNetwork.java [123-125]

    -if (!headerList.isEmpty()) {
    +if (headerList != null && !headerList.isEmpty()) {
       continueRequestParameters.headers(headerList);
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Adding a null check is a minor improvement as the current code already checks for an empty list. The likelihood of headerList being null is low, but the additional check could prevent potential null pointer exceptions.

    5

    @pujagani pujagani force-pushed the add-network-request-handlers branch from 76cac6a to 1b941bd Compare August 26, 2024 05:24
    @pujagani pujagani force-pushed the add-network-request-handlers branch from aa05b61 to 9e52090 Compare September 17, 2024 05:46
    @pujagani pujagani force-pushed the add-network-request-handlers branch from 18617a8 to 652d07b Compare November 13, 2024 04:53
    @pujagani pujagani merged commit 0e8059d into SeleniumHQ:trunk Nov 13, 2024
    28 of 32 checks passed
    jkim2492 pushed a commit to jkim2492/selenium that referenced this pull request Nov 17, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant