[grid] Improve SlotMatcher for Node relay to Appium server with hybrid browser and native app

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java

@@ -50,6 +50,10 @@ public class DefaultSlotMatcher implements SlotMatcher, Serializable {
   */
   private static final List<String> EXTENSION_CAPABILITIES_PREFIXES =
       Arrays.asList("goog:", "moz:", "ms:", "se:");
+  public static final List<String> SPECIFIC_RELAY_CAPABILITIES_APP =
+      Arrays.asList("appium:app", "appium:appPackage", "appium:bundleId");
+  public static final List<String> MANDATORY_CAPABILITIES =
+      Arrays.asList("platformName", "browserName", "browserVersion");
 
   @Override
   public boolean matches(Capabilities stereotype, Capabilities capabilities) {
@@ -75,33 +79,22 @@ 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())
-            || Objects.equals(stereotype.getBrowserName(), capabilities.getBrowserName());
-    boolean browserVersionMatch =
-        (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())
-            || (stereotype.getPlatformName() != null
-                && stereotype.getPlatformName().is(capabilities.getPlatformName()));
-    return browserNameMatch && browserVersionMatch && platformNameMatch;
-  }
+    boolean browserNameMatch = browserNameMatch(stereotype, capabilities);
+    boolean browserVersionMatch = browserVersionMatch(stereotype, capabilities);
+    boolean platformNameMatch = platformNameMatch(stereotype, capabilities);
 
-  private boolean browserVersionMatch(String stereotype, String capabilities) {
-    return new SemanticVersionComparator().compare(stereotype, capabilities) == 0;
+    return browserNameMatch && browserVersionMatch && platformNameMatch;
   }
 
-  private Boolean initialMatch(Capabilities stereotype, Capabilities capabilities) {
+  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))
+        // Mandatory capabilities match is done at the end
+        .filter(
+            name ->
+                MANDATORY_CAPABILITIES.stream()
+                    .noneMatch(mandatory -> mandatory.equalsIgnoreCase(name)))
         .map(
             name -> {
               if (capabilities.getCapability(name) instanceof String) {
@@ -119,7 +112,7 @@ private Boolean initialMatch(Capabilities stereotype, Capabilities capabilities)
         .orElse(true);
   }
 
-  private Boolean managedDownloadsEnabled(Capabilities stereotype, Capabilities capabilities) {
+  private boolean managedDownloadsEnabled(Capabilities stereotype, Capabilities capabilities) {
     // First lets check if user wanted a Node with managed downloads enabled
     Object raw = capabilities.getCapability("se:downloadsEnabled");
     if (raw == null || !Boolean.parseBoolean(raw.toString())) {
@@ -132,7 +125,7 @@ private Boolean managedDownloadsEnabled(Capabilities stereotype, Capabilities ca
     return raw != null && Boolean.parseBoolean(raw.toString());
   }
 
-  private Boolean platformVersionMatch(Capabilities stereotype, Capabilities capabilities) {
+  private boolean platformVersionMatch(Capabilities stereotype, Capabilities capabilities) {
     /*
      This platform version match is not W3C compliant but users can add Appium servers as
      Nodes, so we avoid delaying the match until the Slot, which makes the whole matching
@@ -149,7 +142,7 @@ private Boolean platformVersionMatch(Capabilities stereotype, Capabilities capab
         .orElse(true);
   }
 
-  private Boolean extensionCapabilitiesMatch(Capabilities stereotype, Capabilities capabilities) {
+  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
@@ -175,4 +168,44 @@ private Boolean extensionCapabilitiesMatch(Capabilities stereotype, Capabilities
         .reduce(Boolean::logicalAnd)
         .orElse(true);
   }
+
+  private boolean browserNameMatch(Capabilities stereotype, Capabilities capabilities) {
+    return (capabilities.getBrowserName() == null || capabilities.getBrowserName().isEmpty())
+        || Objects.equals(stereotype.getBrowserName(), capabilities.getBrowserName())
+        || specificRelayCapabilitiesAppMatch(capabilities);
+  }
+
+  private boolean browserVersionMatch(String stereotype, String capabilities) {
+    return new SemanticVersionComparator().compare(stereotype, capabilities) == 0;
+  }
+
+  private boolean browserVersionMatch(Capabilities stereotype, Capabilities capabilities) {
+    return (capabilities.getBrowserVersion() == null
+            || capabilities.getBrowserVersion().isEmpty()
+            || Objects.equals(capabilities.getBrowserVersion(), "stable"))
+        || browserVersionMatch(stereotype.getBrowserVersion(), capabilities.getBrowserVersion())
+        || specificRelayCapabilitiesAppMatch(capabilities);
+  }
+
+  private boolean platformNameMatch(Capabilities stereotype, Capabilities capabilities) {
+    return capabilities.getPlatformName() == null
+        || Objects.equals(stereotype.getPlatformName(), capabilities.getPlatformName())
+        || (stereotype.getPlatformName() != null
+            && stereotype.getPlatformName().is(capabilities.getPlatformName()));
+  }
+
+  public static boolean specificRelayCapabilitiesAppMatch(Capabilities capabilities) {
+    /*
+    This match is specific for the Relay capabilities that are related to the Appium server for native application.
+    - If browserName is defined then we always assume it’s a hybrid browser session, so no app-related caps should be provided
+    - If app is provided then the assumption is that app should be fetched from somewhere first and then installed on the destination device
+    - If only appPackage is provided for uiautomator2 driver or bundleId for xcuitest then the assumption is we want to automate an app that is already installed on the device under test
+    - If both (app and appPackage) or (app and bundleId). This will then save some small-time for the driver as by default it tries to autodetect these values anyway by analyzing the fetched package’s manifest
+     */
+    return SPECIFIC_RELAY_CAPABILITIES_APP.stream()
+        .anyMatch(
+            name ->
+                capabilities.getCapability(name) != null
+                    && !capabilities.getCapability(name).toString().isEmpty());
+  }
 }

java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java

@@ -17,6 +17,7 @@
 
 package org.openqa.selenium.grid.node.relay;
 
+import static org.openqa.selenium.grid.data.DefaultSlotMatcher.specificRelayCapabilitiesAppMatch;
 import static org.openqa.selenium.remote.RemoteTags.CAPABILITIES;
 import static org.openqa.selenium.remote.RemoteTags.CAPABILITIES_EVENT;
 import static org.openqa.selenium.remote.tracing.AttributeKey.DOWNSTREAM_DIALECT;
@@ -72,7 +73,6 @@
 public class RelaySessionFactory implements SessionFactory {
 
   private static final Logger LOG = Logger.getLogger(RelaySessionFactory.class.getName());
-
   private final Tracer tracer;
   private final HttpClient.Factory clientFactory;
   private final Duration sessionTimeout;
@@ -140,24 +140,30 @@ public boolean test(Capabilities capabilities) {
             .orElse(false);
   }
 
+  public Capabilities filterRelayCapabilities(Capabilities capabilities) {
+    /*
+    Remove browserName capability if 'appium:app' (or similar based on driver) is present as it breaks appium tests when app is provided
+    they are mutually exclusive
+    */
+    if (specificRelayCapabilitiesAppMatch(capabilities)) {
+      MutableCapabilities filteredStereotype = new MutableCapabilities(capabilities);
+      filteredStereotype.setCapability(CapabilityType.BROWSER_NAME, (String) null);
+      return filteredStereotype;
+    }
+    return capabilities;
+  }
+
   @Override
   public Either<WebDriverException, ActiveSession> apply(CreateSessionRequest sessionRequest) {
     Capabilities capabilities = sessionRequest.getDesiredCapabilities();
+    capabilities = filterRelayCapabilities(capabilities);
+
     if (!test(capabilities)) {
       return Either.left(
           new SessionNotCreatedException(
               "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")) {

java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java

@@ -52,6 +52,129 @@ void testBrowserVersionMatch() {
     assertThat(comparator.compare("130.0", "131")).isEqualTo(-1);
   }
 
+  @Test
+  public void testSpecificRelayCapabilitiesAppMatch() {
+    Capabilities capabilitiesWithApp =
+        new ImmutableCapabilities(
+            "appium:app",
+            "link.to.apk",
+            "appium:appPackage",
+            "com.example.app",
+            "appium:platformVersion",
+            "15",
+            "platformName",
+            "Android",
+            "appium:automationName",
+            "uiautomator2");
+    assertThat(DefaultSlotMatcher.specificRelayCapabilitiesAppMatch(capabilitiesWithApp)).isTrue();
+    capabilitiesWithApp =
+        new ImmutableCapabilities(
+            "browserName",
+            "chrome",
+            "appium:platformVersion",
+            "15",
+            "platformName",
+            "Android",
+            "appium:automationName",
+            "uiautomator2");
+    assertThat(DefaultSlotMatcher.specificRelayCapabilitiesAppMatch(capabilitiesWithApp)).isFalse();
+  }
+
+  @Test
+  public void testRelayNodeMatchByRemovingBrowserNameWhenAppSet() {
+    /*
+    Relay node stereotype does not have browserName (where user wants to restrict to run a native app only)
+    Request capabilities have both browserName (it might initialize by ChromeOptions) and app set
+    The browserName will be filter out when validating match
+     */
+    Capabilities stereotype =
+        new ImmutableCapabilities(
+            CapabilityType.PLATFORM_NAME, Platform.ANDROID, "appium:platformVersion", "14");
+    Capabilities capabilities =
+        new ImmutableCapabilities(
+            CapabilityType.BROWSER_NAME,
+            "chrome",
+            CapabilityType.PLATFORM_NAME,
+            Platform.ANDROID,
+            "appium:platformVersion",
+            "14",
+            "appium:app",
+            "link.to.apk",
+            "appium:automationName",
+            "uiautomator2");
+    assertThat(slotMatcher.matches(stereotype, capabilities)).isTrue();
+  }
+
+  @Test
+  public void testRelayNodeNotMatchHybridBrowserVersionWhenStereotypeWithoutBrowserName() {
+    /*
+    Relay node 1 has stereotype does not have browserName (where user wants to restrict to run a native app only)
+    Request capabilities want to run a hybrid app (browserName is set) and app isn't set
+    Request capabilities should not match the stereotype
+    Relay node 2 has stereotype with browserName set should match the request capabilities
+     */
+    Capabilities stereotype1 =
+        new ImmutableCapabilities(
+            CapabilityType.PLATFORM_NAME, Platform.ANDROID, "appium:platformVersion", "14");
+    Capabilities capabilities =
+        new ImmutableCapabilities(
+            CapabilityType.BROWSER_NAME,
+            "chrome",
+            CapabilityType.PLATFORM_NAME,
+            Platform.ANDROID,
+            "appium:platformVersion",
+            "14",
+            "appium:automationName",
+            "uiautomator2");
+    assertThat(slotMatcher.matches(stereotype1, capabilities)).isFalse();
+    Capabilities stereotype2 =
+        new ImmutableCapabilities(
+            CapabilityType.BROWSER_NAME,
+            "chrome",
+            CapabilityType.PLATFORM_NAME,
+            Platform.ANDROID,
+            "appium:platformVersion",
+            "14");
+    assertThat(slotMatcher.matches(stereotype2, capabilities)).isTrue();
+  }
+
+  @Test
+  public void testRelayNodeNotMatchWhenNonW3CCompliantPlatformVersionSet() {
+    /*
+    There are Appium server plugins which allow to “fix” non W3C compliant capabilities by automatically adding `appium:` prefix to them
+    Relay Node 1: When `platformVersion` is set in both stereotype and capabilities, the non W3C compliant `platformVersion` should be not matched
+    Relay Node 2: When `platformVersion` is set in stereotype and capabilities, the non W3C compliant `platformVersion` should be matched
+    */
+    Capabilities stereotype1 =
+        new ImmutableCapabilities(
+            CapabilityType.BROWSER_NAME,
+            "chrome",
+            CapabilityType.PLATFORM_NAME,
+            Platform.ANDROID,
+            "platformVersion",
+            "14");
+    Capabilities capabilities =
+        new ImmutableCapabilities(
+            CapabilityType.BROWSER_NAME,
+            "chrome",
+            CapabilityType.PLATFORM_NAME,
+            Platform.ANDROID,
+            "platformVersion",
+            "15",
+            "appium:automationName",
+            "uiautomator2");
+    assertThat(slotMatcher.matches(stereotype1, capabilities)).isFalse();
+    Capabilities stereotype2 =
+        new ImmutableCapabilities(
+            CapabilityType.BROWSER_NAME,
+            "chrome",
+            CapabilityType.PLATFORM_NAME,
+            Platform.ANDROID,
+            "platformVersion",
+            "15");
+    assertThat(slotMatcher.matches(stereotype2, capabilities)).isTrue();
+  }
+
   @Test
   void fullMatch() {
     Capabilities stereotype =

java/test/org/openqa/selenium/grid/node/relay/BUILD.bazel

@@ -15,5 +15,6 @@ java_test_suite(
         "//java/test/org/openqa/selenium/remote/tracing:tracing-support",
         artifact("org.junit.jupiter:junit-jupiter-api"),
         artifact("org.assertj:assertj-core"),
+        artifact("org.mockito:mockito-core"),
     ] + JUNIT5_DEPS,
 )

java/test/org/openqa/selenium/grid/node/relay/RelaySessionFactoryTest.java

@@ -0,0 +1,72 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.openqa.selenium.grid.node.relay;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.when;
+
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+import org.openqa.selenium.Capabilities;
+import org.openqa.selenium.ImmutableCapabilities;
+
+public class RelaySessionFactoryTest {
+
+  // Add the following test method to the `RelaySessionFactoryTest` class
+  @Test
+  public void testFilterRelayCapabilities() {
+    Capabilities capabilitiesWithApp =
+        new ImmutableCapabilities(
+            "browserName", "chrome", "platformName", "Android", "appium:app", "/link/to/app.apk");
+    Capabilities capabilitiesWithAppPackage =
+        new ImmutableCapabilities(
+            "browserName",
+            "chrome",
+            "platformName",
+            "Android",
+            "appium:appPackage",
+            "com.example.app");
+    Capabilities capabilitiesWithBundleId =
+        new ImmutableCapabilities(
+            "browserName",
+            "chrome",
+            "platformName",
+            "Android",
+            "appium:bundleId",
+            "com.example.app");
+    Capabilities capabilitiesWithoutApp =
+        new ImmutableCapabilities("browserName", "chrome", "platformName", "Android");
+
+    RelaySessionFactory factory = Mockito.mock(RelaySessionFactory.class);
+
+    when(factory.filterRelayCapabilities(capabilitiesWithApp)).thenCallRealMethod();
+    when(factory.filterRelayCapabilities(capabilitiesWithAppPackage)).thenCallRealMethod();
+    when(factory.filterRelayCapabilities(capabilitiesWithBundleId)).thenCallRealMethod();
+    when(factory.filterRelayCapabilities(capabilitiesWithoutApp)).thenCallRealMethod();
+
+    capabilitiesWithApp = factory.filterRelayCapabilities(capabilitiesWithApp);
+    capabilitiesWithAppPackage = factory.filterRelayCapabilities(capabilitiesWithAppPackage);
+    capabilitiesWithBundleId = factory.filterRelayCapabilities(capabilitiesWithBundleId);
+    capabilitiesWithoutApp = factory.filterRelayCapabilities(capabilitiesWithoutApp);
+
+    assertEquals(null, capabilitiesWithApp.getCapability("browserName"));
+    assertEquals(null, capabilitiesWithAppPackage.getCapability("browserName"));
+    assertEquals(null, capabilitiesWithBundleId.getCapability("browserName"));
+    assertEquals("chrome", capabilitiesWithoutApp.getCapability("browserName"));
+  }
+}