[java] Feat 14291/jspecify nullable annotation edge driver service

[iampopovich]

User description

🔗 Related Issues

fixes #14291

💥 What does this PR do?

This pull request introduces changes to the EdgeDriverService and its parent class DriverService to improve nullability handling by adding @Nullable annotations to various parameters and fields. These changes enhance type safety and make the codebase more robust when dealing with potentially null values.

🔧 Implementation Notes

Nullability Enhancements in EdgeDriverService:

• Added @Nullable annotations to parameters in the EdgeDriverService constructor, including File executable, Duration timeout, List<String> args, and Map<String, String> environment. This ensures better handling of optional values during service creation.
• Updated the Builder class fields (e.g., disableBuildCheck, logLevel, allowedListIps) with @Nullable annotations to explicitly mark them as optional.
• Modified methods in the Builder class, such as withLoglevel and withAllowedListIps, to include @Nullable annotations for their parameters, improving clarity about optional inputs. [1] [2] [3]
• Adjusted the createDriverService method in EdgeDriverService to use @Nullable annotations for its parameters, aligning with the updated constructor.

Nullability Enhancements in DriverService:

• Added @Nullable annotations to the DriverService constructor parameters (File executable, Duration timeout, List<String> args, and Map<String, String> environment) to propagate nullability handling to the parent class.

💡 Additional Considerations

🔄 Types of changes

• Cleanup (formatting, renaming)

---

PR Type

Enhancement

---

Description

• Add @nullable annotations to EdgeDriverService constructor parameters

• Add @nullable annotations to Builder class fields and methods

• Update DriverService constructor with @nullable annotations

• Add jspecify dependency to BUILD.bazel

---

Changes diagram

flowchart LR
  A["EdgeDriverService constructor"] -- "add @Nullable" --> B["Parameters: executable, timeout, args, environment"]
  C["Builder class fields"] -- "add @Nullable" --> D["Fields: disableBuildCheck, logLevel, allowedListIps, etc."]
  E["Builder methods"] -- "add @Nullable" --> F["Method parameters: withLoglevel, withAllowedListIps"]
  G["BUILD.bazel"] -- "add dependency" --> H["org_jspecify_jspecify"]

Loading

---

Changes walkthrough 📝

	Relevant files
Enhancement	EdgeDriverService.java Add @Nullable annotations for null safety                                java/src/org/openqa/selenium/edge/EdgeDriverService.java Add @nullable annotations to constructor parameters (executable, timeout, args, environment) Add @nullable annotations to Builder class fields (disableBuildCheck, logLevel, allowedListIps, etc.) Add @nullable annotations to Builder method parameters (withLoglevel, withAllowedListIps, withReadableTimestamp) Update createDriverService method signature with @nullable annotations +20/-15  DriverService.java Add @Nullable annotations to constructor                                  java/src/org/openqa/selenium/remote/service/DriverService.java Add @nullable annotations to constructor parameters (executable, timeout, args, environment) +4/-4
Dependencies	BUILD.bazel Add jspecify dependency                                                                    java/src/org/openqa/selenium/edge/BUILD.bazel Add jspecify dependency to support @nullable annotations +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.

[qodo-code-review]

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 Null Safety The constructor and createDriverService method use List.copyOf() and Map.copyOf() on potentially null parameters without null checks, which could throw NullPointerException when null values are passed despite the @nullable annotations. super(executable, port, timeout, List.copyOf(args), Map.copyOf(environment)); } Inconsistent Handling The constructor has a null check for executable parameter but no explicit null checks for other @nullable parameters like timeout, args, and environment, which may be used elsewhere in the constructor or class methods. if (executable != null) { this.executable = executable.getCanonicalPath(); }

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle null parameters before copying The List.copyOf() and Map.copyOf() methods will throw NullPointerException if the arguments are null. Since args and environment are now marked as @Nullable, you should handle null values before copying. java/src/org/openqa/selenium/edge/EdgeDriverService.java [106] -super(executable, port, timeout, List.copyOf(args), Map.copyOf(environment)); +super(executable, port, timeout, + args != null ? List.copyOf(args) : List.of(), + environment != null ? Map.copyOf(environment) : Map.of()); Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that List.copyOf(args) and Map.copyOf(environment) will throw a NullPointerException if args or environment are null. Since the PR makes these parameters @Nullable, this change is critical to prevent a runtime crash.	High
General	Handle null log level appropriately When logLevel is null, setting silent and verbose to false may not be the intended behavior. Consider handling the null case explicitly or documenting the expected behavior when null is passed. java/src/org/openqa/selenium/edge/EdgeDriverService.java [200-205] public Builder withLoglevel(@Nullable ChromiumDriverLogLevel logLevel) { this.logLevel = logLevel; - this.silent = false; - this.verbose = false; + if (logLevel != null) { + this.silent = false; + this.verbose = false; + } return this; } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that unconditionally setting silent and verbose to false might be an unintended side effect when withLoglevel(null) is called. The proposed change improves the logic by only resetting these flags when a non-null logLevel is provided, making the method's behavior more robust and intuitive.	Low
Update

[diemol]

Thank you, @iampopovich!