Skip to content

Conversation

@iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Nov 4, 2025

User description

🔗 Related Issues

💥 What does this PR do?

This pull request refactors several places in the codebase to use modern Java collection factory methods (Map.of, Map.ofEntries, Map.copyOf, and List.of) instead of older patterns that manually create and wrap collections. This improves code readability, reduces boilerplate, and leverages immutable collections directly from the JDK.

Refactoring to use modern collection factory methods:

  • Replaced manual construction and wrapping of maps with Map.of in DeviceRotation.parameters() for more concise and immutable map creation.
  • Updated CapabilityCount and CreateSessionRequest to use Map.copyOf for immutable maps instead of unmodifiableMap(new HashMap<>(...)). [1] [2]
  • Changed NumberCoercer.PRIMITIVE_NUMBER_TYPES initialization to use Map.ofEntries for cleaner and more direct mapping.
  • Refactored VirtualAuthenticatorOptions.toMap() to use Map.ofEntries instead of manual map creation and wrapping.

Modernizing collection usage in tests:

  • Updated test code in DefaultSlotSelectorTest to use List.of instead of Arrays.asList, aligning with modern Java idioms.

🔧 Implementation Notes

I primarily changed unmodifiable collections that do not affect the contract of classes and methods
There are still unmodifiableSet and unmodifiableMap types in the code, but I haven't figured out yet if I can do anything with them.

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Replace manual collection wrapping with modern Java factory methods

  • Use Map.of, Map.copyOf, and Map.ofEntries for cleaner code

  • Eliminate unnecessary HashMap and Collections.unmodifiableMap patterns

  • Update test code to use List.of instead of Arrays.asList


Diagram Walkthrough

flowchart LR
  A["Manual HashMap + unmodifiableMap"] -->|Refactor| B["Map.of / Map.copyOf"]
  C["Arrays.asList"] -->|Modernize| D["List.of"]
  B -->|Result| E["Cleaner, more concise code"]
  D -->|Result| E
Loading

File Walkthrough

Relevant files
Enhancement
DeviceRotation.java
Simplify map creation using Map.of                                             

java/src/org/openqa/selenium/DeviceRotation.java

  • Replaced manual HashMap construction with Map.of in parameters()
    method
  • Removed unnecessary imports for Collections and HashMap
  • Simplified map creation from 5 lines to 1 line
+1/-7     
CapabilityCount.java
Use Map.copyOf for immutable map creation                               

java/src/org/openqa/selenium/grid/data/CapabilityCount.java

  • Replaced unmodifiableMap(new HashMap<>(counts)) with
    Map.copyOf(counts)
  • Removed static import of unmodifiableMap
  • Simplified constructor initialization
+1/-2     
CreateSessionRequest.java
Modernize metadata map initialization                                       

java/src/org/openqa/selenium/grid/data/CreateSessionRequest.java

  • Replaced unmodifiableMap(new HashMap<>(...)) with Map.copyOf for
    metadata field
  • Removed static import of unmodifiableMap
  • Removed unused HashMap import
+1/-3     
NumberCoercer.java
Use Map.ofEntries for primitive type mapping                         

java/src/org/openqa/selenium/json/NumberCoercer.java

  • Refactored PRIMITIVE_NUMBER_TYPES static initialization to use
    Map.ofEntries
  • Removed manual HashMap construction and Collections.unmodifiableMap
    wrapping
  • Simplified from 9 lines to 6 lines with improved readability
+8/-11   
VirtualAuthenticatorOptions.java
Simplify toMap method using Map.ofEntries                               

java/src/org/openqa/selenium/virtualauthenticator/VirtualAuthenticatorOptions.java

  • Refactored toMap() method to use Map.ofEntries instead of manual map
    construction
  • Removed unnecessary HashMap and Collections.unmodifiableMap usage
  • Simplified from 8 lines to 6 lines
+7/-10   
Tests
DefaultSlotSelectorTest.java
Modernize test collections to use List.of                               

java/test/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelectorTest.java

  • Replaced Arrays.asList with List.of in four test cases
  • Modernized collection initialization to use Java 9+ idioms
  • Improved code consistency and readability
+4/-4     

@selenium-ci selenium-ci added B-grid Everything grid and server related C-java Java Bindings labels Nov 4, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 4, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Unchecked nulls in Map.of

Description: Using Map.of for parameters() creates an unmodifiable map that throws NullPointerException
if any value is null; if x, y, or z can be null via external input, this may cause runtime
exceptions and potential denial-of-service paths.
DeviceRotation.java [96-98]

Referred Code
public Map<String, Integer> parameters() {
  return Map.of("x", this.x, "y", this.y, "z", this.z);
}
Null-sensitive immutable map

Description: Map.ofEntries throws NullPointerException if any entry value is null; if optional fields
like transport or protocol can be unset, this could cause runtime failures on
serialization paths.
VirtualAuthenticatorOptions.java [95-103]

Referred Code
public Map<String, Object> toMap() {
  return Map.ofEntries(
      Map.entry("protocol", protocol.id),
      Map.entry("transport", transport.id),
      Map.entry("hasResidentKey", hasResidentKey),
      Map.entry("hasUserVerification", hasUserVerification),
      Map.entry("isUserConsenting", isUserConsenting),
      Map.entry("isUserVerified", isUserVerified));
}
Ticket Compliance
🟡
🎫 #5678
🔴 Investigate and resolve repeated "Error: ConnectFailure (Connection refused)" when
instantiating multiple ChromeDriver instances on Ubuntu 16.04 with Chrome 65 and
ChromeDriver 2.35 using Selenium 3.9.0.
Ensure subsequent ChromeDriver instantiations do not log connection failures after the
first succeeds.
Provide guidance or code changes that address environment-specific causes if applicable.
🟡
🎫 #1234
🔴 Fix regression where WebDriver 2.48.0/2.48.2 does not trigger JavaScript in link href on
click() in Firefox 42, which worked in 2.47.1.
Add tests to verify click() triggers JavaScript href handlers in affected environments.
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

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

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

Generic: Comprehensive Audit Trails

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

Status:
No audit logs: The changes refactor collection creation without introducing or modifying any logging for
critical actions, so there is no evidence that relevant actions are audited.

Referred Code
public CreateSessionRequest(
    Set<Dialect> downstreamDialects, Capabilities capabilities, Map<String, Object> metadata) {
  this.downstreamDialects =
      unmodifiableSet(new HashSet<>(Require.nonNull("Downstream dialects", downstreamDialects)));
  this.capabilities = ImmutableCapabilities.copyOf(Require.nonNull("Capabilities", capabilities));
  this.metadata = Map.copyOf(Require.nonNull("Metadata", metadata));
}
Generic: Robust Error Handling and Edge Case Management

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

Status:
No error handling: The refactor changes map initialization only and does not add error handling for potential
edge cases, leaving uncertainty whether failures are handled elsewhere.

Referred Code
class NumberCoercer<T extends Number> extends TypeCoercer<T> {

  private static final Map<Class<?>, Class<?>> PRIMITIVE_NUMBER_TYPES;

  static {
    PRIMITIVE_NUMBER_TYPES =
        Map.ofEntries(
            Map.entry(byte.class, Byte.class),
            Map.entry(double.class, Double.class),
            Map.entry(float.class, Float.class),
            Map.entry(int.class, Integer.class),
            Map.entry(long.class, Long.class),
            Map.entry(short.class, Short.class));
  }
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input validation unclear: The constructor now uses Map.copyOf for metadata but does not show validation or
sanitization of external inputs within the diff, so secure handling cannot be confirmed.

Referred Code
public CreateSessionRequest(
    Set<Dialect> downstreamDialects, Capabilities capabilities, Map<String, Object> metadata) {
  this.downstreamDialects =
      unmodifiableSet(new HashSet<>(Require.nonNull("Downstream dialects", downstreamDialects)));
  this.capabilities = ImmutableCapabilities.copyOf(Require.nonNull("Capabilities", capabilities));
  this.metadata = Map.copyOf(Require.nonNull("Metadata", metadata));
}
  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Use Map.of for conciseness

Replace Map.ofEntries(...) with the more concise Map.of(...) to initialize the
PRIMITIVE_NUMBER_TYPES map, as there are fewer than 10 entries.

java/src/org/openqa/selenium/json/NumberCoercer.java [32-39]

 PRIMITIVE_NUMBER_TYPES =
-    Map.ofEntries(
-        Map.entry(byte.class, Byte.class),
-        Map.entry(double.class, Double.class),
-        Map.entry(float.class, Float.class),
-        Map.entry(int.class, Integer.class),
-        Map.entry(long.class, Long.class),
-        Map.entry(short.class, Short.class));
+    Map.of(
+        byte.class, Byte.class,
+        double.class, Double.class,
+        float.class, Float.class,
+        int.class, Integer.class,
+        long.class, Long.class,
+        short.class, Short.class);
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a more concise way to initialize the map using Map.of() instead of Map.ofEntries(), which aligns with the PR's goal of code modernization and simplification.

Low
  • Update

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 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants