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

Fix URL endpoint for injected NessieApi from OldNessieServer #7958

Merged
merged 5 commits into from
Jan 16, 2024
Merged

Fix URL endpoint for injected NessieApi from OldNessieServer #7958

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a6fe710
Add failing test injectedNessieApiUrlChanged
XN137 Jan 16, 2024
eee45f1
fix test by making resolveNessieUri version aware
XN137 Jan 16, 2024
ed94a2c
adjustment comments
XN137 Jan 16, 2024
ba05a2a
rename new version constant
XN137 Jan 16, 2024
4f67113
use assertNoFailedTestEvents where applicable
XN137 Jan 16, 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
3 changes: 3 additions & 0 deletions compatibility/common/src/main/java/org/projectnessie/tools/compatibility/api/Version.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ public class Version implements Comparable<Version> {
/** See <a href="https://github.com/projectnessie/nessie/pull/6894">PR #6894</a>. */
public static final Version MERGE_KEY_BEHAVIOR_FIX = Version.parseVersion("0.59.1");

/** see <a href="https://github.com/projectnessie/nessie/pull/7854">PR #7854</a>. */
public static final Version INJECTED_NESSIE_API_URL_CHANGE = Version.parseVersion("0.75.0");

public static final String CURRENT_STRING = "current";
public static final String NOT_CURRENT_STRING = "not-current";
private final int[] tuple;
Expand Down
2 changes: 1 addition & 1 deletion ...mon/src/main/java/org/projectnessie/tools/compatibility/internal/CurrentNessieServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ static CurrentNessieServer currentNessieServer(

@Override
public URI getUri(Class<? extends NessieApi> apiType) {
return Util.resolveNessieUri(jersey.getUri().resolve("api/"), apiType);
return Util.resolveNessieUri(jersey.getUri(), serverKey.getVersion(), apiType);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#7854 added api/ here but nothing version-aware in OldNessieServer

any ideas how to stay "ahead" of such problems in the future?
i.e. do we need server/client compatibility tests that use a Version.LAST_RELEASE value? then at least after a new release we might get notified.

or a test that uses Version.CURRENT with OldNessieServer somehow?

Copy link
Member

Choose a reason for hiding this comment

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

OldNessieServer relies on published jars, so it is currently at least one release behind (it cannot test main due to lack of artifacts). One option may be to do publishToMavenLocal and use those artifacts in CI.

Copy link
Member

Choose a reason for hiding this comment

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

Add the version to nessie-compatibility.properties?

Copy link
Member

@dimas-b dimas-b Jan 16, 2024 •
edited

Choose a reason for hiding this comment

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

If we go with publishToMavenLocal I believe some extra changes will be required to support "snapshot" versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add the version to nessie-compatibility.properties?

doesnt this require awareness of the developer that made the change that compatibility was broken?
CI tests should find this for the developer "automatically"

Copy link
Member

@dimas-b dimas-b Jan 16, 2024 •
edited

Choose a reason for hiding this comment

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

What I'm proposing is to run publishToMavenLocal in CI and use that version for tests automatically, indeed... but let's do that in another PR (I believe it will have some ripple effect in the code that handles versions).

}

@Override
Expand Down
2 changes: 1 addition & 1 deletion .../common/src/main/java/org/projectnessie/tools/compatibility/internal/OldNessieServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public URI getUri(Class<? extends NessieApi> apiType) {
tryStart();
}

return Util.resolveNessieUri(uri, apiType);
return Util.resolveNessieUri(uri, serverKey.getVersion(), apiType);
}

private void tryStart() {
Expand Down
6 changes: 5 additions & 1 deletion compatibility/common/src/main/java/org/projectnessie/tools/compatibility/internal/Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.junit.platform.engine.UniqueId;
import org.projectnessie.client.api.NessieApi;
import org.projectnessie.client.api.NessieApiV2;
import org.projectnessie.tools.compatibility.api.Version;

final class Util {

Expand Down Expand Up @@ -71,8 +72,11 @@ static <T> T withClassLoader(ClassLoader classLoader, Callable<T> callable) thro
}
}

static URI resolveNessieUri(URI base, Class<? extends NessieApi> apiType) {
static URI resolveNessieUri(URI base, Version version, Class<? extends NessieApi> apiType) {
String suffix = NessieApiV2.class.isAssignableFrom(apiType) ? "v2" : "v1";
if (version.isGreaterThanOrEqual(Version.INJECTED_NESSIE_API_URL_CHANGE)) {
XN137 marked this conversation as resolved.
Show resolved Hide resolved
suffix = "api/" + suffix;
}
return base.resolve(suffix);
}
}
93 changes: 83 additions & 10 deletions ...ava/org/projectnessie/tools/compatibility/internal/TestNessieCompatibilityExtensions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.assertj.core.api.SoftAssertions;
import org.assertj.core.api.junit.jupiter.InjectSoftAssertions;
Expand All @@ -33,11 +34,13 @@
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.platform.testkit.engine.EngineExecutionResults;
import org.junit.platform.testkit.engine.EngineTestKit;
import org.junit.platform.testkit.engine.Events;
import org.projectnessie.client.api.NessieApiV1;
import org.projectnessie.junit.engine.MultiEnvTestEngine;
import org.projectnessie.junit.engine.MultiEnvTestFilter;
import org.projectnessie.model.Branch;
import org.projectnessie.model.NessieConfiguration;
import org.projectnessie.model.Reference;
import org.projectnessie.tools.compatibility.api.NessieAPI;
import org.projectnessie.tools.compatibility.api.NessieBaseUri;
import org.projectnessie.tools.compatibility.api.NessieVersion;
Expand All @@ -48,6 +51,20 @@
class TestNessieCompatibilityExtensions {
@InjectSoftAssertions protected SoftAssertions soft;

private void assertNoFailedTestEvents(EngineExecutionResults result) {
soft.assertThat(result.testEvents().count())
.withFailMessage("Failed to run any tests")
.isGreaterThan(0);

Events failedEvents = result.testEvents().failed();
soft.assertThat(failedEvents.count())
.withFailMessage(
() ->
"The following test events have failed:\n:"
+ failedEvents.stream().map(e -> e.toString()).collect(Collectors.joining()))
.isEqualTo(0);
}

@Test
void noVersions() {
soft.assertThatThrownBy(
Expand Down Expand Up @@ -100,11 +117,14 @@ void tooManyExtensions() {

@Test
void olderClients() {
EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID)
.configurationParameter(
"nessie.versions", NEW_STORAGE_MODEL_WITH_COMPAT_TESTING.toString() + ",current")
.selectors(selectClass(OldClientsSample.class))
.execute();
XN137 marked this conversation as resolved.
Show resolved Hide resolved
EngineExecutionResults results =
EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID)
.configurationParameter(
"nessie.versions", NEW_STORAGE_MODEL_WITH_COMPAT_TESTING.toString() + ",current")
.selectors(selectClass(OldClientsSample.class))
.execute();
assertNoFailedTestEvents(results);

soft.assertThat(OldClientsSample.allVersions)
.containsExactly(NEW_STORAGE_MODEL_WITH_COMPAT_TESTING, Version.CURRENT);
soft.assertThat(OldClientsSample.minVersionHigh).containsExactly(Version.CURRENT);
Expand All @@ -115,11 +135,14 @@ void olderClients() {

@Test
void olderServers() {
EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID)
.configurationParameter(
"nessie.versions", NEW_STORAGE_MODEL_WITH_COMPAT_TESTING.toString() + ",current")
.selectors(selectClass(OldServersSample.class))
.execute();
EngineExecutionResults results =
EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID)
.configurationParameter(
"nessie.versions", NEW_STORAGE_MODEL_WITH_COMPAT_TESTING.toString() + ",current")
.selectors(selectClass(OldServersSample.class))
.execute();
assertNoFailedTestEvents(results);

soft.assertThat(OldServersSample.allVersions)
.containsExactly(NEW_STORAGE_MODEL_WITH_COMPAT_TESTING, Version.CURRENT);
soft.assertThat(OldServersSample.minVersionHigh).containsExactly(Version.CURRENT);
Expand All @@ -131,6 +154,30 @@ void olderServers() {
soft.assertThat(OldServersSample.uris).allMatch(uri -> uri.getPath().equals("/"));
}

@Test
void injectedNessieApiUrlChanged() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test fails like this:

Multiple Failures (1 failure)
-- failure 1 --The following test events have failed:
:Event [type = FINISHED, testDescriptor = TestMethodTestDescriptor: [engine:nessie-multi-env]/[nessie-version:0.75.0]/[class:org.projectnessie.tools.compatibility.internal.TestNessieCompatibilityExtensions$ApiEndpointServerSample]/[method:testSome()], timestamp = 2024-01-16T13:46:43.453995109Z, payload = TestExecutionResult [status = FAILED, throwable = org.projectnessie.client.rest.NessieServiceException: Not Found (HTTP/404): 

Additionally, the client-side exception below was caught while decoding the HTTP response:
com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot deserialize value of type `org.projectnessie.error.ImmutableNessieError` from [Unavailable value] (token `JsonToken.NOT_AVAILABLE`)
 at [Source: UNKNOWN; byte offset: #UNKNOWN]
	at com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:59)
	at com.fasterxml.jackson.databind.DeserializationContext.reportInputMismatch(DeserializationContext.java:1767)
	at com.fasterxml.jackson.databind.DeserializationContext.handleUnexpectedToken(DeserializationContext.java:1541)
	at com.fasterxml.jackson.databind.DeserializationContext.handleUnexpectedToken(DeserializationContext.java:1488)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeOther(BeanDeserializer.java:224)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:187)
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:342)
	at com.fasterxml.jackson.databind.ObjectReader._bind(ObjectReader.java:2099)
	at com.fasterxml.jackson.databind.ObjectReader.readValue(ObjectReader.java:1249)
	at com.fasterxml.jackson.databind.ObjectReader.readValue(ObjectReader.java:1267)
	at com.fasterxml.jackson.databind.ObjectReader.treeToValue(ObjectReader.java:2044)
	at org.projectnessie.client.rest.ResponseCheckFilter.decodeErrorObject(ResponseCheckFilter.java:111)
	at org.projectnessie.client.rest.ResponseCheckFilter.checkResponse(ResponseCheckFilter.java:55)
	at org.projectnessie.client.rest.NessieHttpResponseFilter.filter(NessieHttpResponseFilter.java:29)
	at org.projectnessie.client.http.impl.jdk11.JavaRequest.lambda$executeRequest$1(JavaRequest.java:143)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.Collections$UnmodifiableCollection.forEach(Collections.java:1092)
	at org.projectnessie.client.http.impl.jdk11.JavaRequest.executeRequest(JavaRequest.java:143)
	at org.projectnessie.client.http.HttpRequest.get(HttpRequest.java:106)
	at org.projectnessie.client.rest.v1.RestV1TreeClient.getAllReferences(RestV1TreeClient.java:58)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.projectnessie.client.rest.v1.RestV1Client$ExceptionRewriter.invoke(RestV1Client.java:84)
	at jdk.proxy3/jdk.proxy3.$Proxy293.getAllReferences(Unknown Source)
	at org.projectnessie.client.rest.v1.HttpGetAllReferences.get(HttpGetAllReferences.java:42)
	at org.projectnessie.client.rest.v1.HttpGetAllReferences.get(HttpGetAllReferences.java:22)
	at org.projectnessie.client.builder.BaseGetAllReferencesBuilder.get(BaseGetAllReferencesBuilder.java:70)
	at org.projectnessie.tools.compatibility.internal.TestNessieCompatibilityExtensions$ApiEndpointServerSample.testSome

// apiUrl 0.74.0: host:port/v2 (via OldNessieServer)
// apiUrl 0.75.0: host:port/v2 (via OldNessieServer) should be /api/v2 as well
// apiUrl current: host:port/api/v2 (via CurrentNessieServer)
final Version versionBeforeApiUrlChange = Version.parseVersion("0.74.0");
EngineExecutionResults result =
EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID)
.configurationParameter(
"nessie.versions",
versionBeforeApiUrlChange
+ ","
+ Version.INJECTED_NESSIE_API_URL_CHANGE
+ ",current")
.selectors(selectClass(ApiEndpointServerSample.class))
.execute();

soft.assertThat(ApiEndpointServerSample.allVersions)
.containsExactly(
versionBeforeApiUrlChange, Version.INJECTED_NESSIE_API_URL_CHANGE, Version.CURRENT);

assertNoFailedTestEvents(result);
}

@Test
void nestedTests() {
EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID)
Expand Down Expand Up @@ -258,6 +305,32 @@ void never() {
}
}

@ExtendWith({OlderNessieServersExtension.class, SoftAssertionsExtension.class})
static class ApiEndpointServerSample {
@InjectSoftAssertions protected SoftAssertions soft;

@NessieAPI NessieApiV1 api;
@NessieAPI static NessieApiV1 apiStatic;
@NessieBaseUri URI uri;
@NessieBaseUri static URI uriStatic;
@NessieVersion Version version;
@NessieVersion static Version versionStatic;

static final List<Version> allVersions = new ArrayList<>();

@Test
void testSome() throws Exception {
soft.assertThat(api).isNotNull().isSameAs(apiStatic);
soft.assertThat(uri).isNotNull().isEqualTo(uriStatic);
soft.assertThat(version).isNotNull().isEqualTo(versionStatic);
allVersions.add(version);

// use api
List<Reference> references = api.getAllReferences().get().getReferences();
soft.assertThat(references).isNotEmpty();
Copy link
Contributor Author

@XN137 XN137 Jan 16, 2024 •
edited

Choose a reason for hiding this comment

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

note that without assertNoFailedTestEvents the outer assertion for allVersions would pass just fine, even though the API usage causes an error (since it comes last)

}
}

@ExtendWith(OlderNessieServersExtension.class)
static class OuterSample {
static final List<Version> outerVersions = new ArrayList<>();
Expand Down