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] Annotate nullability on HttpCommandExecutor aside from Response #14871

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

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Dec 6, 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

Annotates most of HttpCommandExecutor for nullability. This PR avoids the Execute and CreateResponse methods, because those may require bigger refactorings and those changes may need discussion. Everything in this PR should be straightforward.
 

Motivation and Context

Contributes to #14640

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

  • Annotated nullability for most of the HttpCommandExecutor class to improve code safety and clarity.
  • Converted several fields to readonly to ensure immutability where possible.
  • Simplified property accessors by using expression-bodied members.
  • Added constructors with nullability checks for HttpResponseInfo and DiagnosticsHttpHandler.
  • Used MemberNotNull attribute to ensure client is initialized before use.
  • Improved resource management by using null-conditional operators and suppressing finalization in Dispose method.

Changes walkthrough 📝

Relevant files
Enhancement
HttpCommandExecutor.cs
Enhance nullability annotations and refactor `HttpCommandExecutor`

dotnet/src/webdriver/Remote/HttpCommandExecutor.cs

  • Added nullability annotations to various methods and properties.
  • Converted fields to readonly where applicable.
  • Simplified property getters and setters.
  • Introduced constructor for HttpResponseInfo with nullability checks.
  • +44/-40 

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

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 6, 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 Check
    The CommandInfoRepository property setter has a null check but the getter could still return null since the field is not marked as readonly or non-null

    Potential Bug
    The HttpResponseInfo constructor requires a non-null body but there's no validation that responseMessage.Content.ReadAsStringAsync() doesn't return null

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 6, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Prevent usage of disposed objects by adding disposal state validation

    Add a check for isDisposed in ExecuteAsync to prevent using disposed resources.

    dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [177-180]

     public virtual async Task<Response> ExecuteAsync(Command commandToExecute)
     {
    +    if (this.isDisposed)
    +    {
    +        throw new ObjectDisposedException(nameof(HttpCommandExecutor));
    +    }
         if (this.client == null)
         {
             this.CreateHttpClient();
         }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This is a critical safety check that prevents potential bugs and resource misuse by throwing an appropriate exception when attempting to use a disposed object. This is a fundamental improvement for object lifecycle management.

    9
    Add parameter validation to prevent null reference exceptions

    Add a null check for commandToExecute parameter in the HttpRequestInfo constructor
    to prevent potential null reference exceptions.

    dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [366-372]

     public HttpRequestInfo(Uri serverUri, Command commandToExecute, HttpCommandInfo commandInfo)
     {
    +    if (commandToExecute is null)
    +    {
    +        throw new ArgumentNullException(nameof(commandToExecute));
    +    }
         if (commandInfo is null)
         {
             throw new ArgumentNullException(nameof(commandInfo));
         }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds an important null check for a critical constructor parameter that could cause runtime errors. This is a significant improvement for robustness and error handling.

    8

    💡 Need additional feedback ? start a PR chat

    @nvborisenko
    Copy link
    Member

    We should make a checkpoint, you created a lot of dependent PRs. I appreciate your contribution, but not so fast :D

    @RenderMichael
    Copy link
    Contributor Author

    I'm doing my best to keep each PR completely isolated from the other ones, so they can take as much time as they need to merge.

    Some points I am more active, other times I will go days without any contributions. I am eager for the nullability to be project-wide!

    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