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] Refactor away private constructor from Response #14877

Merged
merged 5 commits into from
Dec 11, 2024

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Dec 9, 2024

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

This removes one of the three constructors on the Response type, moving dictionary parsing responsibility to the correct method FromJson.

Motivation and Context

Incremental step, part of desires I expressed in #14839 about making the Response type more immutable.

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

enhancement


Description

  • Refactored the Response class to remove the private constructor that accepted a dictionary, simplifying the class design.
  • Enhanced the FromJson method to handle JSON deserialization and object creation, improving code readability and maintainability.
  • Improved error handling by throwing an exception when a null JSON response is encountered.
  • Simplified the logic for setting SessionId and Value properties, ensuring more robust and clear code execution.

Changes walkthrough 📝

Relevant files
Enhancement
Response.cs
Refactor `Response` class to improve JSON handling             

dotnet/src/webdriver/Response.cs

  • Removed the private constructor that accepted a dictionary.
  • Refactored FromJson method to handle JSON deserialization and object
    creation.
  • Improved error handling for null JSON responses.
  • Simplified logic for setting SessionId and Value properties.
  • +22/-22 

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

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

    Null Reference
    The code doesn't check if rawResponse["sessionId"] is null before calling ToString() on it, which could cause a NullReferenceException

    Code Smell
    The method contains commented TODO about removing an if statement, but the code still keeps the logic. This should either be addressed or the comment should be removed/updated

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 9, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add defensive null handling to prevent potential null reference exceptions when accessing dictionary values

    Add null check for rawResponse["sessionId"] before calling ToString() to prevent
    potential NullReferenceException.

    dotnet/src/webdriver/Response.cs [73-76]

    -if (rawResponse["sessionId"] != null)
    +if (rawResponse["sessionId"] is not null)
     {
    -    @this.SessionId = rawResponse["sessionId"].ToString();
    +    var sessionId = rawResponse["sessionId"];
    +    @this.SessionId = sessionId?.ToString();
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential null reference vulnerability by adding proper null checks and safe access patterns, which is important for code robustness and preventing runtime exceptions.

    8
    Validate dictionary key existence before accessing to prevent potential exceptions

    Add validation for dictionary key existence before accessing
    valueDictionary["value"] to prevent potential KeyNotFoundException.

    dotnet/src/webdriver/Response.cs [115-117]

    -else
    +else if (valueDictionary.ContainsKey("value"))
     {
         @this.Value = valueDictionary["value"];
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion prevents a potential KeyNotFoundException by validating the key existence before accessing the dictionary value, which is a critical defensive programming practice.

    8

    💡 Need additional feedback ? start a PR chat

    Copy link
    Member

    @nvborisenko nvborisenko left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thanks @RenderMichael !

    @nvborisenko nvborisenko merged commit 0174bce into SeleniumHQ:trunk Dec 11, 2024
    9 of 10 checks passed
    @RenderMichael RenderMichael deleted the response-ctor branch December 11, 2024 19:25
    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.

    2 participants