[grid] Only ignore extension caps with object/array values

[sbabcoc]

User description

Description

The existing handling of extension capabilities assigns special significance to four recognized prefixes: goog:, moz:, ms:, and se:. Capabilities with these prefixes are entirely ignored by the current default slot matcher implementation, but custom prefixes are considered, as well as those for Safari and Appium. This inconsistency means that properties in extension capabilities can cause affected newSession requests to fail even though extension capabilities are supposed to be vendor-specific and should probably not be evaluated by the default slot matcher.

This PR eliminates the "special" status of the four recognized prefixes, opting instead to ignore all extension capabilities with names that end with "options" (case-insensitive). This maintains the existing behavior regarding the Options objects of Chrome, Firefox, and Edge while allowing node configurations and newSession requests to include extension capabilities that won't affect slot matching.

Motivation and Context

Revolves #14461

Note that this is technically a breaking change, because it revises the default slot matcher to consider "special" extension capabilities without name suffix "options" that would previously have been ignored, and the matcher will now ignore "non-special" extension capabilities with name suffix "options" that would previously have been considered.
However, I'm unaware of any current use cases that will be adversely affected by this change.

The strategy employed by the PR is that extension capabilities which should be ignored for matching can be expressed as capabilities with name suffix "options" and those which should be considered can be specified as capabilities without name suffix "options". This is a generalization/extrapolation of existing patterns from current use cases.

Recommended slot matcher enhancements

To reduce the need for custom slot matchers, we could extend the WebDriverInfo interface to add a new method that vendors could implement to evaluate their own criteria:

Boolean matches(Capabilities stereotype, Capabilities capabilities);

The default implementation would return null to indicate that no evaluation has been performed. If the vendor chooses to implement the matches method, their initial step must be to invoke their own isSupporting method, returning null if the corresponding driver doesn't support the specified capabilities.

The implementation in DefaultSlotMatcher would be updated to iterate over the available WebDriverInfo providers, retaining only those whose isSupporting methods return true. The matches methods of this filtered list of providers will then be applied to the available nodes to determine which of these satisfies the criteria specified by the client request. The nodes that satisfy these evaluations (if any) would then be evaluated against the remaining common criteria.

Another potential enhancement would be to enable vendor-specific matches methods to return a weighted integer result instead of a simple Boolean. This would enable best-match behavior. Perhaps this is getting too complicated, though.

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

Bug fix, Tests

---

Description

• Removed the special status of certain extension capability prefixes in DefaultSlotMatcher, opting to ignore all extension capabilities with names ending in "options" (case-insensitive).
• Simplified the logic for matching capabilities by removing unnecessary checks and conditions.
• Updated RelaySessionFactory to streamline session creation by removing redundant capability filtering.
• Revised test cases in DefaultSlotMatcherTest to align with the new handling of extension capabilities.

---

Changes walkthrough 📝

	Relevant files
Bug fix	DefaultSlotMatcher.java Revise extension capabilities handling in DefaultSlotMatcher java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java Removed special handling for specific extension capability prefixes. Updated logic to ignore all extension capabilities with names ending in "options" (case-insensitive). Simplified capability matching logic. +31/-51  RelaySessionFactory.java Simplify session creation in RelaySessionFactory                  java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java Removed logic to filter out browserName when appium:app is present. Simplified session creation process. +0/-11
Tests	DefaultSlotMatcherTest.java Update DefaultSlotMatcher tests for revised capability handling java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java Updated test cases to reflect changes in extension capability handling. +10/-8

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-merge-pro]

PR Reviewer Guide 🔍

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Potential Performance Issue The new implementation of initialMatch method uses reduce(Boolean::logicalAnd) which may not short-circuit on the first false condition. Consider using allMatch for better performance. Possible Bug The extensionCapabilitiesMatch method now ignores complex objects and arrays, which might lead to unintended matching behavior for certain extension capabilities.

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Improve precision in matching complex capability values Consider using a more specific check for complex objects instead of returning true for all non-String, non-Number, and non-Boolean values. This could prevent unintended matches for complex objects. java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java [144-148] if (capabilities.getCapability(name) instanceof Number || - capabilities.getCapability(name) instanceof Boolean) { + capabilities.getCapability(name) instanceof Boolean || + capabilities.getCapability(name) instanceof Map) { return Objects.equals(stereotype.getCapability(name), capabilities.getCapability(name)); } -return true; +return false; Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion enhances the precision of the matching logic by returning false for unsupported complex objects, preventing unintended matches and improving the accuracy of the matching process.	8
Enhancement	Improve handling of complex capability values in extension capabilities matching Consider using Map.of() or ImmutableMap.of() instead of creating a new HashMap for the complex capability values. This can make the code more concise and potentially more efficient. java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java [144-148] if (capabilities.getCapability(name) instanceof Number || - capabilities.getCapability(name) instanceof Boolean) { + capabilities.getCapability(name) instanceof Boolean || + capabilities.getCapability(name) instanceof Map) { return Objects.equals(stereotype.getCapability(name), capabilities.getCapability(name)); } return true; Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies the need to handle complex capability values like Maps, which aligns with the changes in the test cases. This improves the robustness of the capability matching logic.	7
	Add logging for skipped extension capabilities to improve debugging Consider adding a log statement when skipping extension capabilities to improve debugging and traceability of the matching process. java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java [88] -.filter(name -> !name.contains(":")) +.filter(name -> { + boolean isExtensionCapability = name.contains(":"); + if (isExtensionCapability) { + LOG.fine("Skipping extension capability: " + name); + } + return !isExtensionCapability; +}) Apply this suggestion Suggestion importance[1-10]: 6 Why: Adding logging for skipped extension capabilities can aid in debugging and understanding the matching process, though it is not critical for functionality.	6
Best practice	Use a constant for the extension capability separator to improve code maintainability Consider using a constant for the ":" character used to identify extension capabilities. This would improve maintainability and reduce the risk of typos. java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java [88] -.filter(name -> !name.contains(":")) +private static final String EXTENSION_CAPABILITY_SEPARATOR = ":"; +// In the method: +.filter(name -> !name.contains(EXTENSION_CAPABILITY_SEPARATOR)) + Apply this suggestion Suggestion importance[1-10]: 5 Why: Using a constant for the separator improves maintainability and reduces the risk of errors, but it is a minor improvement in terms of code quality.	5

[sbabcoc]

@VietND96 I'm trying to understand why the comparison in the test you added should return true. Not only are the values for moz:debuggerAddress not the same; the values are of completely different types. I understand that this comparison would return true in the current implementation, but this is actually one of the bugs my PR is addressing.
I strongly suspect that there are no real-world scenarios in which this sort of mismatch should be ignored. This seems like a test that's been tailored to verify an arbitrary behavior that's an unintended artifact of the current implementation. If the extension prefixes were replaced with appium, the current implementation returns false, and this is a behavior that existing clients actually depend on.

[sbabcoc]

@diemol Changing the behavior to only ignore extension capabilities whose names contain "options" would produce the same behavior that is causing the test @VietND96 added to fail. The values of moz:debuggerAddress would be compared and found to be unequal, and the comparison would return false. As I stated in my previous reply, I believe this is the correct behavior - unequal "simple" values should cause the comparison to return false.

The approach I'm presenting in this PR provides the greatest level of control and consistency for clients.

• If the client wants the capability to be considered for matching, declare it with a "simple" value.
• If the client wants the capability to be ignored for matching, declare it with a "complex" value.
• No extension capability prefixes get "special" treatment, which avoids confusing, inconsistent behavior for custom extensions.

[diemol]

The values of moz:debuggerAddress would be compared and found to be unequal, and the comparison would return false.

No, because we will still ignore moz:. The only change we need is to ignore :options, which makes sense to be ignored when matching.

[diemol]

All other changes should be reverted, please.

[sbabcoc]

@diemol I understand that we currently ignore all "special" extension capabilities, but there appears to be no real-world scenarios of "special" extension capabilities with "simple" values in which this behavior is actually needed. For Appium extension capabilities, current clients rely on capabilities with "simple" values being considered for node matching.
I believe that the existing treatment of "special" extension capabilities is undesirably permissive, and the inconsistent behavior we see for "non-special" extension capabilities is also undesirable.

[diemol]

I believe that the existing treatment of "special" extension capabilities is undesirably permissive, and the inconsistent behavior we see for "non-special" extension capabilities is also undesirable.

Why?

[sbabcoc]

@diemol Why is it desirable to support the scenario enforced by the test that @VietND96 added? We match nodes even when capabilities like moz:debuggerAddress are entirely different between requested and actual values. We don't exhibit this behavior for Appium nodes, where capabilities like appium:automationName are considered when matching requested and actual values.
The revised behavior you recommend (only ignoring extension capabilities whose names contain "options") would produce behavior that doesn't satisfy the condition expected by the test that @VietND96 added.

[diemol]

I cannot see what @VietND96 added because you are force-pushing the whole time, and I don't know who added what. I don't know which scenario you mean.

[VietND96]

I added it back to trunk 10119a9

[diemol]

@sbabcoc is correct, this moz:debuggerAddress should not be considered for matching. That is an internal Firefox thing.

[sbabcoc]

@diemol I force-push after rebasing to keep the commit history clean. My attempt to rebase after @VietND96 added his unit test went horribly wrong and I had to check out a prior commit to recover, which removed his test. I let him know, and he created a separate PR to add his test to trunk.

[sbabcoc]

@diemol It seems like you're advocating for ignoring all extension capabilities for purposes of slot matching. This approach would mean that custom matchers would be required in all scenarios where matching based on extension capabilities is desired (e.g. - appium:automationName). The approach implemented by this PR is a compromise. It enables clients add slot matching criteria by adding extension capabilities with "simple" values, while extension capabilities specified as "complex" will be ignored.

The enhancements I recommended in the PR description provide the outline of a mechanism for vendors to define slot matcher extensions that evaluate their specific matching criteria. This would enable vendors to define their own custom criteria without the need to implement custom slot matchers.

[diemol]

@diemol It seems like you're advocating for ignoring all extension capabilities for purposes of slot matching.

Not really. Just the ones named options, as in nord:options.

[sbabcoc]

@diemol I can revise my PR to implement this strategy. It is significantly less complex to implement and much easier to describe. I'll update the unit tests accordingly.

Is the concept of a slot matcher extension mechanism worth exploring further? This would be an additional method for the WebDriverInfo interface, which is already being scanned for service providers.

[diemol]

You can already implement your own SlotMatcher, see --slot-matcher

[sbabcoc]

@diemol Yes, but you can only specify one custom matcher. If you need sessions from multiple vendors, this means running with multiple hubs. With a slot matcher extension mechanism, vendor-specific matching gets activated automatically and standing up a grid that supplies sessions from multiple vendors is much easier.

[diemol]

The matcher is used both at the Distributor and the Node. You would need to decouple that logic to allow more matchers.

[sbabcoc]

@diemol The extension would be implemented in DefaultSlotMatcher, which is already integrated with Distributor and Node.

[sbabcoc]

@diemol I've revised the matcher to only ignore extension capabilities with suffix "options" (case-insensitive) and updated the affected unit test accordingly. I also updated the PR description to indicate the revised implementation.