feat: add directConnect feature by KazuCocoa · Pull Request #1747 · appium/java-client

KazuCocoa · 2022-08-25T05:42:06Z

Change list

I'd like to add https://appiumpro.com/editions/86-connecting-directly-to-appium-hosts-in-distributed-environments for Java client. Other clients such as webdriverio, Ruby and Python already have them for long. If the response had directConnect capabilities, the appium client attempt to send requests to the directConnect one instead of the original URL.

directConnectProtocol
directConnectHost
directConnectPort
directConnectPath

or appium: prefixed ones.

Types of changes

What types of changes are you proposing/introducing to Java client?
Put an x in the boxes that apply

No changes in production code.
Bugfix (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 not work as expected)

Details

The main change is setDirectConnect in src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
To allow users to turn it on/off, I've added AppiumClientConfig and give the instance to the driver.

The example code is like...

     * AppiumClientConfig clientConfig = AppiumClientConfig.defaultConfig()
     *     .baseUri(URI.create("WebDriver URL"))
     *     .readTimeout(Duration.ofMinutes(5));
     * WindowsOptions options = new WindowsOptions();
     * WindowsDriver driver = new WindowsDriver(clientConfig, options);

By inheriting ClientConfig, we can set a custom User Agent as well. This PR includes the way to set the UA like appium/8.3.0 (selenium/4.5.0 (java mac))

The naming and implementation methods in AppiumClientConfig follow ClientConfig as possible.
ImeHandler removal in the test code was selenium 4.5.0 removed it.

src/main/java/io/appium/java_client/AppiumDriver.java

src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java

mykola-mokhnach · 2022-08-25T07:48:49Z

I assume a vanilla redirect (code 302) would work better for such purpose than all this logic with response parsing and multiple values that eventually construct the same resulting url

KazuCocoa · 2022-08-25T08:28:01Z

I agree with your intention basically, but I think the behavior was a bit different by this.:
Redirect changes the url after calling the default webdriver URL by getting 302, so basically, every request goes through the default webdrier URL (and redirects to another URL).
https://appiumpro.com/editions/86-connecting-directly-to-appium-hosts-in-distributed-environments idea was a new session request went thought the webdriver URL and created a session against behind a middle server (managed distributed hosts/devices like selenium grid), and then following request in the session would reach directConnect one directly instead of the original webdrievr URL to reduce HTTP request's latency.

mykola-mokhnach · 2022-08-25T10:03:19Z

I agree with your intention basically, but I think the behavior was a bit different by this.:
Redirect changes the url after calling the default webdriver URL by getting 302, so basically, every request goes through the default webdrier URL (and redirects to another URL).
https://appiumpro.com/editions/86-connecting-directly-to-appium-hosts-in-distributed-environments idea was a new session request went thought the webdriver URL and created a session against behind a middle server (managed distributed hosts/devices like selenium grid), and then following request in the session would reach directConnect one directly instead of the original webdrievr URL to reduce HTTP request's latency.

Thanks for the explanation. In such case redirect is probably won't work, although I still don't see an actual need to provide 4 different values if we still need to combine them into a single url. It would probably too late to change the current implementation as it is already present in other clients

KazuCocoa · 2022-08-25T16:39:23Z

thanks.
Yea, I came up with the same thought after a while... > a single url

KazuCocoa · 2022-08-25T16:40:13Z

Changed to draft to update reviews

…o direct-connect

KazuCocoa · 2022-10-20T07:16:10Z

Updated. #1779 includes UA stuff as another PR

mykola-mokhnach · 2022-10-20T18:22:12Z

src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java

+
+    private final AppiumClientConfig appiumClientConfig;
+
+    private static class DirectConnect {


I would say it is ok to have this class public. Also, you could use lombok for this purpose, which greatly reduces the amount of boilerplate code for POJOs

also having it public simplifies unit testing

Moved to a new public class file. I'll try the lambok later as another pr after the UA change (So far, I've left a TODO comment)

src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java

mykola-mokhnach · 2022-10-21T18:28:00Z

src/main/java/io/appium/java_client/remote/DirectConnect.java

+    private static final String DIRECT_CONNECT_HOST = "directConnectHost";
+    private static final String DIRECT_CONNECT_PORT = "directConnectPort";
+
+    public final String protocol;


please create public accessors rather than making properties public (e.g. protocol -> getProtocol()). Later we could simplify that code with lombok

mykola-mokhnach · 2022-10-21T18:29:30Z

src/test/java/io/appium/java_client/internal/DirectConnectTest.java

+class DirectConnectTest {
+
+    @Test
+    void hasValidDirectConnectValuesWithoutAppiumPrefix() throws MalformedURLException {


mykola-mokhnach · 2022-10-21T18:29:57Z

src/test/java/io/appium/java_client/internal/DirectConnectTest.java

+    }
+
+    @Test
+    void hasValidDirectConnectInvalid() {


this test name does not make much sense to me

tksjh · 2023-07-11T20:29:47Z

src/main/java/io/appium/java_client/AppiumClientConfig.java

+
+    @Override
+    public AppiumClientConfig readTimeout(Duration timeout) {
+        ClientConfig clientConfig = super.connectionTimeout(timeout);


you should call super.readTimeout(timeout)

Thank you, fixed in #1959

KazuCocoa added 8 commits August 24, 2022 18:46

feat: support directConnect

51d7c1e

add an argument for directConnect

65e494f

add flag in android drivers

05d0976

add config

5c3f950

add for ios

a5c3baa

add config to other drivers

48b42e8

update imports, license

55ad185

use a tag

d0ff28a

KazuCocoa changed the title ~~Direct connect~~ feat: add directConnect feature Aug 25, 2022

fix lint

16dd824

KazuCocoa marked this pull request as ready for review August 25, 2022 05:58

mykola-mokhnach reviewed Aug 25, 2022

View reviewed changes

src/main/java/io/appium/java_client/AppiumDriver.java Outdated Show resolved Hide resolved

mykola-mokhnach reviewed Aug 25, 2022

View reviewed changes

src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java Outdated Show resolved Hide resolved