Apply updates from yorkie-js-sdk v0.4.27 #192
Conversation
WalkthroughThis pull request introduces several enhancements to the Yorkie framework, focusing on error handling, service interaction, and testing. The changes include adding a new Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
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: 6
🔭 Outside diff range comments (1)
Sources/Core/Client.swift (1)
Line range hint
171-182: Potential API Breaking Change with New Initializer ParameterAdding the
isMockingEnabledparameter to the public initializer changes the method signature of a public API. This can potentially break existing code that relies on the previous initializer without this parameter.
- Recommendation: To maintain backward compatibility, consider providing an overloaded initializer or assigning a default value to
isMockingEnabled.Apply this diff to add a default value and preserve the previous initializer behavior:
public nonisolated init(_ urlString: String, _ options: ClientOptions = ClientOptions(), isMockingEnabled: Bool = false) { self.key = options.key ?? UUID().uuidString self.syncLoopDuration = options.syncLoopDuration self.reconnectStreamDelay = options.reconnectStreamDelay self.maximumAttachmentTimeout = options.maximumAttachmentTimeout let protocolClient = ProtocolClient(httpClient: URLSessionHTTPClient(), config: ProtocolClientConfig(host: urlString, networkProtocol: .connect, codec: ProtoCodec())) - self.yorkieService = YorkieService(rpcClient: YorkieServiceClient(client: protocolClient), isMockingEnabled: isMockingEnabled) + self.yorkieService = YorkieService(rpcClient: YorkieServiceClient(client: protocolClient), isMockingEnabled: isMockingEnabled) self.authHeader = AuthHeader(apiKey: options.apiKey, token: options.token) }
🧹 Nitpick comments (12)
Tests/Integration/PresenceTests.swift (2)
584-585: Consider using a named constant for the expected presence count.The assertions correctly verify the presence count and structure. However, the magic number "1" could be replaced with a named constant to make the test more maintainable and self-documenting.
+ private static let EXPECTED_SELF_PRESENCE_COUNT = 1 + func test_can_receive_unwatched_event_when_a_client_detaches() async throws { // ... existing code ... - XCTAssertEqual(resultPresence1.count, 1) + XCTAssertEqual(resultPresence1.count, Self.EXPECTED_SELF_PRESENCE_COUNT)
587-587: Enhance the comment about SDK differences.The comment about SDK differences could be more detailed to help future maintainers understand the rationale behind this behavior.
- // NOTE(hiddenviewer): js-sdk return [ActorIDs.initial: [:]], but ios-sdk return empty + // NOTE(hiddenviewer): Platform-specific behavior: + // - js-sdk: Returns [ActorIDs.initial: [:]] to maintain a consistent presence structure + // - ios-sdk: Returns an empty array as there are no active presences after detachmentTests/Integration/SnapshotTests.swift (4)
12-12: Consider making the RPC address configurable.The hardcoded RPC address could make tests brittle. Consider moving it to a test configuration file or environment variable for better flexibility across different environments.
- let rpcAddress = "http://localhost:8080" + let rpcAddress = ProcessInfo.processInfo.environment["YORKIE_TEST_RPC_ADDRESS"] ?? "http://localhost:8080"
11-13: Add documentation for test requirements.Consider adding documentation comments explaining:
- Prerequisites for running these tests (e.g., running local server)
- Test environment setup requirements
- Purpose of these snapshot tests
17-21: Document the significance of 700 updates.The magic number 700 appears to be related to the snapshot threshold. Consider:
- Extracting it as a named constant
- Documenting why this specific number was chosen
- Adding assertions to verify the snapshot was actually triggered
+ // Number of updates required to trigger snapshot mechanism + private let snapshotThreshold = 700 + func test_should_handle_snapshot() async throws { try await withTwoClientsAndDocuments(self.description) { c1, d1, c2, d2 in - for idx in 0 ..< 700 { + for idx in 0 ..< snapshotThreshold {
11-99: Consider expanding test coverage.While the tests cover basic snapshot functionality, consider adding:
- Error cases (network failures, concurrent modifications)
- Different data types beyond numbers and text
- Edge cases (empty documents, maximum sizes)
- Performance benchmarks for snapshot operations
Sources/Util/Errors.swift (1)
90-98: Consider enhancing error code extractionThe current implementation only uses the first detail and returns an empty string if no error code is found. Consider:
- Logging when multiple error details are present
- Providing a default error code instead of an empty string
func errorCodeOf(error: ConnectError) -> String { let infos: [ErrorInfo] = error.unpackedDetails() + if infos.count > 1 { + print("Warning: Multiple error details found, using first one") + } for info in infos { - return info.metadata["code"] ?? "" + return info.metadata["code"] ?? "unknown_error" } - return "" + return "unknown_error" }Sources/Core/YorkieService.swift (2)
28-84: Consider reducing duplication in error handlingThe error handling pattern is repeated across all methods. Consider extracting this into a helper method:
+ private func handleMockError<T>(for method: Connect.MethodSpec) -> ResponseMessage<T>? { + if self.isMockingEnabled, let error = getMockError(for: method) { + return .init(result: .failure(error)) + } + return nil + } @available(iOS 13, *) public func activateClient(request: Yorkie_V1_ActivateClientRequest, headers: Connect.Headers = [:]) async -> ResponseMessage<Yorkie_V1_ActivateClientResponse> { - if self.isMockingEnabled, let error = getMockError(for: YorkieServiceClient.Metadata.Methods.activateClient) { - return .init(result: .failure(error)) + if let mockResponse = handleMockError(for: YorkieServiceClient.Metadata.Methods.activateClient) { + return mockResponse } return await self.rpcClient.activateClient(request: request, headers: headers) }
75-77: Document watchDocument methodAdd documentation for the public
watchDocumentmethod to maintain consistency with other methods.+ /** + * `watchDocument` creates a server-only stream for watching document changes. + * This method does not support mocking as it returns a stream interface. + */ public func watchDocument(headers: Connect.Headers = [:], onResult: @escaping @Sendable (Connect.StreamResult<Yorkie_V1_WatchDocumentResponse>) -> Void) -> any Connect.ServerOnlyStreamInterface<Yorkie_V1_WatchDocumentRequest> {Tests/Integration/ClientIntegrationTests.swift (1)
782-797: Consider using defer for client deactivation.While the test effectively verifies sequential request handling, it could benefit from using
deferto ensure client deactivation even if an error occurs.func test_should_handle_each_request_one_by_one() async throws { for index in 0 ..< 10 { let client = Client(rpcAddress) try await client.activate() + defer { + try? await client.deactivate() + } let docKey = "\(Date().description)-\(self.description)-\(index)".toDocKey let doc = Document(key: docKey) do { try await client.attach(doc) - try await client.deactivate() } catch { XCTFail("\(error.localizedDescription)") } } }Sources/API/V1/googleapis/google/rpc/error_details.proto (2)
17-17: Update package name to include version.The package name should be suffixed with a version (e.g.,
google.rpc.v1) to follow proto best practices.-package google.rpc; +package google.rpc.v1;🧰 Tools
🪛 buf (1.47.2)
17-17: Files with package "google.rpc" must be within a directory "google/rpc" relative to root but were in directory "Sources/API/V1/googleapis/google/rpc".
(PACKAGE_DIRECTORY_MATCH)
17-17: Package name "google.rpc" should be suffixed with a correctly formed version, such as "google.rpc.v1".
(PACKAGE_VERSION_SUFFIX)
19-19: Remove unused import.The
google/protobuf/duration.protoimport is not used in this file.-import "google/protobuf/duration.proto";🧰 Tools
🪛 buf (1.47.2)
19-19: Import "google/protobuf/duration.proto" is unused.
(IMPORT_USED)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Sources/API/V1/Generated/googleapis/google/rpc/error_details.pb.swiftis excluded by!**/generated/**
📒 Files selected for processing (15)
Package.swift(1 hunks)Sources/API/GRPCTypeAlias.swift(1 hunks)Sources/API/V1/googleapis/google/rpc/error_details.proto(1 hunks)Sources/Core/Attachment.swift(1 hunks)Sources/Core/Client.swift(23 hunks)Sources/Core/YorkieService.swift(1 hunks)Sources/Document/CRDT/CRDTCounter.swift(1 hunks)Sources/Document/Document.swift(2 hunks)Sources/Util/Errors.swift(1 hunks)Tests/Integration/ClientIntegrationTests.swift(4 hunks)Tests/Integration/DocumentIntegrationTests.swift(1 hunks)Tests/Integration/PresenceTests.swift(1 hunks)Tests/Integration/SnapshotTests.swift(1 hunks)Yorkie.xcodeproj/project.pbxproj(13 hunks)Yorkie.xcodeproj/xcshareddata/xcschemes/Yorkie.xcscheme(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- Yorkie.xcodeproj/xcshareddata/xcschemes/Yorkie.xcscheme
- Sources/Document/Document.swift
🧰 Additional context used
🪛 buf (1.47.2)
Sources/API/V1/googleapis/google/rpc/error_details.proto
17-17: Files with package "google.rpc" must be within a directory "google/rpc" relative to root but were in directory "Sources/API/V1/googleapis/google/rpc".
(PACKAGE_DIRECTORY_MATCH)
17-17: Package name "google.rpc" should be suffixed with a correctly formed version, such as "google.rpc.v1".
(PACKAGE_VERSION_SUFFIX)
19-19: Import "google/protobuf/duration.proto" is unused.
(IMPORT_USED)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (20)
Tests/Integration/PresenceTests.swift (1)
589-589: LGTM!The assertion correctly verifies that the presence is empty after detachment, which is the expected behavior for the iOS SDK.
Yorkie.xcodeproj/project.pbxproj (5)
810-810: Project settings have been updated.The LastUpgradeCheck has been updated from 1530 to 1620, which indicates the project settings have been upgraded to a newer version of Xcode.
61-62: New source files have been added to support the yorkie-js-sdk v0.4.27 update.The following files have been added:
error_details.pb.swift: Generated protobuf code for error detailsYorkieService.swift: New service for mocking YorkieServiceClient APISnapshotTests.swift: New tests for snapshot functionalityThese additions align with the PR objectives.
Also applies to: 123-123, 416-416
432-479: New group structure added for error details proto files.A well-organized group hierarchy has been created under the Generated section:
googleapis/ └── google/ └── rpc/ └── error_details.protoThis follows standard protobuf organization practices for Google API types.
Line range hint
1-1293: Overall project structure remains well-organized.The project maintains a clean organization with:
- Clear separation between Sources and Tests
- Proper grouping of related files
- Consistent file naming
- Appropriate build phases including SwiftFormat and SwiftLint
Line range hint
1235-1243: New package dependency added for RPC functionality.The connect-swift package has been added with version constraints:
XCRemoteSwiftPackageReference "connect-swift" */ = { repositoryURL = "https://github.com/connectrpc/connect-swift.git"; requirement = { kind = upToNextMajorVersion; minimumVersion = 0.13.0; }; };This is required for the new RPC functionality.
Let's verify if this is the latest compatible version:
✅ Verification successful
Version constraint is properly set for connect-swift package ✅
The current setting (
minimumVersion = 0.13.0withupToNextMajorVersion) is optimal as it:
- Allows automatic updates to the stable 1.0.0 release
- Includes all necessary features from 0.13.0 and above
- Protects against future breaking changes by staying below 2.0.0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest version of connect-swift curl -s https://api.github.com/repos/connectrpc/connect-swift/releases/latest | jq -r .tag_nameLength of output: 103
Script:
#!/bin/bash # Check release notes between 0.13.0 and 1.0.0 curl -s "https://api.github.com/repos/connectrpc/connect-swift/releases" | jq -r '.[] | select(.tag_name >= "0.13.0" and .tag_name <= "1.0.0") | "Version: \(.tag_name)\n\(.body)\n"'Length of output: 10473
Tests/Integration/SnapshotTests.swift (2)
33-35: Simplify multiple sync calls.The purpose of multiple sync calls isn't clear. Consider:
- Adding comments explaining why multiple syncs are needed
- Or reducing to a single sync if possible
- Adding assertions between syncs to verify intermediate states
45-49: Clarify the purpose of initial updates.The initial 700 updates with empty JSONText objects seem unnecessary. Consider:
- Documenting why these updates are needed
- Or removing them if they're not essential to the test
Sources/Core/Client.swift (2)
66-80: Introduction ofClientConditionEnhances State ManagementThe addition of the
ClientConditionenum and theconditionsdictionary improves the clarity and scalability of client state management. This structured approach allows for better tracking of client conditions likesyncLoopandwatchLoop.
426-435: Controlled Access to Client ConditionsThe addition of
getConditionandsetConditionmethods provides encapsulated access to client conditions. This enhances maintainability and thread safety by controlling how condition states are read and modified.Package.swift (1)
30-30: Verify Exclusion oferror_details.protoExcluding
"API/V1/googleapis/google/rpc/error_details.proto"may lead to issues if the project references types defined in this proto file.
- Recommendation: Ensure that all necessary types from
error_details.protoare correctly generated and included in the build. If the exclusion is intentional, double-check that there are no dependencies on this file.Run the following script to check for references to
ErrorInfoin the codebase:Sources/Document/CRDT/CRDTCounter.swift (1)
67-67: Assess Change in Thrown Error TypeChanging the thrown error from
YorkieError.typetoYorkieError.unimplementedalters the semantics of the error.
- Issue: Existing code that catches
YorkieError.typemay no longer handle this error, potentially causing unhandled exceptions.- Recommendation: Evaluate whether
unimplementedaccurately represents the error scenario. If the type is unsupported,invalidArgumentor a more descriptive error might be more appropriate.Consider this alternative:
- throw YorkieError.unimplemented(message: "unimplemented type: \(type(of: primitive.value))") + throw YorkieError.invalidArgument(message: "Unsupported primitive type: \(type(of: primitive.value))")Sources/Core/Attachment.swift (1)
20-20: Verify thread safety with@unchecked SendableThe class is marked as
@unchecked Sendablebut contains mutable state. This could lead to data races if the class is used across different threads. Consider either:
- Adding synchronization mechanisms for the mutable properties
- Documenting the thread-safety requirements
- Using
actorinstead if targeting iOS 13+Sources/API/GRPCTypeAlias.swift (1)
65-65: LGTM!The new
ErrorInfotype alias follows the established pattern and enhances error handling capabilities.Sources/Util/Errors.swift (1)
22-35: LGTM! Error cases are well-organizedThe new error cases provide better granularity and clarity for error handling.
Sources/Core/YorkieService.swift (1)
17-25: LGTM! Well-structured service layerThe service layer effectively wraps the RPC client and provides mocking capabilities.
Tests/Integration/DocumentIntegrationTests.swift (1)
44-44: LGTM! Error case name updated for better clarity.The change from
clientNotActivetoclientNotActivatedmakes the error case more descriptive and consistent.Tests/Integration/ClientIntegrationTests.swift (2)
61-91: LGTM! Comprehensive test for document attachment/detachment.The test thoroughly covers error cases for document attachment and detachment operations, ensuring proper error handling and cleanup.
868-892: LGTM! Well-structured network failure retry test.The test effectively verifies the retry mechanism using mock errors and sync loop condition checks.
Sources/API/V1/googleapis/google/rpc/error_details.proto (1)
27-53: LGTM! Well-structured ErrorInfo message.The ErrorInfo message is well-documented with clear field descriptions and validation rules:
- reason: UPPER_SNAKE_CASE with max 63 chars
- domain: Service name or infrastructure identifier
- metadata: lowerCamelCase keys with max 64 chars
| // 01. Updates 700 changes over snapshot threshold by c1. | ||
| for _ in 0 ..< 700 { | ||
| try await d1.update { root, _ in | ||
| (root.k1 as? JSONText)?.setStyle(0, 1, ["bold": true]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add style attribute verification.
The test sets style attributes but doesn't verify them. Consider:
- Adding assertions to verify the applied styles
- Testing different attribute combinations
- Using safe casting for JSONText
- (root.k1 as? JSONText)?.setStyle(0, 1, ["bold": true])
+ guard let text = root.k1 as? JSONText else {
+ XCTFail("Expected root.k1 to be JSONText")
+ return
+ }
+ text.setStyle(0, 1, ["bold": true])
+
+ // Verify style was applied
+ let styles = text.getStyles(0, 1)
+ XCTAssertEqual(styles["bold"] as? Bool, true)📝 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.
| (root.k1 as? JSONText)?.setStyle(0, 1, ["bold": true]) | |
| guard let text = root.k1 as? JSONText else { | |
| XCTFail("Expected root.k1 to be JSONText") | |
| return | |
| } | |
| text.setStyle(0, 1, ["bold": true]) | |
| // Verify style was applied | |
| let styles = text.getStyles(0, 1) | |
| XCTAssertEqual(styles["bold"] as? Bool, true) |
| // 01. Updates 500 changes over snapshot threshold by c1. | ||
| for idx in 0 ..< 500 { | ||
| try await d1.update { root, _ in | ||
| (root.k1 as? JSONText)?.edit(idx, idx, "x") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use safe casting and add content verification.
The force casting of JSONText using as? could silently fail. Consider:
- Adding guard statement for type safety
- Verifying the actual text content after edits
- (root.k1 as? JSONText)?.edit(idx, idx, "x")
+ guard let text = root.k1 as? JSONText else {
+ XCTFail("Expected root.k1 to be JSONText")
+ return
+ }
+ text.edit(idx, idx, "x")📝 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.
| (root.k1 as? JSONText)?.edit(idx, idx, "x") | |
| guard let text = root.k1 as? JSONText else { | |
| XCTFail("Expected root.k1 to be JSONText") | |
| return | |
| } | |
| text.edit(idx, idx, "x") |
Sources/Core/Client.swift
Outdated
| throw attachResponse.error ?? YorkieError.rpcError(message: attachResponse.error.debugDescription) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Repeated Error Handling Logic
The pattern of potentially unsafe error handling appears in multiple methods (activate, deactivate, attach, etc.). Repeating this logic increases the risk of nil dereferences and code duplication.
- Recommendation: Create a utility function to handle error responses consistently and safely across all methods.
Example utility function:
private func handleErrorResponse(_ error: Error?, defaultMessage: String) throws -> Error {
if let error = error {
return error
} else {
return YorkieError.rpcError(message: defaultMessage)
}
}Refactored error handling in methods:
guard attachResponse.error == nil, let message = attachResponse.message else {
throw try self.handleErrorResponse(attachResponse.error, defaultMessage: "Unknown attach error")
}
Sources/Core/Client.swift
Outdated
| guard activateResponse.error == nil, let message = activateResponse.message else { | ||
| throw activateResponse.error ?? YorkieError.rpcError(message: activateResponse.error.debugDescription) | ||
| } |
There was a problem hiding this comment.
Potential Nil Dereference in Error Handling
In the activate method, there is a possibility of a nil dereference when accessing activateResponse.error.debugDescription if activateResponse.error is nil.
- Issue: If
activateResponse.errorisnil, accessingdebugDescriptionwill cause a runtime exception. - Recommendation: Safely unwrap
activateResponse.errorbefore accessing its properties or provide a default error message.
Apply this diff to prevent nil dereference:
guard activateResponse.error == nil, let message = activateResponse.message else {
- throw activateResponse.error ?? YorkieError.rpcError(message: activateResponse.error.debugDescription)
+ let errorMessage = activateResponse.error?.debugDescription ?? "Unknown activation error"
+ throw activateResponse.error ?? YorkieError.rpcError(message: errorMessage)
}📝 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.
| guard activateResponse.error == nil, let message = activateResponse.message else { | |
| throw activateResponse.error ?? YorkieError.rpcError(message: activateResponse.error.debugDescription) | |
| } | |
| guard activateResponse.error == nil, let message = activateResponse.message else { | |
| let errorMessage = activateResponse.error?.debugDescription ?? "Unknown activation error" | |
| throw activateResponse.error ?? YorkieError.rpcError(message: errorMessage) | |
| } |
Sources/Core/Client.swift
Outdated
| guard deactivateResponse.error == nil else { | ||
| throw deactivateResponse.error ?? YorkieError.rpcError(message: deactivateResponse.error.debugDescription) | ||
| } |
There was a problem hiding this comment.
Consistent Error Handling: Prevent Nil Dereference
Similar to the activate method, the deactivate method may attempt to access deactivateResponse.error.debugDescription when deactivateResponse.error is nil.
- Recommendation: Safely handle the nil case to avoid runtime exceptions.
Apply this diff to fix the issue:
guard deactivateResponse.error == nil else {
- throw deactivateResponse.error ?? YorkieError.rpcError(message: deactivateResponse.error.debugDescription)
+ let errorMessage = deactivateResponse.error?.debugDescription ?? "Unknown deactivation error"
+ throw deactivateResponse.error ?? YorkieError.rpcError(message: errorMessage)
}📝 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.
| guard deactivateResponse.error == nil else { | |
| throw deactivateResponse.error ?? YorkieError.rpcError(message: deactivateResponse.error.debugDescription) | |
| } | |
| guard deactivateResponse.error == nil else { | |
| let errorMessage = deactivateResponse.error?.debugDescription ?? "Unknown deactivation error" | |
| throw deactivateResponse.error ?? YorkieError.rpcError(message: errorMessage) | |
| } |
Sources/Core/Client.swift
Outdated
| do { | ||
| try self.deactivateInternal() | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Silently Ignoring Errors in handleConnectError
In the handleConnectError method, errors thrown by self.deactivateInternal() are caught but not handled or logged.
- Issue: Silently ignoring errors can make debugging difficult and may hide critical issues.
- Recommendation: Log the error or handle it appropriately to ensure that any issues are not overlooked.
Apply this diff to handle the error:
do {
try self.deactivateInternal()
-} catch {}
+} catch {
+ Logger.error("Error during client deactivation: \(error)")
+}📝 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.
| do { | |
| try self.deactivateInternal() | |
| } catch {} | |
| } | |
| do { | |
| try self.deactivateInternal() | |
| } catch { | |
| Logger.error("Error during client deactivation: \(error)") | |
| } |
…d and ErrClientNotFound
hiddenviewer
force-pushed
the
apply_v0.4.27
branch
from
ef6f302 to
f0afc0f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #192 +/- ##
==========================================
- Coverage 45.77% 45.14% -0.63%
==========================================
Files 105 109 +4
Lines 26814 27190 +376
==========================================
+ Hits 12275 12276 +1
- Misses 14539 14914 +375 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/Core/Client.swift (1)
204-222: 🛠️ Refactor suggestionConsider consolidating error handling.
The error handling pattern is repeated across multiple methods. Consider extracting it to a reusable method.
Create a utility method for consistent error handling:
private func handleRPCResponse<T>(_ response: ResponseMessage<T>, operation: String) throws -> T { guard response.error == nil, let message = response.message else { Logger.error("Failed to \(operation) client(\(self.key)).") let error = response.error ?? YorkieError.rpcError(message: response.error.debugDescription) self.handleConnectError(error) throw error } return message }Usage:
let message = try handleRPCResponse(activateResponse, operation: "activate")Also applies to: 233-249
🧹 Nitpick comments (5)
Sources/Core/YorkieService.swift (1)
94-104: Consider adding thread safety for mock errors.The
mockErrorsdictionary is accessed from multiple async methods without synchronization. While the current usage in tests might be safe, adding thread safety would prevent potential race conditions if the mocking functionality is expanded.Consider using a serial queue or actor to synchronize access:
- private var mockErrors: [String: ConnectError] = [:] + private let mockErrorQueue = DispatchQueue(label: "com.yorkie.mockErrors") + private var _mockErrors: [String: ConnectError] = [:] + + private func getMockError(for method: Connect.MethodSpec) -> ConnectError? { + guard self.isMockingEnabled else { return nil } + return mockErrorQueue.sync { + return self._mockErrors.removeValue(forKey: method.name) + } + } + + public func setMockError(for method: Connect.MethodSpec, error: ConnectError) { + mockErrorQueue.sync { + self._mockErrors[method.name] = error + } + }Sources/Util/Errors.swift (2)
38-68: Consider adding error code documentation.While the error codes are self-explanatory, adding documentation for each case would help developers understand when each error occurs and how to handle them.
Add documentation for each error code:
enum Code: String { + /// Returned when the operation completed successfully. case ok + /// Returned when attempting operations on an inactive client. case errClientNotActivated = "ErrClientNotActivated" // Add docs for remaining cases... }
87-108: Consider adding error code constants.The error code strings are used directly in the code. Consider extracting them to constants for better maintainability.
+private enum ErrorCodes { + static let codeMetadataKey = "code" +} func errorCodeOf(error: ConnectError) -> String { let infos: [ErrorInfo] = error.unpackedDetails() for info in infos { - return info.metadata["code"] ?? "" + return info.metadata[ErrorCodes.codeMetadataKey] ?? "" } return "" }Tests/Integration/ClientIntegrationTests.swift (1)
782-797: Consider adding assertions for client state.While the test verifies the basic flow, it could be enhanced by checking the client's state after each operation.
try await client.activate() +XCTAssertTrue(await client.isActive) let docKey = "\(Date().description)-\(self.description)-\(index)".toDocKey let doc = Document(key: docKey) do { try await client.attach(doc) + XCTAssertEqual(doc.status, .attached) try await client.deactivate() + XCTAssertFalse(await client.isActive) } catch { XCTFail("\(error.localizedDescription)") }Sources/Core/Client.swift (1)
825-859: Consider adding logging for error handling decisions.The error handling logic makes important decisions about retrying operations, but the reasoning isn't logged.
private func handleConnectError(_ error: Error?) -> Bool { guard let connectError = error as? ConnectError else { + Logger.debug("Non-ConnectError received, not retryable: \(String(describing: error))") return false } if connectError.code == .canceled || connectError.code == .unknown || connectError.code == .resourceExhausted || connectError.code == .unavailable { + Logger.info("Retryable error received: \(connectError.code)") return true } let yorkieErrorCode = YorkieError.Code(rawValue: errorCodeOf(error: connectError)) if yorkieErrorCode == YorkieError.Code.errClientNotActivated || yorkieErrorCode == YorkieError.Code.errClientNotFound { + Logger.info("Client state error received: \(yorkieErrorCode ?? "unknown"), deactivating client") do { try self.deactivateInternal() } catch { + Logger.error("Failed to deactivate client: \(error)") } } return false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Sources/API/V1/Generated/googleapis/google/rpc/error_details.pb.swiftis excluded by!**/generated/**
📒 Files selected for processing (15)
Package.swift(1 hunks)Sources/API/GRPCTypeAlias.swift(1 hunks)Sources/API/V1/googleapis/google/rpc/error_details.proto(1 hunks)Sources/Core/Attachment.swift(1 hunks)Sources/Core/Client.swift(23 hunks)Sources/Core/YorkieService.swift(1 hunks)Sources/Document/CRDT/CRDTCounter.swift(1 hunks)Sources/Document/Document.swift(2 hunks)Sources/Util/Errors.swift(1 hunks)Tests/Integration/ClientIntegrationTests.swift(4 hunks)Tests/Integration/DocumentIntegrationTests.swift(1 hunks)Tests/Integration/PresenceTests.swift(1 hunks)Tests/Integration/SnapshotTests.swift(1 hunks)Yorkie.xcodeproj/project.pbxproj(13 hunks)Yorkie.xcodeproj/xcshareddata/xcschemes/Yorkie.xcscheme(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- Sources/Core/Attachment.swift
- Sources/Document/CRDT/CRDTCounter.swift
- Sources/API/GRPCTypeAlias.swift
- Tests/Integration/DocumentIntegrationTests.swift
- Yorkie.xcodeproj/xcshareddata/xcschemes/Yorkie.xcscheme
- Sources/Document/Document.swift
- Package.swift
- Tests/Integration/PresenceTests.swift
- Tests/Integration/SnapshotTests.swift
🧰 Additional context used
🪛 buf (1.47.2)
Sources/API/V1/googleapis/google/rpc/error_details.proto
17-17: Files with package "google.rpc" must be within a directory "google/rpc" relative to root but were in directory "Sources/API/V1/googleapis/google/rpc".
(PACKAGE_DIRECTORY_MATCH)
17-17: Package name "google.rpc" should be suffixed with a correctly formed version, such as "google.rpc.v1".
(PACKAGE_VERSION_SUFFIX)
19-19: Import "google/protobuf/duration.proto" is unused.
(IMPORT_USED)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (14)
Yorkie.xcodeproj/project.pbxproj (4)
810-810: Project configuration has been updated.The LastUpgradeCheck has been updated from 1530 to 1620, which is good practice for maintaining compatibility with the latest Xcode version.
432-479: Well-organized group structure for generated files.The new group hierarchy (googleapis/google/rpc) follows the standard protobuf file organization pattern, which is good for maintainability.
61-62: New files align with PR objectives.The added files match the PR objectives:
error_details.protoanderror_details.pb.swiftfor structured error detailsYorkieService.swiftfor mocking the YorkieServiceClient APISnapshotTests.swiftfor enhanced testing frameworkAlso applies to: 123-123, 194-196, 258-258
Line range hint
1213-1236: Verify package dependencies are compatible.The project uses several Swift packages. Let's verify their versions are compatible and up-to-date:
- connect-swift: v0.13.0
- Semaphore: v0.0.8
- swift-log: v1.4.0
Sources/Core/YorkieService.swift (2)
11-16: LGTM! Well-documented service wrapper.The class documentation clearly explains the purpose and responsibilities of the
YorkieServiceclass.
27-33: LGTM! Consistent error handling pattern.The RPC methods follow a consistent pattern for error handling and mocking, making the code maintainable and predictable.
Also applies to: 35-41, 43-49, 51-57, 59-65, 67-73, 75-77, 79-85
Sources/Util/Errors.swift (2)
22-32: LGTM! Well-structured error cases.The new error cases provide clear and specific error handling capabilities.
70-84: LGTM! Clear error code mapping.The
codeproperty provides a clean mapping between error cases and codes.Tests/Integration/ClientIntegrationTests.swift (2)
61-91: LGTM! Comprehensive document lifecycle test.The test thoroughly verifies the document attachment and detachment behavior.
867-892: LGTM! Robust network failure test.The test effectively verifies the retry mechanism and condition tracking.
Sources/Core/Client.swift (2)
66-79: LGTM! Clear client condition management.The
ClientConditionenum provides a clean way to track client state.
Line range hint
171-182: LGTM! Clean service integration.The initialization properly integrates the new
YorkieServicewith mocking support.Sources/API/V1/googleapis/google/rpc/error_details.proto (2)
17-17: Package location and versioning need attention.The package declaration has two compliance issues:
- The file location should match the package name hierarchy
- The package name should include a version suffix (e.g.,
google.rpc.v1)Run this script to verify the package structure:
🧰 Tools
🪛 buf (1.47.2)
17-17: Files with package "google.rpc" must be within a directory "google/rpc" relative to root but were in directory "Sources/API/V1/googleapis/google/rpc".
(PACKAGE_DIRECTORY_MATCH)
17-17: Package name "google.rpc" should be suffixed with a correctly formed version, such as "google.rpc.v1".
(PACKAGE_VERSION_SUFFIX)
27-53: LGTM! Well-structured error details message.The
ErrorInfomessage is well-documented and follows Google's API design guidelines. The field constraints are clearly specified:
reason: UPPER_SNAKE_CASE format, max 63 charsmetadatakeys: lowerCamelCase format, max 64 charsLet's verify that the error reasons in the codebase follow the specified format:
✅ Verification successful
Implementation follows the error format requirements
The codebase correctly implements the ErrorInfo message format requirements:
- Error codes use proper UPPER_SNAKE_CASE format (e.g., "ErrClientNotActivated")
- Metadata keys follow lowerCamelCase convention (e.g., "instanceLimitPerRequest")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error reason format compliance # Look for error reason string literals rg '"[A-Z][A-Z0-9_]+[A-Z0-9]"' --type swift # Look for error metadata key definitions rg '"[a-z][a-zA-Z0-9-_]+"' --type swiftLength of output: 78362
Script:
#!/bin/bash # Search for error-related files and their content echo "=== Error-related files ===" fd "error|err" --type f echo -e "\n=== Error reason definitions ===" # Look for error reason constants/definitions rg "reason = |reason:|reason =" --type swift --type proto echo -e "\n=== Error metadata usage ===" # Look for error metadata implementations rg "metadata\[|metadata\." --type swiftLength of output: 810
Script:
#!/bin/bash # Examine error implementations echo "=== Errors.swift content ===" cat Sources/Util/Errors.swift echo -e "\n=== Generated error_details.pb.swift relevant sections ===" # Extract sections related to error reason and metadata rg -A 5 -B 5 "reason|metadata" Sources/API/V1/Generated/googleapis/google/rpc/error_details.pb.swiftLength of output: 7866
|
|
||
| package google.rpc; | ||
|
|
||
| import "google/protobuf/duration.proto"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import.
The import of google/protobuf/duration.proto is not used in this file.
Apply this diff to remove the unused import:
-import "google/protobuf/duration.proto";🧰 Tools
🪛 buf (1.47.2)
19-19: Import "google/protobuf/duration.proto" is unused.
(IMPORT_USED)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Tests/Integration/ClientIntegrationTests.swift (1)
61-91: Consider enhancing test coverage.While the test covers basic attach/detach scenarios, consider adding:
- Verification of document state after attachment/detachment
- Testing concurrent attach/detach operations
- Testing with invalid document keys
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tests/Integration/ClientIntegrationTests.swift(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
| try await client.deactivate() | ||
| try await client.deactivate() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate client deactivation call.
The client is being deactivated twice consecutively, which is unnecessary.
- try await client.deactivate()
- try await client.deactivate()
+ try await client.deactivate()📝 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.
| try await client.deactivate() | |
| try await client.deactivate() | |
| try await client.deactivate() |
| func test_should_handle_each_request_one_by_one() async throws { | ||
| for index in 0 ..< 10 { | ||
| let client = Client(rpcAddress) | ||
| try await client.activate() | ||
|
|
||
| let docKey = "\(Date().description)-\(self.description)-\(index)".toDocKey | ||
| let doc = Document(key: docKey) | ||
|
|
||
| do { | ||
| try await client.attach(doc) | ||
| try await client.deactivate() | ||
| } catch { | ||
| XCTFail("\(error.localizedDescription)") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test robustness and coverage.
The test could be improved in several ways:
- Add assertions to verify successful attachment state
- Test parallel client operations to ensure proper request handling
- Add specific error type handling instead of generic error catching
Consider this enhanced version:
func test_should_handle_each_request_one_by_one() async throws {
+ async let operations = withTaskGroup(of: Void.self) { group in
for index in 0 ..< 10 {
- let client = Client(rpcAddress)
- try await client.activate()
-
- let docKey = "\(Date().description)-\(self.description)-\(index)".toDocKey
- let doc = Document(key: docKey)
-
- do {
- try await client.attach(doc)
- try await client.deactivate()
- } catch {
- XCTFail("\(error.localizedDescription)")
- }
+ group.addTask {
+ let client = Client(rpcAddress)
+ do {
+ try await client.activate()
+ let docKey = "\(Date().description)-\(self.description)-\(index)".toDocKey
+ let doc = Document(key: docKey)
+
+ try await client.attach(doc)
+ XCTAssertTrue(await doc.isAttached(), "Document should be attached")
+
+ try await client.deactivate()
+ } catch let error as YorkieError {
+ XCTFail("Operation failed with Yorkie error: \(error)")
+ } catch {
+ XCTFail("Operation failed with error: \(error.localizedDescription)")
+ }
+ }
}
+ }
+ try await operations
}📝 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.
| func test_should_handle_each_request_one_by_one() async throws { | |
| for index in 0 ..< 10 { | |
| let client = Client(rpcAddress) | |
| try await client.activate() | |
| let docKey = "\(Date().description)-\(self.description)-\(index)".toDocKey | |
| let doc = Document(key: docKey) | |
| do { | |
| try await client.attach(doc) | |
| try await client.deactivate() | |
| } catch { | |
| XCTFail("\(error.localizedDescription)") | |
| } | |
| } | |
| } | |
| func test_should_handle_each_request_one_by_one() async throws { | |
| async let operations = withTaskGroup(of: Void.self) { group in | |
| for index in 0 ..< 10 { | |
| group.addTask { | |
| let client = Client(rpcAddress) | |
| do { | |
| try await client.activate() | |
| let docKey = "\(Date().description)-\(self.description)-\(index)".toDocKey | |
| let doc = Document(key: docKey) | |
| try await client.attach(doc) | |
| XCTAssertTrue(await doc.isAttached(), "Document should be attached") | |
| try await client.deactivate() | |
| } catch let error as YorkieError { | |
| XCTFail("Operation failed with Yorkie error: \(error)") | |
| } catch { | |
| XCTFail("Operation failed with error: \(error.localizedDescription)") | |
| } | |
| } | |
| } | |
| } | |
| try await operations | |
| } |
| @MainActor | ||
| func test_should_retry_on_network_failure_and_eventually_succeed() async throws { | ||
| let c1 = Client(rpcAddress, isMockingEnabled: true) | ||
| try await c1.activate() | ||
|
|
||
| let docKey = "\(Date().description)-\(self.description)".toDocKey | ||
| let d1 = Document(key: docKey) | ||
| try await c1.attach(d1) | ||
|
|
||
| c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges, | ||
| error: connectError(from: .unknown)) | ||
|
|
||
| try d1.update { root, _ in | ||
| root.t = JSONText() | ||
| (root.t as? JSONText)?.edit(0, 0, "a") | ||
| } | ||
|
|
||
| XCTAssertTrue(c1.getCondition(.syncLoop)) | ||
|
|
||
| c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges, | ||
| error: connectError(from: .failedPrecondition)) | ||
|
|
||
| try await Task.sleep(nanoseconds: 1_000_000_000) | ||
|
|
||
| XCTAssertFalse(c1.getCondition(.syncLoop)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test reliability and assertions.
The test has a few areas for improvement:
- The fixed sleep duration could lead to flaky tests
- Missing assertions for the actual document state
- Could verify retry attempts count
Consider enhancing the test:
@MainActor
func test_should_retry_on_network_failure_and_eventually_succeed() async throws {
let c1 = Client(rpcAddress, isMockingEnabled: true)
try await c1.activate()
let docKey = "\(Date().description)-\(self.description)".toDocKey
let d1 = Document(key: docKey)
try await c1.attach(d1)
c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges,
error: connectError(from: .unknown))
try d1.update { root, _ in
root.t = JSONText()
(root.t as? JSONText)?.edit(0, 0, "a")
}
XCTAssertTrue(c1.getCondition(.syncLoop))
c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges,
error: connectError(from: .failedPrecondition))
- try await Task.sleep(nanoseconds: 1_000_000_000)
+ // Use expectation to avoid fixed sleep
+ let exp = expectation(description: "Sync loop should end")
+ Task {
+ while c1.getCondition(.syncLoop) {
+ try await Task.sleep(nanoseconds: 100_000_000)
+ }
+ exp.fulfill()
+ }
+ await fulfillment(of: [exp], timeout: 5)
XCTAssertFalse(c1.getCondition(.syncLoop))
+
+ // Verify document state
+ let text = await (d1.getRoot().t as? JSONText)?.toString
+ XCTAssertEqual(text, "a", "Document should contain the updated text")
+
+ try await c1.deactivate()
}📝 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.
| @MainActor | |
| func test_should_retry_on_network_failure_and_eventually_succeed() async throws { | |
| let c1 = Client(rpcAddress, isMockingEnabled: true) | |
| try await c1.activate() | |
| let docKey = "\(Date().description)-\(self.description)".toDocKey | |
| let d1 = Document(key: docKey) | |
| try await c1.attach(d1) | |
| c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges, | |
| error: connectError(from: .unknown)) | |
| try d1.update { root, _ in | |
| root.t = JSONText() | |
| (root.t as? JSONText)?.edit(0, 0, "a") | |
| } | |
| XCTAssertTrue(c1.getCondition(.syncLoop)) | |
| c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges, | |
| error: connectError(from: .failedPrecondition)) | |
| try await Task.sleep(nanoseconds: 1_000_000_000) | |
| XCTAssertFalse(c1.getCondition(.syncLoop)) | |
| } | |
| @MainActor | |
| func test_should_retry_on_network_failure_and_eventually_succeed() async throws { | |
| let c1 = Client(rpcAddress, isMockingEnabled: true) | |
| try await c1.activate() | |
| let docKey = "\(Date().description)-\(self.description)".toDocKey | |
| let d1 = Document(key: docKey) | |
| try await c1.attach(d1) | |
| c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges, | |
| error: connectError(from: .unknown)) | |
| try d1.update { root, _ in | |
| root.t = JSONText() | |
| (root.t as? JSONText)?.edit(0, 0, "a") | |
| } | |
| XCTAssertTrue(c1.getCondition(.syncLoop)) | |
| c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges, | |
| error: connectError(from: .failedPrecondition)) | |
| // Use expectation to avoid fixed sleep | |
| let exp = expectation(description: "Sync loop should end") | |
| Task { | |
| while c1.getCondition(.syncLoop) { | |
| try await Task.sleep(nanoseconds: 100_000_000) | |
| } | |
| exp.fulfill() | |
| } | |
| await fulfillment(of: [exp], timeout: 5) | |
| XCTAssertFalse(c1.getCondition(.syncLoop)) | |
| // Verify document state | |
| let text = await (d1.getRoot().t as? JSONText)?.toString | |
| XCTAssertEqual(text, "a", "Document should contain the updated text") | |
| try await c1.deactivate() | |
| } |
| @@ -258,14 +258,11 @@ public class Document { | |||
| try self.applyChanges(pack.getChanges()) | |||
| } | |||
|
|
|||
| // 01. Remove local changes applied to server. | |||
| // 02. Remove local changes applied to server. | |||
There was a problem hiding this comment.
I think It's the wrong number.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Sources/API/V1/googleapis/google/rpc/error_details.proto (1)
17-17: Consider addressing package structure recommendations.Two structural improvements could be made to align with best practices:
- The package name should include a version suffix (e.g., "google.rpc.v1")
- The file's directory structure should match the package name
However, since this appears to be a standard Google API proto file, these changes might need to be coordinated with upstream changes in the Google APIs repository.
🧰 Tools
🪛 buf (1.47.2)
17-17: Files with package "google.rpc" must be within a directory "google/rpc" relative to root but were in directory "Sources/API/V1/googleapis/google/rpc".
(PACKAGE_DIRECTORY_MATCH)
17-17: Package name "google.rpc" should be suffixed with a correctly formed version, such as "google.rpc.v1".
(PACKAGE_VERSION_SUFFIX)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Sources/API/V1/googleapis/google/rpc/error_details.proto(1 hunks)Sources/Core/Client.swift(23 hunks)Tests/Integration/ClientIntegrationTests.swift(4 hunks)Tests/Integration/PresenceTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/Integration/PresenceTests.swift
🧰 Additional context used
🪛 buf (1.47.2)
Sources/API/V1/googleapis/google/rpc/error_details.proto
17-17: Files with package "google.rpc" must be within a directory "google/rpc" relative to root but were in directory "Sources/API/V1/googleapis/google/rpc".
(PACKAGE_DIRECTORY_MATCH)
17-17: Package name "google.rpc" should be suffixed with a correctly formed version, such as "google.rpc.v1".
(PACKAGE_VERSION_SUFFIX)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (9)
Sources/API/V1/googleapis/google/rpc/error_details.proto (1)
25-51: Consider implementing format validation for fields.The message definition includes specific format requirements in comments:
reason: UPPER_SNAKE_CASE format, max 63 charsmetadatakeys: lowerCamelCase format, max 64 charsThese constraints should be validated at runtime to ensure data consistency.
Would you like me to provide an implementation for field validation in the generated code? This could include:
- Regular expression validation for the formats
- Length checks for the specified limits
Tests/Integration/ClientIntegrationTests.swift (4)
61-87: LGTM! Well-structured test with good error handling coverage.The test thoroughly verifies document attachment and detachment functionality, including error cases.
89-90: Remove duplicate client deactivation call.The client is being deactivated twice consecutively, which is unnecessary.
- try await client.deactivate() - try await client.deactivate() + try await client.deactivate()
782-797: Enhance test robustness and coverage.The test could be improved in several ways:
- Add assertions to verify successful attachment state
- Test parallel client operations to ensure proper request handling
- Add specific error type handling instead of generic error catching
func test_should_handle_each_request_one_by_one() async throws { + async let operations = withTaskGroup(of: Void.self) { group in for index in 0 ..< 10 { - let client = Client(rpcAddress) - try await client.activate() - - let docKey = "\(Date().description)-\(self.description)-\(index)".toDocKey - let doc = Document(key: docKey) - - do { - try await client.attach(doc) - try await client.deactivate() - } catch { - XCTFail("\(error.localizedDescription)") - } + group.addTask { + let client = Client(rpcAddress) + do { + try await client.activate() + let docKey = "\(Date().description)-\(self.description)-\(index)".toDocKey + let doc = Document(key: docKey) + + try await client.attach(doc) + XCTAssertTrue(await doc.isAttached(), "Document should be attached") + + try await client.deactivate() + } catch let error as YorkieError { + XCTFail("Operation failed with Yorkie error: \(error)") + } catch { + XCTFail("Operation failed with error: \(error.localizedDescription)") + } + } } + } + try await operations }
867-892: Improve test reliability and assertions.The test has a few areas for improvement:
- The fixed sleep duration could lead to flaky tests
- Missing assertions for the actual document state
- Could verify retry attempts count
@MainActor func test_should_retry_on_network_failure_and_eventually_succeed() async throws { let c1 = Client(rpcAddress, isMockingEnabled: true) try await c1.activate() let docKey = "\(Date().description)-\(self.description)".toDocKey let d1 = Document(key: docKey) try await c1.attach(d1) c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges, error: connectError(from: .unknown)) try d1.update { root, _ in root.t = JSONText() (root.t as? JSONText)?.edit(0, 0, "a") } XCTAssertTrue(c1.getCondition(.syncLoop)) c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges, error: connectError(from: .failedPrecondition)) - try await Task.sleep(nanoseconds: 100_000_000) + // Use expectation to avoid fixed sleep + let exp = expectation(description: "Sync loop should end") + Task { + while c1.getCondition(.syncLoop) { + try await Task.sleep(nanoseconds: 100_000_000) + } + exp.fulfill() + } + await fulfillment(of: [exp], timeout: 5) XCTAssertFalse(c1.getCondition(.syncLoop)) + + // Verify document state + let text = await (d1.getRoot().t as? JSONText)?.toString + XCTAssertEqual(text, "a", "Document should contain the updated text") + + try await c1.deactivate() }Sources/Core/Client.swift (4)
66-79: LGTM! Well-designed condition tracking system.The implementation provides a clean and type-safe way to track client conditions with proper encapsulation.
Also applies to: 147-150, 423-435
825-869: LGTM! Robust error handling implementation.The error handling implementation:
- Clearly documents retryable error conditions
- Properly handles client state for specific errors
- Provides consistent error response handling
Line range hint
171-183: LGTM! Clean service initialization with mock support.The initialization changes properly separate concerns and add testing capabilities through the YorkieService wrapper.
591-621: LGTM! Well-structured stream and loop management.The implementation:
- Properly manages stream lifecycle
- Handles disconnections gracefully
- Maintains clear separation between watch and sync loops
- Ensures proper cleanup of resources
Also applies to: 721-746
hiddenviewer
force-pushed
the
apply_v0.4.27
branch
from
478c25a to
6615b47
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
Sources/API/V1/googleapis/google/rpc/error_details.proto (1)
25-51: Consider implementing field validation.The message definition includes specific constraints in comments that aren't enforced by the proto schema:
reasonfield: Must match[A-Z][A-Z0-9_]+[A-Z0-9]and be ≤ 63 charactersmetadatakeys: Must match[a-z][a-zA-Z0-9-_]+, be lowerCamelCase, and be ≤ 64 charactersConsider implementing validation logic in the generated code to enforce these constraints at runtime. Would you like me to help create validation helpers for these fields?
Tests/Integration/ClientIntegrationTests.swift (3)
61-91: Well-structured test with comprehensive error handling.The test effectively verifies document attachment/detachment behavior and error cases. However, there's a duplicate client deactivation call that should be removed.
- try await client.deactivate() - try await client.deactivate() + try await client.deactivate()
782-797: Enhance test robustness and coverage.The test could be improved by:
- Adding assertions to verify successful attachment state
- Testing parallel client operations to ensure proper request handling
- Adding specific error type handling instead of generic error catching
func test_should_handle_each_request_one_by_one() async throws { + async let operations = withTaskGroup(of: Void.self) { group in for index in 0 ..< 10 { - let client = Client(rpcAddress) - try await client.activate() - - let docKey = "\(Date().description)-\(self.description)-\(index)".toDocKey - let doc = Document(key: docKey) - - do { - try await client.attach(doc) - try await client.deactivate() - } catch { - XCTFail("\(error.localizedDescription)") - } + group.addTask { + let client = Client(rpcAddress) + do { + try await client.activate() + let docKey = "\(Date().description)-\(self.description)-\(index)".toDocKey + let doc = Document(key: docKey) + + try await client.attach(doc) + XCTAssertTrue(await doc.isAttached(), "Document should be attached") + + try await client.deactivate() + } catch let error as YorkieError { + XCTFail("Operation failed with Yorkie error: \(error)") + } catch { + XCTFail("Operation failed with error: \(error.localizedDescription)") + } + } } + } + try await operations }
867-899: Improve test reliability and assertions.The test has a few areas for improvement:
- The fixed sleep duration could lead to flaky tests
- Missing assertions for the actual document state
- Could verify retry attempts count
@MainActor func test_should_retry_on_network_failure_and_eventually_succeed() async throws { let c1 = Client(rpcAddress, isMockingEnabled: true) try await c1.activate() let docKey = "\(Date().description)-\(self.description)".toDocKey let d1 = Document(key: docKey) try await c1.attach(d1) c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges, error: connectError(from: .unknown)) try d1.update { root, _ in root.t = JSONText() (root.t as? JSONText)?.edit(0, 0, "a") } XCTAssertTrue(c1.getCondition(.syncLoop)) c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges, error: connectError(from: .failedPrecondition)) - try await Task.sleep(nanoseconds: 1_000_000_000) + // Use expectation to avoid fixed sleep + let exp = expectation(description: "Sync loop should end") + Task { + while c1.getCondition(.syncLoop) { + try await Task.sleep(nanoseconds: 100_000_000) + } + exp.fulfill() + } + await fulfillment(of: [exp], timeout: 5) XCTAssertFalse(c1.getCondition(.syncLoop)) + + // Verify document state + let text = await (d1.getRoot().t as? JSONText)?.toString + XCTAssertEqual(text, "a", "Document should contain the updated text") + + try await c1.deactivate() }Sources/Core/Client.swift (6)
147-150: Consider using a more concise dictionary initialization.The current initialization is correct but could be more concise.
- private var conditions: [ClientCondition: Bool] = [ - ClientCondition.syncLoop: false, - ClientCondition.watchLoop: false - ] + private var conditions: [ClientCondition: Bool] = Dictionary(uniqueKeysWithValues: + ClientCondition.allCases.map { ($0, false) })This would require adding
CaseIterableto theClientConditionenum.
204-222: Enhance error logging for better debugging.While the error handling is solid, the error logging could be more detailed to aid in debugging.
- Logger.error("Failed to request activate client(\(self.key)).") + Logger.error("Failed to request activate client(\(self.key)): \(error)") - Logger.error("Failed to request deactivate client(\(self.key)).") + Logger.error("Failed to request deactivate client(\(self.key)): \(error)")Also applies to: 233-249
600-621: Add more detailed logging for stream disconnection.The stream disconnect handling is correct but could benefit from more detailed logging to aid in debugging.
- Logger.debug("[WD] c:\"\(self.key)\" unwatches") + Logger.debug("[WD] c:\"\(self.key)\" unwatches, error: \(String(describing: error))") - Logger.warning("[WL] c:\"\(self.key)\" has Error \(String(describing: error))") + Logger.warning("[WL] c:\"\(self.key)\" disconnected with error: \(String(describing: error)), will retry")
825-861: Add retry attempt logging in handleConnectError.The error handling logic is solid, but adding retry attempt logging would help with monitoring and debugging.
@discardableResult private func handleConnectError(_ error: Error?) -> Bool { + static var retryAttempts: [String: Int] = [:] guard let connectError = error as? ConnectError else { return false } if connectError.code == .canceled || connectError.code == .unknown || connectError.code == .resourceExhausted || connectError.code == .unavailable { + retryAttempts[self.key, default: 0] += 1 + Logger.info("Retry attempt \(retryAttempts[self.key]!) for client \(self.key)") return true } + retryAttempts[self.key] = 0 // Handle other cases... return false }
863-869: Enhance error response with more context.The error handling could be more informative by including additional context.
private func handleErrorResponse(_ error: Error?, defaultMessage: String) -> Error { if let error = error { return error } else { - return YorkieError.rpcError(message: defaultMessage) + return YorkieError.rpcError(message: "\(defaultMessage) (Client: \(self.key))") } }
872-878: Add validation for mock error setup.Consider adding validation to ensure mock errors are only set when mocking is enabled.
public extension Client { func setMockError(for method: Connect.MethodSpec, error: ConnectError) { + guard self.yorkieService.isMockingEnabled else { + Logger.warning("Attempted to set mock error when mocking is disabled") + return + } self.yorkieService.setMockError(for: method, error: error) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Sources/API/V1/googleapis/google/rpc/error_details.proto(1 hunks)Sources/Core/Client.swift(23 hunks)Tests/Integration/ClientIntegrationTests.swift(4 hunks)Tests/Integration/PresenceTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/Integration/PresenceTests.swift
🧰 Additional context used
🪛 buf (1.47.2)
Sources/API/V1/googleapis/google/rpc/error_details.proto
17-17: Files with package "google.rpc" must be within a directory "google/rpc" relative to root but were in directory "Sources/API/V1/googleapis/google/rpc".
(PACKAGE_DIRECTORY_MATCH)
17-17: Package name "google.rpc" should be suffixed with a correctly formed version, such as "google.rpc.v1".
(PACKAGE_VERSION_SUFFIX)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
Sources/Core/Client.swift (1)
66-79: LGTM! Well-documented enum for client conditions.The
ClientConditionenum is well-designed with clear documentation for each case.
|
|
||
| syntax = "proto3"; | ||
|
|
||
| package google.rpc; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update package declaration to follow protobuf best practices.
The package declaration has two issues that should be addressed:
- The package name should include a version suffix (e.g.,
google.rpc.v1). - The file's location in the directory structure should match its package name.
Apply this diff to update the package declaration:
-package google.rpc;
+package google.rpc.v1;Additionally, ensure the file is placed in a directory structure that matches its package name, relative to the proto root directory.
📝 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.
| package google.rpc; | |
| package google.rpc.v1; |
🧰 Tools
🪛 buf (1.47.2)
17-17: Files with package "google.rpc" must be within a directory "google/rpc" relative to root but were in directory "Sources/API/V1/googleapis/google/rpc".
(PACKAGE_DIRECTORY_MATCH)
17-17: Package name "google.rpc" should be suffixed with a correctly formed version, such as "google.rpc.v1".
(PACKAGE_VERSION_SUFFIX)
What this PR does / why we need it:
Applied changes from https://github.com/yorkie-team/yorkie-js-sdk/releases/tag/v0.4.27
Which issue(s) this PR fixes:
Applied
Not Applied
Additional Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
ErrorInfomessage.Improvements
Bug Fixes