http-interop / http-middleware

PSR-15 interfaces for HTTP Middleware
MIT License
73 stars 7 forks source link

What happened to Response? #46

Closed designermonkey closed 7 years ago

designermonkey commented 7 years ago

Hi there,

I've not been following the progress of the PSR standard that closely unfortunately for me, and I'm confused as to the current state. Initially, it used the Request, Response, next pattern which I thought to be the most useful.

Now I see it has been boiled down to just Request, next. Can anyone shed some light onto this?

A big concern for me is that how am I supposed to make alterations to the response? Say my middleware has run through, done it's application work, and is on it's way back out the other side, how can I inspect the response that has been generated and make any alterations? For example, I may want to inject a google analytics script into any html if the response is set to a html header.

atanvarno69 commented 7 years ago

The meta document explains why this approach was taken.

The quick answer to you question:

how am I supposed to make alterations to the response?

You would do something like:

class SomeMiddleware implements ServerMiddlewareInterface
{
    public function process(ServerRequestInterface $request, DelegateInterface $delegate)
    {
        // Perform various actions on $request here
        // Get your response:
        $response = $delegate->process($request);
        // Modify your response here
        return $response;
    }
}
designermonkey commented 7 years ago

I may be missing something, but somewhere along the line I will need to create a response instance? Doesn't that couple the code to an implementation of some kind of factory?

designermonkey commented 7 years ago

Some have argued that passing the response helps ensure dependency inversion. While it is true that it helps avoid depending on a specific implementation of HTTP messages, the problem can also be resolved by injecting factories into the middleware to create HTTP message objects, or by injecting empty message instances. With the creation of HTTP Factories in PSR-17, a standard approach to handling dependency inversion is possible.

Ok, so I am concerned here that the response to the concern of dependency, is to become dependent. This is just bad. There is no PSR-17 yet, as I see there isn't a PSR for messages either, but that's a moot point.

This is all just creating more work to solve something that works perfectly fine already.

The most severe is that passing an empty response has no guarantees that the response is in a usable state. This is further exacerbated by the fact that a middleware may modify the response before passing it for further dispatching.

This is not the concern of the pattern, and should not be. This is the concern of the developer to make his/her code actually work! There is such a thing as defensive programming, but paranoid programming? I could implement an observer pattern and one of the observers be broken. Is that therefore the problem of the pattern? No, it's bad code.


I don't mean to rant about this, but I see progress going backwards here. There is a reason that double pass was adopted so widely over the age old single pass; it worked better, and allowed more consistent decoupling.

jschreuder commented 7 years ago

You might want to read @ircmaxell 's blog post on this subject: http://blog.ircmaxell.com/2016/05/all-about-middleware.html

designermonkey commented 7 years ago

Thanks will do.

schnittstabil commented 7 years ago

The main reason to me is that providing $request and $next is very error prone:

public function __invoke(RequestInterface $request,  ResponseInterface $response, callable $next
    )
{
    // modify $response [1]
    $response2 = $next($request, $response);
    // modify $response and/or $response2

   return $response2;  // or $response?
}

In my opinion, the @ircmaxell's blog post ist flawed in many aspects, but I fully agree with him about this:

What does $response mean inside of the middleware?

As long as $next returns a response too, it is not clear what happens, when we modify it at [1]. And the list of errors because of this is long – far too long.


This is all just creating more work to solve something that works perfectly fine already.

In most cases nothing important changes (the current interfaces look a bit different, but in essence):

public function __invoke(RequestInterface $request, callable $next)
{
-    // modify $response [1]
-    $response = $next($request, $response);
+    $response = $next($request);
+    // modify $response [1]

   return $response;
}

Just the order changed – and in many cases the new order was intended in the first place.

atanvarno69 commented 7 years ago

Ok, so I am concerned here that the response to the concern of dependency, is to become dependent. This is just bad. There is no PSR-17 yet, as I see there isn't a PSR for messages either, but that's a moot point.

PSR for (HTTP) messages is PSR-7, unless I misunderstand you?

As for the dependency point: I disagree that this approach encourages one to become dependent (which I'm reading as tightly coupled).

Your preferred PSR-15 delegate implementation will likely require constructor injection of a PSR-7 ResponseInterface implementation (in order to provide a response when asked to when the middleware queue/stack is empty) and will work with equally well with any PSR-7 implementation. Or it will require injection of a PSR-17 ResponseFactoryInterface, which achieves much the same thing but without the need for the end user to generate a blank response themselves. None of this is tightly coupled.

A PSR-15 middleware that needs to generate its own response (such as returning a 404/500 error response without calling the delegate for a response to modify), will likewise require constructor injection of either ResponseInterface or ResponseFactoryInterface. Perhaps this is where your concern about dependence -- that the class itself requires a dependency, rather than receiving response as a parameter -- arises? However, since this middleware codes against an interface, rather than an implementation, I do not think it should be a concern.

Given that the PSR is targeted at frameworks: all frameworks make use of dependency injection containers (indeed, a frameworkless developer will mostly likely be using a DI container), it is up to the end user to configure their container to provide their preferred PSR-7/17 implementation to their middleware and delegate. So I don't see how this PSR encourages/necessitates tight coupling to a particular implementation.

Finally, yes, PSR-17 is not adopted yet. However, it looks to me (and I may be speaking out of turn here) pretty much ready for submission, with only a few minor tweaks left. I expect its review and adoption to be far easier than PSR-15's (which, while I support and see the reasoning behind its current design, is bound to rub some people the wrong way whatever design it finally has).

weierophinney commented 7 years ago

Your preferred PSR-15 delegate implementation will likely require constructor injection of a PSR-7 ResponseInterface implementation [...] Or it will require injection of a PSR-17 ResponseFactoryInterface,

Or, if it is middleware specific to your application, and not middleware you are distributing as a package, you can also always choose to instantiate and return a concrete implementation:

use Zend\Diactoros\Response\JsonResponse;

function (ServerRequestInterface $request, DelegateInterface $delegate)
{
    // do some work
    return new JsonResponse($data);
}

All three of these approaches are valid. Passing a response prototype, or an application-specific factory that can generate a response, can be done now, as @atanvarno69 has indicated; you can also create and return a response now. PSR-17 is in the future, but not that far distant. It can coexist with the other approaches, allowing you to use them immediately, and later adopt the new specification.

designermonkey commented 7 years ago

Just to begin this response properly, I am not saying any of this work is wrong; I always feel the need to question things which I don't fully understand, and mostly if I don't understand it, it's because something feels wrong. This is not by any means a question of the efforts of individuals involved, it's just the way my brain works :)

@atanvarno69 no, my bad, I meant PSR-17 and the word middleware.

@schnittstabil I agree with your point about @ircmaxell's post, and from that and your message, I have one more question...

public function process(RequestInterface $request, DelegateInterface $delegate)
{
    // Make my own response
    $response = new ResponseFromSomewhere;

    // get response from other middleware
    $response2 = $delegate->process($request);
    // modify $response and/or $response2

   return $response2;  // or $response?
}

What would happen if two middlewares wanted to make a response object? Basically, the same issues arise as being argued for not passing it in. In this situation, should we expect only the innermost item (usually the core application) to be allowed to produce a response?

...but I fully agree with him about this:

What does $response mean inside of the middleware?

To me, this opens up the conversation further...

Do we actually have a separation of concerns issue here? Should middleware declare itself as request middleware or response middleware? Yes, this would mean that this lovely clean and clever approach (both of them) with only one queue in and then out wouldn't work, but maybe separating them into two interfaces from one root interface would be a better solution?

<?php

namespace Psr\Http\Middleware;

interface MiddlewareInterface
{
}

interface RequestMiddlewareInterface extends MiddlewareInterface
{
    public function process(RequestInterface $request, DelegateInterface $delegate): RequestInterface;
}

interface ResponseMiddlewareInterface extends MiddlewareInterface
{
    public function process(ResponseInterface $response, DelegateInterface $delegate): ResponseInterface;
}

There could still be a single queue implementation that is iterated over twice for example.

schnittstabil commented 7 years ago

What would happen if two middlewares wanted to make a response object?

I believe you've overlooked something, which I cannot exactly spot. Maybe the following helps to figure it out.


The following is a pattern we can use in ExpressJS:

app.get('/user/:id', function (req, res, next) {
  if (req.params.id === 'admin') {
    next();
    return;
  }

  res.send('<h1>Hi non admin!</h1>')
});

app.get('/user/admin', function (req, res, next) {
  res.send('<h1>Hi admin!</h1>')
});

That works nicely, calling next() means: I do not feel responsible for that request, the next middleware should take care of it. As there are only one single request and one single response object in ExpressJS, the concerns are strictly separated:

And this is also the reason why next() neither needs arguments nor returns a response, and moreover it is the reason why middlewares do not return response objects in ExpressJS.


Sadly/Thankfully that does not fit well with PSR-7. As PSR-7 responses are immutable (except the body stream), the middleware must return a response:

public function process(ServerRequestInterface $request, DelegateInterface $delegate)
{
    $response1 = $delegate->process($request); // [1]
    $response2 = new Response(); // [2]
    $response3 = $response2->withHeader('secret', 'unicorns are awesome'); // [3]

    return $response1;  // or $response2, or $response3?
}

In all three cases above a new response will be created; and the middleware must decide which is the right one and must return it.

If we now provide an additional $response due to a parameter, then we create a separation of concerns issue: Now the framework and the middlewares are responsible to create new responses – and that leads to some questions:

  1. Which is the right response?
  2. Who decides question 1 – the middleware or the framework, or both?

That would be an extremely undesirable situation, which caused many of the bugs mentioned by @ircmaxell in my opinion.


Should middleware declare itself as request middleware or response middleware?

I would go even further, I believe the middleware pattern is sometimes over- and misused. I would like to split them into 3 groups for a moment:

interface RequestParserInterface
{
    public function process(RequestInterface $request): RequestInterface;
}

interface ResponseDecoratorInterface
{
    public function process(ResponseInterface $response): ResponseInterface;
}

interface MiddlewareInterface
{
    public function process(ServerRequestInterface $request, DelegateInterface $delegate): ResponseInterface;
}

In my opinion the middlewares/client-ip is an example of a request parser:

public function process(ServerRequestInterface $request, DelegateInterface $delegate)
{
     $ip = $this->getIp($request);
     return $delegate->process($request->withAttribute($this->attribute, $ip));
}

It is not necessary to call return $delegate->process(…);, return $request->withAttribute(…); would make more sense. But if we change it, we can not use it in middleware stacks anymore.

However, I believe RequestParserInterface and ResponseDecoratorInterface should be considered out of scope of PSR-15.


Hence, what are true Middlewares? Obviously, those middlewares intend to modify the response based on either the request and/or the response of the next middleware. Therefore, I believe the middlewares/whoops is a good non-trivial example:

public function process(ServerRequestInterface $request, DelegateInterface $delegate)
{
    // …
    try {
        $response = $delegate->process($request);
    } catch (\Throwable $exception) {
        $response = Utils\Factory::createResponse(500);
        $response->getBody()->write($whoops->$method($exception));
    }
    //…
    return $response;
}

And I hope this answers your question in the end:

What would happen if two middlewares wanted to make a response object?

Of course, the next middleware wants to create a response object too, but the whoops middleware is responsible to decide if the (partially) created response of the inner middleware(s) is the right one. And I believe this meets our requirements best.

designermonkey commented 7 years ago

I understand your points very well, thanks for sharing them.

I'm not trying to show that I don't understand these problems, I'm trying to raise the concern that there are just as many serious flaws in the single pass method as the double pass method.

Double pass receives the response object as part of it's call parameters, whether this is the response from the initial instantiation, or the response returned (to the middleware stack handler) from the previous middleware; either way, it is the proper response object in the chain of the application's middleware. Yes there is a SRP violation here though.

Single pass doesn't. It offloads making a response object to somewhere else that it doesn't even care about. There is no insurance that it will even get a response object from the delegate! How are we supposed to know as a middleware, that it's not us that is supposed to make the middleware? Should it be the responsibility of the innermost middleware? Am I the innermost middleware? I don't know.

I'll be honest here, I never liked this middleware pattern anyway:

A method call can return a new instance of something, I don't disagree with this at all, apart from if it is in a chain. In that scenario, it should be passed all the arguments it needs to work with to become part of that chain; a request to interrogate, and a response to pass along the chain.

There is a big hole in the single pass method with responsibility. Who is responsible for creating the response object in the chain? In real terms, no one knows. So every single middleware will now have to test the return of $delegate->process() to see if a response comes out? That's a mess of DRY if you ask me.


I'm sorry to keep on about this, but I think one bad pattern is being replaced with another bad pattern, when the whole thing needs to be seen as bad and replaced with something better that actually works.

atanvarno69 commented 7 years ago

There is a big hole in the single pass method with responsibility. Who is responsible for creating the response object in the chain? In real terms, no one knows. So every single middleware will now have to test the return of $delegate->process() to see if a response comes out? That's a mess of DRY if you ask me.

From the current DelegateInterface:

/**
 * Dispatch the next available middleware and return the response.
 *
 * @param ServerRequestInterface $request
 *
 * @return ResponseInterface
 */

process() MUST return a response in order to adhere to the spec. There is no need for any given middleware to test the return of $delegate->process() when used with a compliant PSR-15 delegate. (Likewise, your PSR-15 compliant middleware MUST return a response and you can rely on the delegate to get you started on that front.)

A PHP 7+ delegate will use a signature with a return type hint:

public function process(ServerRequestInterface $request): ResponseInterface

However, a PHP 5.6 delegate is still required to give a response even though it can't use a return type hint.

designermonkey commented 7 years ago

Maybe I didn't word it right, or you've missed the point.

Who is responsible for creating the response object in the chain?

Lets keep picking small holes in this, rather than tackling the wider issue I'm trying to bring to light here.

I'm sorry to keep on about this, but I think one bad pattern is being replaced with another bad pattern, when the whole thing needs to be seen as bad and replaced with something better that actually works.

jschreuder commented 7 years ago

Actually, the return value type safety was half your argument against it:

So every single middleware will now have to test the return of $delegate->process() to see if a response comes out? That's a mess of DRY if you ask me.

But about the other half, about the responsibility for creating the Response. You're now saying the entire middleware design is bad because it doesn't mandate how objects are created? Have you ever heard of dependency injection? You don't need to create these objects, that's something that is easily (and rather justly) delegated. One could inject a response through a setter, through a constructor or through a factory. There's no reason for a middleware standard to be concerned with this because there's a lot of options out there that help you deal with that concern.

But the authors did feel it was part of their responsibility to at least offer a possible solution with PSR-17.

designermonkey commented 7 years ago

Right then, my point and concern is not getting across. Maybe it's me wording things not entirely perfectly, or something else.

I'm not baiting an argument here, and won't be involved in one.

This is not going anywhere.

ircmaxell commented 7 years ago

Single pass doesn't. It offloads making a response object to somewhere else that it doesn't even care about. There is no insurance that it will even get a response object from the delegate! How are we supposed to know as a middleware, that it's not us that is supposed to make the middleware? Should it be the responsibility of the innermost middleware? Am I the innermost middleware? I don't know.

I think the answer I would give would be to invert the question. How, as a middleware, should you know that you ARE supposed to create a response?

The answer in that case is simple: do you (as a middleware) want to and know how to respond? If so, then create and return a response. If not, delegate to an inner middleware (there will always be an inner middleware, the innermost being the application itself).

Let's take an example. Let's say you're adding cache control headers in your middleware. Well, you don't know how to respond (since you can't predict the status code yet), so you delegate inwards to find the response. Then you add your headers and return (or create a new response for a 304).

Another example would be if you wanted to reject a request because of rate limiting. Well, in that case you do know how to respond, and in fact don't want anything further to happen (you want to "abort" the request). So you create a new 429 response object and return it (without calling the inner middleware).

There is a big hole in the single pass method with responsibility. Who is responsible for creating the response object in the chain? In real terms, no one knows.

That's actual the clear benefit to it as well. Anyone can create a response who needs to. And to any individual layer, it doesn't care (if you're in the inner Application, you know you must create a response, if you're a middleware that wants to you know you must, if you're a middleware that's delegating you don't care). As long as someone in the chain does, then all is well.

So every single middleware will now have to test the return of $delegate->process() to see if a response comes out? That's a mess of DRY if you ask me.

Nope, that's the job of the middleware "runner". The return of process() is typed to be a response, and therefore it MUST be a response. The middleware should safely assume that it will be. (Also, if you dislike this, let's get PHP7 adoption high enough where we can have the engine enforcing this for us).

schnittstabil commented 7 years ago

Yes there is a SRP violation here though.

As far as I can see, that is your main concern: With single pass the middleware is responsible for probably three things: creating, modifying and returning responses.

But please take note that PSR-7 Responses are immutable, thus middlewares MUST return the response in the double pass world as well! Furthermore, also double pass middlewares must create response objects from time to time to do their job.

I see; the response creation is at least annoying – why should I take care of DI etc.? Therefore, I want to give three examples, which shows that a concrete framework can lighten your workload.

Example 1. A framework may provide you a response object for the application logic, e.g. like Slim already does:

$app = new \Slim\App;
$app->get('/hello/{name}', function (Request $request, Response $response) {
    $name = $request->getAttribute('name');
    $response->getBody()->write("Hello, $name");

    return $response;
});

The above is neither a middleware in the sense of PSR-15 nor in the sense of (pure) double pass. But it shows that frameworks can still provide pre-configured response objects – PSR-15 does not prohibit this.

Example 2. A framework may provide/inject a factory for you:

// e.g. $app implements the PSR-17 ResponseFactoryInterface:
$app->add(function (ServerRequestInterface $request, DelegateInterface $delegate) use ($app) {
    $response = $app->createResponse();
    // or, for example:
    $response = $this->createResponse();
    //…
    return $response;
});

Again, the above is not a middleware in the sense of PSR-15 (it is not a MiddlewareInterface instance) and you can not share it across different frameworks, but that might be okay for your use-cases.

Example 3. But, with PSR-15 we want reusable middlewares, similar to those in the Awesome PSR-15 Middlewares List, and with constructor injection and a PSR-17 compatible framework, it probably will look like this:

// e.g. $app implements the PSR-17 ResponseFactoryInterface:
$app->add(new Middlewares\Whoops($app));

Hence, I believe that end-consumers do not often feel the need to create response objects. But if they do and delegate the creation to a factory, then it is not a SRP violation in my opinion.