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

Jump to bottom

Support of headers to 'HttpJsonRequest'. Adding 'Connection: close' header while pinging workspace agent #7898

Merged
merged 1 commit into from
Dec 18, 2017
Merged

Support of headers to 'HttpJsonRequest'. Adding 'Connection: close' header while pinging workspace agent #7898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 23 additions & 3 deletions ...che-core-api-core/src/main/java/org/eclipse/che/api/core/rest/DefaultHttpJsonRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ public class DefaultHttpJsonRequest implements HttpJsonRequest {
private String method;
private Object body;
private List<Pair<String, ?>> queryParams;
private List<Pair<String, String>> headers;
private String authorizationHeaderValue;

protected DefaultHttpJsonRequest(String url, String method) {
Expand Down Expand Up @@ -118,6 +119,16 @@ public HttpJsonRequest addQueryParam(@NotNull String name, @NotNull Object value
return this;
}

public HttpJsonRequest addHeader(@NotNull String name, @NotNull String value) {
requireNonNull(name, "Required non-null header name");
requireNonNull(value, "Required non-null header value");
if (headers == null) {
headers = new ArrayList<>();
}
headers.add(Pair.of(name, value));
return this;
}

@Override
public HttpJsonRequest setAuthorizationHeader(@NotNull String value) {
requireNonNull(value, "Required non-null header value");
Expand Down Expand Up @@ -149,7 +160,7 @@ public HttpJsonResponse request()
if (method == null) {
throw new IllegalStateException("Could not perform request, request method wasn't set");
}
return doRequest(timeout, url, method, body, queryParams, authorizationHeaderValue);
return doRequest(timeout, url, method, body, queryParams, authorizationHeaderValue, headers);
}

/**
Expand Down Expand Up @@ -182,7 +193,8 @@ protected DefaultHttpJsonResponse doRequest(
String method,
Object body,
List<Pair<String, ?>> parameters,
String authorizationHeaderValue)
String authorizationHeaderValue,
List<Pair<String, String>> headers)
throws IOException, ServerException, ForbiddenException, NotFoundException,
UnauthorizedException, ConflictException, BadRequestException {
final String authToken = EnvironmentContext.getCurrent().getSubject().getToken();
Expand All @@ -202,6 +214,15 @@ protected DefaultHttpJsonResponse doRequest(
final HttpURLConnection conn = (HttpURLConnection) new URL(url).openConnection();
conn.setConnectTimeout(timeout > 0 ? timeout : 60000);
conn.setReadTimeout(timeout > 0 ? timeout : 60000);

final boolean hasHeaders = headers != null && !headers.isEmpty();
Copy link

Choose a reason for hiding this comment

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

Can you inline this into if clause?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have decided not to break existing design of the class (see processing of query params above). I can rework both if needed ?

Copy link

Choose a reason for hiding this comment

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

you are free to use the style of code you want as long as it doesn't make the code inefficient or too complex (probably empirical for maintainers values ;) ).
So my comment about this line is just matter of my taste )
Feel free to do as you want here.
And since I approved the PR in the initial state I was OK to merge it at that stage, my comments were just insignificant details.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we initialise headers = new ArrayList<>() in constructor to not to have if (headers == null) { all other the code.

Copy link
Member Author

@ibuziuk ibuziuk Dec 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@skabashnyuk I'm quite sure that the initial designer of the class who seems to be really keen on performance and even used custom capacity[1] for query list initialization would like this idea. Basically, the rule of thumb for such cases is either do a massive refactoring or follow the design that is already in place e.g [2] (even if you think it is not smth. you would normally do if implementing from scratch)

[1] https://github.com/eclipse/che/pull/7898/files#diff-570b40ab3f5e69b575585ffb160ef3b5L115
[2] https://github.com/eclipse/che/pull/7898/files#diff-570b40ab3f5e69b575585ffb160ef3b5R201

Copy link

Choose a reason for hiding this comment

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

+1 to what @ibuziuk said


if (hasHeaders) {
for (Pair<String, String> header : headers) {
conn.setRequestProperty(header.first, header.second);
}
}

try {
conn.setRequestMethod(method);
// drop a hint for server side that we want to receive application/json
Expand All @@ -225,7 +246,6 @@ protected DefaultHttpJsonResponse doRequest(
output.write(DtoFactory.getInstance().toJson(body).getBytes());
}
}

final int responseCode = conn.getResponseCode();
if ((responseCode / 100) != 2) {
InputStream in = conn.getErrorStream();
Expand Down
22 changes: 22 additions & 0 deletions core/che-core-api-core/src/main/java/org/eclipse/che/api/core/rest/HttpJsonRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,16 @@ public interface HttpJsonRequest {
*/
HttpJsonRequest addQueryParam(@NotNull String name, @NotNull Object value);

/**
* Adds header to the request.
*
* @param name header name
* @param value header value
* @return this request instance
* @throws NullPointerException when either header name or value is null
*/
HttpJsonRequest addHeader(@NotNull String name, @NotNull String value);

/**
* Adds authorization header to the request.
*
Expand Down Expand Up @@ -206,4 +216,16 @@ default HttpJsonRequest addQueryParams(@NotNull Map<String, ?> params) {
params.forEach(this::addQueryParam);
return this;
}

/**
* Adds set of headers to this request.
*
* @param headers map with headers
* @return this request instance
*/
default HttpJsonRequest addHeaders(@NotNull Map<String, String> headers) {
Objects.requireNonNull(headers, "Required non-null headers");
headers.forEach(this::addHeader);
return this;
}
}
31 changes: 26 additions & 5 deletions ...core-api-core/src/test/java/org/eclipse/che/api/core/rest/DefaultHttpJsonRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,11 @@ public void shouldUseUrlAndMethodFromTheLinks() throws Exception {
anyString(),
nullable(Object.class),
nullable(List.class),
nullable(String.class));
nullable(String.class),
nullable(List.class));
request.request();

verify(request).doRequest(0, DEFAULT_URL, "POST", null, null, null);
verify(request).doRequest(0, DEFAULT_URL, "POST", null, null, null, null);
}

@Test
Expand All @@ -119,6 +120,7 @@ public void shouldBeAbleToMakeRequest() throws Exception {
.setTimeout(10_000_000)
.addQueryParam("name", "value")
.addQueryParam("name2", "value2")
.addHeader("Connection", "close")
.request();

verify(request)
Expand All @@ -128,7 +130,8 @@ public void shouldBeAbleToMakeRequest() throws Exception {
"PUT",
body,
asList(Pair.of("name", "value"), Pair.of("name2", "value2")),
null);
null,
asList(Pair.of("Connection", "close")));
}

@Test
Expand All @@ -146,6 +149,7 @@ public void shouldBeAbleToUseStringMapAsRequestBody() throws Exception {
eq("POST"),
mapCaptor.capture(),
eq(null),
eq(null),
eq(null));
assertTrue(mapCaptor.getValue() instanceof JsonStringMap);
assertEquals(mapCaptor.getValue(), body);
Expand All @@ -164,6 +168,7 @@ public void shouldBeAbleToUseListOfJsonSerializableElementsAsRequestBody() throw
eq("POST"),
listCaptor.capture(),
eq(null),
eq(null),
eq(null));
assertTrue(listCaptor.getValue() instanceof JsonArray);
assertEquals(listCaptor.getValue(), body);
Expand All @@ -172,7 +177,7 @@ public void shouldBeAbleToUseListOfJsonSerializableElementsAsRequestBody() throw
@Test
public void defaultMethodIsGet() throws Exception {
request.request();
verify(request).doRequest(0, DEFAULT_URL, HttpMethod.GET, null, null, null);
verify(request).doRequest(0, DEFAULT_URL, HttpMethod.GET, null, null, null, null);
}

@Test(expectedExceptions = NullPointerException.class)
Expand Down Expand Up @@ -216,6 +221,21 @@ public void shouldThrowNullPointerExceptionWhenQueryParamsAreNull() throws Excep
new DefaultHttpJsonRequest("http://localhost:8080").addQueryParams(null);
}

@Test(expectedExceptions = NullPointerException.class)
public void shouldThrowNullPointerExceptionWhenHeaderNameIsNull() throws Exception {
new DefaultHttpJsonRequest("http://localhost:8080").addHeader(null, "close");
}

@Test(expectedExceptions = NullPointerException.class)
public void shouldThrowNullPointerExceptionWhenHeaderValueIsNull() throws Exception {
new DefaultHttpJsonRequest("http://localhost:8080").addHeader("Connection", null);
}

@Test(expectedExceptions = NullPointerException.class)
public void shouldThrowNullPointerExceptionWhenHeadersAreNull() throws Exception {
new DefaultHttpJsonRequest("http://localhost:8080").addHeaders(null);
}

@Test(expectedExceptions = NullPointerException.class)
public void shouldThrowNullPointerExceptionWhenLinkHrefIsNull() throws Exception {
new DefaultHttpJsonRequest(createLink("GET", null, null));
Expand Down Expand Up @@ -375,6 +395,7 @@ private void prepareResponse(String response) throws Exception {
anyString(),
nullable(Object.class),
nullable(List.class),
nullable(String.class));
nullable(String.class),
nullable(List.class));
}
}
6 changes: 5 additions & 1 deletion ...i-workspace/src/main/java/org/eclipse/che/api/agent/server/WsAgentPingRequestFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public class WsAgentPingRequestFactory {
"Workspace agent server not found in dev machine.";
private static final String WS_AGENT_URL_IS_NULL_OR_EMPTY_ERROR =
"URL of Workspace Agent is null or empty.";
private static final String CONNECTION_HEADER = "Connection";
private static final String CONNECTION_CLOSE = "close";

private final HttpJsonRequestFactory httpJsonRequestFactory;
private final int wsAgentPingConnectionTimeoutMs;
Expand Down Expand Up @@ -80,9 +82,11 @@ public HttpJsonRequest createRequest(Machine machine) throws ServerException {
if (!wsAgentPingUrl.endsWith("/")) {
wsAgentPingUrl = wsAgentPingUrl.concat("/");
}

return httpJsonRequestFactory
.fromUrl(wsAgentPingUrl)
.setMethod(HttpMethod.GET)
.setTimeout(wsAgentPingConnectionTimeoutMs);
.setTimeout(wsAgentPingConnectionTimeoutMs)
.addHeader(CONNECTION_HEADER, CONNECTION_CLOSE);
}
}
7 changes: 7 additions & 0 deletions ...rkspace/src/test/java/org/eclipse/che/api/agent/server/WsAgentPingRequestFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ public class WsAgentPingRequestFactoryTest {
private static final String WS_AGENT_SERVER_NOT_FOUND_ERROR =
"Workspace agent server not found in dev machine.";
private static final String WS_AGENT_SERVER_URL = "ws_agent";
private static final String CONNECTION_HEADER = "Connection";
private static final String CONNECTION_CLOSE = "close";

@Mock private HttpJsonRequestFactory httpJsonRequestFactory;
@Mock private HttpJsonRequest httpJsonRequest;
Expand All @@ -62,6 +64,10 @@ public void setUp() throws Exception {

when(httpJsonRequestFactory.fromUrl(anyString())).thenReturn(httpJsonRequest);
when(httpJsonRequest.setMethod(HttpMethod.GET)).thenReturn(httpJsonRequest);
when(httpJsonRequest.setTimeout(WS_AGENT_PING_CONNECTION_TIMEOUT_MS))
.thenReturn(httpJsonRequest);
when(httpJsonRequest.addHeader(CONNECTION_HEADER, CONNECTION_CLOSE))
.thenReturn(httpJsonRequest);
when(server.getProperties()).thenReturn(serverProperties);
when(serverProperties.getInternalUrl()).thenReturn(WS_AGENT_SERVER_URL);
when(devMachine.getRuntime()).thenReturn(machineRuntimeInfo);
Expand Down Expand Up @@ -105,5 +111,6 @@ public void pingRequestShouldBeCreated() throws Exception {
verify(httpJsonRequestFactory).fromUrl(WS_AGENT_SERVER_URL + '/');
verify(httpJsonRequest).setMethod(javax.ws.rs.HttpMethod.GET);
verify(httpJsonRequest).setTimeout(WS_AGENT_PING_CONNECTION_TIMEOUT_MS);
verify(httpJsonRequest).addHeader(CONNECTION_HEADER, CONNECTION_CLOSE);
}
}