Better HTTP Client MiddlewarePipeline

[jobala]

The current implementation of the pipeline is such that middlewares have to take a dependency on BaseMiddleware this makes it easy to break user defined middlewares.

I am proposing a better middleware pipeline implementation that is simpler and avoids this inheritance relationship.

   class MiddlewarePipeline(HTTPAdapter):
       def __init__(self, middlewares):
           super().__init__()
           self.poolmanager = PoolManager(ssl_version=ssl.PROTOCOL_TLSv1_2)
           self.middleware_index = 0
           self.middlewares = middlewares

      def send(self, req, **kwargs):
           return self.next(req, **kwargs)

       def next(self, req, **kwargs):
           if self.middleware_index < len(self.middlewares):
               # Get middleware from list of middlewares
               middleware = self.middlewares[self.middleware_index]
               
               # Increment middleware_index so that we get the next middleware, the next time **next** is called
               self.middleware_index += 1

               # Intercept the request object.
               return middleware.intercept(self, req, **kwargs)
           return super().send(req, **kwargs)

All a middleware has to do is implement the intercept method that takes instance of pipeline, request object and other **kwargs.

   class SampleMiddleware:
       def intercept(self, pipeline, request, **kwargs):
           request.headers.update({'fun': 'header'})
           return pipeline.next(request, **kwargs)

[samwelkanda]

What is the expected input for middlewares argument when initializing the pipeline?

[jobala]

It will be a list of these objects. They will be called in the order in which they are inserted in the list. A middleware is enabled when it's in the list disabled when it's removed from the list.

[samwelkanda]

Yeah thought so. I think this format is more pragmatic. Allows for creating new middleware from scratch without unnecessary inheritance, but without limiting any kind of reuse.

[MIchaelMainer]

The tradeoff is no inheritance but a requirement that the customer implement intercept.

So, if a customer wants to create their own pipeline, they'd do this:

# Read documentation that provides requirements for making their custom middleware:
class SampleMiddleware:
    def intercept(self, pipeline, request, **kwargs):
        # Custom middleware actions happen here.
        return pipeline.next(request, **kwargs)

# Creates sample middleware
sample_middleware = SampleMiddleware()

# Creates authorization middleware
auth_middleware = AuthorizationHandler()

pipeline = MiddlewarePipeline([auth_middleware, sample_middleware])

It appears now that instead of handing off a request to the pipeline, we have to handoff the request to the first handler. How does the client get set with the pipeline and how is the request handed off to the pipeline?

[jobala]

How does the client get set with the pipeline

We hook up the pipeline as a HTTPAdapter.

self.mount('https://', pipeline)

How is the request handed off to the pipeline

The MiddlewarePipeline extends HTTPAdapter and overrides its send method to take the request object through the pipeline. See in the above pipeline, the send method calls next.

[MIchaelMainer]

Got it. So when we create a new GraphSession object, we pass in the middleware not as an array but as an HttpAdapter and set it with mount. The ability to mount different adapters may come in handy when we do support calls not sent to graph.microsoft.com.

[MIchaelMainer]

We'll open a PR on with this information in this part of the design repo:

https://github.com/microsoftgraph/msgraph-sdk-design/tree/master/middleware

[ddyett]

i'm not sure i get the advantage here. BaseMiddleware meant you had to implement a send method, now you implement an intercept method instead which couples the pipeline to the middleware.

pipeline next and pipeline send do the same thing so why both?

Are you also managing being able to manage the response as well? What happens with managing that index when a middleware looks at the response and reissues the request?

[jobala]

@MIchaelMainer oh I see we could do that by

class SampleMiddleware:
    def __init__(self):
        self.retry_count = 0

    def intercept(self, pipeline, request, **kwargs) -> Response:
        request.headers.update({'fun': 'header'})
        resp = pipeline.make_request(request, **kwargs)

        while resp.status_code == 200 and self.retry_count < 2:
            request.headers.update({'retry': 'header'})
            resp = pipeline.next(request, **kwargs)
            self.retry_count += 1

        return resp

[ddyett]

i think where i'm getting most caught off is the notion that there's no inheritance relationship which to means there's no contract and no check that a middleware is valid. I am expecting that SampleMiddleware needs to be inheriting from Middleware which describes intercept.

pipeline next and pipeline send do the same thing so why both?

In the proposed implementation pipeline send calls pipeline next. This makes it possible for the pipeline to move the request object through middlewares.

I think the answer to my question is that HttpAdapter means you must have a send method. Ideally all a person who implements intercept would see is next but I get it.

Are you also managing being able to manage the response as well? What happens with managing that index when a middleware looks at the response and reissues the request?

Yes. It will be possible to read responses and react to them. The intercept method returns the HTTP response object.

My concern here was that self.middleware_index is never decrementing.

[jobala]

i think where i'm getting most caught off is the notion that there's no inheritance relationship which to means there's no contract and no check that a middleware is valid.

Python being a dynamic language means that the contract is implied, we could also expose an abstract class that devs will have to implement to make the contract more explicit.

My concern here was that self.middleware_index is never decrementing

We can pass the middleware index to the middlewares so that each middleware knows its position in the pipeline.

[samwelkanda]

Proposed implementation that uses inheritance. Inspired by this library

class MiddlewarePipeline(HTTPAdapter):
    """An HTTPAdapter onto which :class:`BaseMiddleware <BaseMiddleware>`
    can be registered. Middleware methods are called in the order of
    registration. 
    """
    def __init__(self, middlewares=None, *args, **kwargs):
        self.middlewares = middlewares or []
        super().__init__(*args, **kwargs)

    def register(self, middleware):
        """Add a middleware to the middleware stack."""
        self.middlewares.append(middleware)

    def init_poolmanager(self, connections, maxsize, block=False):
        """Keyword arguments to be passed to `PoolManager`.
        Middlewares are called in reverse order, so if multiple middlewares
        define conflicting arguments, the higher-priority middleware will take
        precedence. 
        """
        kwargs = {}
        for middleware in self.middlewares[::-1]:
            value = middleware.before_init_poolmanager(
                connections, maxsize, block
            )
            kwargs.update(value or {})
        print(kwargs)
        self._pool_connections = connections
        self._pool_maxsize = maxsize
        self._pool_block = block

        self.poolmanager = PoolManager(
            num_pools=connections, maxsize=maxsize, block=block,
            **kwargs
        )

    def send(self, request, *args, **kwargs):
        """Send the request. If any middleware in the stack returns a `Response`
        or `HTTPResponse` value from its `before_send` method, short-circuit;
        else delegate to `HTTPAdapter::send`.
        """
        for middleware in self.middlewares:
            value = middleware.before_send(request, **kwargs)
            if isinstance(value, Response):
                return value
            if isinstance(value, HTTPResponse):
                return self.build_response(request, value)
            if value:
                raise ValueError('Middleware "before_send" methods must return '
                                 '`Response`, `HTTPResponse`, or `None`')
        return super().send(request, *args, **kwargs)

    def build_response(self, req, resp):
        """Build the response. Call `HTTPAdapter::build_response`, then pass
        the response object to the `after_build_response` method of each
        middleware in the stack, in reverse order.
        """
        for middleware in reversed(self.middlewares):
            req, resp = middleware.before_build_response(req, resp)
        response = super().build_response(req, resp)
        for middleware in reversed(self.middlewares):
            response = middleware.after_build_response(req, resp, response)
        return response

We add methods to BaseMiddleware class that allow intercept of the request or response objects at
multiple points along the request-response cycle.
It provides a common structure for introducing new handlers into the pipeline without breaking
the existing client code.

class BaseMiddleware(object):

    def before_init_poolmanager(self, connections, maxsize, block=False):
        """Called before `HTTPAdapter::init_poolmanager`. 
        Implemented by middleware that expose actions called during adapter
        initialization and returns a `dict` of keyword arguments or `None` to PoolManager
        """
        pass

    def before_send(self, request, *args, **kwargs):
        """Called before `HTTPAdapter::send`.
        Gives the option to modify the request before it is sent
        """
        pass

    def before_build_response(self, req, resp):
        """Called before `HTTPAdapter::build_response`. 
        Used to optionally modify the returned `PreparedRequest` and 
        `HTTPResponse` objects.
        """
        return req, resp

    def after_build_response(self, req, resp, response):
        """Called after `HTTPAdapter::build_response`. 
        Used to optionally modify the returned `Response` object.
        """
        return response

A middleware inherits from BaseMiddleware and overrides methods with custom logic

class AuthenticationMiddleware(BaseMiddleware):

    def __init__(self, credentials, scopes):
        self.credentials = credentials
        self.scopes = scopes

    def before_send(self, request, *args, **kwargs):
        """Update request header with security token after authentication.
        """
        token = self.credentials.get_token(*self.scopes)[0]
        request.headers.update({'Authorization': f'Bearer {token}'})

Sample implementation of the middleware pipeline. Middleware are called in the order of
registration.

default_credential = DefaultAzureCredential()
scopes = ['https://graph.microsoft.com/.default']
retry_policy = Retry(total=5, backoff_factor=1, status_forcelist=[429, 502, 503, 504])

session = requests.Session()
middlewares = [
    SSLMiddleware(ssl_version=ssl.PROTOCOL_TLSv1_2),
    RetryMiddleware(retries=retry_policy),
    AuthenticationMiddleware(credentials=default_credential, scopes=scopes )
]

adapter = MiddlewarePipeline(middlewares)
session.mount('https://graph.microsoft.com/v1.0', adapter)

[ddyett]

i don't know about this one either because there's really no new functionality I couldn't do by just using send on the existing implementation. What's the customer advantage? It also looks like you might hit some trouble if say during before_build_response you decided to call super().send. In that case it would always start from the first middleware.

[fey101]

I support the use of abstract classes and methods in python to enforce the contract (custom middlewares implementing the intercept method), otherwise the goal which is to prevent breakage of user defined middlewares will not be achieved.