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

Add an API compatibility check to Nessie clients #6818

Merged
merged 18 commits into from
May 17, 2023
2 changes: 2 additions & 0 deletions api/client/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ dependencies {
testFixturesApi(libs.undertow.servlet)
testFixturesImplementation(libs.logback.classic)

testImplementation(libs.wiremock)

intTestImplementation(libs.testcontainers.testcontainers)
intTestImplementation(libs.testcontainers.junit)
intTestImplementation(libs.testcontainers.keycloak) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,12 @@ public final class NessieConfigConstants {
public static final String CONF_FORCE_URL_CONNECTION_CLIENT =
"nessie.force-url-connection-client";

/**
* Enables API compatibility check when creating the Nessie client. The default is {@code true}.
*/
public static final String CONF_ENABLE_API_COMPATIBILITY_CHECK =
"nessie.enable-api-compatibility-check";

public static final int DEFAULT_READ_TIMEOUT_MILLIS = 25000;
public static final int DEFAULT_CONNECT_TIMEOUT_MILLIS = 5000;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package org.projectnessie.client.http;

import static org.projectnessie.client.NessieConfigConstants.CONF_CONNECT_TIMEOUT;
import static org.projectnessie.client.NessieConfigConstants.CONF_ENABLE_API_COMPATIBILITY_CHECK;
import static org.projectnessie.client.NessieConfigConstants.CONF_FORCE_URL_CONNECTION_CLIENT;
import static org.projectnessie.client.NessieConfigConstants.CONF_NESSIE_DISABLE_COMPRESSION;
import static org.projectnessie.client.NessieConfigConstants.CONF_NESSIE_HTTP_2;
Expand Down Expand Up @@ -67,6 +68,8 @@ public class HttpClientBuilder implements NessieClientBuilder<HttpClientBuilder>

private boolean tracing;

private boolean enableApiCompatibilityCheck = true;

protected HttpClientBuilder() {}

public static HttpClientBuilder builder() {
Expand Down Expand Up @@ -174,6 +177,11 @@ public HttpClientBuilder fromConfig(Function<String, String> configuration) {
withForceUrlConnectionClient(Boolean.parseBoolean(s.trim()));
}

s = configuration.apply(CONF_ENABLE_API_COMPATIBILITY_CHECK);
if (s != null) {
withEnableApiCompatibilityCheck(Boolean.parseBoolean(s));
}

return this;
}

Expand Down Expand Up @@ -304,6 +312,12 @@ public HttpClientBuilder withForceUrlConnectionClient(boolean forceUrlConnection
return this;
}

@CanIgnoreReturnValue
public HttpClientBuilder withEnableApiCompatibilityCheck(boolean enable) {
enableApiCompatibilityCheck = enable;
return this;
}

@CanIgnoreReturnValue
public HttpClientBuilder withResponseFactory(HttpResponseFactory responseFactory) {
builder.setResponseFactory(responseFactory);
Expand All @@ -323,12 +337,18 @@ public <API extends NessieApi> API build(Class<API> apiVersion) {
if (apiVersion.isAssignableFrom(HttpApiV1.class)) {
builder.setJsonView(Views.V1.class);
HttpClient httpClient = builder.build();
if (enableApiCompatibilityCheck) {
NessieApiCompatibility.check(1, httpClient);
}
return (API) new HttpApiV1(new NessieHttpClient(httpClient));
}

if (apiVersion.isAssignableFrom(HttpApiV2.class)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this if up (minor nit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this would break things: HttpApiV2 is a subclass of NessieApiV1.

builder.setJsonView(Views.V2.class);
HttpClient httpClient = builder.build();
if (enableApiCompatibilityCheck) {
NessieApiCompatibility.check(2, httpClient);
}
return (API) new HttpApiV2(httpClient);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* Copyright (C) 2023 Dremio
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.projectnessie.client.http;

import org.projectnessie.client.rest.NessieServiceException;
import org.projectnessie.model.NessieConfiguration;

public class NessieApiCompatibility {

/**
* Checks if the API version of the client is compatible with the server's.
*
* @param clientApiVersion the API version of the client
* @param httpClient the underlying HTTP client.
* @throws NessieApiCompatibilityException if the API version is not compatible.
*/
public static void check(int clientApiVersion, HttpClient httpClient)
throws NessieApiCompatibilityException {
NessieConfiguration config = fetchConfig(httpClient);
int minServerApiVersion = config.getMinSupportedApiVersion();
Copy link
Member

Choose a reason for hiding this comment

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

If min/max are not present (see below), then you can safely assume V1.
If those are present, you can safely assume that V2.
No need for any other checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not true, unfortunately.

Min and max were introduced in Nessie 0.55.0, way after the introduction of v2 in 0.46.0. So all servers between [0.46.0,0.55.0[ reply with the same payload for both versions, e.g. 0.47.0:

  • v1: {"defaultBranch":"main","maxSupportedApiVersion":2}
  • v2: {"defaultBranch":"main","maxSupportedApiVersion":2}

Starting with 0.55.0, indeed the response is different:

  • v1: {"defaultBranch":"main","maxSupportedApiVersion":2}
  • v2: {"defaultBranch":"main","minSupportedApiVersion":1,"maxSupportedApiVersion":2,"specVersion":"2.0-beta.1"}

So this is not a good indicator of the actual server version.

Copy link
Member

Choose a reason for hiding this comment

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

v2 is still in beta, we can somewhat ignore older versions w/ v2.

int maxServerApiVersion = config.getMaxSupportedApiVersion();
if (clientApiVersion < minServerApiVersion || clientApiVersion > maxServerApiVersion) {
throw new NessieApiCompatibilityException(
clientApiVersion, minServerApiVersion, maxServerApiVersion);
}
int actualServerApiVersion = fetchActualServerApiVersion(httpClient, config);
if (clientApiVersion != actualServerApiVersion) {
throw new NessieApiCompatibilityException(
clientApiVersion, minServerApiVersion, maxServerApiVersion, actualServerApiVersion);
}
}

private static NessieConfiguration fetchConfig(HttpClient httpClient) {
return httpClient.newRequest().path("config").get().readEntity(NessieConfiguration.class);
Copy link
Member

Choose a reason for hiding this comment

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

You'd actually have to read it into a Map<String, Object>, because newer servers may return more elements, which would breal this check.

}

private static int fetchActualServerApiVersion(
Copy link
Member

Choose a reason for hiding this comment

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

This check requires that authz works for you on that branch.
Also not sure whether it's really worth to run some arbitrary request as a guess implying an API version.

Copy link
Member

Choose a reason for hiding this comment

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

Better to return the "effective" version (the one that was used on the server side) via NessieConfiguration

HttpClient httpClient, NessieConfiguration config) {
try {
httpClient
.newRequest()
.path("trees/tree/{branch}")
.resolveTemplate("branch", config.getDefaultBranch())
.get();
return 1;
} catch (NessieServiceException e) {
// In theory, we could test if the status code is 404; but unfortunately on Jersey,
// the 404 error arrives wrapped in a status code 500.
return 2;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
* Copyright (C) 2023 Dremio
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.projectnessie.client.http;

public class NessieApiCompatibilityException extends RuntimeException {

private final int clientApiVersion;
private final int minServerApiVersion;
private final int maxServerApiVersion;
private final int actualServerApiVersion;

public NessieApiCompatibilityException(
int clientApiVersion, int minServerApiVersion, int maxServerApiVersion) {
this(clientApiVersion, minServerApiVersion, maxServerApiVersion, 0);
}

public NessieApiCompatibilityException(
int clientApiVersion,
int minServerApiVersion,
int maxServerApiVersion,
int actualServerApiVersion) {
super(
formatMessage(
clientApiVersion, minServerApiVersion, maxServerApiVersion, actualServerApiVersion));
this.clientApiVersion = clientApiVersion;
this.minServerApiVersion = minServerApiVersion;
this.maxServerApiVersion = maxServerApiVersion;
this.actualServerApiVersion = actualServerApiVersion;
}

private static String formatMessage(
int clientApiVersion,
int minServerApiVersion,
int maxServerApiVersion,
int actualServerApiVersion) {
if (clientApiVersion < minServerApiVersion) {
return String.format(
"API version %d is too old for server (minimum supported version is %d)",
clientApiVersion, minServerApiVersion);
}
if (clientApiVersion > maxServerApiVersion) {
return String.format(
"API version %d is too new for server (maximum supported version is %d)",
clientApiVersion, maxServerApiVersion);
}
return String.format(
"API version mismatch, check URI prefix (expected: %d, actual: %d)",
clientApiVersion, actualServerApiVersion);
}

public int getClientApiVersion() {
return clientApiVersion;
}

public int getMinServerApiVersion() {
return minServerApiVersion;
}

public int getMaxServerApiVersion() {
return maxServerApiVersion;
}

public int getActualServerApiVersion() {
return actualServerApiVersion;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ static HttpTestServer.RequestHandler handlerForHeaderTest(
return (req, resp) -> {
receiver.set(req.getHeader(headerName));
req.getInputStream().close();
HttpTestUtil.writeResponseBody(resp, "{\"maxSupportedApiVersion\":1}");
HttpTestUtil.writeResponseBody(
resp, "{\"maxSupportedApiVersion\":1,\"defaultBranch\":\"main\"}");
};
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
* Copyright (C) 2023 Dremio
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.projectnessie.client.http;

import static com.github.tomakehurst.wiremock.client.WireMock.get;
import static com.github.tomakehurst.wiremock.client.WireMock.notFound;
import static com.github.tomakehurst.wiremock.client.WireMock.ok;
import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.InstanceOfAssertFactories.type;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.github.tomakehurst.wiremock.client.ResponseDefinitionBuilder;
import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
import com.github.tomakehurst.wiremock.junit5.WireMockTest;
import java.net.URI;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.mockito.junit.jupiter.MockitoExtension;
import org.projectnessie.client.rest.NessieHttpResponseFilter;
import org.projectnessie.model.ImmutableNessieConfiguration;
import org.projectnessie.model.NessieConfiguration;

@ExtendWith(MockitoExtension.class)
@WireMockTest
class TestNessieApiCompatibility {

enum Expectation {
OK,
TOO_OLD,
TOO_NEW,
MISMATCH
}

@ParameterizedTest
@CsvSource(
value = {
"1, 1, 1, 1, OK",
"1, 1, 2, 1, OK",
"1, 1, 2, 2, MISMATCH", // v2 endpoint mistakenly called with v1 client
"1, 2, 2, 0, TOO_OLD",
"2, 1, 1, 0, TOO_NEW",
"2, 1, 2, 1, MISMATCH", // v1 endpoint mistakenly called with v2 client
"2, 1, 2, 2, OK",
"2, 2, 2, 2, OK",
})
void checkApiCompatibility(
int client,
int serverMin,
int serverMax,
int serverActual,
Expectation expectation,
WireMockRuntimeInfo wireMock) {

NessieConfiguration config =
ImmutableNessieConfiguration.builder()
.minSupportedApiVersion(serverMin)
.maxSupportedApiVersion(serverMax)
.defaultBranch("main")
.build();

stubFor(get("/config").willReturn(ResponseDefinitionBuilder.okForJson(config)));
stubFor(get("/trees/tree/main").willReturn(serverActual == 1 ? ok() : notFound()));

try (HttpClient httpClient =
HttpClient.builder()
.setBaseUri(URI.create(wireMock.getHttpBaseUrl()))
.setObjectMapper(new ObjectMapper())
.addResponseFilter(new NessieHttpResponseFilter())
.build()) {

if (expectation == Expectation.OK) {

assertThatCode(() -> NessieApiCompatibility.check(client, httpClient))
.doesNotThrowAnyException();

} else {

assertThatThrownBy(() -> NessieApiCompatibility.check(client, httpClient))
.hasMessageContaining(
expectation == Expectation.MISMATCH
? "mismatch"
: expectation == Expectation.TOO_OLD ? "too old" : "too new")
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: put too old, etc. into enum object fields in Expectation and use smth like expectation.getExpectedText()

.asInstanceOf(type(NessieApiCompatibilityException.class))
.extracting(
NessieApiCompatibilityException::getClientApiVersion,
NessieApiCompatibilityException::getMinServerApiVersion,
NessieApiCompatibilityException::getMaxServerApiVersion,
NessieApiCompatibilityException::getActualServerApiVersion)
.containsExactly(client, serverMin, serverMax, serverActual);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright (C) 2023 Dremio
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.projectnessie.client.http;

import static org.assertj.core.api.Assertions.assertThat;

import org.junit.jupiter.api.Test;

class TestNessieApiCompatibilityException {

@Test
void testMessages() {
NessieApiCompatibilityException e = new NessieApiCompatibilityException(1, 2, 3);
assertThat(e.getMessage())
.isEqualTo("API version 1 is too old for server (minimum supported version is 2)");
e = new NessieApiCompatibilityException(5, 3, 4);
assertThat(e.getMessage())
.isEqualTo("API version 5 is too new for server (maximum supported version is 4)");
e = new NessieApiCompatibilityException(3, 2, 4, 2);
assertThat(e.getMessage())
.isEqualTo("API version mismatch, check URI prefix (expected: 3, actual: 2)");
}

@Test
void testGetters() {
NessieApiCompatibilityException e = new NessieApiCompatibilityException(1, 2, 4, 3);
assertThat(e.getClientApiVersion()).isEqualTo(1);
assertThat(e.getMinServerApiVersion()).isEqualTo(2);
assertThat(e.getMaxServerApiVersion()).isEqualTo(4);
assertThat(e.getActualServerApiVersion()).isEqualTo(3);
}
}
Loading