Skip to content

Commit 2048a9b

Browse files
Abhi347Abhi347
committed
fix: Fix all tests
1 parent 4f4ad39 commit 2048a9b

File tree

9 files changed

+90
-106
lines changed

9 files changed

+90
-106
lines changed

‎src/main/java/com/spotify/github/http/okhttp/OkHttpHttpClient.java‎

Lines changed: 28 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -60,74 +60,31 @@ public OkHttpHttpClient(final OkHttpClient client, final Tracer tracer) {
6060
public CompletableFuture<HttpResponse> send(final HttpRequest httpRequest) {
6161
Request request = buildRequest(httpRequest);
6262
CompletableFuture<HttpResponse> future = new CompletableFuture<>();
63-
if (this.callFactory == null) {
64-
this.callFactory = createTracedClient();
63+
try (Span span = tracer.span(httpRequest)) {
64+
if (this.callFactory == null) {
65+
this.callFactory = createTracedClient();
66+
}
67+
this.callFactory
68+
.newCall(request)
69+
.enqueue(
70+
new Callback() {
71+
72+
@Override
73+
public void onResponse(@NotNull final Call call, @NotNull final Response response)
74+
throws IOException {
75+
future.complete(new OkHttpHttpResponse(httpRequest, response));
76+
}
77+
78+
@Override
79+
public void onFailure(@NotNull final Call call, @NotNull final IOException e) {
80+
future.completeExceptionally(e);
81+
}
82+
});
83+
tracer.attachSpanToFuture(span, future);
6584
}
66-
this.callFactory
67-
.newCall(request)
68-
.enqueue(
69-
new Callback() {
70-
71-
@Override
72-
public void onResponse(@NotNull final Call call, @NotNull final Response response)
73-
throws IOException {
74-
future.complete(new OkHttpHttpResponse(httpRequest, response));
75-
}
76-
77-
@Override
78-
public void onFailure(@NotNull final Call call, @NotNull final IOException e) {
79-
future.completeExceptionally(e);
80-
}
81-
});
8285
return future;
8386
}
8487

85-
// try (Span span = tracer.span(request)) {
86-
// if (this.callFactory == null) {
87-
// this.callFactory = this.tracer.createTracedClient(this.client);
88-
// }
89-
// final Call call = this.callFactory.newCall(request);
90-
//
91-
// final CompletableFuture<Response> future = new CompletableFuture<>();
92-
//
93-
// // avoid multiple redirects
94-
// final AtomicBoolean redirected = new AtomicBoolean(false);
95-
//
96-
// call.enqueue(
97-
// new Callback() {
98-
// @Override
99-
// public void onFailure(@NotNull final Call call, final IOException e) {
100-
// future.completeExceptionally(e);
101-
// }
102-
//
103-
// @Override
104-
// public void onResponse(@NotNull final Call call, final Response response) {
105-
// processPossibleRedirects(response, redirected)
106-
// .handle(
107-
// (res, ex) -> {
108-
// if (Objects.nonNull(ex)) {
109-
// future.completeExceptionally(ex);
110-
// } else if (!res.isSuccessful()) {
111-
// try {
112-
// future.completeExceptionally(mapException(res, request));
113-
// } catch (final Throwable e) {
114-
// future.completeExceptionally(e);
115-
// } finally {
116-
// if (res.body() != null) {
117-
// res.body().close();
118-
// }
119-
// }
120-
// } else {
121-
// future.complete(res);
122-
// }
123-
// return res;
124-
// });
125-
// }
126-
// });
127-
// tracer.attachSpanToFuture(span, future);
128-
// return future;
129-
// }
130-
13188
@Override
13289
public void setTracer(final Tracer tracer) {
13390
this.tracer = tracer;
@@ -142,9 +99,13 @@ private Request buildRequest(final HttpRequest request) {
14299
(key, values) -> {
143100
values.forEach(value -> requestBuilder.addHeader(key, value));
144101
});
145-
requestBuilder.method(
146-
request.method(),
147-
RequestBody.create(parse(javax.ws.rs.core.MediaType.APPLICATION_JSON), request.body()));
102+
if (request.method().equals("GET")) {
103+
requestBuilder.get();
104+
} else {
105+
requestBuilder.method(
106+
request.method(),
107+
RequestBody.create(parse(javax.ws.rs.core.MediaType.APPLICATION_JSON), request.body()));
108+
}
148109
return requestBuilder.build();
149110
}
150111

‎src/main/java/com/spotify/github/http/okhttp/OkHttpHttpResponse.java‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,18 @@
2525
import java.io.IOException;
2626
import java.io.InputStream;
2727
import java.io.UncheckedIOException;
28+
import java.lang.invoke.MethodHandles;
2829
import java.util.List;
2930
import java.util.Map;
3031
import java.util.Optional;
3132

3233
import okhttp3.Response;
3334
import okhttp3.ResponseBody;
35+
import org.slf4j.Logger;
36+
import org.slf4j.LoggerFactory;
3437

3538
public class OkHttpHttpResponse implements HttpResponse {
39+
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
3640
private static final int HTTP_OK = 200;
3741
private static final int HTTP_BAD_REQUEST = 400;
3842

@@ -95,11 +99,15 @@ public boolean isSuccessful() {
9599

96100
@Override
97101
public void close() {
98-
if (response != null) {
99-
response.close();
100-
if (response.body() != null) {
101-
response.body().close();
102+
try {
103+
if (response != null) {
104+
if (response.body() != null) {
105+
response.body().close();
106+
}
107+
response.close();
102108
}
109+
} catch (IllegalStateException e) {
110+
log.debug("Failed closing response: {}", e.getMessage());
103111
}
104112
}
105113

‎src/main/java/com/spotify/github/tracing/opentelemetry/OpenTelemetryTracer.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,11 @@ public String get(
118118
if (carrier == null) {
119119
return null;
120120
}
121-
return carrier.headers().get(key).get(0);
121+
if (carrier.headers().containsKey(key)
122+
&& !carrier.headers().get(key).isEmpty()) {
123+
return carrier.headers().get(key).get(0);
124+
}
125+
return null;
122126
}
123127
});
124128
context.makeCurrent();

‎src/main/java/com/spotify/github/v3/clients/GitHubClient.java‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,7 @@
5151
import java.lang.invoke.MethodHandles;
5252
import java.net.URI;
5353
import java.time.ZonedDateTime;
54-
import java.util.List;
55-
import java.util.Map;
56-
import java.util.Objects;
57-
import java.util.Optional;
54+
import java.util.*;
5855
import java.util.concurrent.CompletableFuture;
5956
import java.util.concurrent.CompletionStage;
6057
import java.util.concurrent.ConcurrentHashMap;
@@ -611,8 +608,7 @@ CompletableFuture<HttpResponse> request(final String path) {
611608
CompletableFuture<HttpResponse> request(
612609
final String path, final Map<String, String> extraHeaders) {
613610
final ImmutableHttpRequest.Builder builder = requestBuilder(path);
614-
toHttpRequestHeaders(builder, extraHeaders);
615-
final HttpRequest request = builder.build();
611+
final HttpRequest request = toHttpRequestHeaders(builder, extraHeaders).build();
616612
log.debug("Making request to {}", request.url().toString());
617613
return call(request);
618614
}
@@ -640,9 +636,7 @@ <T> CompletableFuture<T> request(final String path, final Class<T> clazz) {
640636
<T> CompletableFuture<T> request(
641637
final String path, final Class<T> clazz, final Map<String, String> extraHeaders) {
642638
final ImmutableHttpRequest.Builder builder = requestBuilder(path);
643-
extraHeaders.forEach(
644-
(headerKey, headerValue) -> builder.putHeaders(headerKey, List.of(headerValue)));
645-
final HttpRequest request = builder.build();
639+
final HttpRequest request = toHttpRequestHeaders(builder, extraHeaders).build();
646640
log.debug("Making request to {}", request.url().toString());
647641
return call(request)
648642
.thenApply(response -> json().fromJsonUncheckedNotNull(response.bodyString(), clazz));
@@ -660,8 +654,7 @@ <T> CompletableFuture<T> request(
660654
final TypeReference<T> typeReference,
661655
final Map<String, String> extraHeaders) {
662656
final ImmutableHttpRequest.Builder builder = requestBuilder(path);
663-
toHttpRequestHeaders(builder, extraHeaders);
664-
final HttpRequest request = builder.build();
657+
final HttpRequest request = toHttpRequestHeaders(builder, extraHeaders).build();
665658
log.debug("Making request to {}", request.url().toString());
666659
return call(request)
667660
.thenApply(
@@ -706,8 +699,7 @@ CompletableFuture<HttpResponse> post(final String path, final String data) {
706699
CompletableFuture<HttpResponse> post(
707700
final String path, final String data, final Map<String, String> extraHeaders) {
708701
final ImmutableHttpRequest.Builder builder = requestBuilder(path).method("POST").body(data);
709-
toHttpRequestHeaders(builder, extraHeaders);
710-
final HttpRequest request = builder.build();
702+
final HttpRequest request = toHttpRequestHeaders(builder, extraHeaders).build();
711703
log.debug("Making POST request to {}", request.url().toString());
712704
return call(request);
713705
}
@@ -823,8 +815,7 @@ <T> CompletableFuture<T> patch(
823815
final Class<T> clazz,
824816
final Map<String, String> extraHeaders) {
825817
final ImmutableHttpRequest.Builder builder = requestBuilder(path).method("PATCH").body(data);
826-
toHttpRequestHeaders(builder, extraHeaders);
827-
final HttpRequest request = builder.build();
818+
final HttpRequest request = toHttpRequestHeaders(builder, extraHeaders).build();
828819
log.debug("Making PATCH request to {}", request.url().toString());
829820
return call(request)
830821
.thenApply(response -> json().fromJsonUncheckedNotNull(response.bodyString(), clazz));
@@ -865,16 +856,29 @@ String urlFor(final String path) {
865856
return baseUrl.toString().replaceAll("/+$", "") + "/" + path.replaceAll("^/+", "");
866857
}
867858

868-
private void toHttpRequestHeaders(
859+
private ImmutableHttpRequest.Builder toHttpRequestHeaders(
869860
final ImmutableHttpRequest.Builder builder, final Map<String, String> extraHeaders) {
861+
HttpRequest request = builder.build();
862+
870863
extraHeaders.forEach(
871-
(headerKey, headerValue) -> builder.putHeaders(headerKey, List.of(headerValue)));
864+
(headerKey, headerValue) -> {
865+
if (request.headers().containsKey(headerKey)) {
866+
List<String> headers = new ArrayList<>(request.headers().get(headerKey));
867+
headers.add(headerValue);
868+
builder.putHeaders(headerKey, headers);
869+
} else {
870+
builder.putHeaders(headerKey, List.of(headerValue));
871+
}
872+
});
873+
return builder;
872874
}
873875

874876
private ImmutableHttpRequest.Builder requestBuilder(final String path) {
875877

876878
return ImmutableHttpRequest.builder()
877879
.url(urlFor(path))
880+
.method("GET")
881+
.body("")
878882
.putHeaders(HttpHeaders.ACCEPT, List.of(MediaType.APPLICATION_JSON))
879883
.putHeaders(HttpHeaders.CONTENT_TYPE, List.of(MediaType.APPLICATION_JSON))
880884
.putHeaders(HttpHeaders.AUTHORIZATION, List.of(getAuthorizationHeader(path)));
@@ -1025,15 +1029,19 @@ private RequestNotOkException mapException(
10251029
if (bodyString.contains("Repository was archived so is read-only")) {
10261030
return new ReadOnlyRepositoryException(
10271031
httpRequest.method(),
1028-
httpRequest.url(),
1032+
URI.create(httpRequest.url()).getPath(),
10291033
httpResponse.statusCode(),
10301034
bodyString,
10311035
headersMap);
10321036
}
10331037
}
10341038

10351039
return new RequestNotOkException(
1036-
httpRequest.method(), httpRequest.url(), httpResponse.statusCode(), bodyString, headersMap);
1040+
httpRequest.method(),
1041+
URI.create(httpRequest.url()).getPath(),
1042+
httpResponse.statusCode(),
1043+
bodyString,
1044+
headersMap);
10371045
}
10381046

10391047
CompletableFuture<HttpResponse> processPossibleRedirects(

‎src/main/java/com/spotify/github/v3/clients/RepositoryClient.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,7 @@ private CompletableFuture<Optional<InputStream>> downloadRepository(
308308
final String path, final Optional<String> maybeRef) {
309309
final var repoRef = maybeRef.orElse("");
310310
final var repoPath = String.format(path, owner, repo, repoRef);
311-
return github.request(repoPath).thenApply(response -> Optional.of(response.body()));
311+
return github.request(repoPath).thenApply(response -> Optional.ofNullable(response.body()));
312312
}
313313

314314
/**

‎src/test/java/com/spotify/github/tracing/OpenTelemetryTracerTest.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
package com.spotify.github.tracing;
2222

2323
import com.spotify.github.http.HttpRequest;
24+
import com.spotify.github.http.ImmutableHttpRequest;
2425
import com.spotify.github.tracing.opentelemetry.OpenTelemetryTracer;
2526
import io.opentelemetry.api.GlobalOpenTelemetry;
2627
import io.opentelemetry.api.OpenTelemetry;
@@ -45,6 +46,7 @@
4546
import java.io.IOException;
4647
import java.util.LinkedList;
4748
import java.util.List;
49+
import java.util.Map;
4850
import java.util.concurrent.CompletableFuture;
4951

5052
import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -124,10 +126,13 @@ public void traceCompletionStageWithRequest(final String requestMethod) throws E
124126
Span rootSpan = startRootSpan();
125127
final CompletableFuture<String> future = new CompletableFuture<>();
126128
OpenTelemetryTracer tracer = new OpenTelemetryTracer();
127-
HttpRequest mockRequest = mock(HttpRequest.class);
128-
when(mockRequest.url())
129-
.thenReturn("https://api.github.com/repos/spotify/github-java-client");
130-
when(mockRequest.method()).thenReturn(requestMethod);
129+
HttpRequest mockRequest =
130+
ImmutableHttpRequest.builder()
131+
.url("https://api.github.com/repos/spotify/github-java-client")
132+
.method(requestMethod)
133+
.body("")
134+
.headers(Map.of())
135+
.build();
131136

132137
try (com.spotify.github.tracing.Span span = tracer.span(mockRequest)) {
133138
tracer.attachSpanToFuture(span, future);

‎src/test/java/com/spotify/github/v3/clients/MockHelper.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -66,12 +66,15 @@ public String statusMessage() {
6666

6767
@Override
6868
public InputStream body() {
69-
return new ByteArrayInputStream(body.getBytes(StandardCharsets.UTF_8));
69+
if (body != null) {
70+
return new ByteArrayInputStream(body.getBytes(StandardCharsets.UTF_8));
71+
}
72+
return null;
7073
}
7174

7275
@Override
7376
public String bodyString() {
74-
return body;
77+
return Optional.ofNullable(body).orElse("");
7578
}
7679

7780
@Override
@@ -85,9 +88,7 @@ public boolean isSuccessful() {
8588
}
8689

8790
@Override
88-
public void close() {
89-
90-
}
91+
public void close() {}
9192
};
9293
}
9394

‎src/test/java/com/spotify/github/v3/clients/PullRequestClientTest.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
import com.google.common.io.Resources;
3333
import com.spotify.github.v3.exceptions.RequestNotOkException;
3434
import com.spotify.github.v3.prs.ImmutableRequestReviewParameters;
35-
import com.spotify.github.v3.prs.MergeMethod;
3635
import com.spotify.github.v3.prs.PullRequest;
3736
import com.spotify.github.v3.prs.ReviewRequests;
3837
import com.spotify.github.v3.prs.requests.ImmutablePullRequestCreate;
@@ -44,9 +43,7 @@
4443
import java.net.URI;
4544
import java.util.concurrent.CompletableFuture;
4645
import java.util.concurrent.ExecutionException;
47-
4846
import okhttp3.*;
49-
import okio.Buffer;
5047
import org.apache.commons.io.IOUtils;
5148
import org.junit.jupiter.api.BeforeEach;
5249
import org.junit.jupiter.api.Test;

‎src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -642,7 +642,7 @@ public void createFork() throws IOException {
642642
@Test
643643
public void mergeNoop() {
644644
CompletableFuture<HttpResponse> okResponse =
645-
completedFuture(createMockHttpResponse("http://example.com/whatever", 202, "", Map.of()));
645+
completedFuture(createMockHttpResponse("http://example.com/whatever", 204, null, Map.of()));
646646
when(github.post(any(), any())).thenReturn(okResponse);
647647
final Optional<CommitItem> maybeCommit = repoClient.merge("basebranch", "headbranch").join();
648648
assertThat(maybeCommit, is(Optional.empty()));
@@ -685,7 +685,7 @@ public void shouldDownloadZipball() throws Exception {
685685
@Test
686686
public void shouldReturnEmptyOptionalWhenResponseBodyNotPresent() throws Exception {
687687
CompletableFuture<HttpResponse> fixture =
688-
completedFuture(createMockHttpResponse("http://example.com/whatever", 204, "", Map.of()));
688+
completedFuture(createMockHttpResponse("http://example.com/whatever", 204, null, Map.of()));
689689
when(github.request("/repos/someowner/somerepo/zipball/master")).thenReturn(fixture);
690690

691691
Optional<InputStream> response = repoClient.downloadZipball("master").get();

0 commit comments

Comments
 (0)