Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

T258401: Stop deleting important objects found in commits #128

Merged
merged 7 commits into from
Jun 7, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions perfact/zodbsync/tests/test_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -2023,3 +2023,43 @@ def test_layer_update(self):
self.run('layer-hash', layer)
self.run('layer-update', ident)
assert self.app.Test.title == 'Changed'

def test_keep_acl(self):
'''
Make sure deletions on top level acl_users are NOT synced into
Data.fs

User folders living somewhere else in the application may be
deleted though.
'''
acl_path = os.path.join(
self.repo.path,
'__root__',
'acl_users',
)
shutil.rmtree(acl_path)
self.run('playback', '/')

# this playback will fail horribly if acl_users is gone!
self.run('playback', '/')

# make sure acl_users in toplevel is still present
assert 'acl_users' in self.app.objectIds()

# now create dummy module with its own acl_users folder
self.app.manage_addFolder(id='some_module')
self.app.some_module.manage_addUserFolder()

self.run('record', '/')

assert 'acl_users' in self.app.some_module.objectIds()

module_acl = os.path.join(
self.repo.path,
'__root__',
'some_module',
'acl_users',
)
shutil.rmtree(module_acl)
self.run('playback', '/')
assert 'acl_users' not in self.app.some_module.objectIds()
6 changes: 5 additions & 1 deletion perfact/zodbsync/zodbsync.py
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,11 @@ def _playback_path(self, pathinfo):
srv_contents = obj_contents(obj) if obj else []

# Find IDs in Data.fs object not present in file system
del_ids = [a for a in srv_contents if a not in contents]
del_ids = [
a for a in srv_contents
if a not in contents and
(path != '/acl_users' and a != 'acl_users')
viktordick marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't this prevent the deletion of /some_module/acl_users in recursive mode? I'm a bit confused...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it does prevent it, but the test does not notice because of some transaction shenanigans. I stepped with the debugger into the playback and noticed that both fs_contents and srv_contents were empty, so nothing was deleted. Not sure why the first check for the presence of acl_users in the tests does not fail though...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To explain: If we simply manipulate something with directly accessing self.app, this automatically opens a transaction. When the next self.run(...) is executed, this usually also starts a transaction, aborting the already open transaction. Therefore the creation of /some_module and /some_module/acl_users was never commited, but visible in the first check (since it was still the same transaction), but no longer visible once the playback started.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess playback explicitly starts a new transaction (to be able to roll it back later) while record does not (because it does not expect to change anything inside the Data.FS).

]
if del_ids:
self.logger.warning('Deleting objects ' + repr(del_ids))
obj.manage_delObjects(ids=del_ids)
Expand Down