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.

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

Replace Request#setHeaders with addHeader #30588

Merged
merged 15 commits into from
May 22, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesRequest;
import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesResponse;
import org.elasticsearch.action.bulk.BulkRequest;
import org.elasticsearch.action.bulk.BulkResponse;
import org.elasticsearch.action.delete.DeleteRequest;
Expand Down Expand Up @@ -564,7 +562,7 @@ protected final <Req extends ActionRequest, Resp> Resp performRequest(Req reques
throw validationException;
}
Request req = requestConverter.apply(request);
req.setHeaders(headers);
addHeaders(req, headers);
Response response;
try {
response = client.performRequest(req);
Expand Down Expand Up @@ -614,12 +612,19 @@ protected final <Req extends ActionRequest, Resp> void performRequestAsync(Req r
listener.onFailure(e);
return;
}
req.setHeaders(headers);
addHeaders(req, headers);

ResponseListener responseListener = wrapResponseListener(responseConverter, listener, ignores);
client.performRequestAsync(req, responseListener);
}

private static void addHeaders(Request request, Header... headers) {
Objects.requireNonNull(headers, "headers cannot be null");
for (Header header : headers) {
request.addHeader(header);
}
}

final <Resp> ResponseListener wrapResponseListener(CheckedFunction<Response, Resp, IOException> responseConverter,
ActionListener<Resp> actionListener, Set<Integer> ignores) {
return new ResponseListener() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,12 @@ public void initClients() throws IOException {
final RestClient restClient = mock(RestClient.class);
restHighLevelClient = new CustomRestClient(restClient);

doAnswer(inv -> mockPerformRequest(((Request) inv.getArguments()[0]).getHeaders()[0]))
doAnswer(inv -> mockPerformRequest(((Request) inv.getArguments()[0]).getHeaders().iterator().next()))
.when(restClient)
.performRequest(any(Request.class));

doAnswer(inv -> mockPerformRequestAsync(
((Request) inv.getArguments()[0]).getHeaders()[0],
((Request) inv.getArguments()[0]).getHeaders().iterator().next(),
(ResponseListener) inv.getArguments()[1]))
.when(restClient)
.performRequestAsync(any(Request.class), any(ResponseListener.class));
Expand Down
34 changes: 16 additions & 18 deletions client/rest/src/main/java/org/elasticsearch/client/Request.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,16 @@

package org.elasticsearch.client;

import org.apache.http.entity.ContentType;
import org.apache.http.Header;
import org.apache.http.HttpEntity;
import org.apache.http.entity.ContentType;
import org.apache.http.nio.entity.NStringEntity;
import org.apache.http.nio.protocol.HttpAsyncResponseConsumer;

import java.util.Arrays;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;

Expand All @@ -36,13 +38,12 @@
* HTTP Request to Elasticsearch.
*/
public final class Request {
private static final Header[] NO_HEADERS = new Header[0];
private final String method;
private final String endpoint;
private final Map<String, String> parameters = new HashMap<>();
private final List<Header> headers = new ArrayList<>();

private HttpEntity entity;
private Header[] headers = NO_HEADERS;
private HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory =
HttpAsyncResponseConsumerFactory.DEFAULT;

Expand Down Expand Up @@ -125,21 +126,18 @@ public HttpEntity getEntity() {
}

/**
* Set the headers to attach to the request.
* Add the provided header to the request.
*/
public void setHeaders(Header... headers) {
Objects.requireNonNull(headers, "headers cannot be null");
for (Header header : headers) {
Objects.requireNonNull(header, "header cannot be null");
}
this.headers = headers;
public void addHeader(Header header) {
Copy link
Member

Choose a reason for hiding this comment

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

What about addHeader(String key, String value) instead? I don't see us using anything but BasicHeader anyway and I like exposing a little less of httpclient publicly. And I like not having to import another class everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

yea I think it's a good idea

Objects.requireNonNull(header, "header cannot be null");
this.headers.add(header);
}

/**
* Headers to attach to the request.
*/
public Header[] getHeaders() {
return headers;
public List<Header> getHeaders() {
return Collections.unmodifiableList(headers);
}

/**
Expand Down Expand Up @@ -175,13 +173,13 @@ public String toString() {
if (entity != null) {
b.append(", entity=").append(entity);
}
if (headers.length > 0) {
if (headers.size() > 0) {
b.append(", headers=");
for (int h = 0; h < headers.length; h++) {
for (int h = 0; h < headers.size(); h++) {
if (h != 0) {
b.append(',');
}
b.append(headers[h].toString());
b.append(headers.get(h).toString());
}
}
if (httpAsyncResponseConsumerFactory != HttpAsyncResponseConsumerFactory.DEFAULT) {
Expand All @@ -204,12 +202,12 @@ public boolean equals(Object obj) {
&& endpoint.equals(other.endpoint)
&& parameters.equals(other.parameters)
&& Objects.equals(entity, other.entity)
&& Arrays.equals(headers, other.headers)
&& headers.equals(other.headers)
&& httpAsyncResponseConsumerFactory.equals(other.httpAsyncResponseConsumerFactory);
}

@Override
public int hashCode() {
return Objects.hash(method, endpoint, parameters, entity, Arrays.hashCode(headers), httpAsyncResponseConsumerFactory);
return Objects.hash(method, endpoint, parameters, entity, headers, httpAsyncResponseConsumerFactory);
}
}
33 changes: 23 additions & 10 deletions client/rest/src/main/java/org/elasticsearch/client/RestClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ public void performRequestAsync(Request request, ResponseListener responseListen
@Deprecated
public Response performRequest(String method, String endpoint, Header... headers) throws IOException {
Request request = new Request(method, endpoint);
request.setHeaders(headers);
addHeaders(request, headers);
return performRequest(request);
}

Expand All @@ -237,7 +237,7 @@ public Response performRequest(String method, String endpoint, Header... headers
public Response performRequest(String method, String endpoint, Map<String, String> params, Header... headers) throws IOException {
Request request = new Request(method, endpoint);
addParameters(request, params);
request.setHeaders(headers);
addHeaders(request, headers);
return performRequest(request);
}

Expand All @@ -264,7 +264,7 @@ public Response performRequest(String method, String endpoint, Map<String, Strin
Request request = new Request(method, endpoint);
addParameters(request, params);
request.setEntity(entity);
request.setHeaders(headers);
addHeaders(request, headers);
return performRequest(request);
}

Expand Down Expand Up @@ -305,7 +305,7 @@ public Response performRequest(String method, String endpoint, Map<String, Strin
addParameters(request, params);
request.setEntity(entity);
request.setHttpAsyncResponseConsumerFactory(httpAsyncResponseConsumerFactory);
request.setHeaders(headers);
addHeaders(request, headers);
return performRequest(request);
}

Expand All @@ -325,7 +325,7 @@ public void performRequestAsync(String method, String endpoint, ResponseListener
Request request;
try {
request = new Request(method, endpoint);
request.setHeaders(headers);
addHeaders(request, headers);
} catch (Exception e) {
responseListener.onFailure(e);
return;
Expand All @@ -352,7 +352,7 @@ public void performRequestAsync(String method, String endpoint, Map<String, Stri
try {
request = new Request(method, endpoint);
addParameters(request, params);
request.setHeaders(headers);
addHeaders(request, headers);
} catch (Exception e) {
responseListener.onFailure(e);
return;
Expand Down Expand Up @@ -383,7 +383,7 @@ public void performRequestAsync(String method, String endpoint, Map<String, Stri
request = new Request(method, endpoint);
addParameters(request, params);
request.setEntity(entity);
request.setHeaders(headers);
addHeaders(request, headers);
} catch (Exception e) {
responseListener.onFailure(e);
return;
Expand Down Expand Up @@ -420,7 +420,7 @@ public void performRequestAsync(String method, String endpoint, Map<String, Stri
addParameters(request, params);
request.setEntity(entity);
request.setHttpAsyncResponseConsumerFactory(httpAsyncResponseConsumerFactory);
request.setHeaders(headers);
addHeaders(request, headers);
} catch (Exception e) {
responseListener.onFailure(e);
return;
Expand Down Expand Up @@ -539,9 +539,9 @@ public void cancelled() {
});
}

private void setHeaders(HttpRequest httpRequest, Header[] requestHeaders) {
private void setHeaders(HttpRequest httpRequest, Collection<Header> requestHeaders) {
// request headers override default headers, so we don't add default headers if they exist as request headers
final Set<String> requestNames = new HashSet<>(requestHeaders.length);
final Set<String> requestNames = new HashSet<>(requestHeaders.size());
for (Header requestHeader : requestHeaders) {
httpRequest.addHeader(requestHeader);
requestNames.add(requestHeader.getName());
Expand Down Expand Up @@ -877,10 +877,23 @@ private static class HostTuple<T> {
}
}

/**
* Add all headers from the provided varargs argument to a {@link Request}. This only exists
* to support methods that exist for backwards compatibility.
*/
@Deprecated
private static void addHeaders(Request request, Header... headers) {
Objects.requireNonNull(headers, "headers cannot be null");
for (Header header : headers) {
request.addHeader(header);
}
}

/**
* Add all parameters from a map to a {@link Request}. This only exists
* to support methods that exist for backwards compatibility.
*/
@Deprecated
private static void addParameters(Request request, Map<String, String> parameters) {
Objects.requireNonNull(parameters, "parameters cannot be null");
for (Map.Entry<String, String> entry : parameters.entrySet()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,6 @@

package org.elasticsearch.client;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;

import org.apache.http.Header;
import org.apache.http.HttpEntity;
import org.apache.http.entity.ByteArrayEntity;
Expand All @@ -33,7 +28,13 @@
import org.apache.http.nio.entity.NStringEntity;
import org.elasticsearch.client.HttpAsyncResponseConsumerFactory.HeapBufferedResponseConsumerFactory;

import static org.junit.Assert.assertArrayEquals;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNull;
Expand Down Expand Up @@ -127,31 +128,26 @@ public void testSetJsonEntity() throws IOException {
assertEquals(json, new String(os.toByteArray(), ContentType.APPLICATION_JSON.getCharset()));
}

public void testSetHeaders() {
public void testAddHeaders() {
Copy link
Member

Choose a reason for hiding this comment

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

testAddHeader?

final String method = randomFrom(new String[] {"GET", "PUT", "POST", "HEAD", "DELETE"});
final String endpoint = randomAsciiLettersOfLengthBetween(1, 10);
Request request = new Request(method, endpoint);

try {
request.setHeaders((Header[]) null);
fail("expected failure");
} catch (NullPointerException e) {
assertEquals("headers cannot be null", e.getMessage());
}

try {
request.setHeaders(new Header [] {null});
request.addHeader(null);
fail("expected failure");
} catch (NullPointerException e) {
assertEquals("header cannot be null", e.getMessage());
}

Header[] headers = new Header[between(0, 5)];
for (int i = 0; i < headers.length; i++) {
headers[i] = new BasicHeader(randomAsciiAlphanumOfLength(3), randomAsciiAlphanumOfLength(3));
int numHeaders = between(0, 5);
Set<Header> headers = new HashSet<>();
for (int i = 0; i < numHeaders; i++) {
BasicHeader header = new BasicHeader(randomAsciiAlphanumOfLengthBetween(5, 10), randomAsciiAlphanumOfLength(3));
headers.add(header);
request.addHeader(header);
}
request.setHeaders(headers);
assertArrayEquals(headers, request.getHeaders());
assertEquals(headers, new HashSet<>(request.getHeaders()));
}

public void testEqualsAndHashCode() {
Expand All @@ -168,7 +164,7 @@ public void testEqualsAndHashCode() {
assertNotEquals(mutant, request);
}

private Request randomRequest() {
private static Request randomRequest() {
Request request = new Request(
randomFrom(new String[] {"GET", "PUT", "DELETE", "POST", "HEAD", "OPTIONS"}),
randomAsciiAlphanumOfLength(5));
Expand All @@ -192,11 +188,9 @@ private Request randomRequest() {

if (randomBoolean()) {
int headerCount = between(1, 5);
Header[] headers = new Header[headerCount];
for (int i = 0; i < headerCount; i++) {
headers[i] = new BasicHeader(randomAsciiAlphanumOfLength(3), randomAsciiAlphanumOfLength(3));
request.addHeader(new BasicHeader(randomAsciiAlphanumOfLength(3), randomAsciiAlphanumOfLength(3)));
}
request.setHeaders(headers);
}

if (randomBoolean()) {
Expand All @@ -206,13 +200,13 @@ private Request randomRequest() {
return request;
}

private Request copy(Request request) {
private static Request copy(Request request) {
Request copy = new Request(request.getMethod(), request.getEndpoint());
copyMutables(request, copy);
return copy;
}

private Request mutate(Request request) {
private static Request mutate(Request request) {
if (randomBoolean()) {
// Mutate request or method but keep everything else constant
Request mutant = randomBoolean()
Expand All @@ -231,11 +225,7 @@ private Request mutate(Request request) {
mutant.setJsonEntity("mutant"); // randomRequest can't produce this value
return mutant;
case 2:
if (mutant.getHeaders().length > 0) {
mutant.setHeaders(new Header[0]);
} else {
mutant.setHeaders(new BasicHeader("extra", "m"));
}
mutant.addHeader(new BasicHeader("extra", "m"));
return mutant;
case 3:
mutant.setHttpAsyncResponseConsumerFactory(new HeapBufferedResponseConsumerFactory(5));
Expand All @@ -245,12 +235,14 @@ private Request mutate(Request request) {
}
}

private void copyMutables(Request from, Request to) {
private static void copyMutables(Request from, Request to) {
for (Map.Entry<String, String> param : from.getParameters().entrySet()) {
to.addParameter(param.getKey(), param.getValue());
}
to.setEntity(from.getEntity());
to.setHeaders(from.getHeaders());
for (Header header : from.getHeaders()) {
to.addHeader(header);
}
to.setHttpAsyncResponseConsumerFactory(from.getHttpAsyncResponseConsumerFactory());
}
}
Loading