Skip to content
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.

Sign up for GitHub

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

#655 FIX #682

Merged
merged 5 commits into from
Aug 5, 2017
Merged

#655 FIX #682

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12cb9d9
#655 FIX
TikhomirovSergey Jul 29, 2017
f5ee595
Improvement with optional
TikhomirovSergey Aug 3, 2017
5996661
service field was turned into serviceOptional
TikhomirovSergey Aug 4, 2017
b83b090
check style issues were fixed
TikhomirovSergey Aug 4, 2017
dd15bc0
removal of the unnecessary casting
TikhomirovSergey Aug 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/main/java/io/appium/java_client/AppiumDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.openqa.selenium.remote.DriverCommand;
import org.openqa.selenium.remote.ErrorHandler;
import org.openqa.selenium.remote.ExecuteMethod;
import org.openqa.selenium.remote.HttpCommandExecutor;
import org.openqa.selenium.remote.Response;
import org.openqa.selenium.remote.html5.RemoteLocationContext;
import org.openqa.selenium.remote.http.HttpClient;
Expand Down Expand Up @@ -82,7 +83,7 @@ public class AppiumDriver<T extends WebElement>
* @param capabilities take a look
* at {@link org.openqa.selenium.Capabilities}
*/
public AppiumDriver(AppiumCommandExecutor executor, Capabilities capabilities) {
public AppiumDriver(HttpCommandExecutor executor, Capabilities capabilities) {
super(executor, capabilities);
this.executeMethod = new AppiumExecutionMethod(this);
locationContext = new RemoteLocationContext(executeMethod);
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/io/appium/java_client/MobileCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableMap;

import org.apache.commons.lang3.StringUtils;
import org.openqa.selenium.remote.CommandInfo;
import org.openqa.selenium.remote.http.HttpMethod;

import java.time.Duration;
Expand Down Expand Up @@ -79,7 +80,7 @@ public class MobileCommand {
protected static final String SET_SETTINGS;
protected static final String GET_CURRENT_PACKAGE;

public static final Map<String, AppiumCommandInfo> commandRepository;
public static final Map<String, CommandInfo> commandRepository;

static {
RESET = "reset";
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/io/appium/java_client/android/AndroidDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@
import io.appium.java_client.CommandExecutionHelper;
import io.appium.java_client.FindsByAndroidUIAutomator;
import io.appium.java_client.PressesKeyCode;
import io.appium.java_client.remote.AppiumCommandExecutor;
import io.appium.java_client.remote.MobilePlatform;
import io.appium.java_client.service.local.AppiumDriverLocalService;
import io.appium.java_client.service.local.AppiumServiceBuilder;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.WebElement;
import org.openqa.selenium.remote.HttpCommandExecutor;
import org.openqa.selenium.remote.http.HttpClient;

import java.net.URL;
Expand Down Expand Up @@ -58,7 +58,7 @@ public class AndroidDriver<T extends WebElement>
* @param capabilities take a look
* at {@link org.openqa.selenium.Capabilities}
*/
public AndroidDriver(AppiumCommandExecutor executor, Capabilities capabilities) {
public AndroidDriver(HttpCommandExecutor executor, Capabilities capabilities) {
super(executor, substituteMobilePlatform(capabilities, ANDROID_PLATFORM));
}

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/io/appium/java_client/ios/IOSDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@
import io.appium.java_client.FindsByIosNSPredicate;
import io.appium.java_client.FindsByIosUIAutomation;
import io.appium.java_client.HidesKeyboardWithKeyName;
import io.appium.java_client.remote.AppiumCommandExecutor;
import io.appium.java_client.remote.MobilePlatform;
import io.appium.java_client.service.local.AppiumDriverLocalService;
import io.appium.java_client.service.local.AppiumServiceBuilder;
import org.openqa.selenium.Alert;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.WebElement;
import org.openqa.selenium.remote.DriverCommand;
import org.openqa.selenium.remote.HttpCommandExecutor;
import org.openqa.selenium.remote.Response;
import org.openqa.selenium.remote.http.HttpClient;
import org.openqa.selenium.security.Credentials;
Expand Down Expand Up @@ -64,7 +64,7 @@ public class IOSDriver<T extends WebElement>
* @param capabilities take a look
* at {@link org.openqa.selenium.Capabilities}
*/
public IOSDriver(AppiumCommandExecutor executor, Capabilities capabilities) {
public IOSDriver(HttpCommandExecutor executor, Capabilities capabilities) {
super(executor, substituteMobilePlatform(capabilities, IOS_PLATFORM));
}

Expand Down
161 changes: 38 additions & 123 deletions src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,29 +16,19 @@

package io.appium.java_client.remote;

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Throwables.getRootCause;
import static com.google.common.base.Throwables.throwIfUnchecked;
import static org.openqa.selenium.remote.DriverCommand.GET_ALL_SESSIONS;
import static org.openqa.selenium.remote.DriverCommand.NEW_SESSION;
import static org.openqa.selenium.remote.DriverCommand.QUIT;
import static java.util.Optional.ofNullable;

import com.google.common.base.Function;
import com.google.common.base.Throwables;

import io.appium.java_client.AppiumCommandInfo;
import org.openqa.selenium.NoSuchSessionException;
import org.openqa.selenium.SessionNotCreatedException;
import org.openqa.selenium.UnsupportedCommandException;
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.remote.Command;
import org.openqa.selenium.remote.CommandCodec;
import org.openqa.selenium.remote.CommandExecutor;
import org.openqa.selenium.remote.Dialect;
import org.openqa.selenium.remote.CommandInfo;
import org.openqa.selenium.remote.DriverCommand;
import org.openqa.selenium.remote.HttpSessionId;
import org.openqa.selenium.remote.HttpCommandExecutor;
import org.openqa.selenium.remote.Response;
import org.openqa.selenium.remote.ResponseCodec;
import org.openqa.selenium.remote.http.HttpClient;
import org.openqa.selenium.remote.http.HttpRequest;
import org.openqa.selenium.remote.http.HttpResponse;
import org.openqa.selenium.remote.internal.ApacheHttpClient;
import org.openqa.selenium.remote.service.DriverService;

Expand All @@ -47,131 +37,56 @@
import java.net.URL;
import java.util.Map;

public class AppiumCommandExecutor implements CommandExecutor {
public class AppiumCommandExecutor extends HttpCommandExecutor {

private final URL remoteServer;
private final HttpClient client;
private final Map<String, AppiumCommandInfo> additionalCommands;
private CommandCodec<HttpRequest> commandCodec;
private ResponseCodec<HttpResponse> responseCodec;
private DriverService service;
private final DriverService service;

/**
* Cretes an instance that sends requests and receives responses.
*
* @param additionalCommands is the mapped command repository
* @param addressOfRemoteServer is the url to connect to the Appium remote/local server
* @param httpClientFactory is the http client factory
*/
public AppiumCommandExecutor(Map<String, AppiumCommandInfo> additionalCommands,
URL addressOfRemoteServer, HttpClient.Factory httpClientFactory) {
checkNotNull(addressOfRemoteServer);
remoteServer = addressOfRemoteServer;
this.additionalCommands = additionalCommands;
this.client = httpClientFactory.createClient(remoteServer);
public AppiumCommandExecutor(Map<String, CommandInfo> additionalCommands,
URL addressOfRemoteServer, HttpClient.Factory httpClientFactory) {
super(additionalCommands, addressOfRemoteServer, httpClientFactory);
service = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be a reason of unexpected NPE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It is checked inside this method
https://github.com/TikhomirovSergey/java-client/blob/12cb9d9b6e117274340e8c73f40b0fa7a11396ba/src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java#L64

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Aug 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

then it might be useful to mark the property as Nullable

Copy link
Contributor

Choose a reason for hiding this comment

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

or make it optional (I'd prefer this option)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mykola-mokhnach ok. I will improve it soon.

}

public AppiumCommandExecutor(Map<String, AppiumCommandInfo> additionalCommands, DriverService service,
HttpClient.Factory httpClientFactory) {
this(additionalCommands, service.getUrl(), httpClientFactory);
public AppiumCommandExecutor(Map<String, CommandInfo> additionalCommands, DriverService service,
HttpClient.Factory httpClientFactory) {
super(additionalCommands, service.getUrl(), httpClientFactory);
this.service = service;
}

public AppiumCommandExecutor(Map<String, AppiumCommandInfo> additionalCommands,
URL addressOfRemoteServer) {
public AppiumCommandExecutor(Map<String, CommandInfo> additionalCommands,
URL addressOfRemoteServer) {
this(additionalCommands, addressOfRemoteServer, new ApacheHttpClient.Factory());
}

public AppiumCommandExecutor(Map<String, AppiumCommandInfo> additionalCommands,
DriverService service) {
public AppiumCommandExecutor(Map<String, CommandInfo> additionalCommands,
DriverService service) {
this(additionalCommands, service, new ApacheHttpClient.Factory());
}

public URL getAddressOfRemoteServer() {
return remoteServer;
}

private Response doExecute(Command command) throws IOException, WebDriverException {
if (command.getSessionId() == null) {
if (QUIT.equals(command.getName())) {
return new Response();
}
if (!GET_ALL_SESSIONS.equals(command.getName())
&& !NEW_SESSION.equals(command.getName())) {
throw new NoSuchSessionException(
"Session ID is null. Using WebDriver after calling quit()?");
}
}

if (NEW_SESSION.equals(command.getName())) {
if (commandCodec != null) {
throw new SessionNotCreatedException("Session already exists");
}
AppiumProtocolHandShake handshake = new AppiumProtocolHandShake();
AppiumProtocolHandShake.Result result = handshake.createSession(client, command);
Dialect dialect = result.getDialect();
commandCodec = dialect.getCommandCodec();

additionalCommands.forEach((key, value) -> {
checkNotNull(key);
checkNotNull(value);
commandCodec.defineCommand(key, value.getMethod(), value.getUrl());
} );

responseCodec = dialect.getResponseCodec();
return result.createResponse();
}

if (commandCodec == null || responseCodec == null) {
throw new WebDriverException(
"No command or response codec has been defined. Unable to proceed");
}

HttpRequest httpRequest = commandCodec.encode(command);
try {
HttpResponse httpResponse = client.execute(httpRequest, true);

Response response = responseCodec.decode(httpResponse);
if (response.getSessionId() == null) {
if (httpResponse.getTargetHost() != null) {
response.setSessionId(HttpSessionId.getSessionId(httpResponse.getTargetHost()));
} else {
response.setSessionId(command.getSessionId().toString());
@Override public Response execute(Command command) throws WebDriverException {
if (DriverCommand.NEW_SESSION.equals(command.getName())) {
ofNullable(service).ifPresent(driverService -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the code will be simpler if we make the service variable itself of type Optional

try {
driverService.start();
} catch (IOException e) {
throw new WebDriverException(e.getMessage(), e);
}
}
if (QUIT.equals(command.getName())) {
client.close();
}
return response;
} catch (UnsupportedCommandException e) {
if (e.getMessage() == null || "".equals(e.getMessage())) {
throw new UnsupportedOperationException(
"No information from server. Command name was: " + command.getName(),
e.getCause());
}
throw e;
}
}

@Override public Response execute(Command command) throws IOException, WebDriverException {
if (DriverCommand.NEW_SESSION.equals(command.getName()) && service != null) {
service.start();
});
}

try {
return doExecute(command);
return super.execute(command);
} catch (Throwable t) {
Throwable rootCause = getRootCause(t);
Throwable rootCause = Throwables.getRootCause(t);
if (rootCause instanceof ConnectException
&& rootCause.getMessage().contains("Connection refused")
&& service != null) {
if (service.isRunning()) {
throw new WebDriverException("The session is closed!", t);
}

if (!service.isRunning()) {
throw new WebDriverException("The appium server has accidentally died!", t);
}
&& rootCause.getMessage().contains("Connection refused")) {
throw ofNullable(service).map((Function<DriverService, WebDriverException>) service -> {
if (service.isRunning()) {
return new WebDriverException("The session is closed!", rootCause);
}

return new WebDriverException("The appium server has accidentally died!", rootCause);
}).orElse(new WebDriverException(rootCause.getMessage(), rootCause));
Copy link
Contributor

Choose a reason for hiding this comment

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

probably, orElseGet ?

}
throwIfUnchecked(t);
throw new WebDriverException(t);
Expand All @@ -182,4 +97,4 @@ private Response doExecute(Command command) throws IOException, WebDriverExcepti
}
}

}
}
Loading