Ignore extension caps with special suffixes

[sbabcoc]

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Motivation and Context

Currently, extension capabilities with suffixes "options", "Options", "loggingPrefs", and "debuggerAddress" are considered for purposed of Grid node slot matching. This means that capabilities defined as configuration options are treated as "identity" values. The revisions in this PR enable the extension matcher to ignore capabilities with these suffixes.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Enhancement, Tests

---

Description

• Updated slot matching logic to ignore specific extension capabilities.

• Added new suffix-based filtering for extension capabilities.

• Enhanced test coverage for slot matching with various capability scenarios.

• Improved code readability and logic consistency in slot matching implementation.

---

Changes walkthrough 📝

	Relevant files
Enhancement	DefaultSlotMatcher.java Enhance slot matching with suffix-based filtering                java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java Added suffix-based filtering for extension capabilities. Updated logic to handle ignored prefixes and suffixes in slot matching. Improved readability and consistency in capability matching logic. +60/-41
Tests	DefaultSlotMatcherTest.java Add tests for enhanced slot matching logic                              java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java Added tests for suffix-based filtering of extension capabilities. Verified ignored capabilities with specific prefixes and suffixes. Enhanced test cases for matching logic consistency. +70/-9    NodeOptionsTest.java Update NodeOptionsTest for slot matching changes                  java/test/org/openqa/selenium/grid/node/config/NodeOptionsTest.java Adjusted test logic to align with updated slot matching behavior. Improved handling of browser display names in test cases. +5/-1

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Logic Change The changes to extension capability matching logic could affect existing slot matching behavior. The new suffix-based filtering needs careful validation to ensure no regressions in capability matching. return stereotype.getCapabilityNames().stream() // examine only extension capabilities .filter(name -> name.contains(":")) // ignore special extension capability prefixes .filter(name -> EXTENSION_CAPABILITIES_PREFIXES.stream().noneMatch(name::contains)) // ignore special extension capability suffixes .filter(name -> EXTENSION_CAPABILITY_SUFFIXES.stream().noneMatch(name::endsWith)) // ignore capabilities not specified in the request .filter(name -> capabilities.getCapability(name) != null) .allMatch( name -> { // evaluate capabilities with String values if (stereotype.getCapability(name) instanceof String && capabilities.getCapability(name) instanceof String) { return ((String) stereotype.getCapability(name)) .equalsIgnoreCase((String) capabilities.getCapability(name)); } // evaluate capabilities with non-String values return Objects.equals( stereotype.getCapability(name), capabilities.getCapability(name)); }); Performance Impact Multiple stream operations and string comparisons in capability matching could impact performance with large capability sets. Consider optimizing the filtering logic. private Boolean initialMatch(Capabilities stereotype, Capabilities capabilities) { return stereotype.getCapabilityNames().stream() // Matching of extension capabilities is implementation independent. Skip them .filter(name -> !name.contains(":")) // Platform matching is special, we do it later .filter(name -> !"platformName".equalsIgnoreCase(name)) .filter(name -> capabilities.getCapability(name) != null) .allMatch( name -> { if (stereotype.getCapability(name) instanceof String && capabilities.getCapability(name) instanceof String) { return ((String) stereotype.getCapability(name)) .equalsIgnoreCase((String) capabilities.getCapability(name)); } return Objects.equals( stereotype.getCapability(name), capabilities.getCapability(name)); }); }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent null pointer exception Add null check for stereotype.getCapability(name) before performing String comparison to prevent potential NullPointerException. java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java [131-135] -if (stereotype.getCapability(name) instanceof String +Object stereotypeCap = stereotype.getCapability(name); +if (stereotypeCap != null + && stereotypeCap instanceof String && capabilities.getCapability(name) instanceof String) { - return ((String) stereotype.getCapability(name)) + return ((String) stereotypeCap) .equalsIgnoreCase((String) capabilities.getCapability(name)); } Apply this suggestion Suggestion importance[1-10]: 7 __ Why: The suggestion addresses a potential NullPointerException that could occur if stereotype.getCapability(name) returns null. This is a valid defensive programming practice that improves code robustness.	Medium
	Fix boolean expression precedence The browserNameMatch and browserVersionMatch conditions should be combined with parentheses to ensure proper operator precedence and avoid potential logical errors in the evaluation. java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java [101-109] boolean browserNameMatch = - capabilities.getBrowserName() == null + (capabilities.getBrowserName() == null || capabilities.getBrowserName().isEmpty() - || Objects.equals(stereotype.getBrowserName(), capabilities.getBrowserName()); + || Objects.equals(stereotype.getBrowserName(), capabilities.getBrowserName())); boolean browserVersionMatch = - capabilities.getBrowserVersion() == null + (capabilities.getBrowserVersion() == null || capabilities.getBrowserVersion().isEmpty() || Objects.equals(capabilities.getBrowserVersion(), "stable") - || browserVersionMatch(stereotype.getBrowserVersion(), capabilities.getBrowserVersion()); + || browserVersionMatch(stereotype.getBrowserVersion(), capabilities.getBrowserVersion())); Apply this suggestion Suggestion importance[1-10]: 3 __ Why: While the suggestion to add parentheses for clarity is valid, it doesn't affect the actual behavior since operator precedence is already correct in this case. The improvement is mainly cosmetic for better readability.	Low
Learned best practice	Add null parameter validation at the start of methods to prevent NullPointerExceptions Add null validation checks for the stereotype and capabilities parameters at the start of the matches() method to prevent potential NullPointerExceptions. Use ArgumentNullException.ThrowIfNull() or similar pattern. java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java [77-80] @Override public boolean matches(Capabilities stereotype, Capabilities capabilities) { + if (stereotype == null) { + throw new IllegalArgumentException("stereotype cannot be null"); + } + if (capabilities == null) { + throw new IllegalArgumentException("capabilities cannot be null"); + } if (capabilities.asMap().isEmpty()) { Apply this suggestion Suggestion importance[1-10]: 6	Low
Update

[sbabcoc]

The first two suggestions made by qodo-merge-pro are actually wrong. The instanceof String checks preclude the possibility of further evaluations of null, and parenthesizing the indicated Boolean expressions won't have any effect.
The third suggestion is OK, but the typical pattern I follow for null checks is to use Objects.requireNonNull().