From ff45ab314ca0c77c7435d6b835de3f381ca51d6b Mon Sep 17 00:00:00 2001 From: Viktor Dick Date: Tue, 11 Jun 2024 09:03:33 +0200 Subject: [PATCH] T257301: Add conflicts warning in layer-update (#127) --- CHANGELOG | 3 ++ perfact/zodbsync/commands/layer_update.py | 7 +++ perfact/zodbsync/main.py | 2 +- perfact/zodbsync/tests/test_sync.py | 52 +++++++++++++++++++++-- perfact/zodbsync/zodbsync.py | 23 ++++++---- 5 files changed, 74 insertions(+), 13 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 05aa1f2..7ae860b 100644 --- a/CHANGELOG +++ b/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 diff --git a/perfact/zodbsync/commands/layer_update.py b/perfact/zodbsync/commands/layer_update.py index eeb1d96..5e0d61b 100644 --- a/perfact/zodbsync/commands/layer_update.py +++ b/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) diff --git a/perfact/zodbsync/main.py b/perfact/zodbsync/main.py index 23afe16..344a676 100644 --- a/perfact/zodbsync/main.py +++ b/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): diff --git a/perfact/zodbsync/tests/test_sync.py b/perfact/zodbsync/tests/test_sync.py index 177a60b..47a00f9 100644 --- a/perfact/zodbsync/tests/test_sync.py +++ b/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 diff --git a/perfact/zodbsync/zodbsync.py b/perfact/zodbsync/zodbsync.py index 6babedb..ffeebce 100644 --- a/perfact/zodbsync/zodbsync.py +++ b/perfact/zodbsync/zodbsync.py @@ -634,7 +634,8 @@ 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 @@ -642,13 +643,17 @@ def record(self, paths, recurse=True, skip_errors=False): 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