T257301: Add conflicts warning in layer-update (#127)

CHANGELOG

@@ -1,3 +1,6 @@
+23.1.2.dev0
+  * Add warning for custom layer paths shadowing changes in layer-update
+
 23.1.1
   * Add method that exposes reading the db_modtime
 

perfact/zodbsync/commands/layer_update.py

@@ -76,5 +76,12 @@ def run(self):
         self._playback_paths(sorted(paths))
 
         if not self.args.dry_run:
+            self.sync.record(sorted(paths), recurse=False, skip_errors=True,
+                             ignore_removed=True)
+            for path in paths:
+                if self.sync.fs_pathinfo(path)['layeridx'] == 0:
+                    self.logger.warning(
+                        'Conflict with object in custom layer: ' + path
+                    )
             for oldfname, newfname in fnames:
                 shutil.copyfile(newfname, oldfname)

perfact/zodbsync/main.py

@@ -100,7 +100,7 @@ def parse(self, *argv):
             logger = logging.getLogger('ZODBSync')
             logger.setLevel(logging.INFO)
             logger.addHandler(logging.StreamHandler())
-            logger.propagate = False
+            logger.propagate = True
 
         self.logger = logger
         if getattr(args.command, 'use_config', True):

perfact/zodbsync/tests/test_sync.py

@@ -207,8 +207,11 @@ def test_record(self):
         assert os.path.isfile(
             self.repo.path + '/__root__/acl_users/__meta__'
         )
-        # Recording a non-existent object only logs and does not fail
-        self.run('record', '/nonexist')
+        # Recording a non-existent object fails
+        with pytest.raises(AttributeError):
+            self.run('record', '/nonexist')
+        # ... unless --skip-errors is given
+        self.run('record', '/nonexist', '--skip-errors')
         # Recording with --lasttxn will create the file
         self.run('record', '--lasttxn')
         assert os.path.isfile(os.path.join(self.repo.path, '__last_txn__'))
@@ -1997,7 +2000,7 @@ def test_layer_remove_subfolder(self):
             assert not os.path.exists(os.path.join(root, 'Test/Sub/__meta__'))
             assert os.path.exists(os.path.join(root, 'Test/Sub/__deleted__'))
 
-    def test_layer_update(self):
+    def test_layer_update(self, caplog):
         """
         Set up a layer, initialize its checksum file and register it. Change
         something in the layer, recompute the checksum file and use
@@ -2022,6 +2025,7 @@ def test_layer_update(self):
                 }))
             self.run('layer-hash', layer)
             self.run('layer-update', ident)
+            assert 'Conflict with object' not in caplog.text
             assert self.app.Test.title == 'Changed'
 
     def test_keep_acl(self):
@@ -2106,3 +2110,45 @@ def test_keep_acl_norecurse(self):
             '/some_module/acl_users',
         )
         assert 'acl_users' not in self.app.some_module.objectIds()
+
+    def test_layer_update_warn(self, caplog):
+        """
+        Set up a layer and initialize it. Change an object that is provided by
+        this layer and record the change into the custom layer. Update the base
+        layer such that this object would change and make sure that we are
+        warned that the change is ignored due to a collision.
+        Also check that deletion of an object in the base layer that is not
+        masked in the custom layer, but that has a masked subobject, also leads
+        to a warning.
+        """
+        with self.runner.sync.tm:
+            self.app.manage_addFolder(id='Test')
+            self.app.manage_addFolder(id='ToDelete')
+            self.app.ToDelete.manage_addFolder(id='Sub')
+        with self.addlayer() as layer:
+            self.run('record', '/')
+            ident = self.runner.sync.layers[-1]['ident']
+            src = os.path.join(self.repo.path, '__root__')
+            tgt = os.path.join(layer, '__root__')
+            os.rmdir(tgt)
+            os.rename(src, tgt)
+            os.mkdir(src)
+            self.run('layer-hash', layer)
+            self.run('layer-init')
+            with self.runner.sync.tm:
+                self.app.Test._setProperty('nav_hidden', True, 'boolean')
+                self.app.ToDelete.Sub._setProperty('nav_hidden', True,
+                                                   'boolean')
+            self.run('record', '/')
+            with open(os.path.join(tgt, 'Test/__meta__'), 'w') as f:
+                f.write(zodbsync.mod_format({
+                    'title': 'Changed',
+                    'type': 'Folder'
+                }))
+            shutil.rmtree(os.path.join(tgt, 'ToDelete'))
+            self.run('layer-hash', layer)
+            self.run('layer-update', ident)
+            expect = 'Conflict with object in custom layer: '
+            assert expect + '/Test' in caplog.text
+            assert 'AttributeError' not in caplog.text
+            assert expect + '/ToDelete/Sub' in caplog.text

perfact/zodbsync/zodbsync.py

@@ -634,21 +634,26 @@ def fs_parse(self, fspath, data=None):
 
         return result
 
-    def record(self, paths, recurse=True, skip_errors=False):
+    def record(self, paths, recurse=True, skip_errors=False,
+               ignore_removed=False):
         '''Record Zope objects from the given paths into the local
         filesystem.'''
         # If /a/b as well as /a are to be recorded recursively, drop /a/b
         if recurse:
             remove_redundant_paths(paths)
         for path in paths:
             obj = self.app
-            try:
-                # traverse into the object of interest
-                for part in path.split('/'):
-                    if part:
-                        obj = getattr(obj, part)
-            except AttributeError:
-                self.logger.exception('Unable to record path ' + path)
+            # traverse into the object of interest
+            for part in path.split('/'):
+                if not part:
+                    continue
+                if part not in obj.objectIds():
+                    # Depending on skip_errors, this yields an error or a
+                    # warning later
+                    obj = None
+                    break
+                obj = getattr(obj, part)
+            if obj is None and ignore_removed:
                 continue
             self.record_obj(obj, path, recurse=recurse,
                             skip_errors=skip_errors)
@@ -664,7 +669,7 @@ def record_obj(self, obj, path, recurse=True, skip_errors=False):
             )
         except Exception:
             severity = 'Skipping' if skip_errors else 'ERROR'
-            msg = '%s %s' % (severity, path)
+            msg = '{}: Unable to record path {}'.format(severity, path)
             if skip_errors:
                 self.logger.warning(msg)
                 return