Skip to content

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Jul 5, 2025

User description

🔗 Related Issues

fixes #14291

💥 What does this PR do?

This pull request introduces changes to improve nullability annotations in the ChromeDriverService class and its associated methods, enhancing type safety and clarity in handling nullable parameters. Additionally, a dependency on org_jspecify_jspecify is added to the Bazel build file to support these annotations.

Nullability Enhancements in ChromeDriverService:

  • Constructor and Method Parameters: Updated the ChromeDriverService constructor and several builder methods to include @Nullable annotations for parameters that can accept null values, ensuring better clarity and type safety. (java/src/org/openqa/selenium/chrome/ChromeDriverService.java) [1] [2] [3] [4] [5]
  • Builder Class Fields: Added @Nullable annotations to fields in the Builder class to reflect their potential nullability. (java/src/org/openqa/selenium/chrome/ChromeDriverService.java)

Dependency Updates:

  • Bazel Build File: Added a dependency on @maven//:org_jspecify_jspecify in the Bazel build file to support @Nullable annotations. (java/src/org/openqa/selenium/chrome/BUILD.bazel)

Import Statements:

  • Added Import for @Nullable: Included the necessary import statement for org.jspecify.annotations.Nullable in the ChromeDriverService class. (java/src/org/openqa/selenium/chrome/ChromeDriverService.java)

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Add JSpecify nullable annotations to ChromeDriverService parameters

  • Update constructor and builder methods with @nullable annotations

  • Add JSpecify dependency to Bazel build configuration

  • Improve type safety for nullable parameters


Changes diagram

flowchart LR
  A["ChromeDriverService"] --> B["Add @Nullable annotations"]
  B --> C["Constructor parameters"]
  B --> D["Builder methods"]
  B --> E["Builder fields"]
  F["BUILD.bazel"] --> G["Add JSpecify dependency"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
ChromeDriverService.java
Add nullable annotations to parameters and fields               

java/src/org/openqa/selenium/chrome/ChromeDriverService.java

  • Import JSpecify nullable annotation
  • Add @nullable to constructor parameters (executable, timeout, args,
    environment)
  • Add @nullable to Builder class fields (disableBuildCheck,
    readableTimestamp, etc.)
  • Add @nullable to Builder method parameters (logLevel, allowedListIps,
    readableTimestamp)
  • Update createDriverService method signature with @nullable parameters
  • +20/-15 
    Dependencies
    BUILD.bazel
    Add JSpecify dependency                                                                   

    java/src/org/openqa/selenium/chrome/BUILD.bazel

    • Add JSpecify dependency to Maven dependencies list
    +1/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added C-java Java Bindings B-build Includes scripting, bazel and CI integrations labels Jul 5, 2025
    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 5, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Inconsistent Nullability

    The withVerbose method sets this.verbose = false unconditionally, even when the parameter is true. This appears to be a bug that should be addressed - it should set this.verbose = verbose instead.

    public Builder withVerbose(boolean verbose) {
      if (verbose) {
        this.logLevel = ChromiumDriverLogLevel.ALL;
      }
      this.verbose = false;
      return this;
    Type Safety

    Adding @nullable annotations to method parameters like withLogLevel and withAllowedListIps may break existing code that expects non-null parameters, and the methods don't appear to handle null values appropriately.

    public Builder withLogLevel(@Nullable ChromiumDriverLogLevel logLevel) {
      this.logLevel = logLevel;
      this.silent = false;
      this.verbose = false;
      return this;
    }
    
    /**
     * Configures the driver server for silent output.
     *
     * @param silent Log no output for true, no changes made if false.
     * @return A self reference.
     */
    public Builder withSilent(boolean silent) {
      if (silent) {
        this.logLevel = ChromiumDriverLogLevel.OFF;
      }
      this.silent = false;
      return this;
    }
    
    /**
     * Configures the driver server verbosity.
     *
     * @param verbose Log all output for true, no changes made if false.
     * @return A self reference.
     */
    public Builder withVerbose(boolean verbose) {
      if (verbose) {
        this.logLevel = ChromiumDriverLogLevel.ALL;
      }
      this.verbose = false;
      return this;
    }
    
    /**
     * Configures the comma-separated list of remote IPv4 addresses which are allowed to connect to
     * the driver server.
     *
     * @param allowedListIps Comma-separated list of remote IPv4 addresses.
     * @return A self reference.
     */
    public Builder withAllowedListIps(@Nullable String allowedListIps) {

    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 5, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix verbose field assignment logic

    The verbose field is always set to false regardless of the input parameter
    value. This appears to be a bug where the field should be set to the parameter
    value instead.

    java/src/org/openqa/selenium/chrome/ChromeDriverService.java [233-239]

     public Builder withVerbose(boolean verbose) {
       if (verbose) {
         this.logLevel = ChromiumDriverLogLevel.ALL;
       }
    -  this.verbose = false;
    +  this.verbose = verbose;
       return this;
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies a pre-existing bug in the withVerbose method where this.verbose is always set to false, rendering the verbose parameter ineffective.

    Medium
    • Update

    @diemol diemol merged commit 5d1ee30 into SeleniumHQ:trunk Jul 6, 2025
    58 of 59 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 C-java Java Bindings Review effort 2/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🚀 Feature]: JSpecify Nullness annotations for Java
    3 participants