review: address Copilot review round 2 on sap-demo-java

slayerjain · claude · slayerjain · commit 679fc8b8ce52 · 2026-04-22T11:49:44.000+05:30
Applies 17 fixes from #126 review: Alignment / correctness: - Dockerfile: amazoncorretto:25 -> 21 (match pom java.version=21) - docker-compose.yml, k8s/postgres.yaml: postgres:15-alpine -> 16-alpine (align with README/PR "Postgres 16") - kind-config.yaml: add extraPortMappings for 30080 so the NodePort Service is reachable at http://localhost:30080 (previously 80/443 only) - SapClientConfig: plumb readTimeoutSeconds through RestTemplateBuilder (HttpComponentsClientHttpRequestFactory.setReadTimeout was removed in Spring 6 / HttpClient5) + drop Javadoc reference to the never-implemented IdentityEncodingInterceptor Bugs / robustness: - TagController.remove: trim + lowercase before echoing "tag" in response, so the response matches the value used by the service - GlobalExceptionHandler.handleUnexpected: stop leaking ex.getMessage() to clients; log full exception server-side with correlationId + path, return a generic body that invites the caller to quote the correlationId - TagService.add: replace exists() + findAll().stream().filter().orElseThrow() with a single findByCustomerIdAndTag Optional lookup, eliminating the race window + the unsafe orElseThrow(); CustomerTagRepository gains the matching finder - Customer360AggregatorService: split the combined `catch (InterruptedException | ExecutionException)` so Thread.currentThread().interrupt() fires only for InterruptedException - deploy_kind.sh build_and_load: replace `[ src -nt target/...jar ]` (which only compares the src directory mtime) with a `find src pom.xml -newer` sweep so rebuilds actually trigger after any file change Test / housekeeping: - Customer360ApplicationTests: add @AutoConfigureTestDatabase(Replace.ANY) + H2 test dep so the context-load smoke test runs without a live Postgres; disable Flyway (migrations are Postgres-specific) and let Hibernate generate the schema from entities at test time. Verified: context loads in ~5.6s locally - application.yml: drop logging.pattern.console — logback-spring.xml owns the pattern (and overrides Boot's key when present); avoids drift - keploy.yml: typo "configration" -> "configuration" - README.md Quick start: drop absolute /home/shubham/... path, use repo-relative `cd sap-demo-java` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
diff --git a/sap-demo-java/Dockerfile b/sap-demo-java/Dockerfile
@@ -1,13 +1,14 @@
 # syntax=docker/dockerfile:1.7
 #
-# Uses amazoncorretto:25 — Amazon Linux 2023-based, minimal, JDK-ready.
-# The jar is built outside Docker (mvn package); this image just packages it
-# and runs it. K8s securityContext enforces the non-root runtime UID (1001).
+# Uses amazoncorretto:21 — Amazon Linux 2023-based, minimal, JDK-ready.
+# Matches the Maven toolchain (pom.xml targets Java 21). The jar is built
+# outside Docker (mvn package); this image just packages it and runs it.
+# K8s securityContext enforces the non-root runtime UID (1001).
 #
 # Docker Hub pull rate limits are avoided by relying on the locally cached
-# amazoncorretto:25 image.
+# amazoncorretto:21 image.
 
-FROM amazoncorretto:25
+FROM amazoncorretto:21
 
 WORKDIR /app
 
diff --git a/sap-demo-java/README.md b/sap-demo-java/README.md
@@ -81,7 +81,7 @@ Swagger UI inside the kind cluster: `http://localhost:30080/swagger-ui.html`.
 ## Quick start (kind cluster)
 
 ```bash
-cd /home/shubham/tricentis/sap_testing/sap_demo_java
+cd sap-demo-java
 
 # 1. One-time: drop your SAP API key into .env
 cp .env.example .env
diff --git a/sap-demo-java/deploy_kind.sh b/sap-demo-java/deploy_kind.sh
@@ -194,7 +194,18 @@ build_and_load() {
     fail "cluster '${CLUSTER_NAME}' does not exist — run '$0 -c ${CLUSTER_NAME} cluster' first"
     exit 1
   fi
-  if [ ! -f target/customer360.jar ] || [ src -nt target/customer360.jar ]; then
+  # Rebuild the jar if any source file under src/ or pom.xml is newer than
+  # the jar — `dir -nt file` only compares the directory mtime, which does
+  # NOT move when files inside the directory are edited, so a plain
+  # `[ src -nt target/customer360.jar ]` would happily reuse a stale jar
+  # after a code change.
+  needs_build=1
+  if [ -f target/customer360.jar ]; then
+    if [ -z "$(find src pom.xml -type f -newer target/customer360.jar -print -quit 2>/dev/null)" ]; then
+      needs_build=0
+    fi
+  fi
+  if [ "${needs_build}" -eq 1 ]; then
     say "building customer360.jar (mvn package)"
     mvn -q -B -DskipTests package
   else
diff --git a/sap-demo-java/docker-compose.yml b/sap-demo-java/docker-compose.yml
@@ -5,7 +5,7 @@
 # For the demo, prefer ./deploy_kind.sh (matches production topology).
 services:
   postgres:
-    image: postgres:15-alpine
+    image: postgres:16-alpine
     container_name: customer360-postgres
     environment:
       POSTGRES_DB: customer360
diff --git a/sap-demo-java/k8s/postgres.yaml b/sap-demo-java/k8s/postgres.yaml
@@ -1,4 +1,4 @@
-# Postgres 15 for the Customer 360 local store (tags, notes, audit).
+# Postgres 16 for the Customer 360 local store (tags, notes, audit).
 #
 # Kept in a single file for demo-legibility: Secret + Service + Deployment.
 # emptyDir volume (not PVC) — fresh DB on every pod restart, which is fine
@@ -68,7 +68,7 @@ spec:
       terminationGracePeriodSeconds: 15
       containers:
         - name: postgres
-          image: postgres:15-alpine
+          image: postgres:16-alpine
           imagePullPolicy: IfNotPresent
           ports:
             - name: postgres
diff --git a/sap-demo-java/keploy.yml b/sap-demo-java/keploy.yml
@@ -119,4 +119,4 @@ serverPort: 0
 mockDownload:
     registryIds: []
 
-# Visit [https://keploy.io/docs/running-keploy/configuration-file/] to learn about using keploy through configration file.
+# Visit [https://keploy.io/docs/running-keploy/configuration-file/] to learn about using keploy through configuration file.
diff --git a/sap-demo-java/kind-config.yaml b/sap-demo-java/kind-config.yaml
@@ -14,4 +14,9 @@ nodes:
     protocol: TCP
   - containerPort: 443
     hostPort: 443
+    protocol: TCP
+  # The NodePort Service (k8s/service.yaml) binds 30080 on every node; this
+  # mapping makes http://localhost:30080 reach the single-node kind cluster.
+  - containerPort: 30080
+    hostPort: 30080
     protocol: TCP
diff --git a/sap-demo-java/pom.xml b/sap-demo-java/pom.xml
@@ -125,6 +125,13 @@
             <artifactId>spring-boot-starter-test</artifactId>
             <scope>test</scope>
         </dependency>
+        <!-- H2 replaces Postgres at test time (@AutoConfigureTestDatabase) so
+             the context-load smoke test can run without a live DB. -->
+        <dependency>
+            <groupId>com.h2database</groupId>
+            <artifactId>h2</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>
diff --git a/sap-demo-java/src/main/java/com/tricentisdemo/sap/customer360/config/SapClientConfig.java b/sap-demo-java/src/main/java/com/tricentisdemo/sap/customer360/config/SapClientConfig.java
@@ -12,7 +12,6 @@
 import org.springframework.http.client.ClientHttpRequestExecution;
 import org.springframework.http.client.ClientHttpRequestInterceptor;
 import org.springframework.http.client.ClientHttpResponse;
-import org.springframework.http.client.HttpComponentsClientHttpRequestFactory;
 import org.springframework.scheduling.annotation.EnableAsync;
 import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor;
 import org.springframework.web.client.RestTemplate;
@@ -25,15 +24,10 @@
  * Configures the {@link RestTemplate} used for all outbound calls to the SAP
  * S/4HANA Business Partner OData service.
  *
- * <p>Three interceptors are attached:
+ * <p>Two interceptors are attached:
  * <ol>
  *   <li><b>SapAuthInterceptor</b> — stamps the required {@code APIKey} header
  *       (sandbox) or {@code Authorization: Bearer} (production tenant).</li>
- *   <li><b>IdentityEncodingInterceptor</b> — forces {@code Accept-Encoding: identity}
- *       so bodies are not gzip-compressed in transit. In production this is
- *       optional; here it keeps Keploy-captured YAML mocks human-readable,
- *       which matters for demo-grade visibility and for diff-reviewing
- *       contract changes in pull requests.</li>
  *   <li><b>CorrelationIdInterceptor</b> — propagates the inbound request's
  *       correlation id into the outbound SAP call so traces chain across
  *       the hop.</li>
@@ -77,13 +71,17 @@ public RestTemplate sapRestTemplate(RestTemplateBuilder builder) {
                 + "Outbound SAP calls will fail with 401.");
         }
 
-        HttpComponentsClientHttpRequestFactory factory = new HttpComponentsClientHttpRequestFactory();
-        factory.setConnectTimeout((int) Duration.ofSeconds(connectTimeoutSeconds).toMillis());
-        factory.setConnectionRequestTimeout((int) Duration.ofSeconds(connectTimeoutSeconds).toMillis());
-
+        // Spring Boot auto-selects HttpClient5 when httpclient5 is on the
+        // classpath (see pom.xml). Setting connect+read timeouts via the
+        // RestTemplateBuilder plumbs them through to the underlying
+        // HttpClient5 RequestConfig (connect) and SocketConfig (soTimeout ==
+        // read timeout), which is the only API surface that still exists in
+        // Spring 6 — HttpComponentsClientHttpRequestFactory.setReadTimeout
+        // was removed with the HttpClient5 migration.
         return builder
             .rootUri(baseUrl)
-            .requestFactory(() -> factory)
+            .setConnectTimeout(Duration.ofSeconds(connectTimeoutSeconds))
+            .setReadTimeout(Duration.ofSeconds(readTimeoutSeconds))
             .additionalInterceptors(
                 new SapAuthInterceptor(apiKey, bearerToken),
                 new CorrelationIdInterceptor()
diff --git a/sap-demo-java/src/main/java/com/tricentisdemo/sap/customer360/repository/CustomerTagRepository.java b/sap-demo-java/src/main/java/com/tricentisdemo/sap/customer360/repository/CustomerTagRepository.java
@@ -9,13 +9,14 @@
 import org.springframework.transaction.annotation.Transactional;
 
 import java.util.List;
+import java.util.Optional;
 
 @Repository
 public interface CustomerTagRepository extends JpaRepository<CustomerTag, Long> {
 
     List<CustomerTag> findAllByCustomerIdOrderByCreatedAtDesc(String customerId);
 
-    boolean existsByCustomerIdAndTag(String customerId, String tag);
+    Optional<CustomerTag> findByCustomerIdAndTag(String customerId, String tag);
 
     @Modifying
     @Transactional
diff --git a/sap-demo-java/src/main/java/com/tricentisdemo/sap/customer360/service/Customer360AggregatorService.java b/sap-demo-java/src/main/java/com/tricentisdemo/sap/customer360/service/Customer360AggregatorService.java
@@ -122,13 +122,22 @@ public Customer360View aggregate(String customerId) {
             roles = rolesF.getNow(List.of());
             tags = tagsF.getNow(List.of());
             notes = notesF.getNow(List.of());
-        } catch (InterruptedException | ExecutionException e) {
+        } catch (InterruptedException e) {
+            // Re-assert interrupt so callers higher up can observe it.
             Thread.currentThread().interrupt();
             log.warn("Aggregation interrupted for bp={}: {}", customerId, e.getMessage());
             addresses = addressesF.getNow(List.of());
             roles = rolesF.getNow(List.of());
             tags = tagsF.getNow(List.of());
             notes = notesF.getNow(List.of());
+        } catch (ExecutionException e) {
+            // Execution failed inside an async stage; the interrupt flag of
+            // the caller is unaffected, so do NOT call Thread.interrupt().
+            log.warn("Aggregation failed for bp={}: {}", customerId, e.getMessage());
+            addresses = addressesF.getNow(List.of());
+            roles = rolesF.getNow(List.of());
+            tags = tagsF.getNow(List.of());
+            notes = notesF.getNow(List.of());
         }
 
         int elapsed = (int) Duration.between(start, Instant.now()).toMillis();
diff --git a/sap-demo-java/src/main/java/com/tricentisdemo/sap/customer360/service/TagService.java b/sap-demo-java/src/main/java/com/tricentisdemo/sap/customer360/service/TagService.java
@@ -32,23 +32,20 @@ public CustomerTag add(String customerId, String tag, String createdBy) {
             throw new IllegalArgumentException("tag must not be blank");
         }
         String normalised = tag.trim().toLowerCase();
-        if (repo.existsByCustomerIdAndTag(customerId, normalised)) {
-            // Idempotent — return the existing row instead of 409'ing.
-            return repo.findAllByCustomerIdOrderByCreatedAtDesc(customerId).stream()
-                .filter(t -> t.getTag().equals(normalised))
-                .findFirst()
-                .orElseThrow();
-        }
+        // Single-lookup idempotency: if the row exists, return it; otherwise
+        // insert. We retry the lookup on DataIntegrityViolationException to
+        // cover the narrow window where a concurrent insert beats us.
+        return repo.findByCustomerIdAndTag(customerId, normalised)
+            .orElseGet(() -> insertOrFetchExisting(customerId, normalised, createdBy));
+    }
+
+    private CustomerTag insertOrFetchExisting(String customerId, String tag, String createdBy) {
         try {
-            return repo.save(new CustomerTag(customerId, normalised, createdBy));
+            return repo.save(new CustomerTag(customerId, tag, createdBy));
         } catch (DataIntegrityViolationException race) {
-            // Unique constraint lost a race to another thread — treat as
-            // already-present.
-            log.debug("tag add race on ({},{}); fetching existing", customerId, normalised);
-            return repo.findAllByCustomerIdOrderByCreatedAtDesc(customerId).stream()
-                .filter(t -> t.getTag().equals(normalised))
-                .findFirst()
-                .orElseThrow();
+            log.debug("tag add race on ({},{}); fetching existing", customerId, tag);
+            return repo.findByCustomerIdAndTag(customerId, tag)
+                .orElseThrow(() -> race);
         }
     }
 
diff --git a/sap-demo-java/src/main/java/com/tricentisdemo/sap/customer360/web/GlobalExceptionHandler.java b/sap-demo-java/src/main/java/com/tricentisdemo/sap/customer360/web/GlobalExceptionHandler.java
@@ -89,9 +89,15 @@ public ResponseEntity<ProblemResponse> handleDb(DataAccessException ex, HttpServ
 
     @ExceptionHandler(Exception.class)
     public ResponseEntity<ProblemResponse> handleUnexpected(Exception ex, HttpServletRequest req) {
-        log.error("Unhandled exception", ex);
+        String cid = MDC.get(CorrelationIdInterceptor.MDC_KEY);
+        // Full exception (message, stack, class) stays server-side only. Clients
+        // receive a generic message plus the correlation id, which they can
+        // quote back to operators when opening a ticket.
+        log.error("Unhandled exception [{}] path={} — check server logs", cid, req.getRequestURI(), ex);
         ProblemResponse body = baseProblem(HttpStatus.INTERNAL_SERVER_ERROR,
-            "Internal Server Error", ex.getMessage(), req);
+            "Internal Server Error",
+            "An unexpected error occurred. Quote the correlationId when contacting support.",
+            req);
         return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR)
             .headers(commonHeaders(null))
             .body(body);
diff --git a/sap-demo-java/src/main/java/com/tricentisdemo/sap/customer360/web/TagController.java b/sap-demo-java/src/main/java/com/tricentisdemo/sap/customer360/web/TagController.java
@@ -73,8 +73,9 @@ public ResponseEntity<Map<String, Object>> remove(
         @PathVariable @NotBlank String tag
     ) {
         log.info("DELETE tag={} for bp={}", tag, customerId);
-        boolean deleted = tags.remove(customerId, tag);
+        String normalised = tag.trim().toLowerCase();
+        boolean deleted = tags.remove(customerId, normalised);
         audit.record(customerId, "tags.delete", null);
-        return ResponseEntity.ok(Map.of("deleted", deleted, "tag", tag.toLowerCase()));
+        return ResponseEntity.ok(Map.of("deleted", deleted, "tag", normalised));
     }
 }
diff --git a/sap-demo-java/src/main/resources/application.yml b/sap-demo-java/src/main/resources/application.yml
@@ -166,12 +166,13 @@ springdoc:
     operationsSorter: method
 
 # ----------------------------------------------------------------------------
-# Logging — structured pattern with correlation id so every line is traceable.
+# Logging — levels only. The console/JSON patterns live in logback-spring.xml
+# (which is the single source of truth; Boot's `logging.pattern.*` keys are
+# ignored whenever logback-spring.xml is present, so duplicating them here
+# invited drift).
 # ----------------------------------------------------------------------------
 logging:
   level:
     root: INFO
     com.tricentisdemo.sap: INFO
     org.springframework.web: INFO
-  pattern:
-    console: "%d{yyyy-MM-dd HH:mm:ss.SSS} %-5level [%X{correlationId:-}] %logger{40} - %msg%n"
diff --git a/sap-demo-java/src/test/java/com/tricentisdemo/sap/customer360/Customer360ApplicationTests.java b/sap-demo-java/src/test/java/com/tricentisdemo/sap/customer360/Customer360ApplicationTests.java
@@ -1,21 +1,31 @@
 package com.tricentisdemo.sap.customer360;
 
 import org.junit.jupiter.api.Test;
+import org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabase;
 import org.springframework.boot.test.context.SpringBootTest;
 import org.springframework.test.context.TestPropertySource;
 
 /**
  * Minimal context-load smoke test. Verifies the Spring wiring is correct —
  * RestTemplate, interceptors, executor, Resilience4j decorators all bind.
  *
- * <p>The full integration test suite is intentionally out of scope for this
- * reference service: the regression layer is provided by Keploy mocks
- * recorded from real SAP traffic. That is the whole point of the demo.
+ * <p>{@link AutoConfigureTestDatabase} replaces the configured Postgres
+ * with an in-memory H2 at test time so this test can run in environments
+ * without a live DB. Flyway is disabled for the same reason (migrations
+ * are Postgres-specific); Hibernate creates the schema from entities.
+ *
+ * <p>The full integration test suite is intentionally out of scope for
+ * this reference service: the regression layer is provided by Keploy
+ * mocks recorded from real SAP traffic. That is the whole point of the
+ * demo.
  */
 @SpringBootTest
+@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.ANY)
 @TestPropertySource(properties = {
     "sap.api.base-url=https://example.invalid",
-    "sap.api.key=test-key"
+    "sap.api.key=test-key",
+    "spring.flyway.enabled=false",
+    "spring.jpa.hibernate.ddl-auto=create-drop"
 })
 class Customer360ApplicationTests {
 
