Skip to content

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Jan 14, 2025

User description

Description

In this PR I'm adding nullness annotations for classes:

  • org.openqa.selenium.NoSuchElementException
  • org.openqa.selenium.NotFoundException
  • org.openqa.selenium.StaleElementReferenceException
  • org.openqa.selenium.WebDriverException

NullAway analysis: #14421

Motivation and Context

The JSpecify nullness annotations will give developers better exposure to potential problems with their code to avoid NullPointerExceptions.
Related issue: #14291

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

  • Added JSpecify Nullness annotations to exception classes for better null safety.

  • Marked constructors and methods with @Nullable where applicable.

  • Introduced @NullMarked annotation to enforce nullness checks in classes.

  • Improved IDE and static analyzer interoperability for null pointer warnings.


Changes walkthrough 📝

Relevant files
Enhancement
NoSuchElementException.java
Add nullness annotations to NoSuchElementException             

java/src/org/openqa/selenium/NoSuchElementException.java

  • Added @NullMarked annotation to the class.
  • Marked constructor parameters and methods with @Nullable.
  • Enhanced null safety for getSupportUrl method.
  • +7/-3     
    NotFoundException.java
    Add nullness annotations to NotFoundException                       

    java/src/org/openqa/selenium/NotFoundException.java

  • Added @NullMarked annotation to the class.
  • Marked constructor parameters with @Nullable.
  • Improved null safety for exception handling.
  • +7/-3     
    StaleElementReferenceException.java
    Add nullness annotations to StaleElementReferenceException

    java/src/org/openqa/selenium/StaleElementReferenceException.java

  • Added @NullMarked annotation to the class.
  • Marked constructor parameters and methods with @Nullable.
  • Enhanced null safety for getSupportUrl method.
  • +7/-3     
    WebDriverException.java
    Add nullness annotations to WebDriverException                     

    java/src/org/openqa/selenium/WebDriverException.java

  • Added @NullMarked annotation to the class.
  • Marked constructors, methods, and parameters with @Nullable.
  • Improved null safety for getMessage, getRawMessage, and createMessage
    methods.
  • +10/-7   

    💡 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:

    🎫 Ticket compliance analysis ✅

    14291 - PR Code Verified

    Compliant requirements:

    • Added JSpecify Nullness annotations to exception classes
    • Marked nullable parameters and return values with @nullable
    • Made nullness information transparent to IDEs via annotations

    Requires further human verification:

    • Verify that the annotations improve Kotlin interoperability
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Annotation Consistency

    The createMessage() method is marked to accept @nullable parameter but doesn't handle the null case explicitly. Consider adding null check or documenting the expected behavior.

    private String createMessage(@Nullable String originalMessageString) { String supportMessage = Optional.ofNullable(getSupportUrl()) .map(url -> String.format("For documentation on this error, please visit: %s", url)) .orElse("");
    @mk868
    Copy link
    Contributor Author

    mk868 commented Jan 14, 2025

    Fixed NullAway errors:

    java/src/org/openqa/selenium/WebDriverException.java:62: error: [NullAway] returning @Nullable expression from method with @NonNull return type return getCause() instanceof WebDriverException ^ (see http://t.uber.com/nullaway ) java/src/org/openqa/selenium/WebDriverException.java:64: error: [NullAway] passing @Nullable parameter 'super.getMessage()' where @NonNull is required : createMessage(super.getMessage()); ^ (see http://t.uber.com/nullaway ) java/src/org/openqa/selenium/WebDriverException.java:74: error: [NullAway] returning @Nullable expression from method with @NonNull return type return super.getMessage(); ^ (see http://t.uber.com/nullaway ) java/src/org/openqa/selenium/WebDriverException.java:109: error: [NullAway] returning @Nullable expression from method with @NonNull return type return null; ^ (see http://t.uber.com/nullaway ) 
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add explicit null handling for message parameter to prevent potential null pointer exceptions

    The createMessage method should handle null originalMessageString parameter more
    explicitly by providing a default value or message when null is passed, rather than
    potentially passing null to String.format() calls.

    java/src/org/openqa/selenium/WebDriverException.java [80-84]

     private String createMessage(@Nullable String originalMessageString) { + String message = originalMessageString != null ? originalMessageString : "Unknown error"; String supportMessage = Optional.ofNullable(getSupportUrl()) .map(url -> String.format("For documentation on this error, please visit: %s", url)) .orElse("");
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential null pointer vulnerability by explicitly handling null message parameters. This is particularly important for exception handling and aligns well with the PR's focus on null safety.

    8
    @diemol diemol merged commit 9ef48bf into SeleniumHQ:trunk Jan 15, 2025
    @mk868 mk868 deleted the jspecify-exceptions branch January 15, 2025 16:26
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
    gryznar pushed a commit to gryznar/selenium that referenced this pull request May 17, 2025
    Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    2 participants