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 DevTools and event args #15252

Merged
merged 2 commits into from
Feb 9, 2025

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Feb 7, 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.

Annotate nullability on DevTools and event args

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, bug fix


Description

  • Annotated nullability across multiple classes and interfaces.

  • Improved exception handling with specific exception types.

  • Refactored and optimized code for better readability and maintainability.

  • Added constructors and updated properties for better initialization and null safety.


Changes walkthrough 📝

Relevant files
Enhancement
18 files
ChromiumOptions.cs
Updated method and dictionary to support nullable objects.
+2/-2     
DevToolsSession.cs
Added exception handling and nullability annotations.       
+5/-0     
DevToolsVersionInfo.cs
Enabled nullability and added null checks for properties.
+36/-16 
ICommand.cs
Enabled nullability and simplified interface definitions.
+5/-10   
IDevTools.cs
Added nullability annotations and exception documentation.
+3/-0     
IDevToolsSession.cs
Added exception handling and nullability annotations.       
+5/-1     
WebSocketConnection.cs
Refactored for null safety and improved exception handling.
+21/-11 
V130Network.cs
Refactored HTTP request and response handling for clarity.
+22/-27 
V131Network.cs
Refactored HTTP request and response handling for clarity.
+22/-27 
V132Network.cs
Refactored HTTP request and response handling for clarity.
+22/-27 
V85Network.cs
Refactored HTTP request and response handling for clarity.
+22/-27 
DomMutatedEventArgs.cs
Enabled nullability for better type safety.                           
+2/-0     
DomMutationData.cs
Added nullability annotations and default initializations.
+7/-5     
DriverOptionsMergeResult.cs
Added nullability annotations for conflict option name.   
+5/-3     
DriverProcessStartedEventArgs.cs
Refactored for null safety and added exception handling. 
+16/-21 
DriverProcessStartingEventArgs.cs
Added nullability annotations and exception handling.       
+5/-7     
HttpRequestData.cs
Added constructor and nullability annotations for initialization.
+22/-3   
HttpResponseData.cs
Added constructor and nullability annotations for initialization.
+27/-15 
Bug fix
2 files
JsonEnumMemberConverter.cs
Added null-forgiving operator to prevent null warnings.   
+1/-1     
DriverCommand.cs
Fixed typo in command description and enabled nullability.
+4/-1     
Additional files
12 files
HttpResponseContent.cs +4/-1     
IActionExecutor.cs +5/-0     
IAllowsFileDetection.cs +5/-0     
IHasCommandExecutor.cs +2/-0     
ITargetLocator.cs +1/-0     
JavaScriptCallbackExecutedEventArgs.cs +2/-0     
JavaScriptConsoleApiCalledEventArgs.cs +4/-2     
JavaScriptExceptionThrownEventArgs.cs +2/-0     
NetworkManager.cs +1/-1     
NetworkRequestSentEventArgs.cs +10/-12 
NetworkResponseReceivedEventArgs.cs +14/-17 
Response.cs +2/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    qodo-merge-pro bot commented Feb 7, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Null Safety

    The constructor requires non-null parameters but doesn't validate them. Consider adding null checks for required parameters like requestId, url, and resourceType.

    public HttpResponseData(string requestId, string url, string resourceType, long statusCode, string? errorReason)
    {
        RequestId = requestId;
        Url = url;
        ResourceType = resourceType;
        StatusCode = statusCode;
        ErrorReason = errorReason;
    }
    Potential Bug

    Creating HttpRequestData with all null parameters could cause issues. Consider creating a more appropriate constructor or method for this use case.

    HttpRequestData requestData = new HttpRequestData(null, null, null, null, requestId: e.ResponseData.RequestId);

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix null parameter violation

    Avoid passing null values to the HttpRequestData constructor as it violates the
    constructor's non-null parameter constraints. Instead, use empty strings or
    empty collections for required parameters.

    dotnet/src/webdriver/NetworkManager.cs [265]

    -HttpRequestData requestData = new HttpRequestData(null, null, null, null, requestId: e.ResponseData.RequestId);
    +HttpRequestData requestData = new HttpRequestData(string.Empty, string.Empty, null, new Dictionary<string, string>(), e.ResponseData.RequestId);
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies a potential runtime issue where null values are passed to a constructor that expects non-null parameters. The fix properly initializes required parameters with empty values.

    Medium
    Add regex match validation

    Add null check for regex match groups before accessing values to prevent
    potential NullReferenceException if pattern doesn't match.

    dotnet/src/webdriver/DevTools/DevToolsVersionInfo.cs [53]

    -return Regex.Match(Browser, ".*/(.*)").Groups[1].Value;
    +var match = Regex.Match(Browser, ".*/(.*)");
    +if (!match.Success || match.Groups.Count < 2)
    +    throw new InvalidOperationException("Invalid browser version format");
    +return match.Groups[1].Value;
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion adds important validation for regex match success and group count, which could prevent runtime exceptions if the Browser string doesn't match the expected format.

    Medium
    Add null check for safety

    The null-forgiving operator (!) is used without verifying if GetField() returns
    null, which could cause a NullReferenceException. Add a null check before
    accessing the field.

    dotnet/src/webdriver/DevTools/Json/JsonEnumMemberConverter.cs [47]

    -var enumMember = type.GetField(value.ToString())!;
    +var enumMember = type.GetField(value.ToString()) ?? throw new InvalidOperationException($"Enum field {value} not found in type {type.Name}");
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion addresses a potential NullReferenceException by replacing the null-forgiving operator with a proper null check and meaningful exception message, improving error handling and debugging.

    Medium
    Add null check for buffer array

    Add null check for buffer.Array before using it in ReceiveData() method to
    prevent potential NullReferenceException when creating the buffer.

    dotnet/src/webdriver/DevTools/WebSocketConnection.cs [268]

    -messageBuilder.Append(Encoding.UTF8.GetString(buffer.Array!, 0, receiveResult.Count));
    +if (buffer.Array is null) throw new InvalidOperationException("WebSocket buffer array is null");
    +messageBuilder.Append(Encoding.UTF8.GetString(buffer.Array, 0, receiveResult.Count));
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    __

    Why: The suggestion is valid but unnecessary since the code already uses the null-forgiving operator (!), indicating that the developer has already considered and handled this case.

    Low

    @RenderMichael
    Copy link
    Contributor Author

    From automated PR review:

    The constructor requires non-null parameters but doesn't validate them. Consider adding null checks for required parameters like requestId, url, and resourceType.

    Every non-nullable parameter which doesn't get validated is intentionally so. Each of those values comes from the CDP responses. Values are marked as optional/non-optional in the protocol spec, but I do not want Selenium to be the problem which comes inbetween an accidentally null value.

    @RenderMichael
    Copy link
    Contributor Author

    Test failures are unrelated to this PR

    //java/test/org/openqa/selenium/mobile:NetworkConnectionTest
    //java/test/org/openqa/selenium/remote:RemoteWebDriverScreenshotTest
    //java/test/org/openqa/selenium/remote:RemoteWebDriverScreenshotTest-firefox-beta
    //rb/spec/integration/selenium/webdriver:network-firefox-beta-bidi
    //rb/spec/integration/selenium/webdriver:network-firefox-bidi

    @RenderMichael RenderMichael merged commit a44f1d7 into SeleniumHQ:trunk Feb 9, 2025
    9 of 10 checks passed
    @RenderMichael RenderMichael deleted the devtools-event-args branch February 9, 2025 07:04
    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