Define abstract Mutation.mutate() method [type-hints]

[belkka]

Add default implementation of Mutation.mutate() that raises NotImplementedError. This makes code more clear and also improves work of auto-completion tools (e. g. in IDE). They usually guess if user is overriding a class method and suggest to complete method name/arguments.

Not using python's abc.ABC because of possible metaclass conflicts.

[belkka]

I will be glad to perform more similar refactoring in the project if current change will be considered reasonable.

[belkka]

Sorry for the late reply. I have a lot of thoughts on the topic... (Click on every section to expand)

1. Is it a breaking change for existing apps?

It certainly shouldn't be. Think of

• CLI adding new --flags in a new version (while previous version aborts with "unrecognized option" message).
• REST API adding new endpoint (so 404 becomes 200) or starting to accept a new media type (previous version returns 415 Unsupported Media Type)

After all, it is supposed that existing users do define a .mutate() method.

User code usually must not rely on the fact that library code raises an AssertionError (unless the library is a testing framework ofc). Imagine a library function that raises an AssertionError for argument values other than "yes"/"no":

def foo(x):
    assert x in ("yes", "no"), "My developer did not expect other values"
    ...

In a next version we gonna update this function in a such way, that it can also accept values "YES", "N","on"/"off"/"maybe"/None etc. Someone could argue that updated version does not pass test like

def test_foo():
    with raises(AssertionError):
        foo("maybe")

But I doubt this is widely seen as "breaking backward compatibility". It's worth noting, that it may be not a good example, because TypeError or ValueError are more appropriate for argument validation than AssertionError. Indeed, client code could handle ValueError (if it's a documented exception) and be unprepared otherwise:

def some_user_code(x):
    try:
        root = math.sqrt(x)
    except ValueError:
        return "Permission denied"
    # from this point assume x is non-negative,
    # because we trust that math.sqrt raises ValueError for negative arguments
    ...

But semantics of AssertionError implies that it's never raised in a well-tested code (otherwise RuntimeError is more appropriate), end-user should never try to catch and handle AssertionError. In this particular case, I cannot imagine a working graphql app with incomplete mutation class definition that somehow breaks because of not raising AssertionError anymore.

2. Is current behavior (raise at class definition time) good?

@DarknessRdg while it may be possible to simplify your workaround (by checking cls.method is BaseClass.method), I suggest that current behavior (raising an exception at class definition) is not kept.

Python developers do not usually expect errors raising purely from class definitions — it's counter-intuitive and only possible with metaclasses. Also, current behavior is not in spirit of Open-closed principle:

What we are talking here about is commonly known as abstract method. AFAIK there are two common approaches to define abstract methods in python — (1) NotImplementedError and (2) abc.abstractmethod. In this PR I've implemented the first one. As you have noted, it is generally weaker, since it only shows an error at run time. The python built-in abc module could be a good alternative, but this approach may require that internal graphene metaclasses inherit from abc.ABCMeta (it requires investigation, I didn't try it). Note that abstract classes from abc and, for example, abstract classes in C++ do not actually forbid to define an abstract subclass. What they do is they forbid to create instances of themself & their subclasses unless the subclass defines all abstract methods.

Imagine that someone wanna extend and customize the base Mutation class for their project. E.g.

# just a template, no instances
class FancyBaseMutation(graphene.Mutation):
    fancy_attribute = None

    def cool_helper_method(self, ...):
        do_something(self.fancy_attribute)
        ...


class CreateMessage(FancyBaseMutation):
    fancy_attribute = FancyMessage
    
    @classmethod
    def mutate(cls, ...):
        self.cool_helper_method(...)
        ...

class CreateUser(FancyBaseMutation):
    fancy_attribute = FancyUser

    @classmethod
    def mutate(cls, ...):
        self.cool_helper_method(...)

FancyBaseMutation class definition raises an error. But it's just a "template", so it's not intended to define a .mutate(). What is the easiest solution that comes into head? Just define a dummy mutate method in the base class:

class FancyBaseMutation(graphene.Mutation):
    class_attribute = None
    
    @classmethod
    def mutate(cls, parent, ...):
        raise NotImplementedError("don't forget to override .mutate() method!")

    def cool_helper_method(self, ...):
        ...

Cool, metaclasses are appeased + as a bonus you get IDE autocompletion when you start typing def mu... in subclasses. Are there other workarounds for intermediate subclasses? If no, what's the point of not having a default .mutate() method in original graphene.Mutation?

3. Could abstract base class (raise at object creation time) be better?

An "intermediate" solution that allows earlier (?) error detection while not breaking extensibility is to forbid "abstract class" instantiation (i. e. creating a Mutation instance without .mutate()). This can be achieved either by

• inheriting graphene metaclass(es) from abc.ABCMeta (as mentioned above) and decorating .mutate() as abc.abstractmethod, or
• re-implementing functionality of abc.ABCMeta in graphene classes (if first option does not work for some reason). Note, that add_implemented_flag decorator in your example is kind of reinvented abc.abstractmethod

Nevertheless, I don't feel that this "balanced" solution provides any benefits compared to current PR (raise NotImplementedError).

Imaging user (backend developer) is designing new API and just wanna start with some dummy code, choose names and so on:

class CreatePerson(Mutation):
   ...

class UpdatePerson(Mutation):
   ...
   
class AddFriend(Mutation):
   ...
   
class CreateMessage(Mutation):
   ...

class ForwardMessage(Mutation):
   ...

Then they wanna implement .mutate() methods one-by-one. They may wish to ignore non-implemented mutations (create/forward message) for a while, but test other mutations (create/update person) while work is still in progress. Currently "draft" mutation classes cannot be even defined like above — they have to be commented to avoid AssertionError. With "abstract base class" approach developer can define them, but they must define "dummy" methods in order to run some tests:

# this mutation is ready
class CreatePerson(Mutation):
    @classmethod
    def mutate(cls, parent, *args, **kwargs):
         do_this()
         do_that()  # would be nice to test this before moving to implementing UpdatePerson 
 
 # this is not ready yet
 class UpdatePerson(Mutation):
   @classmethod
    def mutate(cls, parent, *args, **kwargs):
         pass  # todo (adding this stub to test another mutation, otherwise graphene complains)

At this point UpdatePerson.mutate is virtually same as "raise NotImplementedError", because it's not finished, but that can be only detected at run-time. So, what was the whole point of avoiding "method run-time error" in favor of "object creation time error"? It only took us more effort to get the same result.

Summary

While it is possible to keep the current behavior (raise at class definition time) or implement abstract class behavior (raise at object creation time), I believe that most user-friendly (and also KISS) option is to raise an error from .mutate() at run-tme. I could implement the second option though.