Skip to content

[java] Fluent setters in few classes like PrintOptions etc.#17148

Merged
asolntsev merged 8 commits intoSeleniumHQ:trunkfrom
asolntsev:fluent-setters
Mar 1, 2026
Merged

[java] Fluent setters in few classes like PrintOptions etc.#17148
asolntsev merged 8 commits intoSeleniumHQ:trunkfrom
asolntsev:fluent-setters

Conversation

@asolntsev
Copy link
Copy Markdown
Contributor

@asolntsev asolntsev commented Feb 28, 2026

🔗 Related Issues

none

💥 What does this PR do?

Makes some setters return this. In other words, make these setter method "fluent" or "chainable".

This style allows calling multiple setters in a row, thus making code shorter/readable.

Also, added few unit-tests for uncovered constructors of FirefoxOptions etc.

🔧 Implementation Notes

I changed only a few less-critical classes (like PrintOptions),
and didn't yet touch too widely used classes like MutableCapabilities which still has void setters.

🔄 Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

This PR changes signatures of some setter methods. Formally, it's a breaking change - though, nothing should break in users' code.

@selenium-ci selenium-ci added B-grid Everything grid and server related C-java Java Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Feb 28, 2026
@asolntsev asolntsev changed the title Fluent setters [java] Fluent setters in few classes like PrintOptions etc. Feb 28, 2026
@asolntsev asolntsev self-assigned this Feb 28, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

PR Type

Enhancement


Description

  • Make setter methods fluent/chainable by returning this

  • Applied to 8 classes: ChromiumNetworkConditions, ErrorHandler, LoggingOptions, SeleniumManagerOutput, PrintOptions, Preferences, FirefoxProfile, DesiredCapabilities

  • Added static factory method withLatency() to ChromiumNetworkConditions

  • Added new FirefoxOptions constructor accepting FirefoxProfile parameter

  • Refactored test code to use fluent setter chains for improved readability

  • Removed unused validation logic from Preferences class


File Walkthrough

Relevant files
Enhancement
10 files
ChromiumNetworkConditions.java
Make all setters fluent and add factory method                     
+12/-4   
AddHasNetworkConditions.java
Refactor to use fluent setters and static imports               
+17/-17 
FirefoxOptions.java
Add profile constructor and helper methods                             
+14/-1   
FirefoxProfile.java
Make all setters fluent and chainable                                       
+8/-4     
Preferences.java
Make setter fluent and remove validation logic                     
+7/-42   
LoggingOptions.java
Make setLoggingLevel method fluent                                             
+2/-1     
SeleniumManagerOutput.java
Make setters fluent for logs and result                                   
+4/-2     
PrintOptions.java
Make all setters fluent and chainable                                       
+16/-8   
DesiredCapabilities.java
Make all setters fluent and chainable                                       
+8/-4     
ErrorHandler.java
Make setIncludeServerErrors fluent                                             
+2/-1     
Tests
13 files
PrintPageTest.java
Refactor tests to use fluent setter chains                             
+6/-8     
WebScriptExecuteTest.java
Simplify test by inlining PrintOptions creation                   
+1/-4     
BrowsingContextTest.java
Simplify test by inlining PrintOptions creation                   
+1/-3     
ChromeDriverFunctionalTest.java
Use fluent setters and factory method for network conditions
+6/-8     
EdgeDriverFunctionalTest.java
Use fluent factory method for network conditions                 
+2/-4     
FirefoxDriverPreferenceTest.java
Refactor to use fluent setter chains                                         
+4/-3     
FirefoxDriverTest.java
Use new FirefoxOptions constructor with profile                   
+1/-1     
FirefoxOptionsTest.java
Add tests for constructors and fluent setters                       
+23/-9   
PreferencesTest.java
Add test for fluent setter chaining                                           
+17/-1   
PageSizeTest.java
Simplify tests by removing unnecessary setup                         
+8/-21   
PrintOptionsTest.java
Refactor tests to use fluent setter chains                             
+9/-14   
DesiredCapabilitiesTest.java
Add tests for constructors and fluent setters                       
+51/-2   
ErrorHandlerTest.java
Use fluent setter in field initialization                               
+1/-8     

@asolntsev asolntsev added this to the 4.42.0 milestone Feb 28, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Preference validation removed

Description: The updated Preferences implementation exposes all preferences via asMap() and allows any
preference to be set via the fluent setPreference(...) without the previously-enforced
restrictions on overriding frozen defaults and on limiting dom.max_script_run_time, which
could allow untrusted inputs (if any flow into preferences) to weaken security-related
Firefox settings or bypass intended invariants.
Preferences.java [81-132]

Referred Code
Map<String, Object> asMap() {
  return unmodifiableMap(allPrefs);
}

private void readUserPrefs(File userPrefs) {
  try (Reader reader = Files.newBufferedReader(userPrefs.toPath(), Charset.defaultCharset())) {
    readPreferences(reader);
  } catch (IOException e) {
    throw new WebDriverException(e);
  }
}

private void readDefaultPreferences(Reader defaultsReader) {
  try {
    String rawJson;
    try (StringWriter writer = new StringWriter()) {
      defaultsReader.transferTo(writer);
      rawJson = writer.getBuffer().toString();
    }
    Map<String, Object> map = new Json().toType(rawJson, MAP_TYPE);



 ... (clipped 31 lines)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
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: 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: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing validation checks: The updated setPreference no longer validates key/value for null nor enforces previously
protected edge-case constraints (e.g., frozen preferences), allowing invalid overrides or
null inputs to propagate and fail unpredictably later.

Referred Code
public Preferences setPreference(String key, Object value) {
  if (value instanceof String) {
    if (isStringified((String) value)) {
      throw new IllegalArgumentException(
          String.format("Preference values must be plain strings: %s: %s", key, value));
    }
    allPrefs.put(key, value);
  } else if (value instanceof Number) {
    allPrefs.put(key, ((Number) value).intValue());
  } else {
    allPrefs.put(key, value);
  }
  return this;
}

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:
Missing input validation: The new fluent setPreference implementation does not perform any explicit
validation/sanitization of inputs (e.g., null checks for key and value) and exposes
internal mutable state changes without guarding against disallowed overrides.

Referred Code
public Preferences setPreference(String key, Object value) {
  if (value instanceof String) {
    if (isStringified((String) value)) {
      throw new IllegalArgumentException(
          String.format("Preference values must be plain strings: %s: %s", key, value));
    }
    allPrefs.put(key, value);
  } else if (value instanceof Number) {
    allPrefs.put(key, ((Number) value).intValue());
  } else {
    allPrefs.put(key, value);
  }
  return this;
}

Learn more about managing compliance generic rules or creating your own custom rules

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

@asolntsev asolntsev removed the B-devtools Includes everything BiDi or Chrome DevTools related label Feb 28, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Restore validation for frozen preferences

Restore the validation that prevents "frozen" preferences from being overridden.
This check was removed during the refactoring to fluent setters.

java/src/org/openqa/selenium/firefox/Preferences.java [93-132]

+private final Map<String, Object> immutablePrefs = new HashMap<>();
+private final Map<String, Object> allPrefs = new HashMap<>();
+
+// ... constructors ...
+
 private void readDefaultPreferences(Reader defaultsReader) {
   try (Reader reader = defaultsReader) {
     Map<String, Object> map = new Json().toType(reader, MAP_TYPE);
 
     @SuppressWarnings("unchecked")
     Map<String, Object> frozen = (Map<String, Object>) map.get("frozen");
     frozen.forEach(
         (key, value) -> {
           if (value instanceof Long) {
             value = ((Long) value).intValue();
           }
           setPreference(key, value);
+          immutablePrefs.put(key, value);
         });
 
     Map<String, Object> mutable = (Map<String, Object>) map.get("mutable");
     mutable.forEach(this::setPreference);
 
   } catch (IOException e) {
     throw new WebDriverException(e);
   }
 }
 
 public Preferences setPreference(String key, Object value) {
+  if (immutablePrefs.containsKey(key) && !value.equals(immutablePrefs.get(key))) {
+    throw new IllegalArgumentException(
+        String.format(
+            "Preference %s may not be overridden: frozen value=%s, requested value=%s",
+            key, immutablePrefs.get(key), value));
+  }
+
   if (value instanceof String) {
     if (isStringified((String) value)) {
       throw new IllegalArgumentException(
           String.format("Preference values must be plain strings: %s: %s", key, value));
     }
     allPrefs.put(key, value);
   } else if (value instanceof Number) {
     allPrefs.put(key, ((Number) value).intValue());
   } else {
     allPrefs.put(key, value);
   }
   return this;
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a functional regression where validation for "frozen" preferences was removed. Restoring this check is critical for maintaining browser stability and preventing unintended configuration changes.

High
Use Number.longValue for latency

To prevent a potential ClassCastException, cast the latency value to Number and
use longValue() instead of casting directly to Long.

java/src/org/openqa/selenium/chromium/AddHasNetworkConditions.java [85]

-.setLatency(Duration.ofMillis((Long) result.getOrDefault(LATENCY, 0)))
+.setLatency(Duration.ofMillis(((Number) result.getOrDefault(LATENCY, 0)).longValue()))
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out that casting to Long is unsafe and will cause a ClassCastException if the key is not present, as getOrDefault returns an Integer. Using Number and longValue() is more robust and consistent with surrounding code.

Medium
General
Replace requireNonNull with explicit exception

Replace requireNonNull with an explicit null check that throws a descriptive
WebDriverException if the result from executeMethod is null.

java/src/org/openqa/selenium/chromium/AddHasNetworkConditions.java [79-82]

 @SuppressWarnings("unchecked")
 Map<String, Object> result =
-    (Map<String, Object>)
-        requireNonNull(executeMethod.execute(GET_NETWORK_CONDITIONS, null));
+    (Map<String, Object>) executeMethod.execute(GET_NETWORK_CONDITIONS, null);
+if (result == null) {
+  throw new WebDriverException(
+      "Network conditions must be set before they can be retrieved");
+}
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly proposes replacing requireNonNull with a more descriptive, domain-specific exception (WebDriverException), which improves error reporting and is consistent with how a similar case is handled when deleting network conditions.

Low
Learned
best practice
Add input validation to setters

Add explicit null/blank validation for key and null validation for value (with a
clear message) so callers fail fast instead of getting implicit NPEs later.

java/src/org/openqa/selenium/firefox/Preferences.java [119-132]

 public Preferences setPreference(String key, Object value) {
+  if (key == null || key.isBlank()) {
+    throw new IllegalArgumentException("Preference key must be non-null and non-blank");
+  }
+  if (value == null) {
+    throw new IllegalArgumentException("Preference value must be non-null");
+  }
+
   if (value instanceof String) {
     if (isStringified((String) value)) {
       throw new IllegalArgumentException(
           String.format("Preference values must be plain strings: %s: %s", key, value));
     }
     allPrefs.put(key, value);
   } else if (value instanceof Number) {
     allPrefs.put(key, ((Number) value).intValue());
   } else {
     allPrefs.put(key, value);
   }
   return this;
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Validate required inputs early and fail fast with a clear, actionable error message (avoid implicit NPEs / downstream crashes).

Low
  • More

@asolntsev asolntsev merged commit 5db6096 into SeleniumHQ:trunk Mar 1, 2026
47 of 49 checks passed
@asolntsev asolntsev deleted the fluent-setters branch March 1, 2026 19:38
AutomatedTester pushed a commit that referenced this pull request Mar 11, 2026
* make setter fluent: ChromiumNetworkConditions

This style allows calling multiple setters in a row, thus making code shorter/readable.

* make setter fluent: ErrorHandler

This style allows calling multiple setters in a row, thus making code shorter/readable.

* make setter fluent: LoggingOptions

* make setter fluent: SeleniumManagerOutput

* make setter fluent: PrintOptions

* make setter fluent: Preferences

* make setter fluent: FirefoxProfile

* make setter fluent: DesiredCapabilities
This was referenced Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-grid Everything grid and server related C-java Java Bindings Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants