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]: handle named get_cookie and delete_cookie for None and empty strings #15073

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

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Jan 13, 2025

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

Addresses #15044 for py bindings.

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

Bug fix, Tests


Description

  • Added validation to raise ValueError for empty or whitespace cookie names in get_cookie and delete_cookie.

  • Updated get_cookie and delete_cookie docstrings to reflect new behavior.

  • Introduced new tests to verify ValueError is raised for invalid cookie names.

  • Ensured existing functionality remains unaffected by the changes.


Changes walkthrough 📝

Relevant files
Bug fix
webdriver.py
Add validation for cookie name in `get_cookie` and `delete_cookie`

py/selenium/webdriver/remote/webdriver.py

  • Added validation to raise ValueError for empty or whitespace cookie
    names.
  • Updated docstrings for get_cookie and delete_cookie to include new
    behavior.
  • Ensured Firefox-specific behavior is addressed when deleting cookies.
  • +19/-3   
    Tests
    cookie_tests.py
    Add tests for cookie name validation                                         

    py/test/selenium/webdriver/common/cookie_tests.py

  • Added tests for get_cookie to raise ValueError for empty names.
  • Added tests for delete_cookie to raise ValueError for empty names.
  • Verified no unintended deletions occur when invalid names are used.
  • +20/-0   

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling

    Verify that the error handling for empty/whitespace cookie names is consistent across all supported browsers and doesn't break existing functionality

        if not name or name.isspace():
            raise ValueError("Cookie name cannot be empty")
    
        with contextlib.suppress(NoSuchCookieException):
            return self.execute(Command.GET_COOKIE, {"name": name})["value"]
    
        return None
    
    def delete_cookie(self, name) -> None:
        """Deletes a single cookie with the given name. Raises ValueError if
        the name is empty or whitespace.
    
        :Usage:
            ::
    
                driver.delete_cookie('my_cookie')
    
        :Raises:
            ValueError: If the cookie name is empty or whitespace.
        """
    
        # firefox deletes all cookies when "" is passed as name
        if not name or name.isspace():
            raise ValueError("Cookie name cannot be empty")

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 13, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    ✅ Expand test coverage to include whitespace-only input validation
    Suggestion Impact:The commit implemented the suggested test case for whitespace-only cookie names and also added additional validation for None values

    code diff:

    +    with pytest.raises(ValueError, match="Cookie name cannot be empty"):
    +        driver.get_cookie("   ")

    Add test case for whitespace-only cookie names to verify they are properly rejected.

    py/test/selenium/webdriver/common/cookie_tests.py [157-160]

     def test_get_cookie_raises_value_error_for_empty_name(cookie, driver):
         driver.add_cookie(cookie)
         with pytest.raises(ValueError, match="Cookie name cannot be empty"):
             driver.get_cookie("")
    +    with pytest.raises(ValueError, match="Cookie name cannot be empty"):
    +        driver.get_cookie("   ")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This is a valuable suggestion that improves test coverage by verifying the new whitespace validation functionality, which is a key part of the PR's changes. The test would help ensure the new validation works as intended.

    8
    Normalize input by stripping whitespace to handle edge cases consistently

    Strip whitespace from cookie name before validation to handle edge cases where name
    has leading/trailing spaces.

    py/selenium/webdriver/remote/webdriver.py [653-654]

    -if not name or name.isspace():
    +name = name.strip()
    +if not name:
         raise ValueError("Cookie name cannot be empty")
    • Apply this suggestion
    Suggestion importance[1-10]: 2

    Why: While the suggestion aims to handle edge cases, stripping whitespace would actually change the intended behavior. The PR explicitly wants to reject whitespace-only names as invalid, not normalize them.

    2

    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