Home Original page

[java] JSpecify annotations for `Credential` and `MBean` by mk868 · Pull Request #16481 · SeleniumHQ/selenium

@mk868

@mk868 mk868 commented

Oct 21, 2025

•

edited by qodo-code-review bot

Loading

User description

🔗 Related Issues

Related #14291

💥 What does this PR do?

JSpecify annotations added to the:

  • org.openqa.selenium.grid.jmx.MBean
  • org.openqa.selenium.virtualauthenticator.Credential

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement

Description

  • Add JSpecify null-safety annotations to MBean class

  • Add JSpecify null-safety annotations to Credential class

  • Improve null-safety handling in attribute and operation invocation methods

  • Add defensive null checks for map lookups in getter/setter/invoke operations

Diagram Walkthrough

flowchart LR
  A["MBean.java<br/>Credential.java"] -->|Add @NullMarked| B["Class-level<br/>null-safety"]
  A -->|Add @Nullable| C["Method returns<br/>and parameters"]
  A -->|Add null checks| D["getAttribute/setAttribute<br/>invoke methods"]
  C --> E["Improved type<br/>safety"]
  D --> E
Loading

File Walkthrough
Relevant files
Enhancement
MBean.java
Add JSpecify null-safety annotations and null checks         

java/src/org/openqa/selenium/grid/jmx/MBean.java

  • Add @NullMarked class-level annotation for null-safety
  • Add @Nullable annotations to method return types and parameters
    (getter, setter, and various finder methods)
  • Add defensive null checks in getAttribute(), setAttribute(), and
    invoke() methods to handle null map lookups
  • Improve null-safety in AttributeInfo and OperationInfo handling
 +31/-15 
Credential.java
Add JSpecify null-safety annotations to Credential             

java/src/org/openqa/selenium/virtualauthenticator/Credential.java

  • Add @Nullable annotation to rpId field and getRpId() method return
    type
  • Add @Nullable annotation to toMap() method return type for map values
  • Update constructor parameter to reflect nullable rpId
 +4/-4     

@mk868

@qodo-code-review

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪
Incomplete error handling

Description: The change makes DynamicMBean#getAttribute return null when the attribute is missing or
the getter is null, which can silently mask errors and enable information disclosure
patterns if callers assume non-null values; consider throwing AttributeNotFoundException
per JMX conventions instead of returning null.
MBean.java [220-241]

Referred Code 
@Override
public @Nullable Object getAttribute(String attribute) {
  try {
    AttributeInfo attributeInfo = attributeMap.get(attribute);
    if (attributeInfo == null || attributeInfo.getter == null) {
      return null;
    }
    Object res = attributeInfo.getter.invoke(bean);
    if (res instanceof Map<?, ?>) {
      return ((Map<?, ?>) res)
          .entrySet().stream()
              .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().toString()));
    } else if (res instanceof Number) {
      return res;
    } else {
      return res.toString();
    }
  } catch (IllegalAccessException | InvocationTargetException e) {
    LOG.severe("Error during execution: " + e.getMessage());
    return null;
  }


 ... (clipped 1 lines)
Incomplete error handling

Description: The invoke method now returns null when an operation is not found, potentially hiding
management misconfigurations and enabling ambiguous behavior; per JMX, it should signal
errors (e.g., MBeanException or ReflectionException) rather than returning null.
MBean.java [276-287]

Referred Code 
@Override
public @Nullable Object invoke(String actionName, Object[] params, String[] signature) {
  try {
    OperationInfo operationInfo = operationMap.get(actionName);
    if (operationInfo == null) {
      return null;
    }
    return operationInfo.method.invoke(bean, params);
  } catch (IllegalAccessException | InvocationTargetException e) {
    LOG.severe("Error during execution: " + e.getMessage());
    return null;
  }
Ticket Compliance
🟢
🎫 #14291
🟢 Add JSpecify Nullness annotations across Selenium Java code to declare nullable parameters
and return values.
Use @nullable on APIs that can return or accept null to improve IDE/static analyzer
detection.
Use @NullMarked (or equivalent) to enforce non-null by default within scopes where
possible.
Improve null-safety handling to avoid NPEs at runtime where applicable.
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
⚪
No custom compliance provided

Follow the guide to enable custom compliance check.

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Missing return statements in method

Add missing return statements in the findSetter method for branches handling
method names starting with get and is to ensure the found setter method is
correctly returned.

java/src/org/openqa/selenium/grid/jmx/MBean.java [158-175]

 private @Nullable Method findSetter(Method annotatedMethod) {
   ManagedAttribute ma = annotatedMethod.getAnnotation(ManagedAttribute.class);
   if (!"".equals(ma.setter())) {
     return findMethod(annotatedMethod.getDeclaringClass(), ma.setter());
   } else {
     String name = annotatedMethod.getName();
     if (name.startsWith("set")) {
       return annotatedMethod;
     }
     if (name.startsWith("get")) {
-      findMethod(annotatedMethod.getDeclaringClass(), "s" + name.substring(1));
+      return findMethod(annotatedMethod.getDeclaringClass(), "s" + name.substring(1));
     }
     if (name.startsWith("is")) {
-      findMethod(annotatedMethod.getDeclaringClass(), "set" + name.substring(2));
+      return findMethod(annotatedMethod.getDeclaringClass(), "set" + name.substring(2));
     }
   }
   return null;
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a significant bug in the findSetter method where return statements are missing, causing the logic for get and is prefixed methods to fail and always return null.

High
Learned
best practice
Fail on invalid attribute sets

Validate and throw precise exceptions when an attribute is unknown or not
writable, and include the attribute name to aid debugging.

java/src/org/openqa/selenium/grid/jmx/MBean.java [244-254]

 public void setAttribute(Attribute attribute) {
+  String name = attribute.getName();
+  AttributeInfo attributeInfo = attributeMap.get(name);
+  if (attributeInfo == null) {
+    throw new IllegalArgumentException("Unknown attribute: " + name);
+  }
+  if (attributeInfo.setter == null) {
+    throw new IllegalStateException("Attribute not writable: " + name);
+  }
   try {
-    AttributeInfo attributeInfo = attributeMap.get(attribute.getName());
-    if (attributeInfo == null || attributeInfo.setter == null) {
-      return;
-    }
     attributeInfo.setter.invoke(bean, attribute.getValue());
   } catch (IllegalAccessException | InvocationTargetException e) {
     LOG.severe("Error during execution: " + e.getMessage());
+    throw new IllegalStateException("Failed to set attribute: " + name, e);
   }
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs and states early; avoid silent no-ops and null returns in command methods to prevent logic errors and hidden failures.

Low
Throw explicit errors on invalid attributes

Fail fast with precise exceptions when an attribute is missing or not readable
instead of returning null; include the attribute name in the message.

java/src/org/openqa/selenium/grid/jmx/MBean.java [221-241]

-public @Nullable Object getAttribute(String attribute) {
+public Object getAttribute(String attribute) {
+  AttributeInfo attributeInfo = attributeMap.get(attribute);
+  if (attributeInfo == null) {
+    throw new IllegalArgumentException("Unknown attribute: " + attribute);
+  }
+  if (attributeInfo.getter == null) {
+    throw new IllegalStateException("Attribute not readable: " + attribute);
+  }
   try {
-    AttributeInfo attributeInfo = attributeMap.get(attribute);
-    if (attributeInfo == null || attributeInfo.getter == null) {
-      return null;
+    Object res = attributeInfo.getter.invoke(bean);
+    if (res instanceof Map<?, ?>) {
+      return ((Map<?, ?>) res)
+          .entrySet().stream()
+              .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().toString()));
+    } else if (res instanceof Number) {
+      return res;
+    } else {
+      return res.toString();
     }
-    Object res = attributeInfo.getter.invoke(bean);
-    ...
   } catch (IllegalAccessException | InvocationTargetException e) {
     LOG.severe("Error during execution: " + e.getMessage());
-    return null;
+    throw new IllegalStateException("Failed to get attribute: " + attribute, e);
   }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Validate inputs and states early to avoid NPEs and logic errors; avoid returning null where JMX contract expects informative errors.

Low
  • More

diemol

@mk868 mk868 deleted the jspecify-Credential-MBean branch

October 22, 2025 08:48

This was referenced

Oct 25, 2025

This was referenced

Dec 16, 2025

This was referenced

Jan 19, 2026

This was referenced

Feb 11, 2026

This was referenced

Feb 20, 2026