Merge pull request #45 from lutraconsulting/fix-sync-issue

Try to fix sync issue in python client (#119)

Author: varmar05

mergin/client.py

@@ -271,7 +271,6 @@ def get_pull_changes(self, server_files):
         if not self.geodiff:
             return changes
 
-        size_limit = int(os.environ.get('DIFFS_LIMIT_SIZE', 1024 * 1024)) # with smaller values than limit download full file instead of diffs
         not_updated = []
         for file in changes['updated']:
             # for small geodiff files it does not make sense to download diff and then apply it (slow)
@@ -294,8 +293,7 @@ def get_pull_changes(self, server_files):
                     break  # we found force update in history, does not make sense to download diffs
 
             if is_updated:
-                if diffs and file['size'] > size_limit and diffs_size < file['size']/2:
-                    file['diffs'] = diffs
+                file['diffs'] = diffs
             else:
                 not_updated.append(file)
 
@@ -455,9 +453,23 @@ def apply_pull_changes(self, changes, temp_dir):
                             if os.path.exists(f'{dest}-shm'):
                                 os.remove(f'{dest}-shm')
                     else:
-                        # just use server version of file to update both project file and its basefile
-                        shutil.copy(src, dest)
-                        shutil.copy(src, basefile)
+                        # The local file is not modified -> no rebase needed.
+                        # We just apply the diff between our copy and server to both the local copy and its basefile
+                        try:
+                            server_diff = self.fpath(f'{path}-server_diff', temp_dir)  # diff between server file and local basefile
+                            # TODO: it could happen that basefile does not exist.
+                            # It was either never created (e.g. when pushing without geodiff)
+                            # or it was deleted by mistake(?) by the user. We should detect that
+                            # when starting pull and download it as well
+                            self.geodiff.create_changeset(basefile, src, server_diff)
+                            self.geodiff.apply_changeset(dest, server_diff)
+                            self.geodiff.apply_changeset(basefile, server_diff)
+                        except (pygeodiff.GeoDiffLibError, pygeodiff.GeoDiffLibConflictError):
+                            # something bad happened and we have failed to patch our local files - this should not happen if there
+                            # wasn't a schema change or something similar that geodiff can't handle.
+                            # FIXME: this is a last resort and may corrupt data! (we should warn user)
+                            shutil.copy(src, dest)
+                            shutil.copy(src, basefile)
                 else:
                     # backup if needed
                     if path in modified and item['checksum'] != local_files_map[path]['checksum']:
@@ -1011,6 +1023,11 @@ def pull_project(self, directory, parallel=True):
         if local_version == server_info["version"]:
             return  # Project is up to date
 
+        # we either download a versioned file using diffs (strongly preferred),
+        # but if we don't have history with diffs (e.g. uploaded without diffs)
+        # then we just download the whole file
+        _pulling_file_with_diffs = lambda f: 'diffs' in f and len(f['diffs']) != 0
+
         temp_dir = mp.fpath_meta(f'fetch_{local_version}-{server_info["version"]}')
         os.makedirs(temp_dir, exist_ok=True)
         pull_changes = mp.get_pull_changes(server_info["files"])
@@ -1020,7 +1037,7 @@ def pull_project(self, directory, parallel=True):
             fetch_files.append(f)
         # extend fetch files download list with various version of diff files (if needed)
         for f in pull_changes["updated"]:
-            if 'diffs' in f:
+            if _pulling_file_with_diffs(f):
                 for diff in f['diffs']:
                     diff_file = copy.deepcopy(f)
                     for k, v in f['history'].items():
@@ -1039,7 +1056,7 @@ def pull_project(self, directory, parallel=True):
             with concurrent.futures.ThreadPoolExecutor() as executor:
                 futures_map = {}
                 for file in fetch_files:
-                    diff_only = 'diffs' in file
+                    diff_only = _pulling_file_with_diffs(f)
                     future = executor.submit(self._download_file, project_path, file, temp_dir, parallel, diff_only)
                     futures_map[future] = file
 
@@ -1052,13 +1069,13 @@ def pull_project(self, directory, parallel=True):
         else:
             for file in fetch_files:
                 # TODO check it does not fail, do some retry on ClientError
-                diff_only = 'diffs' in file
+                diff_only = _pulling_file_with_diffs(f)
                 self._download_file(project_path, file, temp_dir, parallel, diff_only)
 
         # make sure we can update geodiff reference files (aka. basefiles) with diffs or
         # download their full versions so we have them up-to-date for applying changes
         for file in pull_changes['updated']:
-            if 'diffs' not in file:
+            if not _pulling_file_with_diffs(f):
                 continue
             file['version'] = server_info['version']
             basefile = mp.fpath_meta(file['path'])

mergin/test/test_client.py

@@ -209,18 +209,17 @@ def test_ignore_files(mc):
 
 # (diffs size limit, push geodiff enabled, pull geodiff enabled)
 diff_test_scenarios = [
-    (1024, True, True),
-    (1024*1024, True, True),
-    (1024, True, False),
-    (1024, False, True),
-    (1024, False, False),
+    (True, True),
+    (True, False),
+    (False, True),
+    (False, False),
 ]
 
 
-@pytest.mark.parametrize("diffs_limit, push_geodiff_enabled, pull_geodiff_enabled", diff_test_scenarios)
-def test_sync_diff(mc, diffs_limit, push_geodiff_enabled, pull_geodiff_enabled):
+@pytest.mark.parametrize("push_geodiff_enabled, pull_geodiff_enabled", diff_test_scenarios)
+def test_sync_diff(mc, push_geodiff_enabled, pull_geodiff_enabled):
 
-    test_project = f'test_sync_diff_{diffs_limit}_{int(push_geodiff_enabled)}_{int(pull_geodiff_enabled)}'
+    test_project = f'test_sync_diff_push{int(push_geodiff_enabled)}_pull{int(pull_geodiff_enabled)}'
     project = API_USER + '/' + test_project
     project_dir = os.path.join(TMP_DIR, test_project)  # primary project dir for updates
     project_dir_2 = os.path.join(TMP_DIR, test_project + '_2')  # concurrent project dir with no changes
@@ -277,7 +276,6 @@ def test_sync_diff(mc, diffs_limit, push_geodiff_enabled, pull_geodiff_enabled):
         assert not os.path.exists(mp.fpath_meta('renamed.gpkg'))
 
     # pull project in different directory
-    os.environ['DIFFS_LIMIT_SIZE'] = str(diffs_limit)
     toggle_geodiff(pull_geodiff_enabled)
     mp2 = MerginProject(project_dir_2)
     mc.pull_project(project_dir_2)