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

[py] Implement add_request_handler #14604

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

jman-sketch
Copy link

@jman-sketch jman-sketch commented Oct 15, 2024

User description

Description

Implement add_request_handler as described in #13993.

Add CDDL structures generated from the BIDI specs

Add an asynchronous navigation function driver.network.get

Motivation and Context

#13993

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

  • Implemented a new feature to handle network requests using BiDi protocol in Selenium.
  • Added multiple data classes for network operations with JSON serialization and deserialization.
  • Integrated network request handling into the WebDriver class.
  • Added tests to verify the functionality of the add_request_handler method.

Changes walkthrough 📝

Relevant files
Enhancement
network.py
Implement network request handling and serialization classes

py/selenium/webdriver/common/bidi/network.py

  • Implemented multiple data classes for network operations.
  • Added methods for JSON serialization and deserialization.
  • Introduced event handling for network requests.
  • +491/-0 
    script.py
    Add stack trace data classes with serialization                   

    py/selenium/webdriver/common/bidi/script.py

  • Added StackFrame and StackTrace data classes.
  • Implemented JSON serialization and deserialization methods.
  • +46/-1   
    network.py
    Introduce Network class for request management                     

    py/selenium/webdriver/remote/network.py

  • Created Network class for managing network requests.
  • Implemented add_request_handler method for request interception.
  • +41/-0   
    webdriver.py
    Integrate Network class into WebDriver                                     

    py/selenium/webdriver/remote/webdriver.py

  • Integrated Network class into WebDriver.
  • Added network property for network operations.
  • +9/-0     
    Tests
    bidi_network_tests.py
    Add tests for network request handler                                       

    py/test/selenium/webdriver/common/bidi_network_tests.py

  • Added test for add_request_handler method.
  • Used pytest for testing network request interception.
  • +40/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @CLAassistant
    Copy link

    CLAassistant commented Oct 15, 2024

    CLA assistant check
    All committers have signed the CLA.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Complexity
    The file contains multiple data classes with similar structure and methods. Consider refactoring to reduce code duplication and improve maintainability.

    Error Handling
    The add_request_handler method lacks proper error handling. Consider adding try-except blocks to handle potential exceptions during WebSocket connections or network operations.

    Test Coverage
    The test file contains only one test case. Consider adding more test cases to cover different scenarios and edge cases for the network request interception feature.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Oct 15, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Expand test coverage with parameterized tests for different URL scenarios

    Consider adding more comprehensive test cases to cover different scenarios and edge
    cases for the add_request_handler functionality.

    py/test/selenium/webdriver/common/bidi_network_tests.py [22-40]

     @pytest.mark.xfail_safari
     @pytest.mark.xfail_firefox
     @pytest.mark.xfail_opera
    -async def test_add_request_handler(driver):
    -
    +@pytest.mark.parametrize("original_url,redirected_url", [
    +    ("https://www.example.com/", "https://www.selenium.dev/about/"),
    +    ("https://www.example.org/", "https://www.selenium.dev/documentation/"),
    +    ("https://www.example.net/", "https://www.selenium.dev/downloads/"),
    +])
    +async def test_add_request_handler(driver, original_url, redirected_url):
         def request_filter(params: BeforeRequestSentParameters):
    -        return params.request["url"] == "https://www.example.com/"
    +        return params.request["url"].startswith("https://www.example.")
     
         def request_handler(params: BeforeRequestSentParameters):
             request = params.request["request"]
             json = {
                 "request": request,
    -            "url" : "https://www.selenium.dev/about/"
    +            "url": redirected_url
             }
             return ContinueRequestParameters(**json)
     
         await driver.network.add_request_handler(request_filter, request_handler)
    -    driver.get("https://www.example.com/")
    -    assert driver.current_url == "https://www.selenium.dev/about/"
    +    driver.get(original_url)
    +    assert driver.current_url == redirected_url
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Expanding test coverage with parameterized tests is a strong suggestion that enhances the robustness of the test suite by covering multiple scenarios and edge cases, ensuring the functionality is thoroughly validated.

    8
    Add error handling and logging to improve robustness of request handling

    Consider adding error handling and logging to the _callback method to handle
    potential exceptions during request handling.

    py/selenium/webdriver/remote/network.py [33-41]

    +import logging
    +
     def _callback(self, request_filter, handler):
         async def callback(request):
    -        if request_filter(request):
    -            request = handler(request)
    -        else:
    -            request = default_request_handler(request)
    -        await self.network.continue_request(request)
    +        try:
    +            if request_filter(request):
    +                request = handler(request)
    +            else:
    +                request = default_request_handler(request)
    +            await self.network.continue_request(request)
    +        except Exception as e:
    +            logging.error(f"Error handling request: {e}")
    +            # Consider implementing a fallback behavior here
     
         return callback
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding error handling and logging to the _callback method is a valuable enhancement that can improve the robustness and maintainability of the code by ensuring exceptions are logged and potentially handled.

    7
    Implement lazy initialization with error checking for the Network object

    Consider implementing lazy initialization for the Network object to improve
    performance and resource usage.

    py/selenium/webdriver/remote/webdriver.py [1085-1090]

     @property
     def network(self):
         if not self._network:
    -        self._network = Network(self.caps.get("webSocketUrl"))
    -
    +        if "webSocketUrl" not in self.caps:
    +            raise ValueError("WebSocket URL not found in capabilities")
    +        self._network = Network(self.caps["webSocketUrl"])
         return self._network
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion to add error checking for the WebSocket URL during lazy initialization of the Network object is a reasonable enhancement that can prevent runtime errors due to missing configuration.

    6
    Implement proper resource management for network request handlers

    Consider using a context manager for the WebSocket connection to ensure proper
    resource management and connection closure.

    py/selenium/webdriver/remote/network.py [22-31]

     async def add_request_handler(self, request_filter=lambda _: True, handler=default_request_handler):
         async with open_cdp(self.ws_url) as conn:
             if not self.network:
                 self.network = network.Network(conn)
             params = AddInterceptParameters(["beforeRequestSent"])
             result = await self.network.add_intercept(event=BeforeRequestSent, params=params)
             intercept = result["intercept"]
             callback = self._callback(request_filter, handler)
             await self.network.add_listener(BeforeRequestSent, callback)
    -        return intercept
    +        return intercept, lambda: self.network.remove_listener(BeforeRequestSent, callback)
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to use a context manager for the WebSocket connection is already implemented in the existing code. However, the addition of a lambda function to remove the listener is a useful enhancement for resource management, though it is not critical.

    5

    💡 Need additional feedback ? start a PR chat

    @jman-sketch
    Copy link
    Author

    Currently, the add_listener function is blocking, meaning that page can only be refreshed via the browser itself. However, when navigating using the browser, requests are properly intercepted and modified. I wanted to open the PR for feedback and suggestions.

    @jman-sketch jman-sketch force-pushed the network_intercept branch 2 times, most recently from 820fee6 to 8fbc580 Compare October 17, 2024 03:11
    @jman-sketch
    Copy link
    Author

    It was driver.get that was the blocking operation. Added network.get for asynchronous navigation

    @jman-sketch jman-sketch force-pushed the network_intercept branch 6 times, most recently from bac8478 to 9c0bed1 Compare October 17, 2024 23:30
    @jman-sketch
    Copy link
    Author

    bidi_network_tests.py was throwing errors because usage of | operator for merging dictionaries was introduced in Python 3.9 while the tests were using 3.8. It has been fixed

    @jman-sketch
    Copy link
    Author

    All local chrome- tests are passing

    @VietND96 May the workflow be triggered again?

    @jman-sketch
    Copy link
    Author

    @AutomatedTester I would really appreciate your feedback on this. In particular, what are your thoughts on

    • Using BiDi objects + websocket vs json + HTTP
    • Using trio/nursery vs plain async functions
      • I think being able to call await add_request_handler would be a lot more elegant, but I think it would require more changes to existing modules

    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.

    2 participants