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] JSpecify annotations for capabilities #14397

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

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Aug 14, 2024

User description

Description

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

  • Capabilities
  • HasCapabilities
  • ImmutableCapabilities
  • MutableCapabilities
  • PersistentCapabilities

Capabilities

  • getPlatformName() - can return null value -> marked with @Nullable
  • getCapability(String capabilityName) - can return null value when capability with specified name does not exist -> marked with @Nullable

HasCapabilities

No null values

ImmutableCapabilities

  • getCapability(String capabilityName) - follows from Capabilities#getCapability(String capabilityName) -> marked with @Nullable
  • equals(Object o) - null parameter allowed -> marked with @Nullable

MutableCapabilities

  • setCapability(String key, Object value) - null value allowed (then capability with specified key will be removed) -> marked with @Nullable
  • getCapability(String capabilityName) - follows from Capabilities#getCapability(String capabilityName) -> marked with @Nullable
  • equals(Object o) - null parameter allowed -> marked with @Nullable

PersistentCapabilities

  • getCapability(String capabilityName) - follows from Capabilities#getCapability(String capabilityName) -> marked with @Nullable
  • equals(Object o) - null parameter allowed -> marked with @Nullable

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 (@NullMarked and @Nullable) to several interfaces and classes to improve null safety.
  • Updated Capabilities, HasCapabilities, ImmutableCapabilities, MutableCapabilities, and PersistentCapabilities to include these annotations.
  • Enhanced code to help developers avoid potential NullPointerExceptions.

Changes walkthrough 📝

Relevant files
Enhancement
Capabilities.java
Add nullness annotations to Capabilities interface             

java/src/org/openqa/selenium/Capabilities.java

  • Added @NullMarked annotation to the interface.
  • Marked getPlatformName and getCapability methods with @Nullable.
  • +5/-2     
    HasCapabilities.java
    Add nullness annotations to HasCapabilities interface       

    java/src/org/openqa/selenium/HasCapabilities.java

    • Added @NullMarked annotation to the interface.
    +3/-0     
    ImmutableCapabilities.java
    Add nullness annotations to ImmutableCapabilities class   

    java/src/org/openqa/selenium/ImmutableCapabilities.java

  • Added @NullMarked annotation to the class.
  • Marked getCapability and equals methods with @Nullable.
  • +5/-2     
    MutableCapabilities.java
    Add nullness annotations to MutableCapabilities class       

    java/src/org/openqa/selenium/MutableCapabilities.java

  • Added @NullMarked annotation to the class.
  • Marked setCapability, getCapability, and equals methods with
    @Nullable.
  • +6/-3     
    PersistentCapabilities.java
    Add nullness annotations to PersistentCapabilities class 

    java/src/org/openqa/selenium/PersistentCapabilities.java

  • Added @NullMarked annotation to the class.
  • Marked getCapability and equals methods with @Nullable.
  • +5/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Null Dereference
    The getPlatformName() method now returns @Nullable Platform, but the stream operations that follow don't handle the potential null value explicitly.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Use Optional instead of @nullable Platform for better null handling

    Consider using Optional as the return type for getPlatformName() instead of
    @Nullable Platform. This would make it clearer that the method might not return a
    platform and would allow for more idiomatic handling of the potential absence of a
    platform.

    java/src/org/openqa/selenium/Capabilities.java [38-43]

    -default @Nullable Platform getPlatformName() {
    +default Optional<Platform> getPlatformName() {
       return Stream.of("platformName")
           .map(this::getCapability)
           .filter(Objects::nonNull)
    -      .map(
    +      .map(Platform::fromString)
    +      .findFirst();
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using Optional<Platform> is a more idiomatic way to handle the potential absence of a value in Java, providing clearer semantics and better null handling compared to @Nullable. This change improves code readability and maintainability.

    8
    Performance
    Optimize capability existence check for better performance

    Consider using a more efficient approach to check for capability existence. Instead
    of getting the capability and then checking for null, you could use the containsKey
    method of the underlying map if available.

    java/src/org/openqa/selenium/PersistentCapabilities.java [68-72]

     public @Nullable Object getCapability(String capabilityName) {
       Require.nonNull("Capability name", capabilityName);
    -  Object capability = overrides.getCapability(capabilityName);
    -  if (capability != null) {
    -    return capability;
    +  if (overrides.asMap().containsKey(capabilityName)) {
    +    return overrides.getCapability(capabilityName);
    +  }
    +  return caps.getCapability(capabilityName);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use containsKey can improve performance by avoiding unnecessary retrieval of map values. This change optimizes the code by reducing the number of operations needed to check for capability existence.

    7
    Best practice
    Improve equality check in the equals method for better null handling

    Consider using Objects.equals() for the equality check in the equals method instead
    of the instanceof operator. This would provide a more robust equality check that
    handles null values correctly.

    java/src/org/openqa/selenium/ImmutableCapabilities.java [178-182]

     public boolean equals(@Nullable Object o) {
    -  if (!(o instanceof Capabilities)) {
    -    return false;
    -  }
    +  if (this == o) return true;
    +  if (!(o instanceof Capabilities)) return false;
       return SharedCapabilitiesMethods.equals(this, (Capabilities) o);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion to use Objects.equals() is not implemented in the improved code. However, adding a self-check (this == o) is a good practice for performance optimization. The suggestion partially addresses best practices for equality checks.

    6
    Use a more specific exception type for input validation

    Consider using a more specific exception type instead of the generic
    IllegalArgumentException when validating the capability name. This would provide
    more precise error information and improve error handling.

    java/src/org/openqa/selenium/MutableCapabilities.java [87-88]

     public void setCapability(String key, @Nullable Object value) {
    -  Require.nonNull("Capability name", key);
    +  if (key == null) {
    +    throw new IllegalArgumentException("Capability name cannot be null");
    +  }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: While using a more specific exception type can improve error handling, the suggestion does not actually implement this change, and the existing code already uses Require.nonNull for validation. The suggestion is not directly applicable to the current code.

    5

    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