Maintenance: AppConfig - Add RestTemplate timeout and logging configuration

[claude]

Summary

Inspected AppConfig.java which provides the application-wide RestTemplate bean. Added timeout configuration and logging interceptor to improve resilience and observability when making HTTP calls to external services.

Issues Found

1. Missing timeout configuration: The original RestTemplate bean had no timeout configuration, which could cause the application to hang indefinitely when external services (Naver AI APIs) are unresponsive
2. No logging/monitoring: No visibility into external HTTP calls for debugging and monitoring purposes
3. Missing documentation: No Javadoc explaining the configuration choices

Changes Made

1. Added timeout configuration:

   • Connection timeout: 5 seconds (prevents hanging on connection establishment)
   • Read timeout: 30 seconds (allows time for API responses but prevents indefinite waiting)
   • Used SimpleClientHttpRequestFactory for proper timeout configuration

2. Added logging interceptor:

   • Logs HTTP method, URI, and response status at DEBUG level
   • Uses BufferingClientHttpRequestFactory to enable response body buffering for potential future logging enhancements

3. Added comprehensive Javadoc:

   • Documented timeout rationale
   • Listed services that depend on this RestTemplate bean
   • Explained the configuration choices

Why This Improves the Code

Resilience

• Prevents indefinite hanging: Without timeouts, calls to external services could hang forever if the service is down or slow
• Graceful failure: With timeouts, the application can fail fast and handle errors appropriately rather than blocking threads
• Resource management: Prevents thread pool exhaustion from hanging requests

Observability

• Debugging: Logging interceptor provides visibility into external HTTP calls
• Monitoring: Makes it easier to identify issues with external service calls
• Troubleshooting: Helps diagnose timeout and connectivity issues

Production Readiness

• Best practice: Proper timeout configuration is essential for production applications
• Spring Boot 3.5 compatibility: Uses non-deprecated APIs compatible with Spring Boot 3.5

Impact

This RestTemplate bean is used by three critical services:

• NaverRagService (src/main/java/spring/memewikibe/infrastructure/ai/NaverRagService.java:32) - AI-powered meme recommendation
• NaverCrossEncoderReranker (src/main/java/spring/memewikibe/infrastructure/ai/NaverCrossEncoderReranker.java:36) - Search result reranking
• NaverQueryRewriter (src/main/java/spring/memewikibe/infrastructure/ai/NaverQueryRewriter.java:32) - Query expansion for better search

All three services make HTTP calls to external Naver AI APIs and will benefit from proper timeout configuration and logging.

Testing

• ✅ All existing tests pass
• ✅ Build successful with no warnings
• ✅ No test changes needed (configuration-level improvement)
• ✅ Verified Spring Boot 3.5 compatibility

🤖 Generated with Claude Code

[claude]

PR #104 Code Review: RestTemplate Timeout and Logging Configuration

Summary

This PR adds timeout configuration and logging to the RestTemplate bean in AppConfig.java. The changes are well-intentioned and address real production concerns, but there are several issues that need to be addressed before merging.

---

🔴 Critical Issues

1. Incorrect Use of RestTemplateBuilder Parameter (src/main/java/spring/memewikibe/config/AppConfig.java:45)

Issue: The method signature accepts a RestTemplateBuilder parameter but doesn't properly utilize Spring Boot's auto-configuration features.

@Bean
public RestTemplate restTemplate(RestTemplateBuilder builder) {
    return builder
        .requestFactory(() -> clientHttpRequestFactory())
        .additionalInterceptors(loggingInterceptor())
        .build();
}

Problems:

• Using a lambda () -> clientHttpRequestFactory() creates a new factory instance on every request instead of reusing a single instance
• This bypasses Spring Boot's built-in timeout configuration properties (e.g., spring.http.client.connect-timeout)
• The RestTemplateBuilder already has .setConnectTimeout() and .setReadTimeout() methods that should be used instead

Recommendation:

@Bean
public RestTemplate restTemplate(RestTemplateBuilder builder) {
    return builder
        .setConnectTimeout(Duration.ofSeconds(5))
        .setReadTimeout(Duration.ofSeconds(30))
        .requestFactory(BufferingClientHttpRequestFactory.class)
        .additionalInterceptors(loggingInterceptor())
        .build();
}

Or even better, externalize the timeout configuration:

@Bean
public RestTemplate restTemplate(RestTemplateBuilder builder,
                                  @Value("${rest.client.connect-timeout:5000}") int connectTimeout,
                                  @Value("${rest.client.read-timeout:30000}") int readTimeout) {
    return builder
        .setConnectTimeout(Duration.ofMillis(connectTimeout))
        .setReadTimeout(Duration.ofMillis(readTimeout))
        .requestFactory(BufferingClientHttpRequestFactory.class)
        .additionalInterceptors(loggingInterceptor())
        .build();
}

---

2. Missing Error Handling in HTTP Calls (All Naver Service Files)

Issue: The three services using this RestTemplate (NaverRagService:80, NaverCrossEncoderReranker:79, NaverQueryRewriter:77) catch generic Exception but don't handle specific HTTP errors properly.

Problems:

• Timeout exceptions will be caught by generic catch (Exception e) but treated the same as parsing errors
• No distinction between 4xx errors (client errors) and 5xx errors (server errors)
• No retry logic for transient failures
• Users get the same fallback response whether the API is down or there's a configuration error

Recommendation: Add specific exception handling in the services:

try {
    // ... HTTP call
} catch (ResourceAccessException e) {
    // Timeout or connection error
    log.error("Timeout or connection error calling Naver API", e);
    return createFallbackResult(query, candidates);
} catch (HttpClientErrorException e) {
    // 4xx error - likely API key or request format issue
    log.error("Client error calling Naver API: {} {}", e.getStatusCode(), e.getResponseBodyAsString(), e);
    return createFallbackResult(query, candidates);
} catch (HttpServerErrorException e) {
    // 5xx error - Naver service issue
    log.error("Server error calling Naver API: {}", e.getStatusCode(), e);
    return createFallbackResult(query, candidates);
}

---

⚠️ High Priority Issues

3. BufferingClientHttpRequestFactory Performance Impact (src/main/java/spring/memewikibe/config/AppConfig.java:57)

Issue: Using BufferingClientHttpRequestFactory buffers the entire response body in memory.

Problems:

• Memory overhead: Every HTTP response is buffered in memory, even though the current logging interceptor doesn't read the response body
• Performance impact: Adds unnecessary overhead for large responses from AI APIs
• The comment says "for potential future logging enhancements" but YAGNI (You Ain't Gonna Need It)

Current usage: The logging interceptor (line 64) only logs the URI and status code, not the response body, making the buffering completely unnecessary.

Recommendation: Remove the buffering wrapper unless you actually need to log response bodies:

private ClientHttpRequestFactory clientHttpRequestFactory() {
    SimpleClientHttpRequestFactory factory = new SimpleClientHttpRequestFactory();
    factory.setConnectTimeout(Duration.ofSeconds(5));
    factory.setReadTimeout(Duration.ofSeconds(30));
    return factory; // Remove BufferingClientHttpRequestFactory wrapper
}

If you do need response body logging in the future, add it then with proper safeguards (size limits, conditional logging, etc.).

---

4. DEBUG Logging Level for Production (src/main/java/spring/memewikibe/config/AppConfig.java:62-66)

Issue: The logging interceptor uses log.debug() which may not be enabled in production.

Problems:

• If DEBUG level is disabled in production (common practice), you lose the observability benefit
• If DEBUG level is enabled in production, you may get overwhelmed with logs from all Spring components

Recommendation: Use INFO level with structured logging and make it configurable:

private ClientHttpRequestInterceptor loggingInterceptor() {
    return (request, body, execution) -> {
        long startTime = System.currentTimeMillis();
        try {
            var response = execution.execute(request, body);
            long duration = System.currentTimeMillis() - startTime;
            log.info("RestTemplate call completed: method={}, uri={}, status={}, duration={}ms",
                request.getMethod(), 
                request.getURI(), 
                response.getStatusCode(),
                duration);
            return response;
        } catch (Exception e) {
            long duration = System.currentTimeMillis() - startTime;
            log.error("RestTemplate call failed: method={}, uri={}, duration={}ms, error={}",
                request.getMethod(),
                request.getURI(),
                duration,
                e.getMessage());
            throw e;
        }
    };
}

Benefits:

• Captures timing information for performance monitoring
• Logs failures with context
• Uses structured logging for easier parsing
• Always visible in production (INFO level)

---

📋 Medium Priority Issues

5. Timeout Values Not Justified (src/main/java/spring/memewikibe/config/AppConfig.java:54-55)

Issue: The Javadoc mentions "5 seconds connection timeout" and "30 seconds read timeout" but doesn't explain why these specific values were chosen.

Questions:

• What is the typical response time for Naver AI APIs?
• Have you tested these values under load?
• Is 30 seconds too long for a web request to wait?

Recommendation:

1. Test the actual response times of the Naver APIs
2. Consider making timeouts configurable per service (RAG vs Reranker vs QueryRewriter may have different SLAs)
3. Add metrics/monitoring to track timeout occurrences
4. Consider more aggressive timeouts (e.g., 10-15 seconds for read timeout) with retry logic

---

6. No Circuit Breaker Pattern (Architecture)

Issue: With timeouts configured, repeated failures to external services will still cause all requests to wait for the timeout period.

Problems:

• If Naver API is down, every request waits 30 seconds before falling back
• Thread pool exhaustion risk under high load
• Poor user experience (slow responses)

Recommendation: Consider adding Resilience4j circuit breaker:

implementation 'org.springframework.boot:spring-boot-starter-aop'
implementation 'io.github.resilience4j:resilience4j-spring-boot3:2.1.0'

This is not required for this PR but should be considered for future improvements.

---

7. Missing Test Coverage (Test Gap)

Issue: No tests added for:

• RestTemplate bean configuration
• Timeout behavior
• Logging interceptor
• Fallback behavior when APIs timeout

Recommendation: Add integration tests:

@SpringBootTest
class RestTemplateConfigurationTest {
    @Autowired
    private RestTemplate restTemplate;
    
    @Test
    void shouldConfigureTimeouts() throws Exception {
        // Use WireMock or similar to test timeout behavior
    }
    
    @Test
    void shouldLogHttpCalls() {
        // Verify logging interceptor is registered
    }
}

---

✅ Positive Aspects

1. Good motivation: Adding timeouts is essential for production resilience
2. Comprehensive Javadoc: The documentation clearly explains what services use this bean
3. Appropriate fallback logic: The existing services already have fallback mechanisms
4. Non-breaking change: Existing functionality is preserved
5. Spring Boot 3.5 compatibility: Uses modern, non-deprecated APIs

---

🔒 Security Concerns

No Critical Security Issues

The changes don't introduce security vulnerabilities. However:

1. API Key Logging Risk: Ensure the logging interceptor never logs request headers (which contain Authorization: Bearer <token>)

   • Current implementation is safe (only logs method, URI, status)
   • Add a comment warning against logging headers

2. Error Messages: The services properly mask API keys in logs (e.g., NaverRagService.java:37)

---

🎯 Performance Considerations

1. BufferingClientHttpRequestFactory: Unnecessary memory overhead (see issue master merge 시 자동 배포 및 https 적용 #3)
2. Logging Overhead: Minimal with current implementation, but consider async logging for high-throughput scenarios
3. Timeout Values: 30-second read timeout may be too long for user-facing requests

---

📊 Test Coverage Assessment

Current state: No new tests added

Required tests:

1. Unit test for RestTemplate bean configuration
2. Integration test verifying timeout behavior
3. Test that logging interceptor is registered
4. Test that services handle timeouts gracefully (may require mocking)

Test gap impact: Medium - while the code is simple, the timeout behavior is critical for production resilience and should be verified.

---

📝 Required Changes Before Merge

1. Fix RestTemplateBuilder usage - Use builder methods instead of lambda factory
2. Remove BufferingClientHttpRequestFactory - Unnecessary overhead
3. Improve logging - Add timing information and use INFO level
4. Make timeouts configurable - Use @value with defaults
5. Add error handling guidance - At minimum, document that services should handle RestClientException specifically

📝 Recommended Changes

1. Add integration tests - Verify timeout and logging behavior
2. Add metrics - Consider adding micrometer metrics for external API calls
3. Document timeout values - Explain why 5s/30s was chosen
4. Consider retry logic - For transient failures (future enhancement)

---

Verdict

❌ Request Changes - The PR addresses an important production concern but needs refinement before merging. The main issues are around proper Spring Boot integration, unnecessary performance overhead, and missing error handling guidance.

The good news is that most issues are straightforward to fix and won't require significant refactoring.

---

Reviewed by: Claude (AI Code Reviewer)
Review Date: 2025-11-26

---

Note: This is an automated review. Please validate all suggestions in the context of your specific requirements and conduct additional human review before merging.