Skip to content

Commit

Permalink
Do not compare request methods and response statuses by reference (#913)
Browse files Browse the repository at this point in the history
Motivation:

`HttpRequestMethod` and `HttpResponseStatus` are classes with static
constants, not enums. We should use `equals` to compare them. Otherwise,
users' implementation of these classes won't work correctly.

Modifications:

- Replace all `==` and `!=` for `HttpRequestMetaData.method()` and
`HttpResponseMetaData.status()` with `equals`;

Result:

Correct comparison of `HttpRequestMethod` and `HttpResponseStatus`
objects.
  • Loading branch information
idelpivnitskiy committed Jan 8, 2020
1 parent e137e41 commit 15dbe98
Show file tree
Hide file tree
Showing 4 changed files with 5 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ private GrpcUtils() {
}

static void initRequest(final HttpRequestMetaData request) {
assert request.method() == POST;
assert POST.equals(request.method());
final HttpHeaders headers = request.headers();
headers.set(USER_AGENT, GRPC_USER_AGENT);
headers.set(TE, TRAILERS);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import static io.servicetalk.http.api.HttpHeaderNames.TRANSFER_ENCODING;
import static io.servicetalk.http.api.HttpHeaderValues.CHUNKED;
import static io.servicetalk.http.api.HttpProtocolVersion.HTTP_1_0;
import static io.servicetalk.http.api.HttpRequestMethod.HEAD;
import static io.servicetalk.http.api.HttpResponseStatus.OK;
import static io.servicetalk.http.api.StreamingHttpResponses.newTransportResponse;
import static java.lang.Thread.currentThread;
Expand Down Expand Up @@ -106,7 +107,7 @@ protected void handleSubscribe(final Subscriber<? super StreamingHttpResponse> s
// breaks our HttpResponseDecoder
// TODO(jayv) we can provide an optimized PayloadWriter for HEAD response that
// immediately completes on sendMeta() and disallows further writes or trailers
request.method() != HttpRequestMethod.HEAD) {
!HEAD.equals(request.method())) {
// this is likely not supported in http/1.0 and it is possible that a response has
// neither header and the connection close indicates the end of the response.
// https://tools.ietf.org/html/rfc7230#section-3.3.3
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,7 @@ private static int getWebSocketContentLength(HttpMetaData message) {
HttpRequestMetaData req = (HttpRequestMetaData) message;
// Note that we are using ServiceTalk constants for HttpRequestMethod here, and assume the decoders will
// also use ServiceTalk constants which allows us to use reference check here:
if (req.method() == GET &&
if (GET.equals(req.method()) &&
h.contains(SEC_WEBSOCKET_KEY1) &&
h.contains(SEC_WEBSOCKET_KEY2)) {
return 8;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,6 @@ private String getSecurityContextJson(final String path,
final HttpResponse res = httpClient.request(req);
assertThat(res.status(), is(expectedStatus));

return res.status() == OK ? res.payloadBody().toString(UTF_8) : null;
return OK.equals(res.status()) ? res.payloadBody().toString(UTF_8) : null;
}
}

0 comments on commit 15dbe98

Please sign in to comment.