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

Error in post-attach does not undo the operation #208

Closed
kayjan opened this issue Nov 15, 2022 · 5 comments
Closed

Error in post-attach does not undo the operation #208

kayjan opened this issue Nov 15, 2022 · 5 comments
Assignees

Comments

@kayjan
Copy link

kayjan commented Nov 15, 2022

Taking the read-only example and implementing checks on post_attach instead of pre_attach, it does not undo the operation that is supposed to be atomic.

Sample code to replicate the issue below,

from anytree import NodeMixin


class SampleNode(NodeMixin):
    def __init__(self, foo, parent=None):
        super(SampleNode, self).__init__()
        self.foo = foo
        self.readonly = False
        self.parent = parent
    def _post_attach(self, parent):
        if self.root.readonly:
            raise ValueError()
    def _post_detach(self, parent):
        if self.root.readonly:
            raise ValueError()

a = SampleNode("a")
a0 = SampleNode("a0", parent=a)
a1 = SampleNode("a1", parent=a)
a1a = SampleNode("a1a", parent=a1)
a2 = SampleNode("a2", parent=a)

assert a1.parent == a

a.readonly = True
with pytest.raises(ValueError):
    a1.parent = a2

a1.parent == a2 # returns True, it should undo the operation
@c0fec0de c0fec0de self-assigned this Oct 10, 2023
c0fec0de added a commit that referenced this issue Oct 10, 2023
@c0fec0de
Copy link
Owner

Only the _pre_* hookups prevent the tree manipulation (see Important note here https://anytree.readthedocs.io/en/latest/intro.html#custom-separator)

Will add another note, at the example.

@kayjan
Copy link
Author

kayjan commented Oct 11, 2023

This is not an issue on pre- vs post- hookups, the example was just to simulate an error.

The issue is that any error should rollback to the previous state and undo the operation, but that is not the case.

@c0fec0de c0fec0de reopened this Oct 11, 2023
@c0fec0de
Copy link
Owner

Got your point.

The rollback may be complicated. It is just another operation on tree, which cause another pre/post-hookup-processing. Which might cause errors as well. And how to handle them.

My decision was to keep-it-simple. pre-hookup errors prevent the tree manipulation. post-hookups not. This is what is also clearly stated in the documentation.

What is your exact use case for handling the post-hookup errors?

@kayjan
Copy link
Author

kayjan commented Oct 12, 2023

Yeah understand that it is complicated to implement. I was thinking of using a try-except clause where the usual flow is within the try clause and rollback is implemented in the except clause, instead of implementing another pre/post-hookup-processing.

A possible use case would be to use pre-attach and post-attach to check the tree state. If there are errors in pre- or post-attach checks, then a rollback should happen such that the tree does not exist in an illegal state.

@c0fec0de
Copy link
Owner

I am aware of the try-except approach. Nevertheless, it gets quite complicated if the rollback fails too. Would like to skip for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants