Merge pull request #268 from OpenMined/skip-invalid-syncfile-but-keep…

…-deletes more nuanced logic
OpenMined · Oct 24, 2024 · 2bfa430 · 2bfa430
2 parents 5d1cc50 + 1cdfcca
commit 2bfa430
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 11 deletions.
diff --git a/syftbox/client/plugins/sync/consumer.py b/syftbox/client/plugins/sync/consumer.py
@@ -286,25 +286,41 @@ def get_decisions(self, item: SyncQueueItem) -> SyncDecisionTuple:
 
         return SyncDecisionTuple.from_states(current_local_syncstate, previous_local_syncstate, current_server_state)
 
-    def process_decision(self, path: Path, decision: SyncDecisionTuple):
-        decision.remote_decision.execute(self.client)
+    def invalid_remote_permission_change(self, decision: SyncDecision, local_abs_path: Path):
+        remote_op = decision.operation
+        invalid = (
+            remote_op in [SyncDecisionType.CREATE, SyncDecisionType.MODIFY]
+            and SyftPermission.is_permission_file(local_abs_path)
+            and not SyftPermission.is_valid(local_abs_path)
+        )
+        return invalid
+
+    def process_decision(self, item: SyncQueueItem, decision: SyncDecisionTuple):
+        abs_path = item.data.local_abs_path
+
         decision.local_decision.execute(self.client)
-        logger.debug(f"Saving state for {path}, {decision.result_local_state}")
 
-        self.previous_state.insert(path=path, state=decision.result_local_state)
+        # we want to make sure that
+        # 1) We never upload invalid syftperm files
+        # 2) We allow for modifications/deletions of syftperm files, even if the local version
+        # is corrupted
 
-    def process_filechange(self, item: SyncQueueItem) -> None:
-        if SyftPermission.is_permission_file(item.data.local_abs_path) and not SyftPermission.is_valid(
-            item.data.local_abs_path
-        ):
+        skip_remote = self.invalid_remote_permission_change(decision.remote_decision, abs_path)
+        if skip_remote:
             logger.error(f"Trying to sync invalid permfile {item.data.path}")
-            return
+        else:
+            decision.remote_decision.execute(self.client)
+
+        logger.debug(f"Saving state for {abs_path}, {decision.result_local_state}")
 
+        self.previous_state.insert(path=item.data.path, state=decision.result_local_state)
+
+    def process_filechange(self, item: SyncQueueItem) -> None:
         decisions = self.get_decisions(item)
         logger.debug(
             f"Processing {item.data.path} with decisions {decisions.local_decision.operation}, {decisions.remote_decision.operation}"
         )
-        self.process_decision(item.data.path, decisions)
+        self.process_decision(item, decisions)
 
     def get_current_local_syncstate(self, path: Path) -> Optional[FileMetadata]:
         abs_path = self.client.sync_folder / path

diff --git a/syftbox/client/plugins/sync/sync.py b/syftbox/client/plugins/sync/sync.py
@@ -28,7 +28,7 @@ class FileChangeInfo(SyftBaseModel, frozen=True):
     file_size: int = 1
 
     @property
-    def local_abs_path(self):
+    def local_abs_path(self) -> Path:
         return self.local_sync_folder / self.path
 
     def get_priority(self) -> int: