Skip to content

[grid] Eliminate vendor-specific handling of extension capabilities #14485

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

Conversation

sbabcoc
Copy link
Contributor

@sbabcoc sbabcoc commented Sep 10, 2024

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 retains the "special" status of the "se:" prefix and specifically filters out all extension capabilities with names that end with "options", "Options", "loggingPrefs", and "debuggerAddress". This maintains existing behavior 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 prefix "se:" or one of the "special" name suffixes. All other extension capabilities will be considered for slot matching. 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

    Copy link
    Contributor

    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.

    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch from a82c998 to 14999b7 Compare September 10, 2024 18:15
    Copy link
    Contributor

    qodo-merge-pro bot commented Sep 10, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    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 sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch from 14999b7 to dbe50be Compare September 10, 2024 18:41
    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch 2 times, most recently from ac4802b to 2d9609b Compare September 10, 2024 19:39
    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch from 2d9609b to cca2c28 Compare September 11, 2024 06:48
    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch from d5aeb2e to 84b6795 Compare September 16, 2024 21:35
    @sbabcoc sbabcoc requested a review from pujagani September 16, 2024 21:39
    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch 9 times, most recently from 6a5663f to d993e8c Compare September 22, 2024 19:54
    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch from d993e8c to 378cb93 Compare September 27, 2024 05:42
    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch 2 times, most recently from 790f82e to 1ead41a Compare January 30, 2025 18:50
    @sbabcoc sbabcoc changed the title [grid] Only ignore extension caps with names ending "options" [grid] Eliminate vendor-specific handling of extension capabilities Jan 30, 2025
    filteredStereotype.setCapability(CapabilityType.BROWSER_NAME, (String) null);
    }

    capabilities = capabilities.merge(filteredStereotype);
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Now I remember. This was done because the user can specify something in the stereotype that will be sent to the Node that will receive the request.

    List of prefixed extension capabilities we never should try to match, they should be
    matched in the Node or in the browser driver.
    */
    private static final List<String> EXTENSION_CAPABILITIES_PREFIXES =
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Why is it an inconsistency? Browser vendors just need those capabilities to be passed along. They do not use the Grid and configure stereotypes to do matching.

    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch 7 times, most recently from 395488d to 7bdb2b2 Compare February 7, 2025 19:52
    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch 4 times, most recently from 9c4cbbc to 5c0d6a5 Compare February 13, 2025 21:17
    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch from 5c0d6a5 to 37ef803 Compare February 20, 2025 17:37
    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch 4 times, most recently from 83a1ae5 to 2f42ed8 Compare April 3, 2025 22:12
    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch from 2f42ed8 to 1369878 Compare April 5, 2025 06:39
    @sbabcoc sbabcoc force-pushed the pr/revise-extension-capabilities-handling branch from 1369878 to 43534e9 Compare April 5, 2025 18:02
    @VietND96 VietND96 closed this in 900bbaa Apr 8, 2025
    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 P-bug fix PR addresses a known issue Review effort [1-5]: 3
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    6 participants