Using decorated methods in a subclass of deque to dump its state to a JSON file

Question

I am sub-classing an object in python and I need to overload a bunch of the methods with very similar logic, essentially a call to super() and then some additional operations. I want to avoid explicitly writing out each of those overloaded methods in that way, so I implemented a decorator.

Is there anything wrong with doing the following:

def decorator(function):
    def wrapper(*args, **kwargs):
        # Logic + operations here
        function()
        # Logic and operations here

    return wrapper

class SubClass(SuperClass):
    method1 = decorator(SuperClass.method1)
    method2 = decorator(SuperClass.method2)

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        # Extra logic here

Here is my actual implementation:

def updates_file(function):
    """
    Decorator to call update file method after super call.
    """
    def wrapper(*args, **kargs):
        function(*args, **kargs)
        args[0]._update_file()

    return wrapper

class FileMirroredDeque(deque):

    __delitem__ = updates_file(deque.__delitem__)
    __iadd__    = updates_file(deque.__iadd__)
    __imul__    = updates_file(deque.__imul__)
    __setitem__ = updates_file(deque.__setitem__)
    append      = updates_file(deque.append)
    appendleft  = updates_file(deque.appendleft)
    extend      = updates_file(deque.extend)
    extendleft  = updates_file(deque.extendleft)
    insert      = updates_file(deque.insert)
    pop         = updates_file(deque.pop)
    popleft     = updates_file(deque.popleft)
    remove      = updates_file(deque.remove)
    reverse     = updates_file(deque.reverse)
    rotate      = updates_file(deque.rotate)

    def __init__(self, cache_path, maxlen=None, clean=False, file_indent=None):
        super().__init__((), maxlen)  # Initializes the deque as well TODO: Check the implication of this calling __setitem__

        self.path = cache_path
        self._indent = file_indent
        self._bak_file = None

        if clean:
            self._update_file()  # Overwrite contents of file.
            return  # Don't import data from file.

        with open(self.path, 'a+') as f:  # Creates file if it doesn't exist
            f.seek(0, 0)  # Seek back to beginning of file for json decode.
            contents = f.read()

        try:
            self.extend(json.loads(contents or "[]"))  # initializes internal list with persistent data or empty list.

        except json.decoder.JSONDecodeError:
            new_file = os.path.basename(self.path) \
                       + datetime.datetime.utcnow().strftime("_%Y_%m_%d_%H_%M_%S") + ".bak"

            new_path = os.path.join(os.path.dirname(self.path), new_file)

            logger.warning(f"File: {self.path} was not valid JSON. It will be copied to {new_path} and a new "
                           f"file will be created.")

            shutil.copyfile(self.path, new_path)
            self._bak_file = new_path

    def _update_file(self):
        with open(self.path, 'w') as f:
            json.dump(list(self), f, indent=self._indent)

Answer 1

I don't think that what you're doing is good style. I think you're adding a lot of extra complexity and places for things to go wrong, or become hard to understand, or whatever. I don't think you lose anything by just making very small methods that call super and your special method. I talk about those options at the bottom of the post.

With that out of the way, I think you can accomplish your goal as written and currently designed much more safely by using descriptors and metaclasses. I don't think this is as good as the methods at the bottom of the post, but to review the code as written here is what I would say.

---

What you're trying to do is (semi-automatically) change how certain data elements are retrieved and executed when called. To me, this sounds like a descriptor. Descriptors allow you to override how you set, get, or delete an attribute on an object, like so (example taken from above documentation link):

class RevealAccess(object):
    """A data descriptor that sets and returns values
       normally and prints a message logging their access.
    """

    def __init__(self, initval=None, name='var'):
        self.val = initval
        self.name = name

    def __get__(self, obj, objtype):
        print('Retrieving', self.name)
        return self.val

    def __set__(self, obj, val):
        print('Updating', self.name)
        self.val = val

>>> class MyClass(object):
...     x = RevealAccess(10, 'var "x"')
...     y = 5
...
>>> m = MyClass()
>>> m.x
Retrieving var "x"
10
>>> m.x = 20
Updating var "x"
>>> m.x
Retrieving var "x"
20
>>> m.y
5

Note - if there were a way to safely identify the underlying data structure being modified when any of these operations is called, then that is what you should put the descriptor on (as that's what you actually want). Because I don't know of a safe way to do that, I suggest the approach below.

For yours, what you need is a descriptor that auto-calls super for you. Note - it's important to use super to handle cases of multiple-inheritance; as pointed out in the comments, your current implementation will only ever call the deque method directly, instead of taking the time to deal with __mro__. In the implementation below, we:

1. Use the __get__ method to automatically get the class (objtype) and self (obj) of the instance (not the descriptor). This provides enough information for us to call super().
2. Use getattr to get the attribute in question from the result of super, and lets us ignore the complexities of __mro__.
3. Create a wrapper function (don't forget to use functools.wraps) that calls the wrapped function with the appropriate arguments, and then the extra function you specified.
4. __get__ returns the wrapped function, and now MyClass.<<attr_name>> will be that function

from functools import wraps

class AutoSuperDescriptor(object):
    def __init__(self, attr_name, extra):
        self.attr_name = attr_name
        self.extra = extra

    def __get__(self, obj, objtype):
        wrapped_function = getattr(
            super(objtype, obj), self.attr_name
        )
        extra_bits = self.extra

        @wraps(wrapped_function)
        def wrapper(yourself, *args, **kwargs):
            result = wrapped_function(
                yourself, *args, **kwargs
            )
            extra_bits(yourself)
            return result

        return wrapper

You create your descriptor by providing enough information to find the overwritten attribute, as well as the function we want to add on to the end. For example, you might do this:

class FileMirroredDeque(deque):
    append = AutoSuperDescriptor("append", updates_file)

If that meets your needs, then you're done.

---

But wait, there's more

It's pretty tedious and error-prone to do this by hand for each method you want to apply this to. Wouldn't it be nice if there was a way to just tie into the creation of a class and have it do this for us?

There is, and the answer is metaclasses. Metaclasses are a really complex, and really powerful, tool in the Python toolbox. Anytime you find yourself using them you should think really, really whether that is the best way to do it. I don't think this is one of those times (see start of this answer), but if you wanted to do that, I think we'd have to do something like this:

1. Define a way to identify the elements that should be overridden at class initialization.
2. Define a way to identify what extra behavior needs to be added to them.
3. Add the behavior.

The third one is easy - we implemented it with our descriptor up above.

The first two are a little more complicated, and have a few options. We could choose between adding kwargs to the class initialization:

class FileMirroredDeque(
    deque, 
    metaclass=SuperChargerMetaclass, 
    methods_to_supercharge = [""], 
    method_to_supercharge_with = "method_name"
):

I think this would get clunky and obscure things. Instead, I'm going to propose that we define a special attribute (I used the __<<name>>__ naming convention, which is a no-no, but I thought made it clearer for demonstrative purposes) to list the attributes to override, and then set an attribute on the extra method itself to find it. The final implementation of the class looks like so:

def supercharger(func):
    func.__is_supercharger = True
    return func

class FileMirroredDeque(
    deque, metaclass=SuperChargerMetaClass
):
    __supercharge__ = [
        "__delitem__",
        "__iadd__",
        "__imul__",
        "__setitem__",
        "append",
        "appendleft",
        "extend",
        "extendleft",
        "insert",
        "pop",
        "popleft",
        "remove",
        "reverse",
        "rotate",
    ]


    @supercharger
    def _update_file(self):
        with open(self.path, "w") as f:
            json.dump(list(self), f, indent=self._indent)

    def __init__(
        self,
        cache_path,
        maxlen=None,
        clean=False,
        file_indent=None,
    ):
        super().__init__((), maxlen)

        self.path = cache_path
        self._indent = file_indent
        self._bak_file = None

        if clean:
            self._update_file()  # Overwrite contents of file.
            return  # Don't import data from file.

        with open(
            self.path, "a+"
        ) as f:  # Creates file if it doesn't exist
            f.seek(
                0, 0
            )  # Seek back to beginning of file for json decode.
            contents = f.read()

        try:
            contents = json.loads(contents or "[]")
            self.extend(
                contents
            )  # initializes internal list with persistent data or empty list.

        except json.decoder.JSONDecodeError:
            new_file = (
                os.path.basename(self.path)
                + datetime.datetime.utcnow().strftime(
                    "_%Y_%m_%d_%H_%M_%S"
                )
                + ".bak"
            )

            new_path = os.path.join(
                os.path.dirname(self.path), new_file
            )

            print(
                f"File: {self.path} was not valid JSON. It will be copied to {new_path} and a new "
                f"file will be created."
            )

            shutil.copyfile(self.path, new_path)
            self._bak_file = new_path

But how do we actually get this to happen? Like so!

class SuperChargerMetaClass(type):
    _SUPERCHARGER_ATTRNAME = "__supercharge__"

    @classmethod
    def _get_supercharger(cls, klass_attrs):
        return next(
            method
            for name, method in klass_attrs.items()
            if getattr(method, "__is_supercharger", False)
        )

    def __init__(cls, name, bases, attrs):
        supercharger = cls._get_supercharger(attrs)
        superchargees = attrs.get(
            cls._SUPERCHARGER_ATTRNAME, ()
        )

        for method_to_supercharge in superchargees:

            attrs[
                method_to_supercharge
            ] = AutoSuperDescriptor(
                method_to_supercharge, supercharger
            )

        return super().__init__(name, bases, attrs)

In the __init__ method (__new__ would probably work too, but I ended up getting this to work as __init__ so I left it), we get the "supercharger" method to append to our functions, and our list of "superchargees" that need the behavior added to them. From there, its just a matter of making the relevant attribute look to our descriptor instead of whatever (if anything) was there before.

---

Despite being personally proud of coming up with that, I still feel pretty strongly that its the wrong way to go. As such, I wanted to show how easy this would be. For example, if you inherit from deque:

class FileMirroredDeque(deque):

    # _update_file and __init__ unchanged

    def __delitem__(self, idx):
        super().__delitem__(idx)
        self._update_file()

    # Same for each method

If you don't actually want inheritance, or want to deal with multiple inheritance, then you could just use composition instead, like so:

class FileMirroredDeque:

    def __init__(self, cache_path, maxlen=None, clean=False, file_indent=None):
        # Only addition to __init__
        self.data = deque((), maxlen)

        # Any references to self with the intent of hitting the deque should now reference self.data


    def __delitem__(self, idx):
        self.data.__delitem__(idx)
        self._update_file()

    # Same for each method

We can actually take that a step further then, by creating a generic descriptor that handles file mirroring any arbitrary collection, like so:

class FileMirroredCollection:

    # This is virtually identical to your previous __init__
    def __init__(self, collection, cache_path, clean=False, file_indent=None):
        self.collection = collection
        self.path = cache_path
        self._indent = file_indent
        self._bak_file = None

        if clean:
            self._update_file()  # Overwrite contents of file.
            return  # Don't import data from file.

        with open(self.path, 'a+') as f:  # Creates file if it doesn't exist
            f.seek(0, 0)  # Seek back to beginning of file for json decode.
            contents = f.read()

        try:
            # You'd need to have some intelligent way to extend the arbitrary collection; duck typing is your friend here
            self.collection.extend(json.loads(contents or "[]"))  # initializes internal list with persistent data or empty list.

        except json.decoder.JSONDecodeError:
            new_file = os.path.basename(self.path) \
                       + datetime.datetime.utcnow().strftime("_%Y_%m_%d_%H_%M_%S") + ".bak"

            new_path = os.path.join(os.path.dirname(self.path), new_file)

            logger.warning(f"File: {self.path} was not valid JSON. It will be copied to {new_path} and a new "
                           f"file will be created.")

            shutil.copyfile(self.path, new_path)
            self._bak_file = new_path

    def _update_file(self):
        with open(self.path, 'w') as f:
            json.dump(list(self.collection), f, indent=self._indent)

    def __get__(self, obj, objtype):
        self._update_file()
        return self.collection

    def __set__(self, obj, value):
        self.collection = value
        self._update_file()

    def __delete__(self, obj):
        del self.collection
        self._update_file()


class FileMirroredDeque:

    def __init__(self, cache_path, clean=False, file_indent=None, max_len=None):
        self.data = FileMirroredCollection(deque((), max_len), cache_path, clean, file_indent)

        # other stuff

This still needs a bit more work (e.g. this doesn't handle self.data.append very well), but you can see how it might be useful to separate your concerns.

Lastly, there are a few other miscellaneous improvements that might benefit you, including:

1. Support for more formats - json is nice, but what if I want to serialize it another way?
2. Not writing to file every time; this will be inefficient if you have lots of small changes. Potentially include a flush method, or some way to identify that you need to write any recent changes.
3. As I allude to with the generic descriptor approach, supporting other data structures could be good - you might not want a deque forever, and as-is you're pretty locked down.