Skip to content

[grid] Replace Guava list,set,map and sorted set with Java equivalent #16206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Aug 19, 2025

User description

🔗 Related Issues

Starting PR for replacing Guava's Immutable Set, List, Map, SortSet and Primitives usage with Java 11 or other equivalent in the Grid.

💥 What does this PR do?

PR replaces Guava's Immutable Set, List, Map, SortSet and Primitives usage with Java 11 or other equivalent in the Grid's packages, commands and config. The rest of the change will be done as follow up PRs.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Replace Guava collections with Java 11 equivalents

  • Remove dependencies on ImmutableList, ImmutableSet, ImmutableMap

  • Update sorted collections to use TreeSet

  • Modernize collection creation patterns


Diagram Walkthrough

flowchart LR
  A["Guava Collections"] -- "Replace with" --> B["Java 11 Collections"]
  B --> C["Map.of()"]
  B --> D["Set.of()"]
  B --> E["List.of()"]
  B --> F["TreeSet + Collections.unmodifiable*"]
Loading

File Walkthrough

Relevant files
Enhancement
14 files
DefaultHubConfig.java
Replace ImmutableMap with Map.of                                                 
+5/-5     
DefaultStandaloneConfig.java
Replace ImmutableMap with Map.of                                                 
+4/-5     
EventBusCommand.java
Replace ImmutableSet and ImmutableMap with Java equivalents
+7/-8     
Hub.java
Replace ImmutableSet with Set.of                                                 
+1/-3     
Standalone.java
Replace ImmutableSet with Set.of                                                 
+1/-3     
AnnotatedConfig.java
Replace Guava collections with TreeSet and List.copyOf     
+5/-6     
CompoundConfig.java
Replace Guava collectors with Java stream collectors         
+14/-9   
ConcatenatingConfig.java
Replace ImmutableMap and ImmutableSortedSet with Java equivalents
+11/-9   
Config.java
Replace ImmutableList with List.copyOf                                     
+1/-2     
ConfigFlags.java
Replace ImmutableSet and ImmutableSortedSet with Java equivalents
+11/-12 
DescribedOption.java
Replace Guava Primitives and collections with Java equivalents
+26/-10 
EnvConfig.java
Replace ImmutableList and ImmutableSortedSet with Java equivalents
+10/-7   
MapConfig.java
Replace Guava collections with Java unmodifiable collections
+16/-15 
TomlConfig.java
Replace ImmutableList and ImmutableSortedSet with Java equivalents
+7/-7     

@pujagani pujagani added the C-java Java Bindings label Aug 19, 2025
@selenium-ci selenium-ci added the B-grid Everything grid and server related label Aug 19, 2025
@pujagani pujagani marked this pull request as ready for review August 19, 2025 11:07
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1234 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Fix click() to trigger JavaScript in href for Firefox.
  • Add tests demonstrating the regression and the fix.

Requires further human verification:

  • Manual/browser verification on affected Firefox versions.
  • End-to-end test behavior across supported browsers and versions.

5678 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Resolve ConnectFailure on multiple ChromeDriver instantiations.
  • Add diagnostics and/or retries as needed; include tests or docs.

Requires further human verification:

  • Reproduction on target environment and versions.
  • Runtime logs and concurrency behavior under load.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The replacement for Guava Primitives.wrap uses a custom map PRIMITIVE_TO_WRAPPER. Ensure it fully mirrors Guava behavior for all primitives and is used consistently; also confirm no null case for non-primitive types.

Map.ofEntries(
    Map.entry(boolean.class, Boolean.class),
    Map.entry(byte.class, Byte.class),
    Map.entry(char.class, Character.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.entry(void.class, Void.class));
Behavior Change

getSectionNames() and getOptions() now return sorted/unmodifiable sets; verify callers did not rely on previous insertion order or specific set implementations.

@Override
public Set<String> getSectionNames() {
  return Set.copyOf(raw.keySet());
}

@Override
public Set<String> getOptions(String section) {
  Require.nonNull("Section name to get options for", section);

  Map<String, Object> values = raw.getOrDefault(section, Map.of());
  return Collections.unmodifiableSortedSet(new TreeSet<>(values.keySet()));
}
API Compatibility

JSON response map construction changed to Map.of(...). Confirm no null values can be passed and that map size constraints (<=10 entries) are respected.

private HttpResponse httpResponse(boolean ready, String message) {
  return new HttpResponse()
      .addHeader("Content-Type", JSON_UTF_8)
      .setContent(
          asJson(
              Map.of(
                  "value",
                  Map.of(
                      "ready", ready,
                      "message", message))));
}

Copy link
Contributor

qodo-merge-pro bot commented Aug 19, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Avoid unmodifiable sets during collection

Using Collections.unmodifiableSortedSet as the map value supplier prevents later
additions when keys collide, forcing copies on each merge. Supply a mutable
SortedSet during collection and wrap only once at the end. This reduces copying
and avoids potential UnsupportedOperationException during merges.

java/src/org/openqa/selenium/grid/config/ConfigFlags.java [103-113]

 Map<String, Set<DescribedOption>> allOptions =
     DescribedOption.findAllMatchingOptions(currentRoles).stream()
         .collect(
             Collectors.toMap(
                 DescribedOption::section,
-                option -> Collections.unmodifiableSortedSet(new TreeSet<>(Set.of(option))),
+                option -> {
+                  SortedSet<DescribedOption> set = new TreeSet<>();
+                  set.add(option);
+                  return set;
+                },
                 (l, r) -> {
-                  SortedSet<DescribedOption> merged = new TreeSet<>(l);
-                  merged.addAll(r);
-                  return Collections.unmodifiableSortedSet(merged);
-                }));
+                  l.addAll(r);
+                  return l;
+                }))
+        .entrySet()
+        .stream()
+        .collect(Collectors.toMap(
+            Map.Entry::getKey,
+            e -> Collections.unmodifiableSortedSet(new TreeSet<>(e.getValue()))
+        ));

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that creating an unmodifiable set within the valueMapper of Collectors.toMap is inefficient, as it forces the mergeFunction to create a new copy for every merge operation. The proposed change improves performance by collecting to mutable sets first and then wrapping them as unmodifiable in a final step.

Low
  • Update

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@pujagani
Copy link
Contributor Author

Found an issue in a related PR. Trying to sort it out first, then accordingly update here if required.

@pujagani pujagani marked this pull request as ready for review August 22, 2025 09:30
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

5678 - Partially compliant

Compliant requirements:

  • None

Non-compliant requirements:

  • Investigate and resolve "Error: ConnectFailure (Connection refused)" when instantiating multiple ChromeDriver instances.
  • Ensure subsequent ChromeDriver instantiations do not log connection refusal errors.
  • Provide reproducible steps and validate behavior across runs.

Requires further human verification:

  • N/A

1234 - Partially compliant

Compliant requirements:

  • None

Non-compliant requirements:

  • Ensure click() triggers JavaScript in href in Firefox.
  • Provide/validate a regression fix.

Requires further human verification:

  • N/A
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Behavioral Change

The default events implementation remains "GuavaEventBus" while the PR aims to remove Guava usages. Confirm that this class is still available and intended, or update to a non-Guava implementation to avoid runtime dependency on Guava.

          "events", Map.of("implementation", "org.openqa.selenium.events.local.GuavaEventBus"),
          "sessions",
              Map.of(
                  "implementation",
                  "org.openqa.selenium.grid.sessionmap.local.LocalSessionMap")));
}
Map Copy Semantics

The constructor builds an unmodifiable map of section->values without deep-copying nested mutable maps or collections. If the input map is later mutated externally, inner objects might change. Validate callers never mutate inputs or consider defensive copying.

  Require.nonNull("Underlying map", raw);

  Map<String, Map<String, Object>> validated = new HashMap<>();

  for (Map.Entry<String, Object> entry : raw.entrySet()) {
    if (!(entry.getValue() instanceof Map)) {
      continue;
    }

    Map<String, Object> values =
        ((Map<?, ?>) entry.getValue())
            .entrySet().stream()
                .filter(e -> e.getKey() instanceof String)
                .collect(
                    Collectors.toUnmodifiableMap(
                        e -> String.valueOf(e.getKey()), Map.Entry::getValue));

    validated.put(entry.getKey(), values);
  }

  this.raw = Collections.unmodifiableMap(validated);
}
Set Ordering/Equality

Replaced Guava set operations with TreeSet and Collections.disjoint. Ensure natural ordering and equals/hashCode behavior of DescribedOption remains consistent, and that Locale/flags ordering does not affect output stability across platforms.

  Objects.requireNonNull(roles);

  Set<Role> minimized = Set.copyOf(roles);

  return StreamSupport.stream(ServiceLoader.load(HasRoles.class).spliterator(), false)
      .filter(hasRoles -> !Collections.disjoint(hasRoles.getRoles(), minimized))
      .flatMap(DescribedOption::getAllFields)
      .collect(
          Collectors.collectingAndThen(
              Collectors.toCollection(TreeSet::new), Collections::unmodifiableSortedSet));
}

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Preserve original flag order

Preserve flag ordering defined by JCommander for predictable help output. Using
TreeSet reorders flags lexicographically; instead use an unmodifiable List to
retain the original order.

java/src/org/openqa/selenium/grid/config/DescribedOption.java [83]

-this.flags = Collections.unmodifiableSortedSet(new TreeSet<>(Arrays.asList(parameter.names())));
+this.flags = Collections.unmodifiableSet(
+    new java.util.LinkedHashSet<>(Arrays.asList(parameter.names())));
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that using TreeSet changes the order of flags, and proposes using LinkedHashSet to preserve the original insertion order, which improves consistency in help messages.

Medium
Possible issue
Prevent unmodifiable set merge failures

Avoid returning unmodifiable views during collection to prevent
UnsupportedOperationException in the merge function. Build a mutable set in the
collector and wrap it as unmodifiable only after collection completes.

java/src/org/openqa/selenium/grid/config/ConfigFlags.java [103-113]

 Map<String, Set<DescribedOption>> allOptions =
     DescribedOption.findAllMatchingOptions(currentRoles).stream()
         .collect(
             Collectors.toMap(
                 DescribedOption::section,
-                option -> Collections.unmodifiableSortedSet(new TreeSet<>(Set.of(option))),
+                option -> {
+                  SortedSet<DescribedOption> set = new TreeSet<>();
+                  set.add(option);
+                  return set;
+                },
                 (l, r) -> {
-                  SortedSet<DescribedOption> merged = new TreeSet<>(l);
-                  merged.addAll(r);
-                  return Collections.unmodifiableSortedSet(merged);
-                }));
+                  l.addAll(r);
+                  return l;
+                }))
+        .entrySet().stream()
+        .collect(Collectors.toMap(
+            Map.Entry::getKey,
+            e -> Collections.unmodifiableSortedSet(new TreeSet<>(e.getValue()))
+        ));

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that the merge function can be more efficient, but the justification about preventing an UnsupportedOperationException is incorrect as the original code creates new sets instead of modifying existing ones.

Low
  • More

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.

3 participants