graphql-python / graphql-core-legacy

GraphQL base implementation for Python (legacy version – see graphql-core for the current one)
MIT License
374 stars 184 forks source link

Order of Middlewares #155

Open dfee opened 6 years ago

dfee commented 6 years ago

I've discovered that this line inverts the middleware order: https://github.com/graphql-python/graphql-core/blob/6df8a6312b579a6a1454bcf29a566ce5d0fa9849/graphql/execution/middleware.py#L51

This was somewhat confusing, as I expected the first installed middleware to be the entrypoint. I.e.

schema.execute(
    statement,
    middleware=[mw1, mw2],
)

actually produces the call stack mw2 -> mw1 -> resolve_func, whereas i'd expected mw1 -> mw2 -> resolve_func.

I'm not clear if this is a bug, as this functionality isn't tested anywhere, or if it would just go in documentation somewhere.

dan98765 commented 6 years ago

I confirm that the middleware's resolve() methods get called in reverse order from how they're listed when passed into schema.execute. Let's say your middleware is:

class CharPrintingMiddleware(object):
        def __init__(self, char):
            self.char = char

        def resolve(self, next, *args, **kwargs):
            print(f'resolve() called for middleware {self.char}')
            return next(*args, **kwargs).then(
                lambda x: print(f'then() for {self.char}')
            )

and you call:

schema.execute(
    statement, 
    middleware=[
        CharAppendingMiddleware('a'), 
        CharAppendingMiddleware('b'), 
        CharAppendingMiddleware('c')
    ]
)

Then the printed output is:

resolve() called for middleware c
resolve() called for middleware b
resolve() called for middleware a
then() for a
then() for b
then() for c

This makes me wonder if perhaps it was intentional to have middleware called in reverse order, and the intention was that then() set up by the middleware would wind up being called in order. @syrusakbary you git blame to those lines in middleware.py from back in 2016, do you recall if this is intentional or a minor, fixable bug?