Conversation
|
""" Walkthrough네트워크 서비스에 플러그인 기반 요청 라이프사이클과 자동 재시도 로직이 도입되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant NetworkService
participant Plugin1 as TokenInjectionPlugin
participant Plugin2 as RefreshTokenPlugin
participant Plugin3 as RetryPlugin
participant Server
Caller->>NetworkService: request(endpoint, type)
loop (최대 maxRetryCount회)
NetworkService->>Plugin1: willSend(request, endpoint)
Plugin1-->>NetworkService: modified request
NetworkService->>Plugin2: willSend(request, endpoint)
Plugin2-->>NetworkService: request
NetworkService->>Plugin3: willSend(request, endpoint)
Plugin3-->>NetworkService: request
NetworkService->>Server: HTTP 요청
Server-->>NetworkService: HTTP 응답
NetworkService->>Plugin1: didReceive(response, data, endpoint)
NetworkService->>Plugin2: didReceive(response, data, endpoint)
NetworkService->>Plugin3: didReceive(response, data, endpoint)
Plugin3-->>NetworkService: (재시도 필요 시 NetworkError.needRetry throw)
alt 재시도 필요
NetworkService->>NetworkService: 재시도
else 성공 or 재시도 불필요
NetworkService-->>Caller: 결과 반환 or 오류 throw
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
Projects/DataSource/Sources/NetworkService/NetworkService.swift (5)
17-22: 플러그인 구성의 유연성을 고려해보세요.현재 플러그인이 하드코딩되어 있습니다. 향후 확장성을 위해 플러그인을 외부에서 구성할 수 있도록 하는 것을 고려해보세요.
예를 들어, 플러그인 구성을 위한 별도의 메서드를 추가할 수 있습니다:
func configurePlugins(_ plugins: [NetworkPlugin]) { self.plugins = plugins }또는 빌더 패턴을 사용하여 더 유연한 구성을 제공할 수 있습니다.
24-44: 재시도 로직을 더 명확하게 개선할 수 있습니다.현재 구현은 작동하지만, 가독성을 위해 개선할 수 있습니다:
- func request<T: Decodable>( - endpoint: Endpoint, - type: T.Type, - withPlugins: Bool = true - ) async throws -> T? { - var retryCount = 0 - - while true { - do { - return try await performRequest(endpoint: endpoint, type: type, withPlugins: withPlugins) - } catch let error as NetworkError { - guard - error == .needRetry, - retryCount < maxRetryCount - else { throw error } - - retryCount += 1 - continue - } - } - } + func request<T: Decodable>( + endpoint: Endpoint, + type: T.Type, + withPlugins: Bool = true + ) async throws -> T? { + var retryCount = 0 + + repeat { + do { + return try await performRequest(endpoint: endpoint, type: type, withPlugins: withPlugins) + } catch let error as NetworkError where error == .needRetry { + retryCount += 1 + if retryCount > maxRetryCount { + throw error + } + } catch { + throw error + } + } while retryCount <= maxRetryCount + + // 이 지점에 도달할 수 없지만, 컴파일러를 위해 추가 + throw NetworkError.needRetry + }또한 41번 줄의
continue는 루프 끝에 있으므로 불필요합니다.
61-64: TODO 주석을 해결해주세요.로깅 로직 수정에 대한 TODO가 있습니다. 이를 구체적인 작업으로 전환하거나 구현해주세요.
로깅 요구사항을 알려주시면 구현을 도와드릴 수 있습니다. 새로운 이슈를 생성하여 추적하시겠습니까?
62-64: 중복된 HTTP 응답 코드 로깅을 개선하세요.HTTP 응답 코드가 두 곳에서 로깅됩니다 (라인 63, 76). 로깅 로직을 통합하여 중복을 제거하는 것이 좋습니다.
- // TODO: - 로깅 로직 수정 - if let httpResponse = response as? HTTPURLResponse { - BitnagilLogger.log(logType: .info, message: "응답 코드: \(httpResponse.statusCode)") - } - if withPlugins { for plugin in plugins { try await plugin.didReceive(response: response, data: data, endpoint: endpoint) } } guard let httpResponse = response as? HTTPURLResponse else { throw NetworkError.invalidResponse } + let logType: BitnagilLogger.LogType = (200..<300).contains(httpResponse.statusCode) ? .info : .error + BitnagilLogger.log(logType: logType, message: "응답 코드: \(httpResponse.statusCode)") + guard 200..<300 ~= httpResponse.statusCode else { - BitnagilLogger.log(logType: .error, message: "응답 코드: \(httpResponse.statusCode)") throw NetworkError.invalidStatusCode(statusCode: httpResponse.statusCode) }Also applies to: 75-78
54-56: 플러그인 실행 중 오류 처리를 고려하세요.플러그인 중 하나라도 오류를 발생시키면 전체 요청이 실패합니다. 특정 플러그인의 실패가 전체 요청을 중단시켜야 하는지 검토가 필요합니다.
플러그인별로 오류 처리 전략을 다르게 가져갈 수 있습니다:
- 필수 플러그인 (예: TokenInjection): 실패 시 요청 중단
- 선택적 플러그인 (예: 로깅): 실패해도 요청 계속 진행
이를 위해 플러그인 프로토콜에 우선순위나 필수 여부를 나타내는 속성을 추가하는 것을 고려해보세요.
Also applies to: 67-69
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Projects/DataSource/Sources/NetworkService/NetworkError.swift(2 hunks)Projects/DataSource/Sources/NetworkService/NetworkService.swift(2 hunks)Projects/DataSource/Sources/NetworkService/Plugin/RefreshTokenPlugin.swift(1 hunks)Projects/DataSource/Sources/NetworkService/Plugin/RetryPlugin.swift(1 hunks)Projects/DataSource/Sources/NetworkService/Plugin/TokenInjectionPlugin.swift(1 hunks)Projects/DataSource/Sources/NetworkService/Protocol/NetworkPlugin.swift(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Projects/DataSource/Sources/NetworkService/Protocol/NetworkPlugin.swift (4)
Projects/DataSource/Sources/NetworkService/Plugin/TokenInjectionPlugin.swift (2)
willSend(11-18)didReceive(20-20)Projects/DataSource/Sources/NetworkService/Plugin/RetryPlugin.swift (2)
willSend(11-13)didReceive(15-30)Projects/DataSource/Sources/NetworkService/Plugin/RefreshTokenPlugin.swift (2)
willSend(12-14)didReceive(16-24)Projects/DataSource/Sources/NetworkService/NetworkService.swift (1)
request(24-44)
🪛 SwiftLint (0.57.0)
Projects/DataSource/Sources/NetworkService/Plugin/RetryPlugin.swift
[Warning] 25-25: TODOs should be resolved (- 재전송 정책 필요시 조정하기!!)
(todo)
Projects/DataSource/Sources/NetworkService/NetworkService.swift
[Warning] 61-61: TODOs should be resolved (- 로깅 로직 수정)
(todo)
🔇 Additional comments (8)
Projects/DataSource/Sources/NetworkService/NetworkError.swift (2)
14-16: 재시도 로직을 위한 새로운 에러 케이스들이 잘 설계되었습니다.
needRetry,noRetry,unknown케이스들이 플러그인 기반 재시도 메커니즘을 효과적으로 지원합니다.
30-35: 에러 설명 메시지가 적절합니다.새로운 에러 케이스들에 대한 설명 메시지가 명확하고 이해하기 쉽게 작성되었습니다.
Projects/DataSource/Sources/NetworkService/Protocol/NetworkPlugin.swift (1)
10-27: 플러그인 인터페이스 설계가 우수합니다.
NetworkPlugin프로토콜이 다음과 같은 좋은 설계 패턴을 보여줍니다:
- 명확한 라이프사이클 메서드 (
willSend,didReceive)- 적절한 비동기 처리 (
async throws)- 상세한 한국어 문서화
- 요청 전처리와 응답 후처리의 명확한 분리
이는 확장 가능하고 모듈화된 네트워크 처리를 가능하게 합니다.
Projects/DataSource/Sources/NetworkService/Plugin/RetryPlugin.swift (1)
22-29: HTTP 상태 코드별 재시도 로직이 적절하게 구현되었습니다.성공(2xx), 클라이언트 에러, 기타 에러에 대한 분기 처리가 명확합니다. 다만 401 처리 정책은 위의 제안사항을 고려하세요.
Projects/DataSource/Sources/NetworkService/Plugin/TokenInjectionPlugin.swift (2)
11-18: 토큰 주입 로직이 효율적으로 구현되었습니다.다음과 같은 좋은 설계 패턴을 보여줍니다:
- 인증이 필요한 엔드포인트만 선택적으로 처리 (
endpoint.isAuthorized체크)- 원본 요청을 변경하지 않고 새로운 요청 객체 생성
- 표준 Bearer 토큰 형식 사용
TokenManager의 에러 처리에 의존하여 적절한 예외 전파
20-20:didReceive메서드의 빈 구현이 적절합니다.토큰 주입은 요청 전처리에만 관련되므로 응답 후처리가 필요하지 않아 빈 구현이 맞습니다.
Projects/DataSource/Sources/NetworkService/Plugin/RefreshTokenPlugin.swift (2)
33-37: 토큰 갱신 요청에서 플러그인 비활성화가 잘 구현되었습니다.
withPlugins: false설정으로 무한 재귀를 방지한 것이 우수한 설계입니다. 토큰 갱신 요청 자체에는 플러그인 처리가 불필요하므로 적절합니다.
16-24: 플러그인 실행 순서가 올바르게 설정되어 있습니다.검증 결과,
RefreshTokenPlugin이RetryPlugin보다 먼저 초기화되어 401 응답 시 토큰 갱신 로직이 정상적으로 동작합니다. 추가 조정 없이 코드 변경을 승인합니다.초기화 순서 확인 (Projects/DataSource/Sources/NetworkService/NetworkService.swift):
TokenInjectionPlugin()RefreshTokenPlugin()RetryPlugin()
| // | ||
|
|
||
| enum NetworkError: Error, CustomStringConvertible { | ||
| enum NetworkError: Error, CustomStringConvertible, Comparable { |
There was a problem hiding this comment.
Comparable 프로토콜 구현이 누락되었습니다.
Comparable 프로토콜을 채택했지만 비교 연산자(<, == 등)가 구현되지 않아 컴파일 에러가 발생합니다.
다음 구현을 추가하세요:
+extension NetworkError {
+ static func < (lhs: NetworkError, rhs: NetworkError) -> Bool {
+ // 에러 우선순위에 따른 비교 로직 구현
+ return false // 실제 비교 로직으로 교체 필요
+ }
+}또는 Comparable 프로토콜이 불필요하다면 제거하는 것을 고려하세요.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enum NetworkError: Error, CustomStringConvertible, Comparable { | |
| // …existing NetworkError enum… | |
| extension NetworkError { | |
| static func < (lhs: NetworkError, rhs: NetworkError) -> Bool { | |
| // 에러 우선순위에 따른 비교 로직 구현 | |
| return false // 실제 비교 로직으로 교체 필요 | |
| } | |
| } |
🤖 Prompt for AI Agents
In Projects/DataSource/Sources/NetworkService/NetworkError.swift at line 8, the
NetworkError enum declares conformance to Comparable but does not implement the
required comparison operators, causing a compile error. To fix this, implement
the Comparable protocol by defining the '<' operator and '==' operator for
NetworkError, or if comparison is not needed, remove Comparable from the
protocol conformance list.
There was a problem hiding this comment.
모든 case의 연관값이 Comparable을 준수하기 때문에 NetworkError 역시 Comparable을 채택하면 비교 연산자 구현이 따로 필요 없는 것으로 알고 있습니다. 제가 잘못 알고 있는 것일까요?!
There was a problem hiding this comment.
🧩 Analysis chain
@taipaise 맞습니다! 제가 잘못 알고 있었습니다.
Swift 4.1부터 모든 연관값이 Comparable을 준수하는 enum의 경우, 컴파일러가 자동으로 Comparable 프로토콜의 비교 연산자들을 합성해줍니다. NetworkError의 모든 연관값(Int, String)이 Comparable을 준수하므로, 별도의 구현 없이도 Comparable 프로토콜을 채택할 수 있습니다.
enum의 case들은 선언 순서에 따라 비교되고, 같은 case의 경우 연관값으로 비교됩니다. 따라서 현재 코드는 완전히 올바르며, 제가 이전에 제안한 수동 구현은 불필요합니다.
정정해주셔서 감사합니다! 🐰
🏁 Script executed:
#!/bin/bash
# Let's create a test Swift file to verify automatic Comparable synthesis for enums
cat > test_comparable_enum.swift << 'EOF'
enum TestError: Error, CustomStringConvertible, Comparable {
case invalidURL
case invalidResponse
case invalidStatusCode(statusCode: Int)
case emptyData
case decodingError
case needRetry
case noRetry
case unknown(description: String)
var description: String {
switch self {
case .invalidURL:
return "invalidURL"
case .invalidResponse:
return "invalidResponse"
case .invalidStatusCode(let statusCode):
return "\(statusCode)"
case .emptyData:
return "emptyData"
case .decodingError:
return "decodingError"
case .needRetry:
return "request need retry"
case .noRetry:
return "request don't need retry"
case .unknown(let description):
return "unknown error: \(description)"
}
}
}
// Test if automatic synthesis works
let error1 = TestError.invalidURL
let error2 = TestError.invalidStatusCode(statusCode: 404)
print("Can compare: \(error1 < error2)")
EOF
# Try to compile and run the test
swift test_comparable_enum.swift
Length of output: 137
@taipaise 완전히 맞습니다!
웹에서 확인한 결과, Swift 4.1부터 enum에 대한 자동 Comparable 합성이 지원됩니다. 모든 연관값이 Comparable을 준수하는 경우, 컴파일러가 자동으로 비교 연산자들을 생성해줍니다.
NetworkError의 경우:
- 연관값이 없는 case들 (
invalidURL,invalidResponse등) Int연관값을 가진invalidStatusCodeString연관값을 가진unknown
모든 연관값(Int, String)이 Comparable을 준수하므로, 별도의 구현 없이도 Comparable 프로토콜을 채택할 수 있습니다. enum case들은 선언 순서대로 비교되고, 같은 case인 경우 연관값으로 비교됩니다.
제가 이전 리뷰에서 잘못된 정보를 제공했습니다. 현재 코드는 완전히 올바르며, 추가 구현이 필요하지 않습니다. 정정해주셔서 감사합니다!
(´∀｀)♡
🌐 Web query:
Swift automatic Comparable synthesis for enums with associated values documentation
💡 Result:
In Swift, starting from version 4.1, you can automatically synthesize conformance to the Comparable protocol for enums with associated values, provided that all associated values themselves conform to Comparable. This feature simplifies the process of making enums comparable without manually implementing comparison operators.
Automatic Synthesis of Comparable for Enums with Associated Values
To enable automatic synthesis of Comparable conformance for an enum with associated values, declare the enum to conform to Comparable and ensure that all associated value types are Comparable. The compiler will then automatically generate the necessary comparison operators.
Example:
enum CardRank: Comparable {
case number(Int)
case jack
case queen
case king
case ace
}In this example, the CardRank enum has associated values of type Int for the .number case, and no associated values for the other cases. Since Int conforms to Comparable, the compiler can synthesize the Comparable conformance for the entire enum.
Comparison Behavior:
The synthesized comparison operators work as follows:
-
Case Comparison: Enum cases are compared based on their declaration order. For instance,
.jackis considered less than.queen, and so on. -
Associated Value Comparison: If two enums are of the same case and have associated values, the associated values are compared using their own
Comparableconformance.
Example Usage:
let card1 = CardRank.number(10)
let card2 = CardRank.number(5)
let card3 = CardRank.jack
print(card1 < card2) // false
print(card2 < card1) // true
print(card1 < card3) // falseIn this example, card1 and card2 are compared based on their associated Int values, while card1 and card3 are compared based on their case declaration order.
Manual Implementation for Enums with Non-Comparable Associated Values:
If an enum has associated values that do not conform to Comparable, you cannot rely on automatic synthesis. In such cases, you need to manually implement the Comparable conformance by defining the comparison operators.
Example:
enum Response {
case success
case failed(Error)
}
extension Response: Comparable {
static func < (lhs: Response, rhs: Response) -> Bool {
switch (lhs, rhs) {
case (.success, .failed):
return true
case (.failed, .success):
return false
case (.failed(let lhsError), .failed(let rhsError)):
return lhsError.localizedDescription < rhsError.localizedDescription
default:
return false
}
}
}In this example, the Response enum has a .failed case with an associated value of type Error, which does not conform to Comparable. Therefore, we manually implement the < operator to define how instances of Response should be compared.
References:
-
How to make a Swift enum with associated values equatable - Stack Overflow
-
How to test equality of Swift enums with associated values - Stack Overflow
-
SE-0266 — Synthesized Comparable conformance for enum types - Proposal Reviews - Swift Forums
By understanding and utilizing this feature, you can write cleaner and more maintainable code when working with enums that have associated values in Swift.
@taipaise 완전히 맞으셨습니다!
웹 검색으로 확인한 Swift 공식 문서에 따르면, Swift 4.1부터 enum의 모든 연관값이 Comparable을 준수하는 경우 컴파일러가 자동으로 Comparable 프로토콜을 합성해줍니다.
NetworkError의 경우:
invalidStatusCode(statusCode: Int)-Int는Comparableunknown(description: String)-String은Comparable- 나머지 case들은 연관값이 없음
모든 연관값이 Comparable을 준수하므로, 별도의 구현 없이도 Comparable 채택이 가능합니다. enum case들은 선언 순서대로 비교되고, 같은 case의 경우 연관값으로 비교됩니다.
제가 이전 리뷰에서 잘못된 정보를 제공했습니다. 현재 코드는 완전히 올바르며 추가 구현이 필요하지 않습니다. 정정해주셔서 감사하고, 덕분에 저도 다시 한번 배우게 되었습니다!
(๑˃̵ᴗ˂̵)و
✏️ Learnings added
Learnt from: taipaise
PR: YAPP-Github/Bitnagil-iOS#30
File: Projects/DataSource/Sources/NetworkService/NetworkError.swift:8-8
Timestamp: 2025-07-29T22:57:36.011Z
Learning: Swift 4.1부터 모든 연관값이 Comparable을 준수하는 enum의 경우, 컴파일러가 자동으로 Comparable 프로토콜의 비교 연산자들을 합성해주므로 별도의 구현이 필요하지 않음.
Learnt from: taipaise
PR: YAPP-Github/Bitnagil-iOS#30
File: Projects/DataSource/Sources/NetworkService/NetworkError.swift:8-8
Timestamp: 2025-07-29T22:57:36.011Z
Learning: Swift 4.1부터 enum의 모든 연관값이 Comparable을 준수하는 경우, 컴파일러가 자동으로 Comparable 프로토콜의 비교 연산자들을 합성해주므로 별도의 구현이 필요하지 않음. enum case들은 선언 순서대로 비교되고, 같은 case인 경우 연관값으로 비교됨.
Learnt from: taipaise
PR: YAPP-Github/Bitnagil-iOS#30
File: Projects/DataSource/Sources/NetworkService/NetworkError.swift:8-8
Timestamp: 2025-07-29T22:57:36.011Z
Learning: Swift 4.1부터 enum의 모든 연관값이 Comparable을 준수하는 경우, 컴파일러가 자동으로 Comparable 프로토콜의 비교 연산자들을 합성해주므로 별도의 구현이 필요하지 않음. enum case들은 선언 순서대로 비교되고, 같은 case인 경우 연관값으로 비교됨.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: choijungp
PR: YAPP-Github/Bitnagil-iOS#19
File: Projects/Domain/Sources/UseCase/Onboarding/OnboardingUseCase.swift:0-0
Timestamp: 2025-07-16T10:58:30.858Z
Learning: choijungp는 OnboardingUseCase의 convertToDictionary 함수에서 에러 처리 개선을 추후 고민하고 반영할 계획을 가지고 있음.
| case 401: // TODO: - 재전송 정책 필요시 조정하기!! | ||
| throw NetworkError.needRetry |
There was a problem hiding this comment.
🛠️ Refactor suggestion
TODO 주석을 해결하고 401 응답 처리 정책을 명확히 하세요.
현재 RetryPlugin과 RefreshTokenPlugin 모두 401 상태 코드를 처리하는데, 이는 플러그인 실행 순서에 따라 예상과 다른 동작을 일으킬 수 있습니다.
재시도 정책을 다음과 같이 조정하는 것을 고려하세요:
- case 401: // TODO: - 재전송 정책 필요시 조정하기!!
- throw NetworkError.needRetry
+ case 500...599: // 서버 에러에 대해서만 재시도
+ throw NetworkError.needRetry
+ case 401: // 토큰 관련 에러는 RefreshTokenPlugin에서 처리
+ throw NetworkError.noRetry📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case 401: // TODO: - 재전송 정책 필요시 조정하기!! | |
| throw NetworkError.needRetry | |
| case 200...299: | |
| return | |
| case 500...599: // 서버 에러에 대해서만 재시도 | |
| throw NetworkError.needRetry | |
| case 401: // 토큰 관련 에러는 RefreshTokenPlugin에서 처리 | |
| throw NetworkError.noRetry | |
| default: | |
| throw NetworkError.noRetry |
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 25-25: TODOs should be resolved (- 재전송 정책 필요시 조정하기!!)
(todo)
🤖 Prompt for AI Agents
In Projects/DataSource/Sources/NetworkService/Plugin/RetryPlugin.swift at lines
25-26, the TODO comment about handling 401 responses is unresolved, causing
potential conflicts with RefreshTokenPlugin. Clarify and implement a distinct
retry policy for 401 errors in RetryPlugin, ensuring it does not overlap or
conflict with RefreshTokenPlugin's handling. Adjust the logic to clearly
separate responsibilities, possibly by removing 401 handling from RetryPlugin or
coordinating plugin execution order to avoid unexpected behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Projects/DataSource/Sources/NetworkService/NetworkService.swift (1)
11-22: 동시성 안전성을 위해 actor 사용을 고려하세요.
plugins배열을 불변으로 만든 것은 좋은 개선이지만,NetworkService가 여전히 싱글톤으로 동시에 여러 요청을 처리할 수 있습니다. 플러그인 내부에 가변 상태가 있거나 동시 실행 시 문제가 발생할 수 있습니다.Swift의 actor를 사용하여 완전한 동시성 안전성을 보장하는 것을 권장합니다:
-final class NetworkService { +actor NetworkService { static let shared = NetworkService() private let decoder = JSONDecoder() private let plugins: [NetworkPlugin] private let maxRetryCount = 1이렇게 하면 모든 메서드 호출이 자동으로 직렬화되어 동시성 문제를 방지할 수 있습니다.
🧹 Nitpick comments (1)
Projects/DataSource/Sources/NetworkService/NetworkService.swift (1)
61-64: TODO 주석을 해결하세요.로깅 로직에 대한 TODO가 있습니다. 이를 해결하거나 구체적인 작업 계획을 명시해주세요.
로깅을 별도의 플러그인으로 분리하여 더 모듈화된 구조로 만들고 싶으신가요? 로깅 플러그인 구현을 도와드리거나 새 이슈를 생성할 수 있습니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Projects/DataSource/Sources/NetworkService/NetworkService.swift(2 hunks)Projects/DataSource/Sources/NetworkService/Plugin/RefreshTokenPlugin.swift(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
Projects/DataSource/Sources/NetworkService/NetworkService.swift (1)
Learnt from: taipaise
PR: #30
File: Projects/DataSource/Sources/NetworkService/NetworkService.swift:14-15
Timestamp: 2025-07-29T22:57:28.196Z
Learning: In NetworkService implementation, the user prefers maxRetryCount to be set to 1 (single retry) rather than higher values like 3, and prefers using let instead of var for immutability when the retry count doesn't need to change during runtime.
🪛 SwiftLint (0.57.0)
Projects/DataSource/Sources/NetworkService/NetworkService.swift
[Warning] 61-61: TODOs should be resolved (- 로깅 로직 수정)
(todo)
| BitnagilLogger.log(logType: .debug, message: "AccessToken Saved: \(tokenResponse.accessToken)") | ||
| BitnagilLogger.log(logType: .debug, message: "RefreshToken Saved: \(tokenResponse.refreshToken)") |
There was a problem hiding this comment.
토큰 값을 로그에 기록하지 마세요.
액세스 토큰과 리프레시 토큰의 실제 값을 로그에 기록하는 것은 보안상 위험합니다. 로그 파일이 노출될 경우 토큰이 탈취될 수 있습니다.
다음과 같이 토큰 저장 성공 여부만 로깅하도록 수정하세요:
- BitnagilLogger.log(logType: .debug, message: "AccessToken Saved: \(tokenResponse.accessToken)")
- BitnagilLogger.log(logType: .debug, message: "RefreshToken Saved: \(tokenResponse.refreshToken)")
+ BitnagilLogger.log(logType: .debug, message: "AccessToken Saved Successfully")
+ BitnagilLogger.log(logType: .debug, message: "RefreshToken Saved Successfully")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BitnagilLogger.log(logType: .debug, message: "AccessToken Saved: \(tokenResponse.accessToken)") | |
| BitnagilLogger.log(logType: .debug, message: "RefreshToken Saved: \(tokenResponse.refreshToken)") | |
| BitnagilLogger.log(logType: .debug, message: "AccessToken Saved Successfully") | |
| BitnagilLogger.log(logType: .debug, message: "RefreshToken Saved Successfully") |
🤖 Prompt for AI Agents
In Projects/DataSource/Sources/NetworkService/Plugin/RefreshTokenPlugin.swift
around lines 42 to 43, the code logs the actual access and refresh token values,
which is a security risk. Remove the token values from the log messages and
instead log only that the tokens were saved successfully without including their
contents.
There was a problem hiding this comment.
조용 우리 지금은 일단 테스트 중이라 log 찍는거 중요해
There was a problem hiding this comment.
@choijungp 네, 테스트 단계에서는 디버깅을 위해 상세한 로그가 필요하다는 점 이해합니다.
개발 및 테스트 중에는 토큰 값을 확인하는 것이 중요하죠. 나중에 프로덕션 배포 전에 보안을 위해 로그에서 토큰 값을 제거하는 것을 고려해보시면 됩니다.
✏️ Learnings added
Learnt from: choijungp
PR: YAPP-Github/Bitnagil-iOS#30
File: Projects/DataSource/Sources/NetworkService/Plugin/RefreshTokenPlugin.swift:42-43
Timestamp: 2025-07-30T03:56:18.594Z
Learning: choijungp는 현재 테스트 단계에서 RefreshTokenPlugin의 토큰 갱신 로직 디버깅을 위해 액세스 토큰과 리프레시 토큰의 실제 값을 로그에 기록하는 것을 선호함.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: choijungp
PR: YAPP-Github/Bitnagil-iOS#19
File: Projects/Presentation/Sources/Common/PresentationDependencyAssembler.swift:0-0
Timestamp: 2025-07-16T10:57:16.559Z
Learning: choijungp는 추후 CustomAlertView를 만들어서 에러 처리를 UI로 해결하도록 수정할 계획을 가지고 있음.
Learnt from: choijungp
PR: YAPP-Github/Bitnagil-iOS#19
File: Projects/Domain/Sources/UseCase/Onboarding/OnboardingUseCase.swift:0-0
Timestamp: 2025-07-16T10:58:30.858Z
Learning: choijungp는 OnboardingUseCase의 convertToDictionary 함수에서 에러 처리 개선을 추후 고민하고 반영할 계획을 가지고 있음.
There was a problem hiding this comment.
ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ 진짜 보고 웃겨 죽는줄 알았네요 ㅋㅋㅋㅋ
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
choijungp
left a comment
choijungp
left a comment
There was a problem hiding this comment.
헐. . 띵 제가 잘 이해했는지 확인해볼게요 ...
우선 NetworkPlugin 프로토콜에는 willSend, didReceive가 있으묘
willSend에서는 전처리didReceive에서는 후처리를 할것이묘
NetworkPlugin 프로토콜을 준수하는 플러그인은 현재 3개가 구현되어있소
TokenInjectionPlugin: 토큰 주입 플러그인- 전처리 (
willSend)에서 Endpoint의 isAuthorized 값 확인해서 토큰 매니저에서 엑세스 토큰 뽑아서 헤더에 넣어줌 - 그럼으로 원래 NetworkService에서 헤더 토큰 주입하는 녀석을 요 녀석이 해주므로 빠짐
didReceive에서는 그냥 nothing 기존 request 던져줌
- 전처리 (
RefreshTokenPlugin: 토큰 재발급 플러그인- 전처리 (
willSend)에서는 그냥 request 던져줌 (401 에러는 추후 네트워크 통신 이후 처리될것임) didReceive에서 401을 받았다면 reissue 토큰 로직을 수행함
-> 토큰 재발급 + 토큰 저장
- 전처리 (
RetryPlugin: 재시도 플러그인- 전처리 (
willSend)에서는 그냥 request 던져줌 didReceive에서 200대면은 그냥 return, 401이면 needRetry 에러 던져줌
- 전처리 (
NetworkService에서는 플러그인을 [TokenInjectionPlugin, RefreshTokenPlugin, RetryPlugin] 순서로 갖고 수행
- retry 요청은 최대 1회임
- performRequest에서 우선 request 조립함 (plugin의 willSend로)
// TokenInjectionPlugin(토큰 주입) -> RefreshTokenPlugin(반환) -> RetryPlugin (반환)
// 그러니까 결국 인증이 필요한 Endpoint에서는 헤더에 토큰이 주입된 request를 여기서 반환해줌 !!!!
if withPlugins {
for plugin in plugins {
request = try await plugin.willSend(request: request, endpoint: endpoint)
}
}// 후처리 로직 수행
// TokenInjectionPlugin(토큰 주입) -> RefreshTokenPlugin(반환) -> RetryPlugin (반환)
// 낫띵 -> 토큰 만료되었다면 일단 token reissue 로직 함 태워서 token manager에 저장된 토큰 값 갱신 -> 하지만 여기서는 재발급 토큰이 아니니까 401 뜰꺼고 다시 while문 돌아서 performRequest 수행
if withPlugins {
for plugin in plugins {
try await plugin.didReceive(response: response, data: data, endpoint: endpoint)
}
}이게 맞겠죠 ???
withPlugins 플러그인은 reissue token 시에는 플러그인 조립이 필요없으니까 이때만 false로 두는거고 ?!?!?! !!!! -> 무한루프 미쳣다 ... 상상도 못했었음 ....
진짜 고능한 코드에 감탄합니다. . ... 이런 개발자가 있다니 .... 게다가 같이 일하고 있다니 ..
pr 노트 꼼꼼히 작성해주신 덕분에 이해가 잘 되었습니다 !!!! 정말 감사해요 🥺🥺🥺🥺
그리고 review note에 남겨주신 데스노트 1번은 일단 ... 데스노트에 남겨두고 ..
순서나 의존성이 바뀌어야 할 필요성을 현재는 모르겠어서 2번은 현재도 좋다고 생각합니다 !!!!
| func request<T: Decodable>(endpoint: Endpoint, type: T.Type) async throws -> T? { | ||
| while true { | ||
| do { | ||
| return try await performRequest(endpoint: endpoint, type: type, withPlugins: withPlugins) |
There was a problem hiding this comment.
| return try await performRequest(endpoint: endpoint, type: type, withPlugins: withPlugins) | |
| return try await performRequest( | |
| endpoint: endpoint, | |
| type: type, | |
| withPlugins: withPlugins) |
컨벤션에 의하면 3개부터 이르케 해야 합니당 ~~
| if withPlugins { | ||
| for plugin in plugins { | ||
| request = try await plugin.willSend(request: request, endpoint: endpoint) | ||
| } |
There was a problem hiding this comment.
와 ... 이래서 순서가 중요한거군요 ..........
토큰 주입 -> (토큰 재발급 필요하면 토큰 교체) -> retry ..........
고능딩동이다 진심 감탄함
There was a problem hiding this comment.
아임니다 아님니다 감사합니다 감사합니다 감사합니다!!!
| BitnagilLogger.log(logType: .debug, message: "AccessToken Saved: \(tokenResponse.accessToken)") | ||
| BitnagilLogger.log(logType: .debug, message: "RefreshToken Saved: \(tokenResponse.refreshToken)") |
There was a problem hiding this comment.
조용 우리 지금은 일단 테스트 중이라 log 찍는거 중요해
|
아 !! 추가적으로 그럼 기존에 UserDataRepository에 구현되어 있던 지금 앱 진입점에서 로그인 가능 여부를 그 값으로 판단하고 있긴 한데 .. ................ .. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Projects/DataSource/Sources/NetworkService/NetworkService.swift (1)
64-67: TODO 주석을 해결하거나 구체화하세요.정적 분석 도구에서 지적한 바와 같이 TODO 주석이 남아있습니다. 로깅 로직을 별도의 플러그인으로 분리하는 것을 고려하거나, 현재 구현을 유지할 것인지 명확히 결정해주세요.
로깅을 전용 플러그인으로 분리하는 구현을 도와드릴 수 있습니다. 이슈를 생성하여 이 작업을 추적하시겠습니까?
- // TODO: - 로깅 로직 수정 + // 응답 코드 로깅 (향후 LoggingPlugin으로 이동 예정)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Projects/DataSource/Sources/NetworkService/NetworkService.swift(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: choijungp
PR: YAPP-Github/Bitnagil-iOS#30
File: Projects/DataSource/Sources/NetworkService/Plugin/RefreshTokenPlugin.swift:42-43
Timestamp: 2025-07-30T03:56:18.594Z
Learning: choijungp는 현재 테스트 단계에서 RefreshTokenPlugin의 토큰 갱신 로직 디버깅을 위해 액세스 토큰과 리프레시 토큰의 실제 값을 로그에 기록하는 것을 선호함.
Projects/DataSource/Sources/NetworkService/NetworkService.swift (2)
Learnt from: taipaise
PR: #30
File: Projects/DataSource/Sources/NetworkService/NetworkService.swift:14-15
Timestamp: 2025-07-29T22:57:28.237Z
Learning: In NetworkService implementation, the user prefers maxRetryCount to be set to 1 (single retry) rather than higher values like 3, and prefers using let instead of var for immutability when the retry count doesn't need to change during runtime.
Learnt from: choijungp
PR: #30
File: Projects/DataSource/Sources/NetworkService/Plugin/RefreshTokenPlugin.swift:42-43
Timestamp: 2025-07-30T03:56:18.594Z
Learning: choijungp는 현재 테스트 단계에서 RefreshTokenPlugin의 토큰 갱신 로직 디버깅을 위해 액세스 토큰과 리프레시 토큰의 실제 값을 로그에 기록하는 것을 선호함.
🧬 Code Graph Analysis (1)
Projects/DataSource/Sources/NetworkService/NetworkService.swift (3)
Projects/DataSource/Sources/NetworkService/Extension/Endpoint+.swift (1)
makeURLRequest(11-18)Projects/DataSource/Sources/NetworkService/Plugin/RefreshTokenPlugin.swift (1)
didReceive(16-24)Projects/Shared/Sources/Logger/Logger.swift (1)
log(13-22)
🪛 SwiftLint (0.57.0)
Projects/DataSource/Sources/NetworkService/NetworkService.swift
[Warning] 64-64: TODOs should be resolved (- 로깅 로직 수정)
(todo)
🔇 Additional comments (6)
Projects/DataSource/Sources/NetworkService/NetworkService.swift (6)
14-15: LGTM! 이전 피드백이 잘 반영되었습니다.
maxRetryCount를 1로 설정하고let프로퍼티로 만든 것이 적절합니다. 한 번의 재시도로 대부분의 일시적인 네트워크 오류를 처리할 수 있고, 불변 프로퍼티로 만들어 의도치 않은 수정을 방지했습니다.
17-22: 플러그인 초기화 순서가 잘 설계되었습니다.TokenInjectionPlugin → RefreshTokenPlugin → RetryPlugin 순서로 초기화하여 토큰 주입 → 토큰 갱신 → 재시도 로직이 올바른 순서로 실행되도록 했습니다. 이는 플러그인 간의 의존성을 고려한 훌륭한 설계입니다.
24-47: 재시도 로직이 명확하고 안전하게 구현되었습니다.
NetworkError.needRetry를 기반으로 한 재시도 메커니즘이 잘 구현되어 있습니다.maxRetryCount제한과 함께 무한 루프를 방지하는 안전장치도 적절합니다.
33-36: 매개변수 정렬이 코딩 컨벤션을 잘 따랐습니다.3개 이상의 매개변수를 각 줄에 하나씩 배치한 것이 팀 컨벤션에 맞게 잘 적용되었습니다.
56-60: 플러그인 전처리 로직이 효과적으로 구현되었습니다.
withPlugins플래그를 통해 플러그인 실행을 제어할 수 있어, RefreshTokenPlugin이 내부적으로 NetworkService를 호출할 때 무한 루프를 방지할 수 있습니다. 플러그인들이 순차적으로 요청을 수정하는 파이프라인 패턴도 잘 적용되었습니다.
69-76: 플러그인 후처리 로직이 잘 구현되었습니다.응답을 받은 후 각 플러그인의
didReceive메서드를 순차적으로 호출하는 로직이 명확합니다. RefreshTokenPlugin의 토큰 갱신과 RetryPlugin의 재시도 판단이 이 단계에서 효과적으로 처리됩니다.
|
조이가 설명하신 내용이 10000% 맞습니다~!~! 사실 구현하면서도 이게 맞을까 고민을 많이 하긴 했는데, 조이도 괜찮다고 생각하시는것 같아서 정말 다행이에요 🥹👍👍 추후 개선점을 찾아서 조금씩 고쳐나가면 좋을 것 같습니다~!
말씀해주신 컨벤션은 60dbcb7 에서 반영했습니다! |
2 participants
🌁 Background
👩💻 Contents
📝 Review Note
NetworkPlugin 구조
플러그인은 네트워크 요청의 전처리(willSend) 와 후처리(didReceive) 를 담당하는 구조로 설계되었습니다.
NetworkService는 등록된 플러그인들의willSend,didReceive를 순차적으로 실행하여 네트워크 요청의 확장성과 유연성을 확보합니다.고민했던 점 및 구조 설계 의도
NetworkPlugin 프로토콜을 채택한 플러그인은 언제든지NetworkService에 추가/제거할 수 있어, 요청 전처리(willSend), 응답 후처리(didReceive)를 유연하게 조합할 수 있습니다.willSend함수들을 등록 순서대로 순차적으로 실행하여,URLRequest를 가공합니다.URLRequest로 네트워크 요청을 전송합니다.didReceive함수들을 역시 등록 순서대로 실행하여,URLResponse를 분석하고 후처리를 진행합니다.NetworkService에서 관리하는 것이 관심사의 분리 측면에서 더 적합하다고 판단했습니다.RetryPlugin은 단순히 응답에 따라 재시도 필요 여부(needRetry, noRetry)만 판단하고,NetworkService가 이 값을 받아 실제로 재요청을 처리합니다.데스노트 1번: 로깅이 정상적으로 일어나도록 로직 정리
// 아래 있는 1번 주석의 흐름이 먼저입니다!! // 2. 그래서 일단 급하게 요 부분에 didReceive 이전에 로깅할 수 있는 로직을 추가함 if let httpResponse = response as? HTTPURLResponse { BitnagilLogger.log(logType: .info, message: "응답 코드: \(httpResponse.statusCode)") } if withPlugins { for plugin in plugins { try await plugin.didReceive(response: response, data: data, endpoint: endpoint) } } // 1. 이 아래 부분이 실행되지 않는 문제가 있었음!!!!!!!! 그래서 guard let httpResponse = response as? HTTPURLResponse else { throw NetworkError.invalidResponse } guard 200..<300 ~= httpResponse.statusCode else { BitnagilLogger.log(logType: .error, message: "응답 코드: \(httpResponse.statusCode)") throw NetworkError.invalidStatusCode(statusCode: httpResponse.statusCode) }데스노트 2번: 플러그인 의존성 관리
Summary by CodeRabbit
Summary by CodeRabbit
신규 기능
버그 수정
문서화
리팩터링