zendframework / zend-expressive

PSR-15 middleware in minutes!
BSD 3-Clause "New" or "Revised" License
710 stars 197 forks source link

[Question] "Hanging" objects onto the Request vs. singleton via service #296

Closed ZergMark closed 8 years ago

ZergMark commented 8 years ago

A question of middleware philosophy. Hypothetical situation: I'm developing an API using Expressive with authentication/authorization. One of my prerouting (high priority I guess post-RC7) middlewares is an authentication class to determine if I'm a user. I will need user info later in my routed middleware. I can see two approaches, but not sure which would be more adherent to the "idea" of middleware.

1) The UserService instantiates a singleton User. Subsequent middleware injects UserService if it needs the info. This keeps the Request/Response objects "clean", but adds the needs for factories/callbacks for middleware that might otherwise just be invokable.

2) The Authentication middleware attaches a User object as a parameter onto the Request/Response object. This keeps the subsequent middleware "cleaner" by not having to inject the service all over the place and possibly cutting done on factory/callback complexity. This could however lead to the Request or Response object being the catchall for inter-middleware data.

weierophinney commented 8 years ago

@ZergMark There's a third approach as well: use a service object.

This solution would be similar to your (1) above, but instead of using a singleton, you'd have a service you manage in your container. Middleware that might inject it or consume it then will have that service injected, which means you'll write factories for them:

class MiddlewareUsingUserService
{
    private $userService;

    public function __construct(UserService $userService)
    {
        $this->userService = $userService;
    }

    public function __invoke($request, $response, $next)
    {
        // Pull the current identity:
        $user = $this->userService->getIdentity();

        // Or maybe inject it? (Never actually pull it from a query param, unvalidated!)
        $this->userService->setIdentity($request->getQueryParams()['user_id']);

        /* ... */
    }
}

class MiddlewareUsingUserServiceFactory
{
    public function __invoke($container)
    {
        return new MiddlewareusingUserService($container->get(UserService::class));
    }
}

The above is essentially how we provide the UrlHelper and ServerUrlHelper; I've done similarly on my own website when using opauth.

Option (2), injecting as request attributes, is where I was originally considering handling features like this, and, in point of fact, we use precisely that for communicating the RouteResult from the routing middleware to the dispatch middleware (and any middleware in-between). However, as you note, this can lead to bloating of the request, and becomes more of a "conventions-based" approach. As such, I prefer the dependency injection route in most cases, as it ensures a clean separation, while preventing usage of singletons.

ZergMark commented 8 years ago

@weierophinney Thanks for your reply. I think that your code is what I was trying to convey in my first option. Create a middleware that uses a UserService to authenticate the Request and store a User object. Inject the UserService (via factory) into whatever other middleware needs the User object. I think I used "singleton" improperly in my explanation. I figured that DI of everything would be the prefered way for Expressive projects, but wanted to verify. DI would also make unit testing a bit easier so you weren't be "guessing" what needed to be attached to the Request for any particular middleware.

I have to admit is a part of me that misses the simple, containerless, procedural phly/conduit. I just create a Services middleware at the start and hang all my services off of the Request object. Eh, programming progresses. Expressive it positively svelte compared to the other "jumbo" frameworks.

Feel free to close, or wait for anyone else who might have an opinion.

weierophinney commented 8 years ago

@ZergMark —

I have to admit is a part of me that misses the simple, containerless, procedural phly/conduit.

That's still around. It's called zend-stratigility. :wink:

Also, as I noted, you can shove in request attributes as well. However, if you're using a container already, it's cleaner and easier to just inject the objects you need. as you note, it makes testing far simpler, as you don't have to worry about what you may have missed pushing into the request instance. It's all a matter of tradeoffs.

basz commented 8 years ago

Is there no danger of unintentionally 'leaking' state to other requests with a central service object like this? @weierophinney

Especially when using expressive together with PhpFastCgi https://github.com/PHPFastCGI/ExpressiveAdapter

A third (conceptual) way to counter this is to create a data storage service capable of storing and relating data to the request without actually setting it on the request.

A request uuid should be present as attribute on the request. And things need to be unset at some point when done. No idea of that is simple

So instead of

$request->withAttribute('identity', 'joe');

$this->requestStorageService->set($request, 'identity', 'joe');

Op 28 jan. 2016 om 16:34 heeft weierophinney notifications@github.com het volgende geschreven:

@ZergMark There's a third approach as well: use a service object.

This solution would be similar to your (1) above, but instead of using a singleton, you'd have a service you manage in your container. Middleware that might inject it or consume it then will have that service injected, which means you'll write factories for them:

class MiddlewareUsingUserService { private $userService;

public function __construct(UserService $userService)
{
    $this->userService = $userService;
}

public function __invoke($request, $response, $next)
{
    // Pull the current identity:
    $user = $this->userService->getIdentity();

    // Or maybe inject it? (Never actually pull it from a query param, unvalidated!)
    $this->userService->setIdentity($request->getQueryParams()['user_id']);

    /* ... */
}

}

class MiddlewareUsingUserServiceFactory { public function __invoke($container) { return new MiddlewareusingUserService($container->get(UserService::class)); } } The above is essentially how we provide the UrlHelper and ServerUrlHelper; I've done similarly on my own website when using opauth.

Option (2), injecting as request attributes, is where I was originally considering handling features like this, and, in point of fact, we use precisely that for communicating the RouteResult from the routing middleware to the dispatch middleware (and any middleware in-between). However, as you note, this can lead to bloating of the request, and becomes more of a "conventions-based" approach. As such, I prefer the dependency injection route in most cases, as it ensures a clean separation, while preventing usage of singletons.

— Reply to this email directly or view it on GitHub.

danizord commented 8 years ago

What's the problem of having request-related data inside request? I don't see any drawback, and it makes a lot of sense for me to store authenticated identity in request (this is part of request information after all).

Also, storing this data in request and passing them to services via arguments is the only way to keep your services stateless (https://igor.io/2013/03/31/stateless-services.html). Stateless services allows you to use things like PhpFastCgi and React, and they are much more easier to test and to understand the code flow.

weierophinney commented 8 years ago

@danizord The problem is with regards to the fact that there are not dedicated methods for each such service in the request instance. Instead, you have to use a conventions-based approach with the request attributes, which means you have to do more type-checking:

$identity = $request->getAttribute('identity', null);
if (! $identity instanceof SomeIdentityType) {
    // set a default? skip to the $next()? what?
}
// Okay, now we have an identity we can work with...

In terms of using fastcgi, the approach I suggested already works; the request and bootstrapping occurs on every request still. With React, the question is bootstrapping: do you bootstrap once, and handle many requests, or do you bootstrap on each request?

One benefit to React is that it makes the former possible, which isn't true under traditional web server paradigms. In that particular case, request attributes may make more sense. The other possibility, though, is having the bootstrap do a deep clone of the container, and potentially have it reset or replace specific service instances that contain state. This would be potentially costly, but solves the issue of state bleeding between requests.

Considering that this paradigm of bootstrap once, run multiple, is not what PHP has been architected for, and is fairly uncommon, my take is that you need, at least for now, to engineer differently under that particular paradigm than you would when running under nginx, Apache, or other traditional web servers.

danizord commented 8 years ago

@weierophinney

@danizord The problem is with regards to the fact that there are not dedicated methods for each such service in the request instance. Instead, you have to use a conventions-based approach with the request attributes, which means you have to do more type-checking

We can use utility classes for type-checking

$identity = $request->getAttribute(AuthenticationMiddleware::IDENTITY_ATTRIBUTE);

Assertion::isInstanceOf($identity, Identity::class);// + 1 line of code, -1 stateful service

Or you can move type-checking somewhere else:

$identity = Identity::fromRequest($request);

In terms of using fastcgi, the approach I suggested already works; the request and bootstrapping occurs on every request still. With React, the question is bootstrapping: do you bootstrap once, and handle many requests, or do you bootstrap on each request?

Both React and PHPFastCGI are bootstrap-once, look:
"Using this package, Zend Expressive applications can stay alive between HTTP requests whilst operating behind the protection of a FastCGI enabled web server". https://github.com/PHPFastCGI/ExpressiveAdapter#introduction

Considering that this paradigm of bootstrap once, run multiple, is not what PHP has been architected for, and is fairly uncommon, my take is that you need, at least for now, to engineer differently under that particular paradigm than you would when running under nginx, Apache, or other traditional web servers.

Multiple dispatch cycles is not the only problem, stateful services only increases complexity after all:

Even PSR-7 messages (that are not supposed to be stored, but passed along) are immutable for a number of reasons. So why would we make our services (supposed to be stored everywhere) mutable?

codeliner commented 8 years ago

I read through the discussion and would like to add my 2c

I prefer a mix of both options. Question is why does the authentication service already create a user? It should store an immutable Identity value object in the request. Then the routed middleware gets a stateless UserService injected and the service uses data from the Identity VO to create/load a User, perform an action on or with the User and persist the new state. That's it and it should work for other scenarios too because state is related to entities and entities should be part of the model and not part of the request/response handling layer.

class RoutedMiddleware
{
  private $userService;

  public function __construct(StatelessUserService $userService) { 
    $this->userService = $userService;
  }

  public function __invoke($req, $res, $next)
  {
    $this->userService->performAction($req->getAttribute('identity'));
    //...
  }
}

class StatelessUserService
{
  private $userDataStore;

  public function __construct($userDataStore)
  {
    $this->userDataStore = $userDataStore;
  }

  public function performAction(IdentityVO $identity)
  {
    $user = $this->userDataStore->load($identity->getUserId());

    $user->changeState();

    $this->userDataStore->save($user);
  }
}
danizord commented 8 years ago

Yeah, agree with you @codeliner, so just to sum it up:

Correct?

codeliner commented 8 years ago

yeah, sounds like a very good rule of thumb @danizord