Repository navigation
fix: update executor and driver server URL state on directConnect #2500
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,12 +55,12 @@ public class AppiumCommandExecutor implements CommandExecutor, Closeable { | |
|
|
||
| private final Optional<AppiumDriverLocalService> serviceOptional; | ||
| @Getter | ||
| private final AppiumClientConfig appiumClientConfig; | ||
| private final @Nullable URL remoteServer; | ||
| private volatile AppiumClientConfig appiumClientConfig; | ||
| private volatile @Nullable URL remoteServer; | ||
| private final Map<String, AppiumCommandInfo> additionalCommands; | ||
| @Getter | ||
| private final Factory httpClientFactory; | ||
| private HttpClient client; | ||
| private volatile HttpClient client; | ||
| private @Nullable CommandCodec commandCodec; | ||
| private @Nullable ResponseCodec responseCodec; | ||
|
|
||
|
|
@@ -78,10 +78,14 @@ public AppiumCommandExecutor( | |
| @Nullable Factory httpClientFactory, | ||
| AppiumClientConfig appiumClientConfig) { | ||
| this.additionalCommands = new HashMap<>(requireNonNull(additionalCommands, "Additional commands")); | ||
| this.appiumClientConfig = requireNonNull(appiumClientConfig, "HTTP client configuration"); | ||
| AppiumClientConfig effectiveConfig = requireNonNull(appiumClientConfig, "HTTP client configuration"); | ||
| if (service != null && effectiveConfig.baseUri() == null) { | ||
| effectiveConfig = effectiveConfig.baseUrl(service.getUrl()); | ||
| } | ||
| this.appiumClientConfig = effectiveConfig; | ||
| this.httpClientFactory = ofNullable(httpClientFactory).orElseGet(Factory::createDefault); | ||
| this.client = this.httpClientFactory.createClient(appiumClientConfig); | ||
| this.remoteServer = appiumClientConfig.baseUrl(); | ||
| this.client = this.httpClientFactory.createClient(effectiveConfig); | ||
| this.remoteServer = effectiveConfig.baseUrl(); | ||
| this.serviceOptional = ofNullable(service); | ||
| } | ||
|
|
||
|
|
@@ -168,7 +172,7 @@ public AppiumCommandExecutor(Map<String, AppiumCommandInfo> additionalCommands, | |
| */ | ||
| public AppiumCommandExecutor(Map<String, AppiumCommandInfo> additionalCommands, | ||
| AppiumDriverLocalService service, AppiumClientConfig appiumClientConfig) { | ||
| this(additionalCommands, service, Factory.createDefault(), appiumClientConfig); | ||
| this(additionalCommands, requireNonNull(service), Factory.createDefault(), appiumClientConfig); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] Adding |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -240,9 +244,12 @@ public HttpClient getClient() { | |
| * and other non-special addresses are not rejected by this check. | ||
| */ | ||
| protected void overrideServerUrl(URL serverUrl) { | ||
| DirectConnectUrlSafety.requireSafeOverrideTarget(serverUrl); | ||
| HttpClient newClient = getHttpClientFactory().createClient(appiumClientConfig.baseUrl(serverUrl)); | ||
| HttpClient oldClient = this.client; | ||
| DirectConnectUrlSafety.requireSafeOverrideTarget(requireNonNull(serverUrl, "Server URL")); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] |
||
| AppiumClientConfig updatedConfig = appiumClientConfig.baseUrl(serverUrl); | ||
| HttpClient newClient = getHttpClientFactory().createClient(updatedConfig); | ||
| final HttpClient oldClient = this.client; | ||
| this.appiumClientConfig = updatedConfig; | ||
| this.remoteServer = updatedConfig.baseUrl(); | ||
| this.client = newClient; | ||
| oldClient.close(); | ||
| } | ||
|
|
@@ -292,7 +299,7 @@ private void setDirectConnect(Response response) throws SessionNotCreatedExcepti | |
| return; | ||
| } | ||
|
|
||
| if (!directConnect.getProtocol().equals("https")) { | ||
| if (!"https".equalsIgnoreCase(directConnect.getProtocol())) { | ||
| throw new SessionNotCreatedException( | ||
| String.format("The given protocol '%s' as the direct connection url returned by " | ||
| + "the remote server is not accurate. Only 'https' is supported.", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,6 @@ | |
| import java.net.MalformedURLException; | ||
| import java.net.URL; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.stream.Stream; | ||
|
|
||
| import static io.appium.java_client.internal.CapabilityHelpers.APPIUM_PREFIX; | ||
|
|
@@ -65,11 +64,13 @@ private String getDirectConnectValue(Map<String, ?> responseValue, String key) { | |
| } | ||
|
|
||
| /** | ||
| * Returns true if the {@link DirectConnect} instance member has nonnull values. | ||
| * @return true if each connection information has a nonnull value | ||
| * Returns true if the {@link DirectConnect} instance member has valid values. | ||
| * @return true if protocol, host and port are non-blank and path is non-null | ||
| */ | ||
| public boolean isValid() { | ||
| return Stream.of(this.protocol, this.path, this.host, this.port).noneMatch(Objects::isNull); | ||
| return this.path != null | ||
| && Stream.of(this.protocol, this.host, this.port) | ||
| .allMatch(value -> value != null && !value.isBlank()); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -78,15 +79,42 @@ public boolean isValid() { | |
| * @throws MalformedURLException if the built url was invalid | ||
| */ | ||
| public URL getUrl() throws MalformedURLException { | ||
| String newUrlCandidate = String.format("%s://%s:%s%s", this.protocol, this.host, this.port, this.path); | ||
| String formattedHost = formatHost(this.host); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] |
||
| String normalizedPath = normalizePath(this.path); | ||
| String newUrlCandidate = String.format("%s://%s:%s%s", | ||
| this.protocol, formattedHost, this.port, normalizedPath); | ||
| if (!isValid()) { | ||
| throw createMalformedUrlException(newUrlCandidate); | ||
| } | ||
|
|
||
| try { | ||
| return new URL(newUrlCandidate); | ||
| } catch (MalformedURLException e) { | ||
| throw new MalformedURLException( | ||
| String.format("The remote server returned an invalid value to build the direct connect URL: %s", | ||
| newUrlCandidate) | ||
| ); | ||
| throw createMalformedUrlException(newUrlCandidate); | ||
| } | ||
| } | ||
|
|
||
| private static String formatHost(@Nullable String host) { | ||
| if (host != null && !host.startsWith("[") && host.indexOf(':') != host.lastIndexOf(':')) { | ||
| return "[" + host + "]"; | ||
| } | ||
| return String.valueOf(host); | ||
| } | ||
|
|
||
| private static String normalizePath(@Nullable String path) { | ||
| if (path == null) { | ||
| return "null"; | ||
| } | ||
| if (path.isEmpty() || path.startsWith("/")) { | ||
| return path; | ||
| } | ||
| return "/" + path; | ||
| } | ||
|
|
||
| private static MalformedURLException createMalformedUrlException(String candidate) { | ||
| return new MalformedURLException( | ||
| String.format("The remote server returned an invalid value to build the direct connect URL: %s", | ||
| candidate) | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit]
getRemoteAddress()now reads from the executor, so theremoteAddressfield is only a fallback for a non-AppiumCommandExecutor. Both constructors only ever create or accept anAppiumCommandExecutor, so the field and theinstanceofbranch look dead. Dropping them would make it clear where the value comes from.