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] Add nullability annotations to devtools domains #15143

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

RenderMichael
Copy link
Contributor

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

Add nullability annotations to devtools domains, as well as some event args I missed in a previous PR.

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

  • Added nullability annotations to multiple DevTools-related classes.

  • Improved immutability by replacing mutable properties with readonly or auto-properties.

  • Enhanced exception handling and safety with nullability checks and improved constructors.

  • Updated enum handling with conditional compilation for .NET 8.0 and above.


Changes walkthrough 📝

Relevant files
Enhancement
9 files
DevToolsDomains.cs
Added nullability annotations and improved constructor handling.
+7/-10   
DevToolsExtensionMethods.cs
Enabled nullability annotations for extension methods.     
+2/-0     
DevToolsOptions.cs
Enabled nullability annotations for DevTools options.       
+2/-0     
DevToolsSessionDomains.cs
Added nullability annotations and improved immutability. 
+3/-3     
DevToolsSessionEventReceivedEventArgs.cs
Added nullability annotations and made properties readonly.
+5/-3     
DevToolsSessionLogMessageEventArgs.cs
Enabled nullability annotations for log message event args.
+2/-0     
ExceptionThrownEventArgs.cs
Added nullability annotations and improved constructor usage.
+15/-2   
JsonEnumMemberConverter.cs
Enhanced enum handling with nullability and .NET 8.0 support.
+11/-4   
ResponsePausedEventArgs.cs
Enabled nullability annotations for response paused event args.
+2/-0     
Bug fix
4 files
V130JavaScript.cs
Improved exception handling with constructor-based initialization.
+5/-5     
V131JavaScript.cs
Improved exception handling with constructor-based initialization.
+5/-5     
V132JavaScript.cs
Improved exception handling with constructor-based initialization.
+5/-5     
V85JavaScript.cs
Improved exception handling with constructor-based initialization.
+5/-5     

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

  • foreach (var value in values)
    {
    var enumMember = type.GetMember(value.ToString())[0];
    var enumMember = type.GetField(value.ToString())!;
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Since we know type is an enum, we know each of the values are fields.

    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 code assumes constructor will not be null when calling GetConstructor() and using ! operator. Should add null check to handle case where constructor is not found.

    ConstructorInfo constructor = domainType.GetConstructor(new Type[] { typeof(DevToolsSession) })!;
    return (DevToolsDomains)constructor.Invoke(new object[] { session });

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add null check for domain type

    Add null check for domainType before accessing its constructor to prevent potential
    NullReferenceException if no matching domain type is found.

    dotnet/src/webdriver/DevTools/DevToolsDomains.cs [100-101]

    +if (domainType == null)
    +    throw new ArgumentException("No matching domain type found for the specified version");
     ConstructorInfo constructor = domainType.GetConstructor(new Type[] { typeof(DevToolsSession) })!;
     return (DevToolsDomains)constructor.Invoke(new object[] { session });
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a critical potential null reference exception that could occur if no matching domain type is found, improving the robustness of the code.

    8
    Add null check for attribute value

    Add null check for attr before accessing its value to prevent potential
    NullReferenceException when processing enum members without EnumMemberAttribute.

    dotnet/src/webdriver/DevTools/Json/JsonEnumMemberConverter.cs [49-51]

     var attr = enumMember.GetCustomAttributes(typeof(EnumMemberAttribute), false)
       .Cast<EnumMemberAttribute>()
       .FirstOrDefault();
    +if (attr?.Value == null)
    +    continue;
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion prevents potential null reference exceptions when processing enum members without EnumMemberAttribute, enhancing code stability.

    7


    return domains;
    ConstructorInfo constructor = domainType.GetConstructor(new Type[] { typeof(DevToolsSession) })!;
    return (DevToolsDomains)constructor.Invoke(new object[] { session });
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    The existing code checks for missing constructors, but this can never be the case. We know the 4 possible domain types. All of them have the right constructor, and are an instance of the DevToolsDomains type.

    Looking around at how this method is used, it is expected that the returned value is not null. Otherwise, we would be getting null reference exceptions.

    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