From 5ce067a0340e1cd8ee0db0551b91a9dbb641721c Mon Sep 17 00:00:00 2001 From: ndr_brt Date: Wed, 13 Sep 2023 08:58:42 +0200 Subject: [PATCH] refactor: get rid of `NoopTransferProcessClient` (#3440) * refactor: get rid of NoopTransferProcessClient * simplify test --- .../DataPlaneDefaultServicesExtension.java | 8 --- .../DataPlaneFrameworkExtensionTest.java | 50 +------------- .../dataplane/framework/e2e/EndToEndTest.java | 3 +- .../manager/DataPlaneManagerImplTest.java | 26 ++++---- .../common/http/jetty-core/build.gradle.kts | 1 - .../edc/web/jetty/JettyServiceTest.java | 65 ++++--------------- ...ansferProcessControlApiControllerTest.java | 2 + .../data-plane-http/build.gradle.kts | 1 + .../build.gradle.kts | 3 +- gradle/libs.versions.toml | 1 - .../NoopTransferProcessClient.java | 32 --------- 11 files changed, 33 insertions(+), 159 deletions(-) delete mode 100644 spi/control-plane/control-plane-api-client-spi/src/main/java/org/eclipse/edc/connector/api/client/spi/transferprocess/NoopTransferProcessClient.java diff --git a/core/data-plane/data-plane-framework/src/main/java/org/eclipse/edc/connector/dataplane/framework/DataPlaneDefaultServicesExtension.java b/core/data-plane/data-plane-framework/src/main/java/org/eclipse/edc/connector/dataplane/framework/DataPlaneDefaultServicesExtension.java index de98a76fabe..1c811d22471 100644 --- a/core/data-plane/data-plane-framework/src/main/java/org/eclipse/edc/connector/dataplane/framework/DataPlaneDefaultServicesExtension.java +++ b/core/data-plane/data-plane-framework/src/main/java/org/eclipse/edc/connector/dataplane/framework/DataPlaneDefaultServicesExtension.java @@ -14,8 +14,6 @@ package org.eclipse.edc.connector.dataplane.framework; -import org.eclipse.edc.connector.api.client.spi.transferprocess.NoopTransferProcessClient; -import org.eclipse.edc.connector.api.client.spi.transferprocess.TransferProcessApiClient; import org.eclipse.edc.connector.dataplane.framework.registry.TransferServiceSelectionStrategy; import org.eclipse.edc.runtime.metamodel.annotation.Extension; import org.eclipse.edc.runtime.metamodel.annotation.Provider; @@ -31,12 +29,6 @@ public String name() { return NAME; } - - @Provider(isDefault = true) - public TransferProcessApiClient transferProcessApiClient() { - return new NoopTransferProcessClient(); - } - @Provider(isDefault = true) public TransferServiceSelectionStrategy transferServiceSelectionStrategy() { return TransferServiceSelectionStrategy.selectFirst(); diff --git a/core/data-plane/data-plane-framework/src/test/java/org/eclipse/edc/connector/dataplane/framework/DataPlaneFrameworkExtensionTest.java b/core/data-plane/data-plane-framework/src/test/java/org/eclipse/edc/connector/dataplane/framework/DataPlaneFrameworkExtensionTest.java index 9c22d77140d..f46766b3dcc 100644 --- a/core/data-plane/data-plane-framework/src/test/java/org/eclipse/edc/connector/dataplane/framework/DataPlaneFrameworkExtensionTest.java +++ b/core/data-plane/data-plane-framework/src/test/java/org/eclipse/edc/connector/dataplane/framework/DataPlaneFrameworkExtensionTest.java @@ -14,76 +14,30 @@ package org.eclipse.edc.connector.dataplane.framework; -import org.eclipse.edc.connector.api.client.spi.transferprocess.NoopTransferProcessClient; -import org.eclipse.edc.connector.api.client.spi.transferprocess.TransferProcessApiClient; -import org.eclipse.edc.connector.dataplane.framework.e2e.EndToEndTest; import org.eclipse.edc.connector.dataplane.framework.pipeline.PipelineServiceImpl; -import org.eclipse.edc.connector.dataplane.framework.registry.TransferServiceSelectionStrategy; -import org.eclipse.edc.connector.dataplane.spi.manager.DataPlaneManager; import org.eclipse.edc.connector.dataplane.spi.pipeline.PipelineService; -import org.eclipse.edc.connector.dataplane.spi.pipeline.TransferService; -import org.eclipse.edc.connector.dataplane.spi.registry.TransferServiceRegistry; import org.eclipse.edc.junit.extensions.DependencyInjectionExtension; import org.eclipse.edc.spi.system.ExecutorInstrumentation; import org.eclipse.edc.spi.system.ServiceExtensionContext; -import org.eclipse.edc.spi.system.injection.ObjectFactory; -import org.eclipse.edc.spi.telemetry.Telemetry; -import org.eclipse.edc.spi.types.domain.transfer.DataFlowRequest; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; @ExtendWith(DependencyInjectionExtension.class) class DataPlaneFrameworkExtensionTest { - TransferService transferService1 = mock(TransferService.class); - TransferService transferService2 = mock(TransferService.class); - DataFlowRequest request = EndToEndTest.createRequest("1").build(); - @BeforeEach public void setUp(ServiceExtensionContext context) { - when(transferService1.canHandle(request)).thenReturn(true); - when(transferService2.canHandle(request)).thenReturn(true); - context.registerService(Telemetry.class, mock(Telemetry.class)); context.registerService(ExecutorInstrumentation.class, ExecutorInstrumentation.noop()); - context.registerService(TransferProcessApiClient.class, new NoopTransferProcessClient()); } @Test - void initialize_registers_PipelineService(ServiceExtensionContext context, ObjectFactory factory) { - var extension = factory.constructInstance(DataPlaneFrameworkExtension.class); + void initialize_registers_PipelineService(ServiceExtensionContext context, DataPlaneFrameworkExtension extension) { extension.initialize(context); - assertThat(context.getService(PipelineService.class)).isInstanceOf(PipelineServiceImpl.class); - } - - @Test - void initialize_registers_DataPlaneManager_withInjectedStrategy(ServiceExtensionContext context, ObjectFactory factory) { - // Arrange - // Inject a custom TransferServiceSelectionStrategy that will select the second service - context.registerService(TransferServiceSelectionStrategy.class, - (request, services) -> services.skip(1).findFirst().orElse(null)); - // Act - validateRequest(context, factory); - - // Assert - verify(transferService2).validate(request); - verify(transferService1, never()).validate(request); + assertThat(context.getService(PipelineService.class)).isInstanceOf(PipelineServiceImpl.class); } - private void validateRequest(ServiceExtensionContext context, ObjectFactory factory) { - var extension = factory.constructInstance(DataPlaneFrameworkExtension.class); - extension.initialize(context); - var service = context.getService(TransferServiceRegistry.class); - service.registerTransferService(transferService1); - service.registerTransferService(transferService2); - var m = context.getService(DataPlaneManager.class); - m.validate(request); - } } diff --git a/core/data-plane/data-plane-framework/src/test/java/org/eclipse/edc/connector/dataplane/framework/e2e/EndToEndTest.java b/core/data-plane/data-plane-framework/src/test/java/org/eclipse/edc/connector/dataplane/framework/e2e/EndToEndTest.java index a3ad520f11f..f766c17b9b7 100644 --- a/core/data-plane/data-plane-framework/src/test/java/org/eclipse/edc/connector/dataplane/framework/e2e/EndToEndTest.java +++ b/core/data-plane/data-plane-framework/src/test/java/org/eclipse/edc/connector/dataplane/framework/e2e/EndToEndTest.java @@ -14,7 +14,6 @@ package org.eclipse.edc.connector.dataplane.framework.e2e; -import org.eclipse.edc.connector.api.client.spi.transferprocess.NoopTransferProcessClient; import org.eclipse.edc.connector.dataplane.framework.manager.DataPlaneManagerImpl; import org.eclipse.edc.connector.dataplane.framework.pipeline.PipelineServiceImpl; import org.eclipse.edc.connector.dataplane.spi.pipeline.DataSink; @@ -58,7 +57,7 @@ void testEndToEnd() throws Exception { .monitor(monitor) .pipelineService(pipelineService) .executorInstrumentation(ExecutorInstrumentation.noop()) - .transferProcessClient(new NoopTransferProcessClient()) + .transferProcessClient(mock()) .build(); manager.start(); manager.transfer(new InputStreamDataSource("test", new ByteArrayInputStream("bytes".getBytes())), createRequest("1").build()).get(); diff --git a/core/data-plane/data-plane-framework/src/test/java/org/eclipse/edc/connector/dataplane/framework/manager/DataPlaneManagerImplTest.java b/core/data-plane/data-plane-framework/src/test/java/org/eclipse/edc/connector/dataplane/framework/manager/DataPlaneManagerImplTest.java index 4313eaa441e..21c8863f0e2 100644 --- a/core/data-plane/data-plane-framework/src/test/java/org/eclipse/edc/connector/dataplane/framework/manager/DataPlaneManagerImplTest.java +++ b/core/data-plane/data-plane-framework/src/test/java/org/eclipse/edc/connector/dataplane/framework/manager/DataPlaneManagerImplTest.java @@ -14,12 +14,10 @@ package org.eclipse.edc.connector.dataplane.framework.manager; -import org.eclipse.edc.connector.api.client.spi.transferprocess.NoopTransferProcessClient; import org.eclipse.edc.connector.dataplane.framework.store.InMemoryDataPlaneStore; import org.eclipse.edc.connector.dataplane.spi.pipeline.TransferService; import org.eclipse.edc.connector.dataplane.spi.registry.TransferServiceRegistry; import org.eclipse.edc.connector.dataplane.spi.store.DataPlaneStore; -import org.eclipse.edc.spi.monitor.Monitor; import org.eclipse.edc.spi.result.Result; import org.eclipse.edc.spi.system.ExecutorInstrumentation; import org.eclipse.edc.spi.types.domain.DataAddress; @@ -39,10 +37,10 @@ class DataPlaneManagerImplTest { - TransferService transferService = mock(TransferService.class); + TransferService transferService = mock(); DataPlaneStore store = new InMemoryDataPlaneStore(10); DataFlowRequest request = createRequest(); - TransferServiceRegistry registry = mock(TransferServiceRegistry.class); + TransferServiceRegistry registry = mock(); @BeforeEach public void setUp() { @@ -54,7 +52,7 @@ public void setUp() { * Verifies a request is enqueued, dequeued, and dispatched to the pipeline service. */ @Test - void verifyWorkDispatch() throws InterruptedException { + void verifyWorkDispatch() { var dataPlaneManager = createDataPlaneManager(); when(registry.resolveTransferService(request)) @@ -63,9 +61,8 @@ void verifyWorkDispatch() throws InterruptedException { .thenReturn(true); - when(transferService.transfer(isA(DataFlowRequest.class))).thenAnswer(i -> { - return completedFuture(Result.success("ok")); - }); + when(transferService.transfer(isA(DataFlowRequest.class))) + .thenAnswer(i -> completedFuture(Result.success("ok"))); dataPlaneManager.start(); dataPlaneManager.initiateTransfer(request); @@ -80,7 +77,7 @@ void verifyWorkDispatch() throws InterruptedException { * Verifies that the dispatch thread survives an error thrown by a worker. */ @Test - void verifyWorkDispatchError() throws InterruptedException { + void verifyWorkDispatchError() { var dataPlaneManager = createDataPlaneManager(); when(transferService.canHandle(request)) @@ -89,9 +86,8 @@ void verifyWorkDispatchError() throws InterruptedException { when(transferService.transfer(request)) .thenAnswer(i -> { throw new RuntimeException("Test exception"); - }).thenAnswer((i -> { - return completedFuture(Result.success("ok")); - })); + }) + .thenAnswer((i -> completedFuture(Result.success("ok")))); dataPlaneManager.start(); @@ -105,7 +101,7 @@ void verifyWorkDispatchError() throws InterruptedException { } @Test - void verifyWorkDispatch_onUnavailableTransferService_completesTransfer() throws InterruptedException { + void verifyWorkDispatch_onUnavailableTransferService_completesTransfer() { // Modify store used in createDataPlaneManager() store = mock(DataPlaneStore.class); @@ -139,8 +135,8 @@ private DataPlaneManagerImpl createDataPlaneManager() { .waitTimeout(10) .transferServiceRegistry(registry) .store(store) - .transferProcessClient(new NoopTransferProcessClient()) - .monitor(mock(Monitor.class)) + .transferProcessClient(mock()) + .monitor(mock()) .build(); } diff --git a/extensions/common/http/jetty-core/build.gradle.kts b/extensions/common/http/jetty-core/build.gradle.kts index d3cc33c0a74..8b51286bacb 100644 --- a/extensions/common/http/jetty-core/build.gradle.kts +++ b/extensions/common/http/jetty-core/build.gradle.kts @@ -24,7 +24,6 @@ dependencies { api(project(":spi:common:core-spi")) api(project(":spi:common:web-spi")) - testImplementation(libs.bundles.jersey.core) testImplementation(libs.restAssured) testImplementation(project(":core:common:junit")) } diff --git a/extensions/common/http/jetty-core/src/test/java/org/eclipse/edc/web/jetty/JettyServiceTest.java b/extensions/common/http/jetty-core/src/test/java/org/eclipse/edc/web/jetty/JettyServiceTest.java index 89f62a54b76..cf273991a11 100644 --- a/extensions/common/http/jetty-core/src/test/java/org/eclipse/edc/web/jetty/JettyServiceTest.java +++ b/extensions/common/http/jetty-core/src/test/java/org/eclipse/edc/web/jetty/JettyServiceTest.java @@ -14,24 +14,19 @@ package org.eclipse.edc.web.jetty; -import jakarta.ws.rs.GET; -import jakarta.ws.rs.Path; -import jakarta.ws.rs.Produces; -import jakarta.ws.rs.core.MediaType; +import jakarta.servlet.http.HttpServlet; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; import org.eclipse.edc.spi.EdcException; import org.eclipse.edc.spi.monitor.Monitor; import org.eclipse.edc.spi.system.configuration.ConfigFactory; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.util.component.AbstractLifeCycle; -import org.glassfish.jersey.internal.inject.AbstractBinder; -import org.glassfish.jersey.server.ResourceConfig; -import org.glassfish.jersey.servlet.ServletContainer; -import org.jetbrains.annotations.NotNull; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; +import java.io.IOException; import java.net.ConnectException; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; @@ -44,14 +39,7 @@ class JettyServiceTest { private JettyService jettyService; - private Monitor monitor; - private TestController testController; - - @BeforeEach - void setUp() { - monitor = mock(Monitor.class); - testController = new TestController(); - } + private final Monitor monitor = mock(); @Test void verifyDefaultPortMapping() { @@ -60,8 +48,7 @@ void verifyDefaultPortMapping() { jettyService.start(); - var servletContainer = new ServletContainer(createTestResource()); - jettyService.registerServlet("default", servletContainer); + jettyService.registerServlet("default", new TestServlet()); given() .get("http://localhost:7171/api/test/resource") @@ -76,11 +63,10 @@ void verifyCustomPortMapping() { "web.http.another.port", "9191", "web.http.another.path", "/another")); //default port mapping jettyService = new JettyService(JettyConfiguration.createFromConfig(null, null, config), monitor); - ResourceConfig rc = createTestResource(); jettyService.start(); - jettyService.registerServlet("another", new ServletContainer(rc)); + jettyService.registerServlet("another", new TestServlet()); given() .get("http://localhost:9191/another/test/resource") @@ -103,8 +89,8 @@ void verifyDefaultAndCustomPortMapping() { jettyService.start(); - jettyService.registerServlet("another", new ServletContainer(createTestResource())); - jettyService.registerServlet("default", new ServletContainer(createTestResource())); + jettyService.registerServlet("another", new TestServlet()); + jettyService.registerServlet("default", new TestServlet()); given() .get("http://localhost:9191/another/test/resource") @@ -127,7 +113,7 @@ void verifyConnectorConfigurationCallback() { jettyService.start(); - jettyService.registerServlet("default", new ServletContainer(createTestResource())); + jettyService.registerServlet("default", new TestServlet()); assertThat(listener.getConnectionsOpened()).isEqualTo(0); given() @@ -146,7 +132,7 @@ void verifyCustomPathRoot() { jettyService.start(); - jettyService.registerServlet("default", new ServletContainer(createTestResource())); + jettyService.registerServlet("default", new TestServlet()); given() .get("http://localhost:7171/test/resource") @@ -188,25 +174,6 @@ void teardown() { jettyService.shutdown(); } - @NotNull - private ResourceConfig createTestResource() { - var rc = new ResourceConfig(); - rc.registerClasses(TestController.class); - rc.registerInstances(new TestBinder()); - return rc; - } - - @Produces(MediaType.TEXT_PLAIN) - @Path("/test") - public static class TestController { //needs to be public, otherwise it won't get picked up - - @GET - @Path("/resource") - public String foo() { - return "exists"; - } - } - private static class JettyListener extends AbstractLifeCycle implements Connection.Listener { private final AtomicInteger connectionsOpened = new AtomicInteger(); @@ -225,14 +192,10 @@ public int getConnectionsOpened() { } } - /** - * Maps (JAX-RS resource) instances to types. - */ - private class TestBinder extends AbstractBinder { - + private static class TestServlet extends HttpServlet { @Override - protected void configure() { - bind(testController).to(TestController.class); + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { + resp.getWriter().write("{}"); } } } diff --git a/extensions/control-plane/api/control-plane-api/src/test/java/org/eclipse/edc/connector/api/transferprocess/TransferProcessControlApiControllerTest.java b/extensions/control-plane/api/control-plane-api/src/test/java/org/eclipse/edc/connector/api/transferprocess/TransferProcessControlApiControllerTest.java index 2e194117431..2e6ffe1b388 100644 --- a/extensions/control-plane/api/control-plane-api/src/test/java/org/eclipse/edc/connector/api/transferprocess/TransferProcessControlApiControllerTest.java +++ b/extensions/control-plane/api/control-plane-api/src/test/java/org/eclipse/edc/connector/api/transferprocess/TransferProcessControlApiControllerTest.java @@ -17,6 +17,7 @@ import io.restassured.specification.RequestSpecification; import org.eclipse.edc.connector.spi.transferprocess.TransferProcessService; import org.eclipse.edc.connector.transfer.spi.types.command.TerminateTransferCommand; +import org.eclipse.edc.junit.annotations.ApiTest; import org.eclipse.edc.service.spi.result.ServiceResult; import org.eclipse.edc.web.jersey.testfixtures.RestControllerTestBase; import org.junit.jupiter.api.Test; @@ -31,6 +32,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +@ApiTest public class TransferProcessControlApiControllerTest extends RestControllerTestBase { private final TransferProcessService transferProcessService = mock(); diff --git a/extensions/data-plane/data-plane-http/build.gradle.kts b/extensions/data-plane/data-plane-http/build.gradle.kts index 1a10105ed76..8c64f5726d3 100644 --- a/extensions/data-plane/data-plane-http/build.gradle.kts +++ b/extensions/data-plane/data-plane-http/build.gradle.kts @@ -25,6 +25,7 @@ dependencies { testImplementation(project(":core:common:junit")) testImplementation(project(":core:data-plane:data-plane-core")) + testImplementation(project(":extensions:control-plane:api:control-plane-api-client")) testImplementation(libs.restAssured) testImplementation(libs.mockserver.netty) diff --git a/extensions/data-plane/data-plane-integration-tests/build.gradle.kts b/extensions/data-plane/data-plane-integration-tests/build.gradle.kts index bc413612052..ec3f17d34f9 100644 --- a/extensions/data-plane/data-plane-integration-tests/build.gradle.kts +++ b/extensions/data-plane/data-plane-integration-tests/build.gradle.kts @@ -24,9 +24,10 @@ dependencies { testImplementation(libs.mockserver.netty) testImplementation(libs.mockserver.client) + testImplementation(project(":spi:data-plane:data-plane-spi")) testImplementation(project(":core:common:junit")) + testImplementation(project(":extensions:control-plane:api:control-plane-api-client")) testImplementation(testFixtures(project(":extensions:data-plane:data-plane-http"))) - testImplementation(project(":spi:data-plane:data-plane-spi")) testRuntimeOnly(project(":launchers:data-plane-server")) } diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index a86b1c5898e..5925c8ae0a2 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -108,7 +108,6 @@ kafkaClients = { module = "org.apache.kafka:kafka-clients", version.ref = "kafka [bundles] jersey-core = ["jersey.server", "jersey.common", "jersey.jackson", "jersey.multipart", "jersey.inject", "jersey.servlet", "jersey.servletcore"] -jersey-api = ["jersey.common", "jersey.server"] jackson = ["jackson.core", "jackson.annotations", "jackson.databind", "jackson-datatypeJsr310"] jupiter = ["junit-jupiter-api", "junit-jupiter-params"] diff --git a/spi/control-plane/control-plane-api-client-spi/src/main/java/org/eclipse/edc/connector/api/client/spi/transferprocess/NoopTransferProcessClient.java b/spi/control-plane/control-plane-api-client-spi/src/main/java/org/eclipse/edc/connector/api/client/spi/transferprocess/NoopTransferProcessClient.java deleted file mode 100644 index 00d074c1f8e..00000000000 --- a/spi/control-plane/control-plane-api-client-spi/src/main/java/org/eclipse/edc/connector/api/client/spi/transferprocess/NoopTransferProcessClient.java +++ /dev/null @@ -1,32 +0,0 @@ -/* - * Copyright (c) 2022 Microsoft Corporation - * - * This program and the accompanying materials are made available under the - * terms of the Apache License, Version 2.0 which is available at - * https://www.apache.org/licenses/LICENSE-2.0 - * - * SPDX-License-Identifier: Apache-2.0 - * - * Contributors: - * Microsoft Corporation - initial API and implementation - * - */ - -package org.eclipse.edc.connector.api.client.spi.transferprocess; - -import org.eclipse.edc.spi.types.domain.transfer.DataFlowRequest; - -/** - * Noop Transfer process client - */ -public class NoopTransferProcessClient implements TransferProcessApiClient { - @Override - public void completed(DataFlowRequest request) { - - } - - @Override - public void failed(DataFlowRequest request, String reason) { - - } -}