Skip to content

Conversation

@asolntsev
Copy link
Contributor

@asolntsev asolntsev commented Dec 4, 2025

User description

🔗 Related Issues

This would help to avoid issues like this.

💥 What does this PR do?

adds @NullMarked and @Nullable annotations to "*.bidi.log" package.

🔄 Types of changes

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

PR Type

Enhancement


Description

  • Add nullability annotations using JSpecify to bidi.log package

  • Mark StackTrace parameter as @nullable in constructors

  • Add null-check before serializing stackTrace in toJson methods

  • Create package-info.java with @NullMarked annotation

  • Update test imports to use modern AssertJ API


Diagram Walkthrough

flowchart LR
  A["bidi.log package"] -->|"Add @NullMarked"| B["package-info.java"]
  A -->|"Mark @Nullable"| C["StackTrace parameter"]
  C -->|"in constructors"| D["BaseLogEntry, ConsoleLogEntry, GenericLogEntry, JavascriptLogEntry"]
  D -->|"Add null-checks"| E["toJson methods"]
  A -->|"Add dependency"| F["org.jspecify:jspecify"]
Loading

File Walkthrough

Relevant files
Enhancement
BaseLogEntry.java
Add nullability annotations to BaseLogEntry                           

java/src/org/openqa/selenium/bidi/log/BaseLogEntry.java

  • Add @nullable import from jspecify
  • Mark stackTrace field as @nullable
  • Add @nullable annotation to getStackTrace() method
  • Add @nullable annotation to stackTrace constructor parameter
+5/-3     
ConsoleLogEntry.java
Add nullability annotation to ConsoleLogEntry                       

java/src/org/openqa/selenium/bidi/log/ConsoleLogEntry.java

+2/-1     
GenericLogEntry.java
Add nullability annotation and null-check to GenericLogEntry

java/src/org/openqa/selenium/bidi/log/GenericLogEntry.java

  • Add @nullable import from jspecify
  • Mark stackTrace constructor parameter as @nullable
  • Add null-check in toJson() before putting stackTrace in map
+5/-2     
JavascriptLogEntry.java
Add nullability annotation and null-check to JavascriptLogEntry

java/src/org/openqa/selenium/bidi/log/JavascriptLogEntry.java

  • Add @nullable import from jspecify
  • Mark stackTrace constructor parameter as @nullable
  • Add null-check in toJson() before putting stackTrace in map
+5/-2     
package-info.java
Create package-info with @NullMarked annotation                   

java/src/org/openqa/selenium/bidi/log/package-info.java

  • Create new package-info.java file
  • Add @NullMarked annotation to mark entire package as non-null by
    default
  • Include Apache License header
+22/-0   
Cleanup
LogInspectorTest.java
Update test imports to modern AssertJ API                               

java/test/org/openqa/selenium/bidi/log/LogInspectorTest.java

  • Update AssertJ imports from AssertionsForClassTypes to Assertions
  • Modernize static imports to use generic Assertions class
+3/-3     
Configuration changes
BUILD.bazel
Add JSpecify dependency to BUILD configuration                     

java/src/org/openqa/selenium/bidi/log/BUILD.bazel

  • Add load statement for artifact function from rules_jvm_external
  • Add org.jspecify:jspecify dependency to deps list
+2/-0     

@asolntsev asolntsev requested a review from diemol December 4, 2025 12:18
@selenium-ci selenium-ci added C-java Java Bindings B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related labels Dec 4, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 4, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Nullability regression

Description: Changing the previously non-null field stackTrace to @Nullable and exposing it via
getStackTrace() may introduce null-handling regressions for existing consumers if not
universally guarded, potentially causing NullPointerExceptions in downstream code.
BaseLogEntry.java [20-32]

Referred Code
import org.jspecify.annotations.Nullable;
import org.openqa.selenium.bidi.script.Source;

// @see <a
// href="https://w3c.github.io/webdriver-bidi/#types-log-logentry">https://w3c.github.io/webdriver-bidi/#types-log-logentry</a>
public class BaseLogEntry {

  private final LogLevel level;
  private final Source source;
  private final String text;
  private final long timestamp;
  @Nullable private final StackTrace stackTrace;
Ticket Compliance
🟡
🎫 #3196
🔴 Ensure JavaScript execution works in Firefox via Geckodriver without throwing "angular is
not defined (Unexpected JavaScriptError)" when executing scripts.
Provide a fix or change that enables running JS snippets like
angular.element(document.body) without unexpected JS errors.
Confirm behavior on Windows 10 with Selenium WebDriver where applicable.
🟡
🎫 #1234
🔴 Clicking links with javascript: href should trigger JavaScript in Selenium 2.48.x with
Firefox 42 similar to 2.47.1.
Provide or verify a fix ensuring alerts are triggered as expected on click().
🟡
🎫 #5678
🔴 Resolve or mitigate "ConnectFailure (Connection refused)" when instantiating multiple
ChromeDriver instances on Ubuntu 16.04.4 with Selenium 3.9.0 and ChromeDriver 2.35.
Ensure stable creation of successive ChromeDriver instances without console errors.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
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: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Audit context unclear: The changes focus on nullability and conditional serialization of stack traces without
adding or modifying any auditing fields (user ID, action, outcome), so it is unclear if
critical actions are fully and contextually logged.

Referred Code
private Map<String, Object> toJson() {
  Map<String, Object> toReturn = new TreeMap<>();
  toReturn.put("type", type);
  toReturn.put("level", super.getLevel());
  toReturn.put("text", super.getText());
  toReturn.put("timestamp", super.getTimestamp());
  if (getStackTrace() != null) {
    toReturn.put("stackTrace", getStackTrace());
  }

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:
Stack trace exposure: Although stackTrace is now optional, when present it is still serialized to logs, which
may expose sensitive details depending on upstream usage; repository context is
insufficient to verify sanitization policies.

Referred Code
if (getStackTrace() != null) {
  toReturn.put("stackTrace", getStackTrace());
}

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:
Args validation: The constructor accepts a list of RemoteValue args for logging without visible validation
or sanitization in this diff, leaving uncertainty about potential injection or sensitive
data exposure in logs.

Referred Code
public ConsoleLogEntry(
    LogLevel level,
    Source source,
    String text,
    long timestamp,
    String type,
    String method,
    List<RemoteValue> args,
    @Nullable StackTrace stackTrace) {
  super(level, source, text, timestamp, type, stackTrace);

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
Copy link
Contributor

qodo-code-review bot commented Dec 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Remove redundant code and fix constructor

Simplify the JavascriptLogEntry class by removing the shadowed type field, the
unused type constructor parameter, and the redundant getType() and toJson()
methods, which are already correctly implemented in the parent GenericLogEntry
class.

java/src/org/openqa/selenium/bidi/log/JavascriptLogEntry.java [30-107]

 public class JavascriptLogEntry extends GenericLogEntry {
-
-  private final String type;
 
   public JavascriptLogEntry(
       LogLevel level,
       Source source,
       String text,
       long timestamp,
-      String type,
       @Nullable StackTrace stackTrace) {
     super(level, source, text, timestamp, "javascript", stackTrace);
-    this.type = "javascript";
-  }
-
-  public String getType() {
-    return type;
   }
 
   public static JavascriptLogEntry fromJson(JsonInput input) {
 ...
   }
-
-  private Map<String, Object> toJson() {
-    Map<String, Object> toReturn = new TreeMap<>();
-    toReturn.put("type", type);
-    toReturn.put("level", super.getLevel());
-    toReturn.put("text", super.getText());
-    toReturn.put("timestamp", super.getTimestamp());
-    if (getStackTrace() != null) {
-      toReturn.put("stackTrace", getStackTrace());
-    }
-
-    return unmodifiableMap(toReturn);
-  }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies and proposes removing redundant code (a shadowed field, an overridden method, and a duplicated method) which simplifies the class and improves maintainability.

Medium
High-level
PR justification is misaligned

The PR description links to an incorrect and unrelated issue, making the
motivation for the changes unclear. The description should be updated with a
relevant issue or a clear explanation for the change.

Solution Walkthrough:

Before:

PR Description:
### 🔗 Related Issues
Fixes #[3196](https://github.com/selenide/selenide/issues/3196)

### 💥 What does this PR do?
adds `@NullMarked` and `@Nullable` annotations to "*.bidi.log" package.

After:

PR Description:
### 🔗 Related Issues
Fixes #<relevant_issue_number_in_selenium_project>
or
Part of an ongoing effort to improve null-safety across the codebase.

### 💥 What does this PR do?
This PR improves null-safety in the `org.openqa.selenium.bidi.log` package by adding JSpecify's `@NullMarked` and `@Nullable` annotations. This prevents potential `NullPointerException`s when handling log entries where a stack trace might be absent.

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the PR's justification is invalid, as the linked issue is unrelated, which is a significant process issue that hinders proper review and future maintenance.

Low
  • Update

@asolntsev asolntsev force-pushed the fix/nullable-annotations-in-bidi-log branch from 702faf8 to 484e2ae Compare December 4, 2025 12:47
@asolntsev asolntsev self-assigned this Dec 4, 2025
@asolntsev asolntsev merged commit 790d86b into SeleniumHQ:trunk Dec 4, 2025
37 of 38 checks passed
@asolntsev asolntsev deleted the fix/nullable-annotations-in-bidi-log branch December 4, 2025 16:23
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-devtools Includes everything BiDi or Chrome DevTools related C-java Java Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants