[grid] Fix DefaultSlotMatcher matching requests differentiated only by undeclared automationName - #17898
Open
diemol wants to merge 8 commits into
Open
[grid] Fix DefaultSlotMatcher matching requests differentiated only by undeclared automationName#17898diemol wants to merge 8 commits into
diemol wants to merge 8 commits into
Conversation
…ionName extensionCapabilitiesMatch() only inspected extension capability names the stereotype declared, so a stereotype declaring none at all (e.g. a plain browser node) matched any requested automationName by default. This let a native-automation request differentiated solely by automationName match an unrelated browser-only node. Add automationNameMatch(), gated on the stereotype showing some existing Appium-awareness (a relevant extension capability, or a non-W3C-compliant platformVersion), so relay-node matching keeps working while a stereotype with no such awareness no longer matches on an undeclared automationName. Fixes #17845 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
selenium-ci
added
B-grid
Contributor
PR Summary by QodoGrid: Prevent slot matching on undeclared appium:automationName
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Contributor
Code Review by Qodo
1. Automation name becomes case-sensitive
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Fixes two issues found in CI/review of #17898: - Format / Check Format was failing on ImmutableCapabilities argument wrapping in the new tests; re-ran ./go format to match. - Qodo correctly flagged that treating any non-vendor extension capability as "Appium-aware" was too broad -- a node advertising an unrelated custom extension capability (e.g. prefixed:cheese) would incorrectly bypass the automationName gate. Narrow the signal to appium:-prefixed capabilities and platformVersion specifically, and tighten the automationName check to an exact/suffix match instead of a bare substring. Added a regression test for the narrowed case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Code review by qodo was updated up to the latest commit e62d621 |
google-java-format wants the noneMatch lambda collapsed onto one line; the previous manual edit split it across two. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Code review by qodo was updated up to the latest commit 97ccbef |
Per Qodo review feedback on #17898: document the public matches() method's parameters and return value, since its behavior changed with the new automationName gating. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Code review by qodo was updated up to the latest commit 0d2c0c8 |
Fixes a Qodo-flagged gap in #17898: automationNameMatch() only inspected top-level capability names, so a request nesting automationName inside an options map (e.g. appium:options) bypassed the new gate entirely, since Capabilities.getCapabilityNames() never flattens nested maps and this matcher already treats *options* capabilities as opaque elsewhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Code review by qodo was updated up to the latest commit ae0e1bc |
…in AppiumRelaySlotMatcher DefaultSlotMatcher.automationNameMatch() now requires the stereotype to declare the same automationName the request asks for, unconditionally -- no more exception for stereotypes that merely look Appium-aware. The matching browserName/browserVersion bypass for app-relay capabilities is removed from the default matcher too. Relay-node operators who relied on that leniency can opt back into it via the new AppiumRelaySlotMatcher (config: distributor.slot-matcher), which composes DefaultSlotMatcher's package-private checks and layers the Appium-aware automationName gate and app-relay browserName/browserVersion bypass on top. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment on lines
+45
to
+46
| @Override | ||
| public boolean matches(Capabilities stereotype, Capabilities capabilities) { |
Contributor
There was a problem hiding this comment.
1. matches lacks javadoc 📘 Rule violation ✧ Quality
The new public AppiumRelaySlotMatcher.matches method has no immediately preceding Javadoc documenting its purpose, parameters, and return value. This leaves the newly exposed matching behavior incompletely documented.
Agent Prompt
## Issue description
Add complete Javadoc immediately above the public `matches` method.
## Issue Context
The documentation should include a purpose sentence, `@param` tags for `stereotype` and `capabilities`, and a non-empty `@return` description.
## Fix Focus Areas
- java/src/org/openqa/selenium/grid/data/AppiumRelaySlotMatcher.java[45-46]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+80
to
+82
| || strict.browserVersionMatch( | ||
| stereotype.getBrowserVersion(), capabilities.getBrowserVersion()) | ||
| || DefaultSlotMatcher.matchConditionToRemoveCapability(capabilities); |
Contributor
There was a problem hiding this comment.
2. browserversion bypass remains untested 📘 Rule violation ▣ Testability
The new matcher bypasses browserVersion mismatches for app-relay requests, but its new test class contains no scenario with browser versions. A regression in this advertised behavior could therefore pass the test suite unnoticed.
Agent Prompt
## Issue description
Add a regression test for the new app-relay `browserVersion` bypass.
## Issue Context
Construct an Appium-aware stereotype and request with different non-empty browser versions plus an app-relay capability, then assert that `AppiumRelaySlotMatcher.matches` returns `true`. The assertion should fail if the bypass at lines 80-82 is removed.
## Fix Focus Areas
- java/test/org/openqa/selenium/grid/data/AppiumRelaySlotMatcherTest.java[119-142]
- java/src/org/openqa/selenium/grid/data/AppiumRelaySlotMatcher.java[76-82]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (requestedAutomationName == null) { | ||
| return true; | ||
| } | ||
| return Objects.equals(requestedAutomationName, automationNameValue(stereotype)); |
Contributor
There was a problem hiding this comment.
3. Automation name becomes case-sensitive 🐞 Bug ≡ Correctness
DefaultSlotMatcher now compares extracted automationName values with Objects.equals, so values differing only in case are rejected even though extensionCapabilitiesMatch explicitly accepts all string extension values case-insensitively. A stereotype using XCUITest therefore no longer matches a request using xcuitest, despite matching before this change.
Agent Prompt
## Issue description
`automationNameMatch` uses `Objects.equals`, making string automation names case-sensitive even though existing extension-capability matching uses case-insensitive string comparison. Preserve the established behavior while still handling non-string values safely.
## Issue Context
This new check runs after extension matching and can reverse a successful case-insensitive extension match.
## Fix Focus Areas
- java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[203-214]
- java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java[728-753]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+230
to
+231
| if (name.toLowerCase().contains("options") && value instanceof Map) { | ||
| return ((Map<?, ?>) value).get("automationName"); |
Contributor
There was a problem hiding this comment.
4. Unrelated options maps misclassified 🐞 Bug ≡ Correctness
automationNameValueFor treats every map-valued capability whose name contains options as an automation-name container, including unrelated vendor capabilities such as vendor:customOptions. If such metadata contains an automationName field, an otherwise compatible plain browser stereotype is incorrectly rejected.
Agent Prompt
## Issue description
Nested automationName extraction currently examines every capability whose name contains `options`. Restrict this behavior to capability names that actually represent Appium options and add coverage proving unrelated options maps do not affect matching.
## Issue Context
Capability keys are general extension names; containing the substring `options` does not establish Appium semantics.
## Fix Focus Areas
- java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[224-233]
- java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java[799-818]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+60
to
+61
| if (!strict.extensionCapabilitiesMatch(stereotype, capabilities)) { | ||
| return false; |
Contributor
There was a problem hiding this comment.
5. Relay wildcard check is preempted 🐞 Bug ≡ Correctness
AppiumRelaySlotMatcher runs strict extension matching before its wildcard automationName check, so a stereotype advertising appium:automationName=UiAutomator2 still rejects a request for another framework. This contradicts the new matcher's documented promise that Appium-aware stereotypes act as automationName wildcards for varied frameworks.
Agent Prompt
## Issue description
The relay matcher delegates `appium:automationName` to strict extension matching before applying its wildcard policy. Ensure automationName is excluded from that strict comparison for this matcher, or narrow the documented contract if explicit values are intentionally constraints.
## Issue Context
`DefaultSlotMatcher.extensionCapabilitiesMatch` compares matching Appium extension values, so the later permissive `automationNameMatch` cannot override a mismatch.
## Fix Focus Areas
- java/src/org/openqa/selenium/grid/data/AppiumRelaySlotMatcher.java[24-32]
- java/src/org/openqa/selenium/grid/data/AppiumRelaySlotMatcher.java[60-66]
- java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java[173-200]
- java/test/org/openqa/selenium/grid/data/AppiumRelaySlotMatcherTest.java[74-117]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit aa3fdcd |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
Fixes #17845
💥 What does this PR do?
DefaultSlotMatcher.extensionCapabilitiesMatch()only inspects extension capability names the stereotype declares. If a stereotype declares no extension (:-namespaced) capabilities at all — e.g. a plain browser node — the check short-circuits totrueregardless of what the request asks for. This lets a request differentiated solely by an identity extension capability (appium:automationName) match a completely unrelated, non-Appium node, as long asbrowserNameis absent from the request andplatformNameresolves to the samePlatformfamily.This adds a new
automationNameMatch()check, called alongside the existing extension-capability checks inmatches(). It only rejects a match when the stereotype shows no existing Appium-awareness at all (no relevant extension capability, and no non-W3C-compliantplatformVersion, which the code already treats as an Appium signal) while the request specifies anautomationName. Stereotypes that already show some Appium-awareness (e.g. relay nodes advertisingappium:platformVersionbut omittingautomationName) are left untouched, preserving existing relay-node matching behavior.🔧 Implementation Notes
The issue proposed an unconditional bidirectional
automationNamecheck. That approach was tested against the existing test suite and found to break two passing relay-node tests, where Appium relay nodes intentionally omitautomationNamewhile still being valid targets for varied automation sessions. The fix here gates the check on stereotype Appium-awareness instead, using the sameplatformVersionsignal the codebase already relies on elsewhere in this file, so relay-node flexibility is preserved while the reported misroute is closed.🤖 AI assistance
automationNameMatch()and the two regression tests, drafted from the issue's analysis and root-caused/iterated against the existing test suite to avoid regressing relay-node matching.💡 Additional Considerations
None.
🔄 Types of changes