RFC: Adding arbitrary middlewares to command & query routes

[basz]

From @basz on November 9, 2017 9:2

To add additional middleware to prooph routes one currently has to add those manually to each defined route.

eg.

[
  'name'            => 'query::support-doc::list-faqs',
  'path'            => '/support-doc/categories/{category_id}/faqs',
  'middleware'      => [
    JsonPayload::class,
    JsonApiProblem::class,
    ResourceServerMiddleware::class,
    JsonApiRequestMiddleware::class,
    ContentLanguageMiddleware::class,
    QueryMiddleware::class,
  ],
  'allowed_methods' => ['GET'],
  'options'         => ['values' => [QueryMiddleware::NAME_ATTRIBUTE => QueryName::listFaqs]],
],

We could introduce a way so QueryMiddleware and CommandMiddleware are capable of adding arbitrary middlewares so it can be easily configured.

I'm thinking of this configuration bit and some factory modifications;

    'prooph' => [
        'middleware' => [
            'query'   => [
                'response_strategy' => \HF\Api\ResponseStrategy\JsonApiResponseStrategy::class,
                'message_factory'   => \HF\Api\Messaging\QueryMessageFactory::class,
                'metadata_gatherer' => \HF\Api\Middleware\RequestMetadataGatherer::class,
                'middlewares'       =>
                    [
                        JsonPayload::class,
                        JsonApiProblem::class,
                        JsonApiRequestMiddleware::class,
                        ContentLanguageMiddleware::class,
                    ],
            ],
            'command' => [
                'response_strategy' => \HF\Api\ResponseStrategy\JsonApiResponseStrategy::class,
                'message_factory'   => \HF\Api\Messaging\CommandMessageFactory::class,
                'metadata_gatherer' => \HF\Api\Middleware\RequestMetadataGatherer::class,
                'middlewares'       =>
                    [
                        JsonPayload::class,
                        JsonApiProblem::class,
                        JsonApiRequestMiddleware::class,
                        ContentLanguageMiddleware::class,
                    ],
            ],
            ...
        ],
    ],

These 'middlewares' would need to be fetched from the container by Command and Query Factories and passed to the Command and QueryMiddlewares whom should process them in order and before the MetadataGatherer runs.

Shouldn't be too difficult, thoughts?

Copied from original issue: prooph/psr7-middleware#29

[basz]

From @prolic on November 9, 2017 11:33

I think we would need pre- and post-middlewares. The metadata gatherer can then be removed as option and simply added to the pre-middleware list.

[basz]

The metadata gatherer can then be removed as option and simply added to the pre-middleware list.

As it stands the MetadataGatherer isn't technically a Middleware

[basz]

From @prolic on November 9, 2017 12:20

Ok my bad

On Nov 9, 2017 8:13 PM, "Bas Kamer" notifications@github.com wrote:

The metadata gatherer can then be removed as option and simply added to
the pre-middleware list.

As it stands the MetadataGatherer isn't technically a Middleware

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
prooph/psr7-middleware#29 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAYEvAxxrq-baEewerTq2Tt_eykbrC3hks5s0uxcgaJpZM4QXodv
.

[basz]

I believe the biggest hurdle is to somehow insert the given middlewares list into the pipeline of the current route and have those execute from within the executing Command- & QueryMiddleware. Any idea on how to do that?

[basz]

https://zendframework.slack.com/archives/C4QBQUEG5/p1509550722000242
https://zendframework.slack.com/archives/C4QBQUEG5/p1509117340000326

@danizord could you share your experiences and perhaps have some advise

[basz]

From @prolic on November 13, 2017 13:29

@basz I don't have a slack account for the zendframework workspace. Slack tells me I should ask the admin, so it seems not to be a public forum. Can you share the information for non-zf-slack users?

[basz]

From @weierophinney on November 13, 2017 15:32

@prolic https://zendframework-slack.herokuapp.com for invites.

[basz]

From @prolic on November 13, 2017 16:21

I am not sure, just a shot into the blue:

if (isset($options['middleware'])) {
            $middlewares = (array) $options['middleware'];
            $middlewares[] = $commandMiddleware;
            $pipe = new SplQueue();
            foreach ($middlewares as $middleware) {
                if (is_string($middleware)) {
                    $middleware = $container->get($middleware);
                }
                $pipe->enqueue($middleware);
            }
            return new Next($pipe);
        }

[basz]

From @danizord on November 13, 2017 19:39

I believe the biggest hurdle is to somehow insert the given middlewares list into the pipeline of the current route and have those execute from within the executing Command- & QueryMiddleware. Any idea on how to do that?

@basz @prolic you can also do that with MiddlewarePipe and LazyLoadingMiddleware:

use Zend\Expressive\Middleware\LazyLoadingMiddleware;
use Zend\Stratigility\MiddlewarePipe;

$pipeline = new MiddlewarePipe();
$response = new Response();

foreach ($middlewares as $middleware) {
    $pipeline->pipe(new LazyLoadingMiddleware($container, $response, $middleware));
}

$pipeline->process($request, $queryOrCommandMiddleware);

---

Having said that, let me show you the approach (a bit unrelated) that I've been working with. Most middlewares would require different configuration for each route, e.g an JsonSchemaValidationMiddleware would use a different schema per route. Even QueryMiddleware and CommandMiddleware shipped by this package requires route-based configuration such as the query/command name.

I can see in your example that this package solves that using route options, but it has some problems:

1. Looking at your route config, you can't identify for which middleware each option is for;

2. The route options are not type-checked or analyzable statically, which makes it harder to find bugs before running the code;

3. Due to 1 and 2, people avoid writing too much configurable middlewares and end up writing either fat middlewares that solves multiple problems and violates SRP (hard to remove/replace specific behaviors) or middlewares with their own plugin systems (response strategy, message factory, metadata gatherer...)

The approach I'm using to solve that is passing these configuration as params to middleware constructors and creating different instances for each route:

// These with "Factory" suffix are custom factories that already contains dependencies
// injected by the container and expects the configuration to create actual middlewares.
$inputValidation = $container->get(InputValidationMiddleware::class);
$createQuery     = $container->get(CreateQueryMiddlewareFactory::class);
$createCommand   = $container->get(CreateCommandMiddlewareFactory::class);
$dispatchCommand = DispatchCommandMiddleware::class;
$dispatchQuery   = DispatchQueryMiddleware::class;
$commandResponse = new CommandResponseMiddleware();
$queryResponse   = $container->get(QueryResponseMiddlewareFactory::class);

// Ping
$app->get('/internal/ping', $commandResponse->withSuccessMessage('ok'));

// ScheduleReminder
$app->post('/api/reminders', [
    // I'm using with* methods because I found it more readable, but it can be pure
    // callables like $inputValidation('schedule-reminders.json')
    $inputValidation->withJsonSchema('schedule-reminders.json'),

    // Thanks to callable typehint, PHPStorm recognizes it as the class static method,
    // So you can refactor, find usages, click to navigate and such :)
    $createCommand->withCommandFactory([UpdateSettings::class, 'fromRequest']),

    // This middleware does not require any configuration, so we don't need to
    // instantiate it here, we can pass the name and Expressive will lazy instantiate it
    $dispatchCommand,

    // Some middlewares wouldn't need dependencies from container, so you don't need
    // to create a custom factory, you can implement immutbale with* methods like PSR-7
    $commandResponse->withRedirectTo('/api/reminders/{id}'),
]);

// GetReminders
// You can see how the middleware pipeline looks very readable :)
$app->get('/api/reminders', [
    $createQuery->withQueryFactory([GetReminders::class, 'fromRequest']),
    $dispatchQuery,
    $queryResponse->withResourceClass(ReminderResource::class),
]);

[basz]

Thank you @danizord lots to play with and contemplate. I'll start with the MiddlewarePipe thing, but I think @prolic and @codeliner should have a read over your second point. Definitely a BC but it might be worth investigating...

[basz]

From @sprolic-modus on November 14, 2017 8:16

BC is not a problem, as we have only dev-releases so far and not even 1.0 final.

[basz]

From @sprolic-modus on November 14, 2017 8:18

@danizord your approach loads all the middlewares for all requests, despite of being used, right? I kind of don't like that.

[basz]

From @danizord on November 14, 2017 10:6

@sprolic-modus if this is hurting performance, you can do some workaround to pipe the middlewares lazily:

$app->get('/api/reminders', lazyPipeline(function () use ($container) {
    // Pull from container...

    return [
        $createQuery->withQueryFactory([GetReminders::class, 'fromRequest']),
        $dispatchQuery,
        $queryResponse->withResourceClass(ReminderResource::class),
    ];
}));

But the syntax starts to become confusing (would be nice if PHP had arrow functions) :D

Also since the middleware pipeline is stateless, you can use PHPFastCGI, ReactPHP or something like that to bootstrap once and keep running.

[basz]

From @codeliner on November 14, 2017 20:20

@sprolic-modus are you the real @prolic ? :D

@danizord I really like the approach, but would definitely go with the lazyPipeline approach.
Using reactPHP/amp for bootstrapping once can solve the problem, BUT you introduce complexity in your app that is not always needed. reactPHP/amp is single threaded non-blocking I/O just like Node.js is. While this is a performance boost when done right you have to be very careful. Not only the middleware pipeline needs to be stateless, but the entire app and all dependencies as well. That's not all. You also need to make sure that you don't block the event loop! One mistake and you have a lot of trouble in production. CQRS/ES is an architecture that plays well with PHP's shared nothing environment. I don't want to give up this simplicity so loading only the stuff needed to serve a single request/message is a must-have and support for reactPHP/amp is a nice-to-have IMHO.

[basz]

From @codeliner on November 14, 2017 20:39

@weierophinney Why slack? :D

I mean we use it for work and it is working fine but for an OSS project hosted on github we really like gitter. People can join with their github or twitter account and directly start asking questions. And gitter is completely open source.

[basz]

From @prolic on November 14, 2017 20:45

Confirming, sprolic-modus is me. Sorry was logged in wrong account. This
one is for my current client only.

On Nov 15, 2017 4:40 AM, "Alexander Miertsch" notifications@github.com
wrote:

@weierophinney https://github.com/weierophinney Why slack? :D

I mean we use it for work and it is working fine but for an OSS project
hosted on github we really like gitter. People can join with their github
or twitter account and directly start asking questions. And gitter is
completely open source.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
prooph/psr7-middleware#29 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAYEvGYJOLCwEQUjEcdz1FQbzMJFk1yQks5s2fqdgaJpZM4QXodv
.

[basz]

From @codeliner on November 14, 2017 20:50

ah ok. @sprolic-modus sounds like your hacker account (hopefully white hat) :P

[basz]

From @sprolic-modus on November 14, 2017 20:55

Haha

On Nov 15, 2017 4:50 AM, "Alexander Miertsch" notifications@github.com
wrote:

WARNING - This email was received from outside EDR but claims to be from
an EDR email address. Unless you are specifically expecting this email, it
is spam/phishing and should be deleted.

ah ok. @sprolic-modus https://github.com/sprolic-modus sounds like your
hacker account (hopefully white hat) :P

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
prooph/psr7-middleware#29 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AcsirIhA-vtJv5ntiKbbsThP0XElQpEIks5s2f0KgaJpZM4QXodv
.

[basz]

From @weierophinney on November 14, 2017 21:25

@codeliner —

Why slack?

• Developer familiarity
• Moderation tooling
• Amount of integrations
• Large number of resources for integrating bots
• etc. etc. etc.

Not going to debate it again. It works for us, and, frankly, developer engagement has gone way up since we started using it.

[basz]

From @codeliner on November 14, 2017 21:28

interesting. thx @weierophinney and definitely better than IRC. Never was a fan of IRC, though

[basz]

From @prolic on December 23, 2017 13:47

@basz Please move this issue to https://github.com/prooph/http-middleware/