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

[java] handle getNamedCookie and deleteNamedCookie for empty strings #15092

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

Conversation

Delta456
Copy link
Contributor

@Delta456 Delta456 commented Jan 15, 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

Fixes #15044 for Java 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 prevent empty or null cookie names in deleteCookieNamed and getCookieNamed methods.

  • Introduced unit tests to verify exceptions for empty or whitespace cookie names.

  • Enhanced error handling for cookie management in RemoteWebDriver.


Changes walkthrough 📝

Relevant files
Bug fix
RemoteWebDriver.java
Add validation for empty cookie names in RemoteWebDriver 

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

  • Added validation to check for empty or whitespace cookie names in
    deleteCookieNamed.
  • Added validation to check for empty or whitespace cookie names in
    getCookieNamed.
  • Enhanced error handling for cookie management methods.
  • +6/-0     
    Tests
    CookieImplementationTest.java
    Add tests for empty cookie name validation                             

    java/test/org/openqa/selenium/CookieImplementationTest.java

  • Added tests to verify exceptions for empty or whitespace cookie names
    in deleteCookieNamed.
  • Added tests to verify exceptions for empty or whitespace cookie names
    in getCookieNamed.
  • Ensured robust test coverage for cookie management.
  • +13/-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:

    🎫 Ticket compliance analysis 🔶

    15044 - Partially compliant

    Compliant requirements:

    • Handle cases where cookie name is null or empty

    Non-compliant requirements:

    • Implement dedicated WebDriver Spec endpoint to get named cookie
    • Use GET /session/{sessionId}/cookie/{name} request for getting cookie by name
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Incomplete Validation

    The cookie name validation only checks for empty string and single space. Should also validate for null and other whitespace characters.

    if (name.isEmpty() || name.equals(" ")) {
        throw new IllegalArgumentException("Cookie name cannot be empty or null");
    }
    Spec Non-compliance

    The implementation still uses getCookies() and filters internally instead of using the dedicated WebDriver Spec endpoint as required.

    Set<Cookie> allCookies = getCookies();
    for (Cookie cookie : allCookies) {
      if (cookie.getName().equals(name)) {
        return cookie;

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 15, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Add defensive null check to prevent potential NullPointerException
    Suggestion Impact:Instead of adding an explicit null check, the commit added @NotNull annotation to enforce null safety at compile time

    code diff:

    -    public void deleteCookieNamed(String name) {
    +    public void deleteCookieNamed(@NotNull String name) {
           if (name.isEmpty() || name.equals(" ")) {
    -          throw new IllegalArgumentException("Cookie name cannot be empty or null");
    +        throw new IllegalArgumentException("Cookie name cannot be empty");
           }

    Add null check before string validation to prevent NullPointerException if null name
    is passed.

    java/src/org/openqa/selenium/remote/RemoteWebDriver.java [872-874]

    -if (name.isEmpty() || name.equals(" ")) {
    +if (name == null || name.isEmpty() || name.equals(" ")) {
         throw new IllegalArgumentException("Cookie name cannot be empty or null");
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding a null check is crucial to prevent NullPointerException, which could crash the application. This is a critical defensive programming practice for public methods.

    9
    General
    ✅ Improve whitespace validation by using a more comprehensive string validation method
    Suggestion Impact:The commit implemented the suggestion by replacing isEmpty() || equals(' ') with isBlank() in two places, though with slightly different error messages and added @NotNull annotations

    code diff:

    -    public void deleteCookieNamed(String name) {
    -      if (name.isEmpty() || name.equals(" ")) {
    -          throw new IllegalArgumentException("Cookie name cannot be empty or null");
    +    public void deleteCookieNamed(@NotNull String name) {
    +      if (name.isBlank()) {
    +        throw new IllegalArgumentException("Cookie name cannot be empty");
           }
           execute(DriverCommand.DELETE_COOKIE(name));
         }
    @@ -929,9 +930,9 @@
         }
     
         @Override
    -    public Cookie getCookieNamed(String name) {
    -      if (name.isEmpty() || name.equals(" ")) {
    -          throw new IllegalArgumentException("Cookie name cannot be empty or null");
    +    public Cookie getCookieNamed(@NotNull String name) {
    +      if (name.isBlank()) {
    +        throw new IllegalArgumentException("Cookie name cannot be empty");
           }

    Use String.isBlank() method instead of checking for empty string and single space
    separately. This will handle all whitespace variations more robustly.

    java/src/org/openqa/selenium/remote/RemoteWebDriver.java [872-874]

    -if (name.isEmpty() || name.equals(" ")) {
    -    throw new IllegalArgumentException("Cookie name cannot be empty or null");
    +if (name == null || name.isBlank()) {
    +    throw new IllegalArgumentException("Cookie name cannot be empty, null or whitespace");
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using String.isBlank() is a more robust solution as it handles all types of whitespace characters, not just empty string and single space. This improves code reliability and maintainability.

    7

    @VietND96 VietND96 requested review from pujagani and diemol January 19, 2025 00:44
    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.

    [🚀 Feature]: Use dedicated WebDriver Spec endpoint to get named cookie
    2 participants