Conversation
WalkthroughIntroduces selective Feign retry/error-decoding, centralizes retry bean into FeignConfig, adds message masking in SafeFeignExecutor, adds a test-only Feign dependency, formats a method chain, and adds unit tests for the new ErrorDecoder. Removes the legacy FeignRetryConfig. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FeignClient
participant RemoteAPI
participant ErrorDecoder as SelectiveRetryErrorDecoder
participant Retryer
Caller->>FeignClient: invoke()
FeignClient->>RemoteAPI: HTTP request
alt Success
RemoteAPI-->>FeignClient: 2xx response
FeignClient-->>Caller: result
else Error response
RemoteAPI-->>FeignClient: 4xx/5xx response
FeignClient->>ErrorDecoder: decode(methodKey, response)
alt 429/408/5xx AND idempotent (GET/HEAD/OPTIONS)
ErrorDecoder-->>FeignClient: RetryableException (+optional Retry-After)
FeignClient->>Retryer: backoff & retry (max=3)
Retryer-->>FeignClient: next attempt / give up
else Non-retryable
ErrorDecoder-->>FeignClient: Exception (default)
end
FeignClient-->>Caller: exception
end
sequenceDiagram
participant Caller
participant SafeExec as SafeFeignExecutor
participant FeignClient
participant Mask as mask()
Caller->>SafeExec: run(supplier)
SafeExec->>FeignClient: call
alt FeignException
FeignClient-->>SafeExec: throws FeignException (status, message)
SafeExec->>Mask: sanitize(message)
Mask-->>SafeExec: masked message
SafeExec-->>Caller: throws ExternalApiException("Feign API 호출(<status>): <masked>")
else Other
FeignClient-->>SafeExec: result/other exception
SafeExec-->>Caller: result/propagate
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
build.gradle (1)
83-83: Avoid versionless feign-core in tests
The test scope declaresio.github.openfeign:feign-corewithout a version, and there’s no Spring Cloud BOM imported under thedependencyManagementplugin to manage it. Sincespring-cloud-starter-openfeign:4.1.3already brings in Feign transitively, this line is redundant and can break your Gradle build. You have three options:• Remove the redundant test dependency:
- testImplementation("io.github.openfeign:feign-core")• Pin an explicit Feign version that matches Spring Cloud OpenFeign (e.g., 13.3.0):
- testImplementation("io.github.openfeign:feign-core") + testImplementation("io.github.openfeign:feign-core:13.3.0")• Or import the Spring Cloud BOM so you can keep a versionless declaration:
dependencyManagement { imports { mavenBom "org.springframework.cloud:spring-cloud-dependencies:2023.0.3" } }Please verify that your project’s BOM configuration (if any) already manages
feign-corebefore leaving it versionless.src/main/java/com/example/spot/common/infrastructure/feign/SafeFeignExecutor.java (1)
24-35: Widen masking coverage (tokens with +/ and query params; API keys).The current regex misses some token shapes (e.g., base64 with + or /) and URL query params. Add API key headers as well.
Apply this diff to broaden redaction:
String out = s; out = out.replaceAll("(?i)(Authorization)\\s*[:=]\\s*([^\\r\\n]+)", "$1: [REDACTED]"); out = out.replaceAll("(?i)(Set-Cookie|Cookie)\\s*[:=]\\s*([^\\r\\n]+)", "$1: [REDACTED]"); - out = out.replaceAll("(?i)(access[_-]?token|id[_-]?token|refresh[_-]?token)\\s*[:=]\\s*([\\w\\.-]+)", - "$1=[REDACTED]"); + // API keys in headers + out = out.replaceAll("(?i)(X-Api-Key|Api-Key)\\s*[:=]\\s*([^\\r\\n]+)", "$1: [REDACTED]"); + // token-like key/value pairs (allow +, /, = and stop at whitespace/quote/semicolon) + out = out.replaceAll("(?i)(access[_-]?token|id[_-]?token|refresh[_-]?token)\\s*[:=]\\s*([^\\s\\r\\n;\"']+)", + "$1=[REDACTED]"); out = out.replaceAll("(?i)(\"(?:password|pass|secret|token|authorization)\"\\s*:\\s*\")([^\"]+)(\")", "$1[REDACTED]$3"); + // query parameters (e.g., ?access_token=... or &token=...) + out = out.replaceAll("(?i)([?&](?:access[_-]?token|id[_-]?token|refresh[_-]?token|token|authorization)=)([^&\\s]+)", + "$1[REDACTED]"); return out;src/main/java/com/example/spot/common/infrastructure/feign/retry/SelectiveRetryErrorDecoder.java (3)
31-33: Confirm intended idempotent set (PUT/DELETE are spec-idempotent but currently excluded).By spec, PUT and DELETE are idempotent (though not safe). If your APIs observe idempotency semantics, consider including them; otherwise, keeping the conservative set is fine.
Minimal change if you decide to allow them:
- boolean idempotent = method.equals(GET) || method.equals(HEAD) || method.equals(OPTIONS); + boolean idempotent = method.equals(GET) || method.equals(HEAD) || method.equals(OPTIONS) + || method.equals("PUT") || method.equals("DELETE");
42-54: Handle Retry-After case-insensitively and support HTTP-date format.HTTP header names are case-insensitive, and Retry-After may be an HTTP-date or seconds. Current code only matches exact "Retry-After" and parses integers.
Apply this diff to improve robustness:
- private Long extractRetryAfter(Map<String, Collection<String>> headers) { - Collection<String> value = headers.getOrDefault(RETRY_AFTER, Collections.emptyList()); + private Long extractRetryAfter(Map<String, Collection<String>> headers) { + Collection<String> value = Collections.emptyList(); + for (Map.Entry<String, Collection<String>> e : headers.entrySet()) { + if (RETRY_AFTER.equalsIgnoreCase(e.getKey())) { + value = e.getValue(); + break; + } + } if (value.isEmpty()) { return null; } String v = value.iterator().next().trim(); try { long seconds = Long.parseLong(v); return System.currentTimeMillis() + (seconds * THRESHOLD); - } catch (NumberFormatException ignore) { - return null; + } catch (NumberFormatException ignore) { + // Try HTTP-date (RFC 7231) + try { + return java.time.ZonedDateTime.parse(v, java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME) + .toInstant().toEpochMilli(); + } catch (Exception ignored) { + return null; + } } }No additional imports needed if you inline FQCNs as above. If you prefer imports, add:
import java.time.ZonedDateTime; import java.time.format.DateTimeFormatter;
56-65: Optional: avoid buffering large bodies into memory.If you don’t need the response body for retry logic/logging, you can skip
readBodyand pass null to reduce memory footprint on large error payloads.Example:
- return new RetryableException(status, RETRYABLE_STATUS + status, response.request().httpMethod(), - retryAfter, response.request(), readBody(response), response.headers()); + return new RetryableException(status, RETRYABLE_STATUS + status, response.request().httpMethod(), + retryAfter, response.request(), null, response.headers());src/test/java/com/example/spot/common/feign/SelectiveRetryErrorDecoderTest.java (2)
51-56: Consider asserting retryAfter presence when header provided.This would validate that Retry-After is parsed and propagated.
Example assertion (adapt to your Feign version’s API):
RetryableException re = (RetryableException) ex; // e.g., assertThat(re.retryAfter()).isNotNull(); // if available in your Feign version assertThat(re.getMessage()).contains("retryable status 429");
65-70: Add a few more cases for robustness (optional).
- 408 (Request Timeout) with GET should be retryable.
- Retry-After as HTTP-date and with lowercase header name (case-insensitive lookup).
- HEAD/OPTIONS 5xx should be retryable.
- If you later include PUT/DELETE as idempotent, add matching tests.
Example additions:
@Test void GET_408_is_retryable() { Response res = newResponse(408, "GET", null, null); Exception ex = decoder.decode("key", res); assertThat(ex).isInstanceOf(RetryableException.class); } @Test void GET_429_with_httpDate_retryAfter_is_retryable() { Map<String, Collection<String>> headers = Map.of("retry-after", List.of("Wed, 21 Oct 2015 07:28:00 GMT")); Response res = newResponse(429, "GET", headers, null); Exception ex = decoder.decode("key", res); assertThat(ex).isInstanceOf(RetryableException.class); } @Test void HEAD_503_is_retryable() { Response res = newResponse(503, "HEAD", null, null); Exception ex = decoder.decode("key", res); assertThat(ex).isInstanceOf(RetryableException.class); }src/main/java/com/example/spot/common/config/FeignConfig.java (1)
14-17: Confirm global ErrorDecoder intent and consider guard
Verified there are no otherErrorDecoderbeans, no custom@FeignClient(configuration=…), and no duplicateRetryerbeans in the codebase—this global decoder is currently the sole implementation. If you intend it to apply project-wide, no further action is required; otherwise, to allow future overrides without bean conflicts, add a conditional guard:• Location to update:
src/main/java/com/example/spot/common/config/FeignConfig.java:14-17• Optional refactor diff:
@Configuration public class FeignConfig { - @Bean + @Bean + @ConditionalOnMissingBean(ErrorDecoder.class) public ErrorDecoder errorDecoder() { return new SelectiveRetryErrorDecoder(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
build.gradle(1 hunks)src/main/java/com/example/spot/auth/application/refactor/member/RefreshTokenStore.java(1 hunks)src/main/java/com/example/spot/common/config/FeignConfig.java(1 hunks)src/main/java/com/example/spot/common/config/FeignRetryConfig.java(0 hunks)src/main/java/com/example/spot/common/infrastructure/feign/SafeFeignExecutor.java(1 hunks)src/main/java/com/example/spot/common/infrastructure/feign/retry/SelectiveRetryErrorDecoder.java(1 hunks)src/test/java/com/example/spot/common/feign/SelectiveRetryErrorDecoderTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/example/spot/common/config/FeignRetryConfig.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/test/java/com/example/spot/common/feign/SelectiveRetryErrorDecoderTest.java (1)
src/main/java/com/example/spot/common/infrastructure/feign/retry/SelectiveRetryErrorDecoder.java (1)
SelectiveRetryErrorDecoder(12-66)
src/main/java/com/example/spot/common/config/FeignConfig.java (1)
src/main/java/com/example/spot/common/infrastructure/feign/retry/SelectiveRetryErrorDecoder.java (1)
SelectiveRetryErrorDecoder(12-66)
🔇 Additional comments (7)
src/main/java/com/example/spot/auth/application/refactor/member/RefreshTokenStore.java (1)
17-19: LGTM: style-only chain formatting.No behavioral changes; the builder call remains the same. Looks good.
src/main/java/com/example/spot/common/infrastructure/feign/SafeFeignExecutor.java (1)
16-21: Good: centralized masking with status-aware ExternalApiException message.Catches FeignException, masks sensitive bits, and includes HTTP status in the message. This reduces PII leakage while keeping useful context.
src/main/java/com/example/spot/common/infrastructure/feign/retry/SelectiveRetryErrorDecoder.java (2)
28-35: Selective retry conditions look good.
- Retries on 5xx, 429, 408.
- Restricts to idempotent HTTP methods to avoid unsafe replays.
38-40: Double-check RetryableException constructor compatibility across Feign versions.You’re passing a Long for retryAfter, which is correct for newer Feign. Older Feign used Date. Ensure your resolved feign-core matches this signature to avoid runtime/linking issues.
To verify locally:
- Inspect the constructor in your resolved
feign-corejar (IDE orgradlew dependencyInsight).- If you need broader compatibility, convert to Date when you have a timestamp:
new Date(retryAfter)(and use the constructor that accepts Date).src/test/java/com/example/spot/common/feign/SelectiveRetryErrorDecoderTest.java (2)
44-48: Good: covers GET 5xx as retryable.Solid baseline test ensuring the primary retry condition works.
58-63: Good: ensures non-idempotent method is not retried.This guards against accidental broadening of retry scope.
src/main/java/com/example/spot/common/config/FeignConfig.java (1)
3-5: LGTM: Centralized Feign retry wiring (imports + bean exposure) looks correctNew imports for SelectiveRetryErrorDecoder, Retryer, and ErrorDecoder are appropriate. Pulling retry/error-decoding into FeignConfig consolidates concerns and removes the need for a separate Retry config class.
Also applies to: 7-7
| @Bean | ||
| public Retryer retryer() { | ||
| // 초기대기 200ms, 최대대기 800ms, 최대 3번 재시도 | ||
| return new Retryer.Default(200, 800, 3); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Double-check Feign maxAttempts semantics vs. the comment; adjust either comment or value
In feign.Retryer.Default(period, maxPeriod, maxAttempts), maxAttempts typically represents the total number of attempts (including the initial try), not the number of retries. Your comment says “최대 3번 재시도” which implies 3 retries (i.e., 4 total attempts), while the code sets 3 maxAttempts.
- If you intend 3 retries: set maxAttempts to 4.
- If you intend a total of 3 attempts: keep the value and update the comment.
Option A (3 retries → 4 total attempts):
- // 초기대기 200ms, 최대대기 800ms, 최대 3번 재시도
- return new Retryer.Default(200, 800, 3);
+ // 초기대기 200ms, 최대대기 800ms, 최대 3번 재시도 (총 4회 시도)
+ return new Retryer.Default(200, 800, 4);Option B (3 total attempts → fix comment):
- // 초기대기 200ms, 최대대기 800ms, 최대 3번 재시도
+ // 초기대기 200ms, 최대대기 800ms, 최대 3회 시도 (Feign maxAttempts는 총 시도 수)
return new Retryer.Default(200, 800, 3);To confirm semantics for your Feign version:
🌐 Web query:
In OpenFeign's Retryer.Default constructor, does maxAttempts represent total attempts (including the first) or only retries?
💡 Result:
In OpenFeign's Retryer.Default constructor, the maxAttempts parameter represents the total number of attempts, including the initial call and all retries[3]. This means if maxAttempts is set to 3, Feign will make up to three calls in total: the initial attempt plus up to two retries if necessary[3].
Supporting details:
- The
matez blogclarifies in a code example: by settingmaxAttemptsto 3, the test verifies that Feign makes a total of 3 attempts, not just 3 retries after the initial attempt[3]. - This is further supported by descriptions from other sources stating that Feign will "retry only a given number of times" and the configuration option is named for "maximum number of attempts," not retries, meaning initial plus retries[1][4].
Key points:
- maxAttempts = 1: Only the initial call, no retries.
- maxAttempts = N (>1): Initial call plus up to N-1 retries.
Therefore, for Feign's Retryer.Default, maxAttempts counts total attempts, including the first[3].
Citations:
- 1: https://www.baeldung.com/feign-retry
- 2: https://www.matez.de/index.php/tag/feign/
- 3: https://www.matez.de/index.php/2017/04/12/exploring-feign-retrying/
- 4: https://reflectoring.io/create-openfeign-http-client/
- 5: https://javadoc.io/doc/io.github.openfeign/feign-core/11.0/feign/Retryer.Default.html
- 6: Max number of retry attemps is not working resilience4j/resilience4j#1858
- 7: Retry with OpenFeign Add-On not working resilience4j/resilience4j#2090
Fix Feign Retryer.Default comment to reflect total‐attempts semantics
Feign’s maxAttempts parameter counts total attempts (initial call + retries). With maxAttempts = 3, Feign performs up to 3 calls (1 initial + 2 retries). Update the comment in:
• File: src/main/java/com/example/spot/common/config/FeignConfig.java
Lines: 19–23
- // 초기대기 200ms, 최대대기 800ms, 최대 3번 재시도
+ // 초기대기 200ms, 최대대기 800ms, 총 3회 시도 (1회 호출 + 최대 2회 재시도)
return new Retryer.Default(200, 800, 3);If you actually want 3 retries (4 total attempts), change the code instead:
// 초기대기 200ms, 최대대기 800ms, 최대 3회 재시도 (총 4회 시도)
return new Retryer.Default(200, 800, 4);🤖 Prompt for AI Agents
In src/main/java/com/example/spot/common/config/FeignConfig.java around lines 19
to 23, the comment for Retryer.Default incorrectly describes maxAttempts as
number of retries; update the comment to state that maxAttempts is the total
number of attempts (initial call + retries) so maxAttempts = 3 results in up to
3 calls (1 initial + 2 retries). If the intent was to allow 3 retries (4 total
attempts) instead, change the constructor argument to 4 and adjust the comment
accordingly (e.g., "초기대기 200ms, 최대대기 800ms, 최대 3회 재시도 (총 4회 시도)").
1 participant
#️⃣ 연관된 이슈
🔎 작업 내용
📷 스크린샷 (선택)
💬리뷰 요구사항 (선택)
Summary by CodeRabbit