Home Original page

[java] JSpecify annotations for `org.openqa.selenium.grid.jmx` by mk868 · Pull Request #16431 · SeleniumHQ/selenium

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Return null on registration failure

Modify the register method to return null when an InstanceAlreadyExistsException
is caught, ensuring a consistent return value for all registration failure
scenarios.

java/src/org/openqa/selenium/grid/jmx/JMXHelper.java [36-44]

 try {
   mbs.registerMBean(mBean, mBean.getObjectName());
   return mBean;
 } catch (InstanceAlreadyExistsException t) {
-  return mBean;
+  LOG.warning("Could not register MBean: " + t.getMessage());
+  return null;
 } catch (Throwable t) {
   LOG.severe("Error during execution: " + t.getMessage());
   return null;
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion improves API consistency by proposing to return null on InstanceAlreadyExistsException, making the method's return value a clearer indicator of whether the registration was successful.

Medium
General
Clarify parameter nullability with an annotation

Add the @Nullable annotation to the objectName parameter in the unregister
method to align with its null-checking logic and the class-level @NullMarked
annotation.

java/src/org/openqa/selenium/grid/jmx/JMXHelper.java [47-55]

-public void unregister(ObjectName objectName) {
+public void unregister(@Nullable ObjectName objectName) {
   if (objectName != null) {
     MBeanServer mbs = ManagementFactory.getPlatformMBeanServer();
     try {
       mbs.unregisterMBean(objectName);
     } catch (Throwable ignore) {
     }
   }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies an inconsistency between the newly added @NullMarked class annotation and the implementation of the unregister method, which explicitly handles nulls. Applying @Nullable improves the correctness of the nullability contract.

Low
Learned
best practice
Add null check for input

Add a null check for bean at the start of register to avoid constructing MBean
with a null reference. Throw an IllegalArgumentException if bean is null.

java/src/org/openqa/selenium/grid/jmx/JMXHelper.java [33-45]

 public @Nullable MBean register(Object bean) {
+  if (bean == null) {
+    throw new IllegalArgumentException("bean must not be null");
+  }
   MBeanServer mbs = ManagementFactory.getPlatformMBeanServer();
   MBean mBean = new MBean(bean);
   try {
     mbs.registerMBean(mBean, mBean.getObjectName());
     return mBean;
   } catch (InstanceAlreadyExistsException t) {
     return mBean;
   } catch (Throwable t) {
     LOG.severe("Error during execution: " + t.getMessage());
     return null;
   }
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs early to prevent NPEs and logic errors.

Low
  • Update