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] Nullness annotations for Cookie and Platform #15062

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

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Jan 10, 2025

User description

Description

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

  • org.openqa.selenium.Cookie
  • org.openqa.selenium.Platform

These classes are listed in the 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 Cookie and Platform classes.

  • Marked nullable fields and methods with @Nullable for clarity.

  • Introduced @NullMarked to enforce nullness checking in classes.

  • Improved IDE and static analysis compatibility for null safety.


Changes walkthrough 📝

Relevant files
Enhancement
Cookie.java
Add nullness annotations to `Cookie` class                             

java/src/org/openqa/selenium/Cookie.java

  • Added @NullMarked annotation to the Cookie class.
  • Marked nullable fields and methods with @Nullable.
  • Updated constructors and builder methods to handle nullable
    parameters.
  • Enhanced null safety and static analysis compatibility.
  • +39/-26 
    Platform.java
    Add nullness annotations to `Platform` enum                           

    java/src/org/openqa/selenium/Platform.java

  • Added @NullMarked annotation to the Platform enum.
  • Marked nullable methods and fields with @Nullable.
  • Updated family method to return nullable Platform.
  • Improved null safety and static analysis compatibility.
  • +34/-31 

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

    • Add JSpecify Nullness annotations to Selenium framework code
    • Specify which parameters and return values can be null
    • Make nullness information transparent to IDEs and static code analyzers

    Requires further human verification:

    • Improve interoperability with Kotlin - needs testing with Kotlin codebase
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Null Safety

    Verify that all nullable fields and methods are correctly annotated and that null checks are properly handled in the code, especially for the Cookie constructor parameters

    private final @Nullable String domain;
    private final @Nullable Date expiry;
    private final boolean isSecure;
    private final boolean isHttpOnly;
    private final @Nullable String sameSite;

    @mk868 mk868 force-pushed the nullness-Cookie-Platform branch from 2683582 to 9245a1a Compare January 10, 2025 21:45
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation for required non-null parameters in constructor to fail fast and provide clear error messages

    Add null checks for name and value parameters in constructors since they are
    required fields but not marked as nullable. Consider throwing
    IllegalArgumentException if null is provided.

    java/src/org/openqa/selenium/Cookie.java [134-146]

     public Cookie(
         String name,
         String value,
         @Nullable String domain,
         @Nullable String path,
         @Nullable Date expiry,
         boolean isSecure,
         boolean isHttpOnly,
         @Nullable String sameSite) {
    +  if (name == null) {
    +    throw new IllegalArgumentException("Cookie name cannot be null");
    +  }
    +  if (value == null) {
    +    throw new IllegalArgumentException("Cookie value cannot be null");
    +  }
       this.name = name;
       this.value = value;
       this.path = path == null || path.isEmpty() ? "/" : path;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding null validation for required fields is important for preventing NullPointerExceptions and providing clear error messages. Since name and value are not marked as @nullable, they should be validated early.

    8
    Add validation for required non-null parameters in builder constructor to fail fast

    Add null checks in the Builder constructor for required fields name and value since
    they are not marked as nullable.

    java/src/org/openqa/selenium/Cookie.java [318-321]

     public Builder(String name, String value) {
    +  if (name == null) {
    +    throw new IllegalArgumentException("Cookie name cannot be null");
    +  }
    +  if (value == null) {
    +    throw new IllegalArgumentException("Cookie value cannot be null");
    +  }
       this.name = name;
       this.value = value;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Similar to the main constructor, the Builder should validate its required parameters. This prevents potential NullPointerExceptions and maintains consistency with the main class's null safety.

    8
    Add validation for non-null parameter to prevent NullPointerException

    Add null check for matcher parameter in isBetterMatch() method since it is not
    marked as nullable but is compared with a nullable value.

    java/src/org/openqa/selenium/Platform.java [499-501]

     private static boolean isBetterMatch(@Nullable String previous, String matcher) {
    +  if (matcher == null) {
    +    throw new IllegalArgumentException("matcher cannot be null");
    +  }
       return previous == null || matcher.length() >= previous.length();
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The matcher parameter is not marked as @nullable but is used in length() comparison, which could cause NullPointerException. Adding validation improves null safety and error clarity.

    7

    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