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 DriverOptions #15167

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

RenderMichael
Copy link
Contributor

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

Description

Motivation and Context

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 properties and methods in DriverOptions.

  • Replaced backing fields with auto-implemented properties.

  • Added nullability-related attributes and checks for improved safety.

  • Refactored methods to align with nullability annotations.


Changes walkthrough 📝

Relevant files
Enhancement
DriverOptions.cs
Annotate nullability and refactor `DriverOptions`               

dotnet/src/webdriver/DriverOptions.cs

  • Enabled nullable reference types with #nullable enable.
  • Annotated properties and methods with nullable types and attributes.
  • Replaced private backing fields with auto-implemented properties.
  • Added null checks and exception handling for nullability compliance.
  • +74/-120

    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

    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 ToDictionary() method could throw a NullReferenceException if ToCapabilities() returns null, since the null check is only done after casting to IHasCapabilitiesDictionary.

    ICapabilities? capabilities = this.ToCapabilities();
    if (capabilities is not IHasCapabilitiesDictionary desired)

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add null check for parameter

    Add null check for capabilityName parameter in IsKnownCapabilityName method to
    prevent potential NullReferenceException when calling ContainsKey.

    dotnet/src/webdriver/DriverOptions.cs [402-405]

    -protected bool IsKnownCapabilityName(string capabilityName)
    +protected bool IsKnownCapabilityName(string? capabilityName)
     {
    -    return this.knownCapabilityNames.ContainsKey(capabilityName);
    +    return capabilityName != null && this.knownCapabilityNames.ContainsKey(capabilityName);
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential NullReferenceException by adding a null check for the capabilityName parameter, which is important for runtime stability and follows C# nullable reference type best practices.

    8
    Add null check for proxy

    Add null check for proxyCapability before accessing it to prevent potential
    NullReferenceException in GenerateDesiredCapabilities.

    dotnet/src/webdriver/DriverOptions.cs [540-544]

    -Dictionary<string, object?>? proxyCapability = this.Proxy.ToCapability();
    +Dictionary<string, object?>? proxyCapability = this.Proxy?.ToCapability();
     if (!isSpecificationCompliant)
     {
    -    proxyCapability = this.Proxy.ToLegacyCapability();
    +    proxyCapability = this.Proxy?.ToLegacyCapability();
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion prevents potential NullReferenceException by adding null-conditional operators when accessing Proxy methods, which is crucial for runtime safety in a property that's already marked as nullable.

    8

    @nvborisenko
    Copy link
    Member

    In general I would say everything might be nullable. But changing bool to bool? is a breaking change? Is it transformed from bool to Nullable<bool>? If yes, then proceed as is without breaking changes.

    @RenderMichael
    Copy link
    Contributor Author

    It is a breaking change, users would be affected if they read the property. All the reference types are made nullable here.

    @RenderMichael RenderMichael merged commit 201fd29 into SeleniumHQ:trunk Jan 27, 2025
    10 checks passed
    @RenderMichael RenderMichael deleted the driver-options branch January 27, 2025 19:18
    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