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

[dotnet] Utilize dedicated WebDriver Spec endpoint to get named cookie #14957

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Dec 27, 2024

User description

Description

driver.Manage().Cookies.GetCookieNamed("abc");

Motivation and Context

Related to #14884

Current: Internally it fetches all cookies and then filters out on client side.
Proposed: There is special remote endpoint, so just use it.

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


Description

  • Implements proper cookie handling according to WebDriver specification
  • Adds dedicated NoSuchCookieException class for cookie not found scenarios
  • Changes GetCookieNamed to use specific endpoint instead of filtering all cookies
  • Fixes HTTP method for GetCookie command from POST to GET
  • Improves error handling for cookie-related operations

Changes walkthrough 📝

Relevant files
Enhancement
CookieJar.cs
Refactor cookie retrieval to use dedicated endpoint           

dotnet/src/webdriver/CookieJar.cs

  • Removed try-catch block from AllCookies getter
  • Modified GetCookieNamed to use dedicated endpoint instead of filtering
    all cookies
  • Added handling of NoSuchCookieException
  • +15/-23 
    Error handling
    NoSuchCookieException.cs
    Add NoSuchCookieException class                                                   

    dotnet/src/webdriver/NoSuchCookieException.cs

  • Added new exception class for handling cookie not found scenarios
  • Implements standard exception constructors
  • +63/-0   
    WebDriver.cs
    Add NoSuchCookie error handling                                                   

    dotnet/src/webdriver/WebDriver.cs

    • Added handling for NoSuchCookie error response
    +3/-0     
    Bug fix
    W3CWireProtocolCommandInfoRepository.cs
    Update GetCookie command HTTP method                                         

    dotnet/src/webdriver/Remote/W3CWireProtocolCommandInfoRepository.cs

    • Changed GetCookie command from POST to GET method
    +1/-1     

    💡 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 🔶

    14884 - Partially compliant

    Compliant requirements:

    • API behavior should be consistent with the WebDriver specification

    Non-compliant requirements:

    • GetCookieNamed() should throw an exception when cookie is not found (according to WebDriver spec)
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Incorrect Exception Handling

    GetCookieNamed() catches NoSuchCookieException and returns null, which contradicts the WebDriver spec requirement to throw an exception when cookie is not found

    try
    {
        var rawCookie = driver.InternalExecute(DriverCommand.GetCookie, new() { { "name", name } }).Value;
    
        return Cookie.FromDictionary((Dictionary<string, object>)rawCookie);
    }
    catch (NoSuchCookieException)
    {
        return null;
    }

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add type checking before casting to ensure proper error handling of malformed responses

    Add type checking before casting rawCookie to Dictionary to handle cases where the
    response value might be of unexpected type.

    dotnet/src/webdriver/CookieJar.cs [129]

    -return Cookie.FromDictionary((Dictionary<string, object>)rawCookie);
    +if (rawCookie is Dictionary<string, object> cookieDict)
    +    return Cookie.FromDictionary(cookieDict);
    +throw new WebDriverException("Unexpected cookie format in response");
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion provides comprehensive type safety by using pattern matching and proper error handling for malformed responses. It's crucial for maintaining robust API interaction and clear error reporting.

    8
    Add null check before dictionary type casting to prevent potential runtime exceptions

    Add null check for rawCookie before attempting to cast it to Dictionary to prevent
    potential runtime exceptions.

    dotnet/src/webdriver/CookieJar.cs [127-129]

     var rawCookie = driver.InternalExecute(DriverCommand.GetCookie, new() { { "name", name } }).Value;
    +if (rawCookie == null)
    +    return null;
     return Cookie.FromDictionary((Dictionary<string, object>)rawCookie);
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion addresses a potential runtime error by adding a null check before type casting. This is important for robustness, especially since the code interacts with external responses.

    7

    @nvborisenko
    Copy link
    Member Author

    Ignore it:
    image

    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