From c1f9d3154998044b217a685832002ce26251360d Mon Sep 17 00:00:00 2001 From: Scott Babcock Date: Tue, 10 Sep 2024 00:02:56 -0700 Subject: [PATCH] Only ignore extension caps with object/array values --- .../grid/data/DefaultSlotMatcher.java | 88 ++++++++----------- .../grid/node/relay/RelaySessionFactory.java | 11 --- .../grid/data/DefaultSlotMatcherTest.java | 34 ++++++- .../grid/node/config/NodeOptionsTest.java | 6 +- 4 files changed, 77 insertions(+), 62 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java b/java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java index 3db617bbd5335..26dbc4ffb8432 100644 --- a/java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java +++ b/java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java @@ -18,8 +18,8 @@ package org.openqa.selenium.grid.data; import java.io.Serializable; -import java.util.Arrays; import java.util.List; +import java.util.Map; import java.util.Objects; import org.openqa.selenium.Capabilities; @@ -44,13 +44,6 @@ */ public class DefaultSlotMatcher implements SlotMatcher, Serializable { - /* - 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 EXTENSION_CAPABILITIES_PREFIXES = - Arrays.asList("goog:", "moz:", "ms:", "se:"); - @Override public boolean matches(Capabilities stereotype, Capabilities capabilities) { @@ -76,14 +69,14 @@ public boolean matches(Capabilities stereotype, Capabilities capabilities) { // At the end, a simple browser, browserVersion and platformName match boolean browserNameMatch = - (capabilities.getBrowserName() == null || capabilities.getBrowserName().isEmpty()) + capabilities.getBrowserName() == null + || capabilities.getBrowserName().isEmpty() || Objects.equals(stereotype.getBrowserName(), capabilities.getBrowserName()); boolean browserVersionMatch = - (capabilities.getBrowserVersion() == null - || capabilities.getBrowserVersion().isEmpty() - || Objects.equals(capabilities.getBrowserVersion(), "stable")) - || browserVersionMatch( - stereotype.getBrowserVersion(), capabilities.getBrowserVersion()); + capabilities.getBrowserVersion() == null + || capabilities.getBrowserVersion().isEmpty() + || Objects.equals(capabilities.getBrowserVersion(), "stable") + || browserVersionMatch(stereotype.getBrowserVersion(), capabilities.getBrowserVersion()); boolean platformNameMatch = capabilities.getPlatformName() == null || Objects.equals(stereotype.getPlatformName(), capabilities.getPlatformName()) @@ -102,21 +95,17 @@ private Boolean initialMatch(Capabilities stereotype, Capabilities capabilities) .filter(name -> !name.contains(":")) // Platform matching is special, we do it later .filter(name -> !"platformName".equalsIgnoreCase(name)) - .map( + .filter(name -> capabilities.getCapability(name) != null) + .allMatch( name -> { - if (capabilities.getCapability(name) instanceof String) { - return stereotype - .getCapability(name) - .toString() - .equalsIgnoreCase(capabilities.getCapability(name).toString()); - } else { - return capabilities.getCapability(name) == null - || Objects.equals( - stereotype.getCapability(name), capabilities.getCapability(name)); + if (stereotype.getCapability(name) instanceof String + && capabilities.getCapability(name) instanceof String) { + return ((String) stereotype.getCapability(name)) + .equalsIgnoreCase((String) capabilities.getCapability(name)); } - }) - .reduce(Boolean::logicalAnd) - .orElse(true); + return Objects.equals( + stereotype.getCapability(name), capabilities.getCapability(name)); + }); } private Boolean managedDownloadsEnabled(Capabilities stereotype, Capabilities capabilities) { @@ -140,39 +129,40 @@ private Boolean platformVersionMatch(Capabilities stereotype, Capabilities capab */ return capabilities.getCapabilityNames().stream() .filter(name -> name.contains("platformVersion")) - .map( + .allMatch( platformVersionCapName -> Objects.equals( stereotype.getCapability(platformVersionCapName), - capabilities.getCapability(platformVersionCapName))) - .reduce(Boolean::logicalAnd) - .orElse(true); + capabilities.getCapability(platformVersionCapName))); } private Boolean extensionCapabilitiesMatch(Capabilities stereotype, Capabilities capabilities) { /* - We match extension capabilities when they are not prefixed with any of the - EXTENSION_CAPABILITIES_PREFIXES items. Also, we match them only when the capabilities - of the new session request contains that specific extension capability. + We match extension capabilities in new session requests whose values are of type + 'String', 'Number', or 'Boolean'. We ignore extension capability values of type + 'Map' or 'List'. These are forwarded to the matched node for use in configuration, + but are not considered for node matching. */ return stereotype.getCapabilityNames().stream() + // examine only extension capabilities .filter(name -> name.contains(":")) - .filter(name -> capabilities.asMap().containsKey(name)) - .filter(name -> EXTENSION_CAPABILITIES_PREFIXES.stream().noneMatch(name::contains)) - .map( + // ignore capabilities not specified in the request + .filter(name -> capabilities.getCapability(name) != null) + // ignore capabilities with Map values + .filter(name -> !(capabilities.getCapability(name) instanceof Map)) + // ignore capabilities with List values + .filter(name -> !(capabilities.getCapability(name) instanceof List)) + .allMatch( name -> { - if (capabilities.getCapability(name) instanceof String) { - return stereotype - .getCapability(name) - .toString() - .equalsIgnoreCase(capabilities.getCapability(name).toString()); - } else { - return capabilities.getCapability(name) == null - || Objects.equals( - stereotype.getCapability(name), capabilities.getCapability(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)); } - }) - .reduce(Boolean::logicalAnd) - .orElse(true); + // evaluate capabilities with Number or Boolean values + return Objects.equals( + stereotype.getCapability(name), capabilities.getCapability(name)); + }); } } diff --git a/java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java b/java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java index fc0c9849ece97..e698904e91bd9 100644 --- a/java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java +++ b/java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java @@ -40,7 +40,6 @@ import java.util.logging.Logger; import org.openqa.selenium.Capabilities; import org.openqa.selenium.ImmutableCapabilities; -import org.openqa.selenium.MutableCapabilities; import org.openqa.selenium.SessionNotCreatedException; import org.openqa.selenium.WebDriverException; import org.openqa.selenium.grid.data.CreateSessionRequest; @@ -50,7 +49,6 @@ import org.openqa.selenium.internal.Debug; import org.openqa.selenium.internal.Either; import org.openqa.selenium.internal.Require; -import org.openqa.selenium.remote.CapabilityType; import org.openqa.selenium.remote.Command; import org.openqa.selenium.remote.Dialect; import org.openqa.selenium.remote.DriverCommand; @@ -149,15 +147,6 @@ public Either apply(CreateSessionRequest sess "New session request capabilities do not " + "match the stereotype.")); } - // remove browserName capability if 'appium:app' is present as it breaks appium tests when app - // is provided - // they are mutually exclusive - MutableCapabilities filteredStereotype = new MutableCapabilities(stereotype); - if (capabilities.getCapability("appium:app") != null) { - filteredStereotype.setCapability(CapabilityType.BROWSER_NAME, (String) null); - } - - capabilities = capabilities.merge(filteredStereotype); LOG.info("Starting session for " + capabilities); try (Span span = tracer.getCurrentContext().createSpan("relay_session_factory.apply")) { diff --git a/java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java b/java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java index 80410be005e0f..1e2ded13afd65 100644 --- a/java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java +++ b/java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java @@ -19,6 +19,8 @@ import static org.assertj.core.api.Assertions.assertThat; +import java.util.List; +import java.util.Map; import org.junit.jupiter.api.Test; import org.openqa.selenium.Capabilities; import org.openqa.selenium.ImmutableCapabilities; @@ -515,7 +517,7 @@ void multipleExtensionPrefixedCapabilitiesDoNotMatchWhenOneIsDifferent() { } @Test - void vendorExtensionPrefixedCapabilitiesAreIgnoredForMatching() { + void vendorExtensionPrefixedCapabilitiesWithSimpleValuesAreConsideredForMatching() { Capabilities stereotype = new ImmutableCapabilities( CapabilityType.BROWSER_NAME, @@ -541,6 +543,36 @@ void vendorExtensionPrefixedCapabilitiesAreIgnoredForMatching() { "gouda", "ms:fruit", "orange"); + assertThat(slotMatcher.matches(stereotype, capabilities)).isFalse(); + } + + @Test + void vendorExtensionPrefixedCapabilitiesWithComplexValuesAreIgnoredForMatching() { + Capabilities stereotype = + new ImmutableCapabilities( + CapabilityType.BROWSER_NAME, + "chrome", + CapabilityType.BROWSER_VERSION, + "84", + CapabilityType.PLATFORM_NAME, + Platform.WINDOWS, + "food:dairy", + Map.of("cheese", "amsterdam"), + "food:fruit", + List.of("mango")); + + Capabilities capabilities = + new ImmutableCapabilities( + CapabilityType.BROWSER_NAME, + "chrome", + CapabilityType.BROWSER_VERSION, + "84", + CapabilityType.PLATFORM_NAME, + Platform.WINDOWS, + "food:dairy", + Map.of("cheese", "gouda"), + "food:fruit", + List.of("orange")); assertThat(slotMatcher.matches(stereotype, capabilities)).isTrue(); } diff --git a/java/test/org/openqa/selenium/grid/node/config/NodeOptionsTest.java b/java/test/org/openqa/selenium/grid/node/config/NodeOptionsTest.java index 69f1ba30a9ee9..3def22ea8dde3 100644 --- a/java/test/org/openqa/selenium/grid/node/config/NodeOptionsTest.java +++ b/java/test/org/openqa/selenium/grid/node/config/NodeOptionsTest.java @@ -63,6 +63,7 @@ import org.openqa.selenium.internal.Either; import org.openqa.selenium.json.Json; import org.openqa.selenium.net.NetworkUtils; +import org.openqa.selenium.remote.Browser; import org.openqa.selenium.safari.SafariDriverInfo; @SuppressWarnings("DuplicatedCode") @@ -148,7 +149,10 @@ boolean isDownloadEnabled(WebDriverInfo driver, String customMsg) { reported.add(caps); return Collections.singleton(HelperFactory.create(config, caps)); }); - String expected = driver.getDisplayName(); + String expected = + "Edge".equals(driver.getDisplayName()) + ? Browser.EDGE.browserName() + : driver.getDisplayName(); Capabilities found = reported.stream()