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

[java] Add nullness for exceptions #15081

Merged
merged 3 commits into from
Jan 15, 2025
Merged

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