[asolntsev]

🔗 Related Issues

Part of #14291

💥 What does this PR do?

Marks the whole packages org.openqa.selenium.bidi.script and org.openqa.selenium.bidi.permissions as non-nullable by default.

🔄 Types of changes

• Cleanup (formatting, renaming)

[qodo-code-review]

PR Type

Enhancement, Tests

---

Description

• Add nullability annotations to org.openqa.selenium.bidi.script and org.openqa.selenium.bidi.permissions packages via @NullMarked

• Refactor enum findByName() methods to throw IllegalArgumentException instead of returning null

• Add Require.nonNull() validation in fromJson() methods for required fields

• Simplify code using streams, Map.of(), and extract helper methods for better readability

---

File Walkthrough

	Relevant files
Enhancement	21 files package-info.java Create package-info with @NullMarked annotation                    +21/-0    package-info.java Create package-info with @NullMarked annotation                    +21/-0    PermissionState.java Remove class-level nullability, add BiDi spec doc, throw on invalid state +8/-9      ChannelValue.java Add nullability annotations, refactor constructors, simplify toJson +14/-17  ExceptionDetails.java Add field validation, use readNonNull, remove unused toJson method +9/-18    LocalValue.java Refactor getArgument with helper methods, add stream-based conversions +90/-65  MapLocalValue.java Refactor toJson using streams instead of manual iteration +5/-10    Message.java Add field validation using Require.nonNull                              +5/-1      NodeProperties.java Add field validation, make constructor private, improve enum handling +19/-34  ObjectLocalValue.java Refactor toJson using streams instead of manual iteration +5/-10    PrimitiveProtocolValue.java Add nullability annotation, simplify toJson using Map.of  +14/-15  PrimitiveType.java Refactor findByName to throw IllegalArgumentException on invalid type +2/-4      RealmInfo.java Add field validation, remove unused toJson, improve type checking +13/-19  RealmType.java Refactor findByName to throw IllegalArgumentException on invalid type +2/-4      RegExpValue.java Add pattern validation, simplify toJson using Map.of          +5/-10    RemoteReference.java Add nullability annotations to handle and sharedId fields +5/-2      RemoteValue.java Add nullability annotations, make Type enum public, add field validation +9/-16    Source.java Add realm field validation using Require.nonNull                  +2/-1      StackFrame.java Add field validation for line and column numbers                  +10/-5    StackTrace.java Use readNonNull for callFrames, simplify imports                  +3/-3      WindowProxyProperties.java Add browsingContext field validation                                          +2/-1
Documentation	2 files ArrayLocalValue.java Add BiDi specification documentation link                                +4/-0      CallFunctionParameters.java Add BiDi specification documentation link                                +4/-0
Formatting	1 files SetLocalValue.java Remove blank line in toJson method                                              +0/-1

---

[qodo-code-review]

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 Java code to explicitly document which parameters/return values may be null. ⚪ Prefer annotations over relying on JavaDoc so IDEs and static analyzers can surface potential nullability issues. Improve interoperability with Kotlin via explicit nullability.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
Update

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:

Category	Suggestion	Impact
High-level	Consider if throwing exceptions is appropriate The PR alters several findByName methods to throw an IllegalArgumentException instead of returning null for unknown values. This is a breaking semantic change that should be verified as intentional and consistent with the library's error handling strategy. Examples: java/src/org/openqa/selenium/bidi/permissions/PermissionState.java [41-48] public static PermissionState findByName(String name) { for (PermissionState state : values()) { if (state.toString().equalsIgnoreCase(name)) { return state; } } throw new IllegalArgumentException("Unsupported permission state: " + name); } java/src/org/openqa/selenium/bidi/script/RemoteValue.java [77-84] public static Type findByName(String name) { for (Type type : values()) { if (type.toString().equalsIgnoreCase(name)) { return type; } } throw new IllegalArgumentException("Unsupported value type: " + name); } Solution Walkthrough: Before: // In PermissionState.java public static @Nullable PermissionState findByName(String name) { PermissionState result = null; for (PermissionState state : values()) { if (state.toString().equalsIgnoreCase(name)) { result = state; break; } } return result; } After: // In PermissionState.java public static PermissionState findByName(String name) { for (PermissionState state : values()) { if (state.toString().equalsIgnoreCase(name)) { return state; } } throw new IllegalArgumentException("Unsupported permission state: " + name); } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant, potentially breaking API change where multiple findByName methods now throw exceptions instead of returning null, a change not highlighted in the PR description.	Medium
Possible issue	Handle null arguments to prevent NullPointerException Add a null check at the start of the getArgument method to handle null inputs and prevent a potential NullPointerException. java/src/org/openqa/selenium/bidi/script/LocalValue.java [173-204] public static LocalValue getArgument(Object arg) { + if (arg == null) { + return nullValue(); + } if (arg instanceof String) { return stringToLocalValue((String) arg); } else if (arg instanceof Number) { return numberToLocalValue((Number) arg); } else if (arg instanceof Boolean) { return booleanValue((Boolean) arg); } else if (arg instanceof Instant) { return dateValue((Instant) arg); } else if (arg instanceof Map) { return mapToLocalValues((Map<Object, Object>) arg); } else if (arg instanceof List) { return listToLocalValues((List<Object>) arg); } else if (arg instanceof Set) { return setToLocalValues((Set<Object>) arg); } else if (arg instanceof RegExpValue) { return (RegExpValue) arg; } else { Map<Object, Object> rawValues = JSON.toType(JSON.toJson(arg), Map.class); Map<Object, LocalValue> map = rawValues.entrySet().stream() .collect( toMap( entry -> entry.getKey() instanceof String ? entry.getKey() : getArgument(entry.getKey()), entry -> getArgument(entry.getValue()))); return objectValue(map); } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a NullPointerException when arg is null and provides a proper fix by handling the null case explicitly, which improves the robustness of the getArgument method.	Medium
	Prevent NullPointerException on missing JSON fields In the fromJson method, wrap nodeType and childNodeCount with Require.nonNull before passing them to Require.nonNegative to prevent a NullPointerException if the fields are missing from the JSON. java/src/org/openqa/selenium/bidi/script/NodeProperties.java [83-153] public static NodeProperties fromJson(JsonInput input) { Long nodeType = null; Long childNodeCount = null; Optional<Map<String, String>> attributes = Optional.empty(); Optional<List<RemoteValue>> children = Optional.empty(); Optional<String> localName = Optional.empty(); Optional<Mode> mode = Optional.empty(); Optional<String> namespaceURI = Optional.empty(); Optional<String> nodeValue = Optional.empty(); Optional<RemoteValue> shadowRoot = Optional.empty(); input.beginObject(); while (input.hasNext()) { switch (input.nextName()) { case "nodeType": - nodeType = input.read(long.class); + nodeType = input.read(Long.class); break; case "childNodeCount": - childNodeCount = input.read(long.class); + childNodeCount = input.read(Long.class); break; ... } input.endObject(); return new NodeProperties( - Require.nonNegative("nodeType", nodeType), - Require.nonNegative("childNodeCount", childNodeCount), + Require.nonNegative("nodeType", Require.nonNull("nodeType", nodeType)), + Require.nonNegative("childNodeCount", Require.nonNull("childNodeCount", childNodeCount)), attributes, children, localName, mode, namespaceURI, nodeValue, shadowRoot); } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential NullPointerException if nodeType or childNodeCount are missing from the JSON input, due to auto-unboxing a null Long. The proposed fix of wrapping the call with Require.nonNull is accurate and improves the method's robustness.	Medium
General	Support all numeric types Refactor the numberToLocalValue method to simplify numeric type handling. Consolidate integer-like types by using arg.longValue() and handle floating-point and BigInteger types separately. java/src/org/openqa/selenium/bidi/script/LocalValue.java [225-234] private static LocalValue numberToLocalValue(Number arg) { - if (arg instanceof Integer || arg instanceof Long) { - return numberValue(arg.longValue()); + if (arg instanceof BigInteger) { + return bigIntValue((BigInteger) arg); } else if (arg instanceof Double || arg instanceof Float) { return numberValue(arg.doubleValue()); - } else if (arg instanceof BigInteger) { - return bigIntValue((BigInteger) arg); + } else { + return numberValue(arg.longValue()); } - throw new IllegalArgumentException("Cannot convert numeric argument " + arg + " to LocalValue"); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly simplifies the logic in numberToLocalValue by removing the explicit checks for Integer and Long and relying on the longValue() method of Number. This makes the code cleaner and correctly handles other integer-like Number types such as Byte and Short.	Low
Update