selenium icon indicating copy to clipboard operation
selenium copied to clipboard

[java] Add JSpecify nullable annotations to exception classes pt2

Open iampopovich opened this issue 5 months ago • 2 comments

User description

🔗 Related Issues

partially fixes #14291

💥 What does this PR do?

same as #16024

🔧 Implementation Notes

This pull request introduces nullability annotations to improve type safety and clarify the handling of nullable values in several exception classes within the Selenium Java codebase. The changes ensure that parameters and fields that can accept or return null are explicitly marked as nullable, while the classes themselves are marked as null-safe.

Nullability Annotations Added to Exception Classes:

  • NoSuchSessionException:

    • Added @NullMarked annotation to the class.
    • Marked the reason parameter in constructors as @Nullable.
  • ScriptTimeoutException:

    • Added @NullMarked annotation to the class.
    • Marked the message and cause parameters in constructors as @Nullable.
  • SessionNotCreatedException:

    • Added @NullMarked annotation to the class.
    • Marked the msg and cause parameters in constructors as @Nullable.
  • TimeoutException:

    • Added @NullMarked annotation to the class.
    • Marked the message and cause parameters in constructors as @Nullable.
  • UnhandledAlertException:

    • Added @NullMarked annotation to the class.
    • Marked the alertText field and the message and alertText parameters in constructors as @Nullable.
    • Updated the getAlertText method to return @Nullable.

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Added JSpecify nullability annotations to exception classes

  • Marked classes as @NullMarked for null safety

  • Annotated constructor parameters and fields as @Nullable

  • Enhanced type safety for exception handling


Changes diagram

flowchart LR
  A["Exception Classes"] --> B["@NullMarked Annotation"]
  A --> C["@Nullable Parameters"]
  B --> D["Type Safety"]
  C --> D

Changes walkthrough 📝

Relevant files
Enhancement
NoSuchSessionException.java
Added nullability annotations to NoSuchSessionException   

java/src/org/openqa/selenium/NoSuchSessionException.java

  • Added @NullMarked class annotation
  • Marked reason constructor parameters as @Nullable
  • +6/-2     
    ScriptTimeoutException.java
    Added nullability annotations to ScriptTimeoutException   

    java/src/org/openqa/selenium/ScriptTimeoutException.java

  • Added @NullMarked class annotation
  • Marked message and cause constructor parameters as @Nullable
  • +7/-3     
    SessionNotCreatedException.java
    Added nullability annotations to SessionNotCreatedException

    java/src/org/openqa/selenium/SessionNotCreatedException.java

  • Added @NullMarked class annotation
  • Marked msg and cause constructor parameters as @Nullable
  • +6/-2     
    TimeoutException.java
    Added nullability annotations to TimeoutException               

    java/src/org/openqa/selenium/TimeoutException.java

  • Added @NullMarked class annotation
  • Marked message and cause constructor parameters as @Nullable
  • +7/-3     
    UnhandledAlertException.java
    Added nullability annotations to UnhandledAlertException 

    java/src/org/openqa/selenium/UnhandledAlertException.java

  • Added @NullMarked class annotation
  • Marked alertText field and constructor parameters as @Nullable
  • Updated getAlertText method return type as @Nullable
  • +7/-3     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • iampopovich avatar Jul 08 '25 19:07 iampopovich

    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 Handling

    The constructor logic concatenates a nullable msg parameter directly in string operations without null checks, which could result in "null" being displayed in error messages when msg is null.

    public SessionNotCreatedException(@Nullable String msg) {
      super(
          "Could not start a new session. "
              + msg
              + (msg != null && msg.contains("Host info") ? "" : " \n" + getHostInformation()));
    }
    
    Null Concatenation

    The constructor concatenates nullable message and alertText parameters directly without null checks, potentially resulting in "null" strings in the exception message.

    public UnhandledAlertException(@Nullable String message, @Nullable String alertText) {
      super(message + ": " + alertText);
      this.alertText = alertText;
    }
    

    qodo-code-review[bot] avatar Jul 08 '25 19:07 qodo-code-review[bot]

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Handle null string concatenation safely

    The string concatenation with a nullable msg parameter can result in "null"
    being literally concatenated to the message. Consider using a null-safe approach
    to avoid displaying "null" in error messages.

    java/src/org/openqa/selenium/SessionNotCreatedException.java [26-31]

     public SessionNotCreatedException(@Nullable String msg) {
       super(
           "Could not start a new session. "
    -          + msg
    +          + (msg != null ? msg : "")
               + (msg != null && msg.contains("Host info") ? "" : " \n" + getHostInformation()));
     }
    
    • [ ] Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that concatenating a null string msg will result in the literal "null" in the exception message, and the proposed fix prevents this, improving message quality.

    Medium
    • [ ] Update

    qodo-code-review[bot] avatar Jul 08 '25 19:07 qodo-code-review[bot]