[java] JSpecify annotations for capabilities

[mk868]

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: https://github.com/SeleniumHQ/selenium/issues/14291

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• [x] 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

[qodo-code-review[bot]]

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.

[qodo-code-review[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Use Optional instead of @Nullable Platform for better null handling Consider using Optional<Platform> 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 <!-- /improve --apply_suggestion=0 --> 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 <!-- /improve --apply_suggestion=1 --> 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 <!-- /improve --apply_suggestion=2 --> 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 <!-- /improve --apply_suggestion=3 --> 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

[mk868]

I pushed one more commit to fix two more NullAway errors (related: https://github.com/SeleniumHQ/selenium/pull/14421). At this point, these errors have been resolved:

java/src/org/openqa/selenium/Capabilities.java:36: error: [NullAway] returning @Nullable expression from method with @NonNull return type
    return Stream.of("platformName")
    ^
    (see http://t.uber.com/nullaway )
java/src/org/openqa/selenium/ImmutableCapabilities.java:151: error: [NullAway] passing @Nullable parameter 'v' where @NonNull is required
          setCapability(delegate, (String) key, v);
                                                ^
    (see http://t.uber.com/nullaway )
java/src/org/openqa/selenium/ImmutableCapabilities.java:161: error: [NullAway] returning @Nullable expression from method with @NonNull return type
    return delegate.get(capabilityName);
    ^
    (see http://t.uber.com/nullaway )
java/src/org/openqa/selenium/MutableCapabilities.java:111: error: [NullAway] returning @Nullable expression from method with @NonNull return type
    return caps.get(capabilityName);
    ^
    (see http://t.uber.com/nullaway )

Actually, in multithreaded applications PersistentCapabilities#asMap() and ImmutableCapabilities#ImmutableCapabilities(Map<?, ?>) could cause problems.

@diemol could you look at this?

[diemol]

Can you elaborate?

[mk868]

Oh, I meant to ask for a review, Could you please review this PR?