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] Proper implementation of cookies management #14883

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

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Dec 9, 2024

User description

Description

  • Use specification remote endpoint to retrieve a named cookie, instead of fetching all cookies and filter it out on client side
  • Introduced new NoSuchCookieException
  • All methods will throw an exception in case of null cookie name

Motivation and Context

Fix #14876

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, Enhancement


Description

  • Simplified and refactored the CookieJar class to remove redundant code and improve cookie management.
  • Introduced NoSuchCookieException to handle cases where a cookie is not found.
  • Implemented null checks and exception handling for cookie operations.
  • Corrected the HTTP method for retrieving a named cookie to align with the specification.
  • Enhanced documentation for methods in ICookieJar to include exception details.

Changes walkthrough 📝

Relevant files
Enhancement
CookieJar.cs
Simplify CookieJar class and improve cookie management     

dotnet/src/webdriver/CookieJar.cs

  • Simplified the CookieJar class by removing redundant code.
  • Added null checks for cookie and name parameters.
  • Implemented retrieval of a named cookie using a remote endpoint.
  • Introduced ArgumentNullException for null parameters.
  • +25/-68 
    NoSuchCookieException.cs
    Introduce NoSuchCookieException class                                       

    dotnet/src/webdriver/NoSuchCookieException.cs

  • Introduced NoSuchCookieException class for handling missing cookies.
  • +63/-0   
    Documentation
    ICookieJar.cs
    Add exception handling documentation to ICookieJar             

    dotnet/src/webdriver/ICookieJar.cs

  • Added exception documentation for methods.
  • Introduced ArgumentNullException and NoSuchCookieException.
  • +6/-0     
    Bug fix
    W3CWireProtocolCommandInfoRepository.cs
    Correct HTTP method for GetCookie command                               

    dotnet/src/webdriver/Remote/W3CWireProtocolCommandInfoRepository.cs

    • Corrected HTTP method for GetCookie command to GET.
    +1/-1     
    Error handling
    WebDriver.cs
    Handle NoSuchCookieException in WebDriver error responses

    dotnet/src/webdriver/WebDriver.cs

    • Added handling for NoSuchCookieException in error responses.
    +3/-0     

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

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 9, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    14876 - Fully compliant

    Compliant requirements:

    • Fix bug where DeleteCookieNamed(null) deletes all cookies by adding null check
    • Implement proper handling of null cookie names by throwing ArgumentNullException
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    GetCookieNamed method may throw NoSuchCookieException but the interface documentation still mentions returning null for non-existent cookies

    Exception Handling
    The try-catch block in GetAllCookies method has no specific exception type, which could mask important errors

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 9, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Properly handle missing cookie scenarios by throwing the appropriate exception

    The GetCookieNamed method should throw NoSuchCookieException when the cookie is not
    found, as specified in the interface documentation.

    dotnet/src/webdriver/CookieJar.cs [72-74]

     var rawCookie = driver.InternalExecute(DriverCommand.GetCookie, new() { { "name", name } }).Value;
    +if (rawCookie == null)
    +{
    +    throw new NoSuchCookieException($"No cookie found with name '{name}'");
    +}
     return Cookie.FromDictionary(rawCookie as Dictionary<string, object>);
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion ensures compliance with the interface contract by throwing NoSuchCookieException when a cookie is not found, as documented in ICookieJar. This is a critical improvement for proper error handling.

    9
    Add null check for array conversion to prevent runtime exceptions

    The GetAllCookies() method should check if cookies is null before attempting to
    process it, to prevent potential NullReferenceException.

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

     object[] cookies = returned as object[];
    +if (cookies == null)
    +{
    +    return new ReadOnlyCollection<Cookie>(new List<Cookie>());
    +}
     try
     {
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential null reference exception that could occur during runtime. Adding a null check with proper handling improves the robustness of the code significantly.

    8

    💡 Need additional feedback ? start a PR chat

    @nvborisenko
    Copy link
    Member Author

    /automerge in 5 days

    @nvborisenko
    Copy link
    Member Author

    Blocked by #14884

    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.

    [🐛 Bug]: Delete named cookie deletes all cookies
    2 participants