A proposed fix for issue 276 (https://github.com/appium/java-client/i…

[baechul]

…ssues/276)

[baechul]

And here is the usage example:

public class HeaderInjectableHttpClient implements HttpClient {

    private final HttpClient actualClient;
    private final Map<String,String> extraHeaders;

    public HeaderInjectableHttpClient(HttpClient actualClient) {
        this.actualClient = actualClient;
        extraHeaders = new HashMap<>();
    }

    public void addHeader(String header, String value) {
        if(header != null) extraHeaders.put(header, value);
    }

    public void addHeaders(Map<String,String> headers) {
        if(headers != null) extraHeaders.putAll(headers);
    }

    @Override
    public HttpResponse execute(HttpRequest request, boolean followRedirects) throws IOException {      
        // intercept and add the given extra headers.
        for(String header : extraHeaders.keySet()) {
            String value = extraHeaders.get(header);
            if(value != null) {
                request.addHeader(header, value);
            }
        }

        return actualClient.execute(request, followRedirects);
    }
}

HttpClient.Factory httpClientFactory = new ApacheHttpClient.Factory() {
        @Override
        public HttpClient createClient(URL url) {
            HeaderInjectableHttpClient client = new HeaderInjectableHttpClient(super.createClient(url));
            client.addHeader("name", "value");
        }
};

AndroidDriver driver = new AndroidDriver(url, httpClientFactory, desiredCapabilities);

[TikhomirovSergey]

@baechul
Ok. I like this PR 👍 I think it is even better than the passing user's CommandExecutor through constructors because it could be incompatible with whole Appium ecosystem (with client or/and server).

But I have some remarks. My opinion is that proposed changes look bit incomplete. It would be better if you redesigned AppiumCommandExecutor and AppiumDriver the way below :

...
import org.openqa.selenium.remote.internal.ApacheHttpClient;
...

public class AppiumCommandExecutor extends HttpCommandExecutor{

    private final DriverService service;

    public AppiumCommandExecutor(Map<String, CommandInfo> additionalCommands, 
                                 URL addressOfRemoteServer,
                                 HttpClient.Factory httpClientFactory) {
        super(additionalCommands, addressOfRemoteServer, httpClientFactory);
        service = null;
    }

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

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

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

and

...
i@SuppressWarnings("unchecked")
public abstract class AppiumDriver<RequiredElementType extends WebElement> extends DefaultGenericMobileDriver<RequiredElementType> {
...

    private AppiumDriver(HttpCommandExecutor executor, Capabilities capabilities){
        super(executor, capabilities);
        this.executeMethod = new AppiumExecutionMethod(this);
        locationContext = new RemoteLocationContext(executeMethod);
        super.setErrorHandler(errorHandler);
        this.remoteAddress = executor.getAddressOfRemoteServer();
    }



    public AppiumDriver(URL remoteAddress, Capabilities desiredCapabilities) {
        this(new AppiumCommandExecutor(
                getMobileCommands(), remoteAddress), desiredCapabilities);
    }

    public AppiumDriver(URL remoteAddress, HttpClient.Factory httpClientFactory, Capabilities desiredCapabilities) {
        this(new AppiumCommandExecutor(
                        getMobileCommands(), remoteAddress, httpClientFactory), desiredCapabilities);
    }


    public AppiumDriver(AppiumDriverLocalService service, Capabilities desiredCapabilities) {
        this(new AppiumCommandExecutor(
                getMobileCommands(), service), desiredCapabilities);
    }


    public AppiumDriver(AppiumDriverLocalService service, HttpClient.Factory httpClientFactory, Capabilities desiredCapabilities) {
        this(new AppiumCommandExecutor(
                getMobileCommands(), service, httpClientFactory), desiredCapabilities);
    }


    public AppiumDriver(AppiumServiceBuilder builder, Capabilities desiredCapabilities) {
        this(builder.build(), desiredCapabilities);
    }

    public AppiumDriver(AppiumServiceBuilder builder, HttpClient.Factory httpClientFactory, Capabilities desiredCapabilities) {
        this(builder.build(), httpClientFactory, desiredCapabilities);
    }

    public AppiumDriver(Capabilities desiredCapabilities) {
        this(AppiumDriverLocalService.buildDefaultService(), desiredCapabilities);
    }

    public AppiumDriver(HttpClient.Factory httpClientFactory, Capabilities desiredCapabilities) {
        this(AppiumDriverLocalService.buildDefaultService(), httpClientFactory, desiredCapabilities);
    }

...

and add new public constructors to AndroidDriver/IOSDriver.

I just think that other users would like to use the new capability (you probably too) combined with the AppiumDriverLocalService.

I was managed to make this improvement locally. It works!

If you improve your PR the way above then there won't be necessity to cover the new capability with tests and the only way to shoot user's leg will be the implementation of a bad httpClientFactory (and only a user will be responsible) :)

@Jonahss @bootstraponline
Please look at this PR.

[TikhomirovSergey]

#276

[baechul]

Makes sense. Added the same capability for the constructors with localservice and builder parameters as suggested.

BTW when is the next release? Can this be merged for the next release?

[TikhomirovSergey]

I think this feature will be included to the new build. Let's wait for some time.

[Jonahss]

merged.

Ok, let's see where we are on a release. I think there's some outstanding pull requests, and then we should publish.

[baechul]

Great. Thanks.