[py] Implement add_request_handler

[jman-sketch]

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)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• [x] I have read the contributing document.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [x] I have added tests to cover my changes.
• [x] 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]

All committers have signed the CLA.

[qodo-code-review[bot]]

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.

[qodo-code-review[bot]]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	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): - [email protected]("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]

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]

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

[jman-sketch]

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]

All local chrome- tests are passing

@VietND96 May the workflow be triggered again?

[jman-sketch]

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

• Using BiDi objects + websocket vs json + HTTP
  • Ideally, there should be a generic class and each object would only have the type definitions, but I'm not sure where to put the generic class
• 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

[jman-sketch]

Remote tests were timing out due to requesting external URLs. Changing it to internal ones in pages solved it

[jman-sketch]

Factored all the repeated functions into bidi.py class. Tests are still passing (locally)

[shbenzer]

run ./scripts/format.sh to help with your formatting

[jman-sketch]

@shbenzer Aha! Thanks!

[shbenzer]

bidi_tests.py and upload_tests.py are flaky - I've got PRs up to fix them - so don't worry about those results

[jman-sketch]

@shbenzer Perfect! That explains a lot :confetti_ball:

[jman-sketch]

While adding support for multiple intercepts, noticed that "paused" cached requests are served without waiting for continue request. Not sure if this is intended (from browser end).

Since bidi network.setCacheBehavior is not implemented yet, will disable caching with cdp for now.

[jman-sketch]

Closing this PR to open a new one (#14738) from a different account for better access management. Thank you for your feedbacks!