Skip to content

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Oct 14, 2025

User description

🔗 Related Issues

Related #14291

💥 What does this PR do?

JSpecify annotations added to the:

  • org.openqa.selenium.grid.jmx.JMXHelper

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

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

PR Type

Enhancement


Description

  • Added JSpecify null-safety annotations to JMXHelper class

  • Marked register method return type as @Nullable

  • Added @NullMarked annotation at class level

  • Updated Bazel build configuration to include JSpecify dependency


Diagram Walkthrough

flowchart LR
  A["JMXHelper class"] --> B["Add @NullMarked annotation"]
  A --> C["Mark register() return as @Nullable"]
  D["BUILD.bazel"] --> E["Add JSpecify dependency"]
Loading

File Walkthrough

Relevant files
Enhancement
JMXHelper.java
Add null-safety annotations to JMXHelper class                     

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

  • Added @NullMarked annotation to class level
  • Marked register method return type as @Nullable
  • Added JSpecify annotation imports
+4/-1     
Dependencies
BUILD.bazel
Add JSpecify dependency to build configuration                     

java/src/org/openqa/selenium/grid/jmx/BUILD.bazel

  • Added artifact import to Bazel build file
  • Added JSpecify dependency to build configuration
+4/-1     

@selenium-ci selenium-ci added B-grid Everything grid and server related C-java Java Bindings B-build Includes scripting, bazel and CI integrations labels Oct 14, 2025
@mk868 mk868 force-pushed the jspecify-grid-jmx branch from c88d3c5 to 39db1ba Compare October 14, 2025 17:20
Copy link
Contributor

qodo-merge-pro bot commented Oct 14, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #14291
🟢 Add JSpecify nullness annotations to Selenium framework code to declare which parameters
and return values can be null.
Use @nullable on methods/returns that may be null, matching intended behavior.
Annotate classes/packages appropriately (e.g., @NullMarked) to define default nullness.
Ensure build configuration includes JSpecify dependency so annotations are available to
the codebase.
Improve IDE/static analysis null-safety signaling without changing runtime behavior.
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.

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

Copy link
Contributor

qodo-merge-pro bot commented Oct 14, 2025

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

@mk868 mk868 changed the title [java] JSpecify annotations for org.openqa.selenium.grid.jmx.JMXHelper [java] JSpecify annotations for org.openqa.selenium.grid.jmx Oct 14, 2025
@mk868 mk868 force-pushed the jspecify-grid-jmx branch from 39db1ba to 37de88c Compare October 15, 2025 17:53
@diemol diemol merged commit fd0eeae into SeleniumHQ:trunk Oct 16, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations B-grid Everything grid and server related C-java Java Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants