Skip to content

[py][bidi]: add bidi storage module #15669

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

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

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Apr 25, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Implements the storage module for the python bindings - https://w3c.github.io/webdriver-bidi/#module-storage

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement, Tests


Description

  • Introduces BiDi storage module for Python bindings

    • Implements cookie management via BiDi protocol (get, set, delete)
    • Adds support for cookie filtering and partition descriptors
    • Provides typed classes for cookies, filters, and results
  • Adds comprehensive tests for BiDi storage functionality

    • Tests cover cookie creation, retrieval, deletion, and partitioning
    • Validates integration with user contexts and window partitions
  • Exposes storage property on WebDriver for BiDi storage access


Changes walkthrough 📝

Relevant files
Enhancement
storage.py
Add BiDi storage module with cookie management classes     

py/selenium/webdriver/common/bidi/storage.py

  • Implements BiDi storage module for cookie management
  • Defines classes for cookies, filters, partition descriptors, and
    results
  • Provides methods for get, set, and delete cookies via BiDi
  • Adds type annotations and docstrings for clarity
  • +418/-0 
    webdriver.py
    Integrate BiDi storage module into WebDriver API                 

    py/selenium/webdriver/remote/webdriver.py

  • Imports and initializes BiDi Storage module
  • Adds storage property to WebDriver for BiDi storage access
  • Updates internal state to manage storage module instance
  • Documents usage examples for new storage property
  • +25/-0   
    Tests
    bidi_storage_tests.py
    Add tests for BiDi storage module and cookie operations   

    py/test/selenium/webdriver/common/bidi_storage_tests.py

  • Adds comprehensive tests for BiDi storage module
  • Tests cookie creation, retrieval, deletion, and partitioning
  • Validates behavior across user contexts and window partitions
  • Uses fixtures and asserts for robust test coverage
  • +365/-0 

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Apr 25, 2025
    @navin772 navin772 marked this pull request as ready for review April 25, 2025 09:57
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5678 - Not compliant

    Non-compliant requirements:

    • Fix the "Error: ConnectFailure (Connection refused)" issue when instantiating ChromeDriver multiple times

    Requires further human verification:

    • Issue occurs on Ubuntu 16.04.4 with Linux 4.10.0
    • Affects Selenium 3.9.0 with Chrome 65.0.3325.181 and ChromeDriver 2.35
    • First ChromeDriver instance works fine, but subsequent instances fail

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where JavaScript in link's href attribute is not triggered on click() in Firefox

    Requires further human verification:

    • Problem occurs in Selenium 2.48.0 and 2.48.2 but works in 2.47.1
    • Affects Firefox 42.0 32bit on 64bit machine

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

    Error Handling

    The implementation lacks explicit error handling for cases where cookie operations might fail, such as invalid domains, cookie size limits, or connection issues with the browser.

    ) -> GetCookiesResult:
        """Retrieves cookies that match the given parameters.
    
        Parameters:
        -----------
            filter: Optional filter to match cookies.
            partition: Optional partition descriptor.
    
        Returns:
        -------
            GetCookiesResult: The result of the get cookies command.
        """
        params = {}
        if filter is not None:
            params["filter"] = filter.to_dict()
        if partition is not None:
            params["partition"] = partition.to_dict()
    
        result = self.conn.execute(command_builder("storage.getCookies", params))
        return GetCookiesResult.from_dict(result)
    Test Failures

    Several tests are marked with xfail decorators for Chrome and Edge, indicating known failures. These should be investigated to determine if the implementation needs adjustments to work across all browsers.

    @pytest.mark.xfail_chrome
    @pytest.mark.xfail_edge
    def test_get_cookie_in_default_user_context(driver, pages, webserver):
        """Test getting a cookie in the default user context."""
        assert_no_cookies_are_present(driver)
    
        window_handle = driver.current_window_handle
        key = generate_unique_key()
        value = "set"
        assert_cookie_is_not_present_with_name(driver, key)
    
        driver.add_cookie({"name": key, "value": value})
    
        # Test
        cookie_filter = CookieFilter(name=key, value=BytesValue(BytesValue.TYPE_STRING, "set"))
    
        driver.switch_to.new_window(WindowTypes.WINDOW)
    
        descriptor = BrowsingContextPartitionDescriptor(driver.current_window_handle)
    
        params = cookie_filter
        result_after_switching_context = driver.storage.get_cookies(filter=params, partition=descriptor)
    
        assert len(result_after_switching_context.cookies) > 0
        assert result_after_switching_context.cookies[0].value.value == value
    
        driver.switch_to.window(window_handle)
    
        descriptor = BrowsingContextPartitionDescriptor(driver.current_window_handle)
    
        result = driver.storage.get_cookies(filter=cookie_filter, partition=descriptor)
    
        assert len(result.cookies) > 0
        assert result.cookies[0].value.value == value
        partition_key = result.partition_key
    
        assert partition_key.source_origin is not None
        assert partition_key.user_context is not None
        assert partition_key.user_context == "default"

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Handle None value safely

    The code doesn't handle the case where the 'value' key might be missing in the
    data dictionary. If 'data.get("value")' returns None, attempting to access
    .get("type") on None will raise an AttributeError. Add a fallback empty
    dictionary to prevent this potential error.

    py/selenium/webdriver/common/bidi/storage.py [91]

     def from_dict(cls, data: Dict) -> "Cookie":
         """Creates a Cookie instance from a dictionary.
     
         Parameters:
         -----------
             data: A dictionary containing the cookie information.
     
         Returns:
         -------
             Cookie: A new instance of Cookie.
         """
    -    value = BytesValue(data.get("value", {}).get("type"), data.get("value", {}).get("value"))
    +    value_dict = data.get("value", {}) or {}
    +    value = BytesValue(value_dict.get("type"), value_dict.get("value"))

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: This suggestion addresses a potential bug where if the "value" key exists in the data dictionary but is None, accessing .get("type") would raise an AttributeError. The improved code adds a fallback empty dictionary with the or {} pattern, which prevents this error and makes the code more robust when handling unexpected API responses.

    Medium
    Learned
    best practice
    Add connection null check

    Add a null check for the conn property before using it to prevent potential
    NullReferenceExceptions. This ensures the connection is available before
    attempting to execute commands.

    py/selenium/webdriver/common/bidi/storage.py [347-370]

     def get_cookies(
         self,
         filter: Optional[CookieFilter] = None,
         partition: Optional[Union[BrowsingContextPartitionDescriptor, StorageKeyPartitionDescriptor]] = None,
     ) -> GetCookiesResult:
         """Retrieves cookies that match the given parameters.
     
         Parameters:
         -----------
             filter: Optional filter to match cookies.
             partition: Optional partition descriptor.
     
         Returns:
         -------
             GetCookiesResult: The result of the get cookies command.
         """
    +    if self.conn is None:
    +        raise ValueError("Connection is not established")
    +        
         params = {}
         if filter is not None:
             params["filter"] = filter.to_dict()
         if partition is not None:
             params["partition"] = partition.to_dict()
     
         result = self.conn.execute(command_builder("storage.getCookies", params))
         return GetCookiesResult.from_dict(result)
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    Low
    • More

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    B-devtools Includes everything BiDi or Chrome DevTools related C-py Python Bindings Review effort 3/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants