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

Allow per-request base URIs in HttpClient #7922

Merged
merged 7 commits into from
Jan 9, 2024
Merged

Allow per-request base URIs in HttpClient #7922

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
be10518
Allow per-request base URIs in HttpClient
adutra Jan 8, 2024
6e55571
Code review
adutra Jan 8, 2024
0e13702
Try to bypass build cache preparation
adutra Jan 8, 2024
3143891
Try to bypass build cache preparation
adutra Jan 8, 2024
0f8f8b3
Revert
adutra Jan 8, 2024
104a5d6
Review
adutra Jan 8, 2024
7411103
Revert
adutra Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/client/src/main/java/org/projectnessie/client/http/HttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@ enum Method {

HttpRequest newRequest();

HttpRequest newRequest(URI baseUri);

static Builder builder() {
return new HttpClientBuilderImpl();
}

URI getBaseUri();
Copy link
Member

Choose a reason for hiding this comment

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

Please leave this one


@Override
void close();

Expand Down
2 changes: 0 additions & 2 deletions api/client/src/main/java/org/projectnessie/client/http/HttpClientBuilderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,6 @@ public HttpClient.Builder addTracing() {

@Override
public HttpClient build() {
HttpUtils.checkArgument(
baseUri != null, "Cannot construct Http client. Must have a non-null uri");
HttpUtils.checkArgument(
mapper != null, "Cannot construct Http client. Must have a non-null object mapper");
if (sslContext == null) {
Expand Down
15 changes: 14 additions & 1 deletion api/client/src/main/java/org/projectnessie/client/http/HttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
*/
package org.projectnessie.client.http;

import static java.util.Objects.requireNonNull;
import static org.projectnessie.client.http.impl.HttpUtils.checkArgument;

import java.net.URI;
import org.projectnessie.client.http.HttpClient.Method;
import org.projectnessie.client.http.impl.HttpHeaders;
import org.projectnessie.client.http.impl.HttpRuntimeConfig;
Expand All @@ -31,7 +35,16 @@ public abstract class HttpRequest
protected String accept = "application/json; charset=utf-8";

protected HttpRequest(HttpRuntimeConfig config) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can get rid of the single-arg constructor and only use the new one below. WDYT?

this.uriBuilder = new UriBuilder(config.getBaseUri());
this(config, config.getBaseUri());
}

protected HttpRequest(HttpRuntimeConfig config, URI baseUri) {
requireNonNull(baseUri, "Base URI cannot be null");
checkArgument(
"http".equals(baseUri.getScheme()) || "https".equals(baseUri.getScheme()),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe equalsIgnoreCase? Per rfc3986 implementations should accept either case.

"Base URI must be a valid http or https address: %s",
baseUri);
this.uriBuilder = new UriBuilder(baseUri);
this.config = config;

int clientSpec = config.getClientSpec();
Expand Down
5 changes: 5 additions & 0 deletions api/client/src/main/java/org/projectnessie/client/http/impl/BaseHttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.fasterxml.jackson.databind.ObjectWriter;
import java.io.IOException;
import java.io.OutputStream;
import java.net.URI;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.Map;
Expand All @@ -47,6 +48,10 @@ protected BaseHttpRequest(HttpRuntimeConfig config) {
super(config);
}

protected BaseHttpRequest(HttpRuntimeConfig config, URI baseUri) {
super(config, baseUri);
}

protected boolean prepareRequest(RequestContext context) {
headers.put(HEADER_ACCEPT, accept);

Expand Down
12 changes: 8 additions & 4 deletions api/client/src/main/java/org/projectnessie/client/http/impl/HttpRuntimeConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,17 @@ static ImmutableHttpRuntimeConfig.Builder builder() {
@Value.Check
default void check() {
URI baseUri = getBaseUri();
if (!"http".equals(baseUri.getScheme()) && !"https".equals(baseUri.getScheme())) {
throw new IllegalArgumentException(
String.format(
"Cannot start http client. %s must be a valid http or https address", baseUri));
if (baseUri != null) {
if (!"http".equals(baseUri.getScheme()) && !"https".equals(baseUri.getScheme())) {
throw new IllegalArgumentException(
String.format(
"Cannot start http client. %s must be a valid http or https address", baseUri));
}
}
}

@Nullable
@jakarta.annotation.Nullable
URI getBaseUri();

ObjectMapper getMapper();
Expand Down
6 changes: 3 additions & 3 deletions api/client/src/main/java/org/projectnessie/client/http/impl/jdk11/JavaHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,12 @@ public JavaHttpClient(HttpRuntimeConfig config) {

@Override
public HttpRequest newRequest() {
return new JavaRequest(this.config, (req, handler) -> client.send(req, handler));
return new JavaRequest(this.config, client::send);
}

@Override
public URI getBaseUri() {
return config.getBaseUri();
public HttpRequest newRequest(URI baseUri) {
return new JavaRequest(this.config, baseUri, client::send);
}

@Override
Expand Down
5 changes: 5 additions & 0 deletions api/client/src/main/java/org/projectnessie/client/http/impl/jdk11/JavaRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ HttpResponse<T> send(HttpRequest request, HttpResponse.BodyHandler<T> responseBo
this.exchange = exchange;
}

JavaRequest(HttpRuntimeConfig config, URI baseUri, HttpExchange<InputStream> exchange) {
super(config, baseUri);
this.exchange = exchange;
}

@Override
public org.projectnessie.client.http.HttpResponse executeRequest(Method method, Object body)
throws HttpClientException {
Expand Down
4 changes: 2 additions & 2 deletions api/client/src/main/java/org/projectnessie/client/http/impl/jdk8/UrlConnectionClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ public HttpRequest newRequest() {
}

@Override
public URI getBaseUri() {
return config.getBaseUri();
public HttpRequest newRequest(URI baseUri) {
return new UrlConnectionRequest(config, baseUri);
}

@Override
Expand Down
4 changes: 4 additions & 0 deletions api/client/src/main/java/org/projectnessie/client/http/impl/jdk8/UrlConnectionRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ final class UrlConnectionRequest extends BaseHttpRequest {
super(config);
}

UrlConnectionRequest(HttpRuntimeConfig config, URI baseUri) {
super(config, baseUri);
}

@Override
public HttpResponse executeRequest(Method method, Object body) throws HttpClientException {
URI uri = uriBuilder.build();
Expand Down
35 changes: 35 additions & 0 deletions api/client/src/test/java/org/projectnessie/client/http/TestHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,41 @@ void testGet() throws Exception {
}
}

@Test
void testPerRequestBaseUri() throws Exception {
ExampleBean inputBean = new ExampleBean("x", 1, NOW);
HttpTestServer.RequestHandler handler =
(req, resp) -> {
soft.assertThat(req.getMethod()).isEqualTo("GET");
String response = MAPPER.writeValueAsString(inputBean);
writeResponseBody(resp, response);
};
try (HttpTestServer server = new HttpTestServer(handler)) {
try (HttpClient client = createClient(null, b -> {})) {
ExampleBean bean = client.newRequest(server.getUri()).get().readEntity(ExampleBean.class);
soft.assertThat(bean).isEqualTo(inputBean);
}
}
}

@Test
void testNullPerRequestBaseUri() {
try (HttpClient client = createClient(null, b -> {})) {
assertThatThrownBy(() -> client.newRequest(null))
.isInstanceOf(NullPointerException.class)
.hasMessage("Base URI cannot be null");
}
}

@Test
void testInvalidPerRequestBaseUri() {
try (HttpClient client = createClient(null, b -> {})) {
assertThatThrownBy(() -> client.newRequest(URI.create("file:///foo/bar/baz")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Base URI must be a valid http or https address");
}
}

@Test
void testPut() throws Exception {
ExampleBean inputBean = new ExampleBean("x", 1, NOW);
Expand Down
18 changes: 0 additions & 18 deletions api/client/src/test/java/org/projectnessie/client/http/TestHttpClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,24 +69,6 @@ void testIncompatibleAuthProvider() {
.hasMessage("HttpClientBuilder only accepts instances of HttpAuthentication");
}

@Test
void testNullUri() {
assertThatThrownBy(
() ->
createClientBuilderFromSystemSettings()
.withUri((URI) null)
.build(NessieApiV1.class))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot construct Http client. Must have a non-null uri");
}

@Test
void testNoUri() {
assertThatThrownBy(() -> createClientBuilderFromSystemSettings().build(NessieApiV1.class))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot construct Http client. Must have a non-null uri");
}

@Test
void testInvalidUriScheme() {
assertThatThrownBy(
Expand Down