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 NRT to Manage() and friends #14669

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

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Oct 28, 2024

Description

Adds NRT annotations to methods and types reachable from the driver.Manage() method.

Contributes to #14640

For methods that currently throw an exception of null is passed, that has been replaced with argument validation ahead of time. Likewise for as casts that would be null references. Other than that, no behavior changes are present.

Motivation and Context

Nullable reference types aim to fix the billion dollar mistake.

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, error handling


Description

  • Enabled nullable reference types across multiple classes and interfaces to improve null safety.
  • Added ArgumentNullException checks for methods that accept nullable parameters to enhance error handling.
  • Updated constructors and methods to handle nullable strings and objects, ensuring better null management.
  • Improved documentation comments to reflect nullable parameters and exceptions.

PRDescriptionHeader.CHANGES_WALKTHROUGH

Relevant files
Enhancement
18 files
Alert.cs
Enable nullable reference types and improve null handling in Alert
class

dotnet/src/webdriver/Alert.cs

  • Enabled nullable reference types.
  • Added null-forgiving operator to commandResponse.Value.ToString().
  • Added ArgumentNullException for SendKeys method.
  • +5/-2     
    Cookie.cs
    Enable nullable reference types and improve null handling in Cookie
    class

    dotnet/src/webdriver/Cookie.cs

  • Enabled nullable reference types.
  • Updated constructors and methods to handle nullable strings.
  • Added ArgumentNullException for null dictionary in FromDictionary.
  • +26/-23 
    CookieJar.cs
    Enable nullable reference types and improve null handling in CookieJar
    class

    dotnet/src/webdriver/CookieJar.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for null parameters.
  • Updated methods to handle nullable strings and objects.
  • +17/-9   
    ICookieJar.cs
    Enable nullable reference types and update ICookieJar interface

    dotnet/src/webdriver/ICookieJar.cs

  • Enabled nullable reference types.
  • Updated interface methods to handle nullable parameters.
  • +7/-3     
    ILogs.cs
    Enable nullable reference types and improve error handling in ILogs
    interface

    dotnet/src/webdriver/ILogs.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for GetLog method.
  • +4/-0     
    INetwork.cs
    Enable nullable reference types and improve null handling in INetwork
    interface

    dotnet/src/webdriver/INetwork.cs

  • Enabled nullable reference types.
  • Updated event handlers to be nullable.
  • Added ArgumentNullException for AddRequestHandler.
  • +5/-2     
    IOptions.cs
    Enable nullable reference types in IOptions interface       

    dotnet/src/webdriver/IOptions.cs

    • Enabled nullable reference types.
    +2/-0     
    ITargetLocator.cs
    Enable nullable reference types and improve error handling in
    ITargetLocator interface

    dotnet/src/webdriver/ITargetLocator.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for Window method.
  • +3/-0     
    ITimeouts.cs
    Enable nullable reference types in ITimeouts interface     

    dotnet/src/webdriver/ITimeouts.cs

    • Enabled nullable reference types.
    +2/-0     
    IWindow.cs
    Enable nullable reference types in IWindow interface         

    dotnet/src/webdriver/IWindow.cs

    • Enabled nullable reference types.
    +2/-0     
    ReturnedCookie.cs
    Enable nullable reference types and update ReturnedCookie class

    dotnet/src/webdriver/Internal/ReturnedCookie.cs

  • Enabled nullable reference types.
  • Updated constructor parameters to handle nullable strings.
  • +4/-2     
    LogEntry.cs
    Enable nullable reference types and improve error handling in LogEntry
    class

    dotnet/src/webdriver/LogEntry.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for FromDictionary method.
  • +15/-6   
    Logs.cs
    Enable nullable reference types and improve error handling in Logs
    class

    dotnet/src/webdriver/Logs.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for GetLog method.
  • +12/-4   
    NetworkManager.cs
    Enable nullable reference types and improve null handling in
    NetworkManager class

    dotnet/src/webdriver/NetworkManager.cs

  • Enabled nullable reference types.
  • Updated event handlers to be nullable.
  • Added ArgumentNullException for AddRequestHandler.
  • +7/-4     
    OptionsManager.cs
    Enable nullable reference types in OptionsManager class   

    dotnet/src/webdriver/OptionsManager.cs

    • Enabled nullable reference types.
    +2/-0     
    TargetLocator.cs
    Enable nullable reference types and improve error handling in
    TargetLocator class

    dotnet/src/webdriver/TargetLocator.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for Frame and Window methods.
  • +20/-6   
    Timeouts.cs
    Enable nullable reference types in Timeouts class               

    dotnet/src/webdriver/Timeouts.cs

    • Enabled nullable reference types.
    +2/-0     
    Window.cs
    Enable nullable reference types and update Window class   

    dotnet/src/webdriver/Window.cs

  • Enabled nullable reference types.
  • Updated dictionary parameters to be nullable.
  • +5/-3     

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

    Copy link
    Contributor

    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

    Possible Bug
    The SameSite property is now nullable, but the validation logic in the constructor has not been updated to handle null values. This could lead to unexpected behavior.

    Type Safety
    The cast to PasswordCredentials on line 205 is not checked. If authenticationHandler.Credentials is not of type PasswordCredentials, this could lead to a runtime exception.

    Error Handling
    The Window method catches NoSuchWindowException but then attempts to search by name without re-throwing if the search fails. This could mask the original exception and make debugging difficult.

    public static Cookie FromDictionary(Dictionary<string, object> rawCookie)
    {
    if (rawCookie == null)
    {
    throw new ArgumentNullException(nameof(rawCookie), "Dictionary cannot be null");
    }

    string name = rawCookie["name"].ToString();
    string name = rawCookie["name"].ToString()!;
    Copy link
    Contributor Author

    @RenderMichael RenderMichael Oct 28, 2024

    Choose a reason for hiding this comment

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

    If name and value are null, that'll surface in the constructor for Cookie. We can defer the null check here.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Use pattern matching for more idiomatic type checking and casting

    Use pattern matching with 'is' keyword for a more idiomatic C# approach to type
    checking and casting.

    dotnet/src/webdriver/NetworkManager.cs [49-53]

    -IDevTools? devToolsDriver = driver as IDevTools;
    -if (devToolsDriver == null)
    +if (driver is not IDevTools devToolsDriver)
     {
         throw new WebDriverException("Driver must implement IDevTools to use these features");
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion enhances the code by using pattern matching, which is a more idiomatic and modern C# approach for type checking and casting, improving both readability and maintainability.

    9
    Simplify null checking using the null-coalescing operator

    Consider using the null-coalescing operator (??) instead of a ternary operator for a
    more concise null check.

    dotnet/src/webdriver/Cookie.cs [387]

    -return string.IsNullOrEmpty(domain) ? null : domain!.Split(':')[0];
    +return domain?.Split(':')[0];
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion improves code readability and conciseness by using the null-coalescing operator, which is a more modern and idiomatic approach in C# for handling null checks.

    8
    Use switch expression with pattern matching for more concise enum parsing

    Use a switch expression with pattern matching for a more concise and modern approach
    to parsing the log level.

    dotnet/src/webdriver/Logs.cs [105-114]

    -string? levelValue = entryDictionary["level"].ToString();
    +entry.level = entryDictionary["level"].ToString() switch
    +{
    +    string s when Enum.TryParse<LogLevel>(s, ignoreCase: true, out var level) => level,
    +    _ => LogLevel.All
    +};
     
    -if (Enum.TryParse(levelValue, ignoreCase: true, out LogLevel level))
    -{
    -    entry.level = level;
    -}
    -else
    -{
    -    // If the requested log level string is not a valid log level,
    -    // ignore it and use LogLevel.All.
    -}
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion offers a more concise and modern approach to parsing enums using a switch expression with pattern matching, which can improve code readability and reduce potential errors.

    7
    Best practice
    Use null-conditional operator for safer null checking

    Use the null-forgiving operator (!) only when absolutely necessary. Consider using
    the null-conditional operator (?.) for a safer approach to null checking.

    dotnet/src/webdriver/Alert.cs [50]

    -return commandResponse.Value.ToString()!;
    +return commandResponse.Value?.ToString() ?? string.Empty;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances safety by using the null-conditional operator, which prevents potential null reference exceptions and improves code robustness.

    8

    💡 Need additional feedback ? start a PR chat

    {
    Dictionary<string, object> parameters = new Dictionary<string, object>();
    Dictionary<string, object?> parameters = new Dictionary<string, object?>();
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    I wasn't sure how to handle this. When name is null, nothing seems to happen. So maybe we can do the same as DeleteCookie(Cookie) below?

    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