Apply review comments by coderabbitai

yorkie-team · Jan 23, 2025 · 6615b47 · 6615b47
1 parent b26b38e
commit 6615b47
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 11 deletions.
diff --git a/Sources/API/V1/googleapis/google/rpc/error_details.proto b/Sources/API/V1/googleapis/google/rpc/error_details.proto
@@ -16,8 +16,6 @@ syntax = "proto3";
 
 package google.rpc;
 
-import "google/protobuf/duration.proto";
-
 option go_package = "google.golang.org/genproto/googleapis/rpc/errdetails;errdetails";
 option java_multiple_files = true;
 option java_outer_classname = "ErrorDetailsProto";
@@ -50,4 +48,4 @@ message ErrorInfo {
   // `{"instanceLimitPerRequest": "100"}`, if the client exceeds the number of
   // instances that can be created in a single (batch) request.
   map<string, string> metadata = 3;
-}
+}
diff --git a/Sources/Core/Client.swift b/Sources/Core/Client.swift
@@ -206,7 +206,7 @@ public class Client {
             let activateResponse = await self.yorkieService.activateClient(request: activateRequest, headers: self.authHeader.makeHeader(nil))
 
             guard activateResponse.error == nil, let message = activateResponse.message else {
-                throw activateResponse.error ?? YorkieError.rpcError(message: activateResponse.error.debugDescription)
+                throw self.handleErrorResponse(activateResponse.error, defaultMessage: "Unknown activate error")
             }
 
             self.id = message.clientID
@@ -236,7 +236,7 @@ public class Client {
             let deactivateResponse = await self.yorkieService.deactivateClient(request: deactivateRequest)
 
             guard deactivateResponse.error == nil else {
-                throw deactivateResponse.error ?? YorkieError.rpcError(message: deactivateResponse.error.debugDescription)
+                throw self.handleErrorResponse(deactivateResponse.error, defaultMessage: "Unknown deactivate error")
             }
 
             try self.deactivateInternal()
@@ -285,7 +285,7 @@ public class Client {
             let attachResponse = await self.yorkieService.attachDocument(request: attachRequest, headers: self.authHeader.makeHeader(docKey))
 
             guard attachResponse.error == nil, let message = attachResponse.message else {
-                throw attachResponse.error ?? YorkieError.rpcError(message: attachResponse.error.debugDescription)
+                throw self.handleErrorResponse(attachResponse.error, defaultMessage: "Unknown attach error")
             }
 
             let pack = try Converter.fromChangePack(message.changePack)
@@ -351,7 +351,7 @@ public class Client {
             let detachDocumentResponse = await self.yorkieService.detachDocument(request: detachDocumentRequest, headers: self.authHeader.makeHeader(doc.getKey()))
 
             guard detachDocumentResponse.error == nil, let message = detachDocumentResponse.message else {
-                throw detachDocumentResponse.error ?? YorkieError.rpcError(message: detachDocumentResponse.error.debugDescription)
+                throw self.handleErrorResponse(detachDocumentResponse.error, defaultMessage: "Unknown detach error")
             }
 
             let pack = try Converter.fromChangePack(message.changePack)
@@ -400,7 +400,7 @@ public class Client {
             let removeDocumentResponse = await self.yorkieService.removeDocument(request: removeDocumentRequest, headers: self.authHeader.makeHeader(doc.getKey()))
 
             guard removeDocumentResponse.error == nil, let message = removeDocumentResponse.message else {
-                throw removeDocumentResponse.error ?? YorkieError.rpcError(message: removeDocumentResponse.error.debugDescription)
+                throw self.handleErrorResponse(removeDocumentResponse.error, defaultMessage: "Unknown remove error")
             }
 
             let pack = try Converter.fromChangePack(message.changePack)
@@ -790,7 +790,7 @@ public class Client {
             let pushpullResponse = await self.yorkieService.pushPullChanges(request: pushPullRequest, headers: self.authHeader.makeHeader(docKey))
 
             guard pushpullResponse.error == nil, let message = pushpullResponse.message else {
-                throw pushpullResponse.error ?? YorkieError.rpcError(message: pushpullResponse.error.debugDescription)
+                throw self.handleErrorResponse(pushpullResponse.error, defaultMessage: "Unknown pushpull error")
             }
 
             let responsePack = try Converter.fromChangePack(message.changePack)
@@ -852,11 +852,21 @@ public class Client {
         {
             do {
                 try self.deactivateInternal()
-            } catch {}
+            } catch {
+                Logger.error("Failed deactivateInternal for client (\(self.key)) with error: \(error)")
+            }
         }
 
         return false
     }
+
+    private func handleErrorResponse(_ error: Error?, defaultMessage: String) -> Error {
+        if let error = error {
+            return error
+        } else {
+            return YorkieError.rpcError(message: defaultMessage)
+        }
+    }
 }
 
 public extension Client {

diff --git a/Tests/Integration/ClientIntegrationTests.swift b/Tests/Integration/ClientIntegrationTests.swift
@@ -886,7 +886,14 @@ final class ClientIntegrationTests: XCTestCase {
         c1.setMockError(for: YorkieServiceClient.Metadata.Methods.pushPullChanges,
                         error: connectError(from: .failedPrecondition))
 
-        try await Task.sleep(nanoseconds: 1_000_000_000)
+        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))
     }

diff --git a/Tests/Integration/PresenceTests.swift b/Tests/Integration/PresenceTests.swift
@@ -582,6 +582,7 @@ final class PresenceSubscribeTests: XCTestCase {
 
         let resultPresence1 = await doc1.getPresences()
         XCTAssertEqual(resultPresence1.count, 1)
+        XCTAssertEqual(resultPresence1.first?.clientID, c1ID)
         XCTAssertEqual(resultPresence1.first?.presence as? [String: String], ["name": "a"])
 
         // NOTE(hiddenviewer): js-sdk return [ActorIDs.initial: [:]], but ios-sdk return empty