From 0982a7407cc7aebe4a2d524a9a5b5b82f504f1e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20D=C3=BCmont?= Date: Wed, 16 Oct 2024 14:59:37 +0200 Subject: [PATCH 1/5] Apply review comments --- .../com/sap/ai/sdk/core/AiCoreDeployment.java | 17 ++--------------- .../java/com/sap/ai/sdk/core/AiCoreService.java | 6 +++--- .../ai/sdk/core/client/WireMockTestServer.java | 5 ++--- .../client/OrchestrationUnitTest.java | 11 ++++------- 4 files changed, 11 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/com/sap/ai/sdk/core/AiCoreDeployment.java b/core/src/main/java/com/sap/ai/sdk/core/AiCoreDeployment.java index 721b9d74..c58d5895 100644 --- a/core/src/main/java/com/sap/ai/sdk/core/AiCoreDeployment.java +++ b/core/src/main/java/com/sap/ai/sdk/core/AiCoreDeployment.java @@ -95,7 +95,7 @@ public Destination destination() { final var dest = service.baseDestinationHandler.apply(service); final var builder = service.builderHandler.apply(service, dest); destinationSetUrl(builder, dest); - destinationSetHeaders(builder, dest); + destinationSetHeaders(builder); return builder.build(); } @@ -125,10 +125,8 @@ protected void destinationSetUrl( * Update and set the default request headers for the destination. * * @param builder The destination builder. - * @param dest The original destination reference. */ - protected void destinationSetHeaders( - @Nonnull final DefaultHttpDestination.Builder builder, @Nonnull final Destination dest) { + protected void destinationSetHeaders(@Nonnull final DefaultHttpDestination.Builder builder) { builder.property(AI_RESOURCE_GROUP, getResourceGroup()); } @@ -143,17 +141,6 @@ public AiCoreDeployment withResourceGroup(@Nonnull final String resourceGroup) { return new AiCoreDeployment(service, deploymentId, resourceGroup); } - /** - * Set the destination. - * - * @param destination The destination. - * @return A new instance of the AI Core service. - */ - @Nonnull - public AiCoreDeployment withDestination(@Nonnull final Destination destination) { - return new AiCoreDeployment(service.withDestination(destination), deploymentId, resourceGroup); - } - /** * Get the deployment id. * diff --git a/core/src/main/java/com/sap/ai/sdk/core/AiCoreService.java b/core/src/main/java/com/sap/ai/sdk/core/AiCoreService.java index c6401d23..53b65a09 100644 --- a/core/src/main/java/com/sap/ai/sdk/core/AiCoreService.java +++ b/core/src/main/java/com/sap/ai/sdk/core/AiCoreService.java @@ -72,7 +72,7 @@ public AiCoreService withDestination(@Nonnull final Destination destination) { * Set a specific deployment by id. * * @param deploymentId The deployment id to be used for AI Core service calls. - * @return A new instance of the AI Core service. + * @return A new instance of the AI Core Deployment. */ @Nonnull public AiCoreDeployment forDeployment(@Nonnull final String deploymentId) { @@ -83,7 +83,7 @@ public AiCoreDeployment forDeployment(@Nonnull final String deploymentId) { * Set a specific deployment by model name. * * @param modelName The model name to be used for AI Core service calls. - * @return A new instance of the AI Core service. + * @return A new instance of the AI Core Deployment. */ @Nonnull public AiCoreDeployment forDeploymentByModel(@Nonnull final String modelName) { @@ -94,7 +94,7 @@ public AiCoreDeployment forDeploymentByModel(@Nonnull final String modelName) { * Set a specific deployment by scenario id. * * @param scenarioId The scenario id to be used for AI Core service calls. - * @return A new instance of the AI Core service. + * @return A new instance of the AI Core Deployment. */ @Nonnull public AiCoreDeployment forDeploymentByScenario(@Nonnull final String scenarioId) { diff --git a/core/src/test/java/com/sap/ai/sdk/core/client/WireMockTestServer.java b/core/src/test/java/com/sap/ai/sdk/core/client/WireMockTestServer.java index 08e5a89d..1d0a22ba 100644 --- a/core/src/test/java/com/sap/ai/sdk/core/client/WireMockTestServer.java +++ b/core/src/test/java/com/sap/ai/sdk/core/client/WireMockTestServer.java @@ -6,7 +6,6 @@ import com.github.tomakehurst.wiremock.core.WireMockConfiguration; import com.sap.ai.sdk.core.AiCoreService; import com.sap.cloud.sdk.cloudplatform.connectivity.DefaultHttpDestination; -import com.sap.cloud.sdk.cloudplatform.connectivity.Destination; import com.sap.cloud.sdk.services.openapi.apiclient.ApiClient; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; @@ -17,14 +16,14 @@ abstract class WireMockTestServer { wireMockConfig().dynamicPort(); static WireMockServer wireMockServer; - static Destination destination; static ApiClient client; @BeforeAll static void setup() { wireMockServer = new WireMockServer(WIREMOCK_CONFIGURATION); wireMockServer.start(); - destination = DefaultHttpDestination.builder(wireMockServer.baseUrl()).build(); + + final var destination = DefaultHttpDestination.builder(wireMockServer.baseUrl()).build(); client = new AiCoreService().withDestination(destination).client(); } diff --git a/orchestration/src/test/java/com/sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java b/orchestration/src/test/java/com/sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java index 3373f057..dcbd51de 100644 --- a/orchestration/src/test/java/com/sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java +++ b/orchestration/src/test/java/com/sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java @@ -5,6 +5,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.equalToJson; import static com.github.tomakehurst.wiremock.client.WireMock.get; import static com.github.tomakehurst.wiremock.client.WireMock.jsonResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.okJson; import static com.github.tomakehurst.wiremock.client.WireMock.post; import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.stubFor; @@ -43,7 +44,6 @@ import java.util.Map; import java.util.Objects; import java.util.function.Function; -import org.apache.hc.core5.http.HttpStatus; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.web.client.HttpClientErrorException; @@ -81,16 +81,12 @@ public class OrchestrationUnitTest { @BeforeEach void setup(WireMockRuntimeInfo server) { - stubFor( get(urlPathEqualTo("/v2/lm/deployments")) .withHeader("AI-Resource-Group", equalTo("default")) .willReturn( - aResponse() - .withStatus(HttpStatus.SC_OK) - .withHeader("content-type", "application/json") - .withBody( - """ + okJson( + """ { "resources": [ { @@ -103,6 +99,7 @@ void setup(WireMockRuntimeInfo server) { final DefaultHttpDestination destination = DefaultHttpDestination.builder(server.getHttpBaseUrl()).build(); + final var apiClient = new AiCoreService() .withDestination(destination) From 957510986a5d5ef499d4fcc586a60e193a4e07ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20D=C3=BCmont?= Date: Wed, 16 Oct 2024 15:35:23 +0200 Subject: [PATCH 2/5] Apply review comments --- .../sap/ai/sdk/core/AiCoreServiceTest.java | 23 ++++--------------- .../client/OrchestrationUnitTest.java | 3 ++- 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/core/src/test/java/com/sap/ai/sdk/core/AiCoreServiceTest.java b/core/src/test/java/com/sap/ai/sdk/core/AiCoreServiceTest.java index c12f77c5..d9e03756 100644 --- a/core/src/test/java/com/sap/ai/sdk/core/AiCoreServiceTest.java +++ b/core/src/test/java/com/sap/ai/sdk/core/AiCoreServiceTest.java @@ -56,7 +56,7 @@ void testLazyEvaluation() { } @Test - void testSimpleCase() { + void testDefaultCase() { // setup final var accessor = mock(ServiceBindingAccessor.class); DestinationResolver.setAccessor(accessor); @@ -95,24 +95,6 @@ void testBaseDestination() { assertThat(client.getBasePath()).isEqualTo("https://foo.bar/v2/"); } - @Test - void testDeployment() { - final var accessor = mock(ServiceBindingAccessor.class); - DestinationResolver.setAccessor(accessor); - doReturn(List.of(BINDING)).when(accessor).getServiceBindings(); - - // execution without errors - final var destination = new AiCoreService().destination(); - final var client = new AiCoreService().client(); - - // verification - assertThat(destination.get(DestinationProperty.URI)).contains("https://srv/v2/"); - assertThat(destination.get(DestinationProperty.AUTH_TYPE)).isEmpty(); - assertThat(destination.get(DestinationProperty.NAME)).singleElement(STRING).contains("aicore"); - assertThat(destination.get(AI_CLIENT_TYPE_KEY)).contains(AI_CLIENT_TYPE_VALUE); - assertThat(client.getBasePath()).isEqualTo("https://srv/v2/"); - verify(accessor, times(2)).getServiceBindings(); - } @Test void testCustomization() { @@ -139,5 +121,8 @@ protected ApiClient getApiClient(@Nonnull Destination destination) { final var destination = customServiceForDeployment.destination().asHttp(); assertThat(destination.getUri()).hasToString("https://ai/v2/inference/deployments/deployment/"); + + final var resourceGroup = customServiceForDeployment.getResourceGroup(); + assertThat(resourceGroup).isEqualTo("group"); } } diff --git a/orchestration/src/test/java/com/sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java b/orchestration/src/test/java/com/sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java index dcbd51de..28272a24 100644 --- a/orchestration/src/test/java/com/sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java +++ b/orchestration/src/test/java/com/sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java @@ -83,7 +83,7 @@ public class OrchestrationUnitTest { void setup(WireMockRuntimeInfo server) { stubFor( get(urlPathEqualTo("/v2/lm/deployments")) - .withHeader("AI-Resource-Group", equalTo("default")) + .withHeader("AI-Resource-Group", equalTo("my-resource-group")) .willReturn( okJson( """ @@ -104,6 +104,7 @@ void setup(WireMockRuntimeInfo server) { new AiCoreService() .withDestination(destination) .forDeploymentByScenario("orchestration") + .withResourceGroup("my-resource-group") .client(); client = new OrchestrationCompletionApi(apiClient); } From 2b5221c5aa2bb08a81f42c630c8987c7d06e4d6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20D=C3=BCmont?= Date: Wed, 16 Oct 2024 15:37:22 +0200 Subject: [PATCH 3/5] Add assertion on AI-Client-Type header --- .../sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/orchestration/src/test/java/com/sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java b/orchestration/src/test/java/com/sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java index 28272a24..f810db42 100644 --- a/orchestration/src/test/java/com/sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java +++ b/orchestration/src/test/java/com/sap/ai/sdk/orchestration/client/OrchestrationUnitTest.java @@ -84,6 +84,7 @@ void setup(WireMockRuntimeInfo server) { stubFor( get(urlPathEqualTo("/v2/lm/deployments")) .withHeader("AI-Resource-Group", equalTo("my-resource-group")) + .withHeader("AI-Client-Type", equalTo("AI SDK Java")) .willReturn( okJson( """ From 505ce61766e2761fcf380ee58e2c47a8075f9557 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20D=C3=BCmont?= Date: Wed, 16 Oct 2024 15:44:30 +0200 Subject: [PATCH 4/5] Minor JavaDoc fix --- core/src/main/java/com/sap/ai/sdk/core/AiCoreService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/com/sap/ai/sdk/core/AiCoreService.java b/core/src/main/java/com/sap/ai/sdk/core/AiCoreService.java index 53b65a09..eee44dee 100644 --- a/core/src/main/java/com/sap/ai/sdk/core/AiCoreService.java +++ b/core/src/main/java/com/sap/ai/sdk/core/AiCoreService.java @@ -61,7 +61,7 @@ public Destination destination() { * Set a specific base destination. * * @param destination The destination to be used for AI Core service calls. - * @return A new instance of the AI Core service. + * @return A new instance of the AI Core Service based on the provided destination. */ @Nonnull public AiCoreService withDestination(@Nonnull final Destination destination) { From d4d98fc921311728504989a4da04c7af0cb4197e Mon Sep 17 00:00:00 2001 From: SAP Cloud SDK Bot Date: Wed, 16 Oct 2024 13:45:18 +0000 Subject: [PATCH 5/5] Formatting --- core/src/test/java/com/sap/ai/sdk/core/AiCoreServiceTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/test/java/com/sap/ai/sdk/core/AiCoreServiceTest.java b/core/src/test/java/com/sap/ai/sdk/core/AiCoreServiceTest.java index d9e03756..7a275818 100644 --- a/core/src/test/java/com/sap/ai/sdk/core/AiCoreServiceTest.java +++ b/core/src/test/java/com/sap/ai/sdk/core/AiCoreServiceTest.java @@ -95,7 +95,6 @@ void testBaseDestination() { assertThat(client.getBasePath()).isEqualTo("https://foo.bar/v2/"); } - @Test void testCustomization() { final var customService =