Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BodyParsingMiddleware #2798

Merged
merged 10 commits into from Aug 15, 2019
19 changes: 17 additions & 2 deletions Slim/App.php
Expand Up @@ -19,6 +19,7 @@
use Slim\Interfaces\CallableResolverInterface;
use Slim\Interfaces\RouteCollectorInterface;
use Slim\Interfaces\RouteResolverInterface;
use Slim\Middleware\BodyParsingMiddleware;
use Slim\Middleware\ErrorMiddleware;
use Slim\Middleware\RoutingMiddleware;
use Slim\Routing\RouteCollectorProxy;
Expand Down Expand Up @@ -100,7 +101,7 @@ public function addMiddleware(MiddlewareInterface $middleware): self
}

/**
* Add the slim built-in routing middleware to the app middleware stack
* Add the Slim built-in routing middleware to the app middleware stack
*
* @return RoutingMiddleware
*/
Expand All @@ -115,7 +116,7 @@ public function addRoutingMiddleware(): RoutingMiddleware
}

/**
* Add the slim built-in error middleware to the app middleware stack
* Add the Slim built-in error middleware to the app middleware stack
*
* @param bool $displayErrorDetails
* @param bool $logErrors
Expand All @@ -139,6 +140,20 @@ public function addErrorMiddleware(
return $errorMiddleware;
}

/**
* Add the Slim body parsing middleware to the app middleware stack
*
* @param callable[] $bodyParsers
*
* @return BodyParsingMiddleware
*/
public function addBodyParsingMiddleware(array $bodyParsers = []): BodyParsingMiddleware
{
$bodyParsingMiddleware = new BodyParsingMiddleware($bodyParsers);
$this->add($bodyParsingMiddleware);
return $bodyParsingMiddleware;
}

/**
* Run application
*
Expand Down
175 changes: 175 additions & 0 deletions Slim/Middleware/BodyParsingMiddleware.php
@@ -0,0 +1,175 @@
<?php
/**
* Slim Framework (https://slimframework.com)
*
* @license https://github.com/slimphp/Slim/blob/4.x/LICENSE.md (MIT License)
*/

declare(strict_types=1);

namespace Slim\Middleware;

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Server\MiddlewareInterface;
use Psr\Http\Server\RequestHandlerInterface;
use RuntimeException;

class BodyParsingMiddleware implements MiddlewareInterface
{
/**
* @var callable[]
*/
protected $bodyParsers;

/**
* @param callable[] $bodyParsers list of body parsers as an associative array of mediaType => callable
*/
public function __construct(array $bodyParsers = [])
{
$this->registerDefaultBodyParsers();

foreach ($bodyParsers as $mediaType => $parser) {
$this->registerBodyParser($mediaType, $parser);
}
}

/**
* @param ServerRequestInterface $request
* @param RequestHandlerInterface $handler
* @return ResponseInterface
*/
public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
{
$parsedBody = $request->getParsedBody();
if ($parsedBody === null || empty($parsedBody)) {
$parsedBody = $this->parseBody($request);
$request = $request->withParsedBody($parsedBody);
}

return $handler->handle($request);
}

/**
* @param string $mediaType A HTTP media type (excluding content-type params).
* @param callable $callable A callable that returns parsed contents for media type.
* @return self
*/
public function registerBodyParser(string $mediaType, callable $callable): self
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than a generic callable, why not create an interface?

interface BodyParser
{
    public function canParse(string $contentType): bool;

    /**
     * @return mixed
     */
    public function parse(StreamInterface $stream);
}

Then you can do:

$middleware = new BodyParsingMiddleware();
$middleware->register(new JsonParser());
$middleware->register(new XmlParser());
// etc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't disagree with you on this. This to me feels like it should be extracted in its own package if we're going to go that route so we can contain the files contextually. The middleware we ship right now are self-contained into one file.

{
$this->bodyParsers[$mediaType] = $callable;
return $this;
}

/**
* @param string $mediaType A HTTP media type (excluding content-type params).
* @return boolean
*/
public function hasBodyParser(string $mediaType): bool
{
return isset($this->bodyParsers[$mediaType]);
}

/**
* @param string $mediaType A HTTP media type (excluding content-type params).
* @return callable
* @throws RuntimeException
*/
public function getBodyParser(string $mediaType): callable
{
if (!isset($this->bodyParsers[$mediaType])) {
throw new RuntimeException('No parser for type ' . $mediaType);
akrabat marked this conversation as resolved.
Show resolved Hide resolved
}
return $this->bodyParsers[$mediaType];
}


protected function registerDefaultBodyParsers(): void
{
$this->registerBodyParser('application/json', function ($input) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for basic applications, but what about users of JSON-API that use application/vnd.api+json as the content type? The parsing is exactly the same but this hard lock on application/json won't work.

$result = json_decode($input, true);

if (!is_array($result)) {
return null;
}

return $result;
});

$this->registerBodyParser('application/x-www-form-urlencoded', function ($input) {
parse_str($input, $data);
return $data;
});

$xmlCallable = function ($input) {
$backup = libxml_disable_entity_loader(true);
$backup_errors = libxml_use_internal_errors(true);
$result = simplexml_load_string($input);

libxml_disable_entity_loader($backup);
libxml_clear_errors();
libxml_use_internal_errors($backup_errors);

if ($result === false) {
return null;
}

return $result;
};

$this->registerBodyParser('application/xml', $xmlCallable);
$this->registerBodyParser('text/xml', $xmlCallable);
}

/**
* @param ServerRequestInterface $request
* @return null|array|object
*/
protected function parseBody(ServerRequestInterface $request)
{
$mediaType = $this->getMediaType($request);
if ($mediaType === null) {
return null;
}

// Check if this specific media type has a parser registered first
if (!isset($this->bodyParsers[$mediaType])) {
// If not, look for a media type with a structured syntax suffix (RFC 6839)
$parts = explode('+', $mediaType);
if (count($parts) >= 2) {
$mediaType = 'application/' . $parts[count($parts) - 1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I get that this is supposed to handle application/vnd.api+json using application/json parser... that behavior is not particularly obvious though, and would be better (IMO) handled on a case by case basis in a specific parser.

}
}

if (isset($this->bodyParsers[$mediaType])) {
$body = (string)$request->getBody();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the stream is not seekable (rewindable)?
This would be a problem if any subsequent middleware, for whatever reason, needed to access the raw body.
An example would be authentication via HMAC.

One solution would be to simply copy the non-seekable stream to a new seekable stream.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the stream is not seekable (rewindable)?
This would be a problem if any subsequent middleware, for whatever reason, needed to access the raw body.
An example would be authentication via HMAC.

__toString() will rewind (if it can) and leaves the pointer back where it was before the call, so the stream won't look any different for subsequent middleware. (If it can't rewind, then nothing's changed of course.)

One solution would be to simply copy the non-seekable stream to a new seekable stream.

If the stream is not rewindable, then there's no guarantee that the data at the beginning is anywhere in memory now, so I don't see any advantage to copying to seekable stream?

At the end of the day, I think that if you're using a non-rewindable string in your ServerRequest, then you're incompatible with getParsedBody() and won't need or want this middleware.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__toString() will rewind (if it can) and leaves the pointer back where it was before the call, so the stream won't look any different for subsequent middleware. (If it can't rewind, then nothing's changed of course.)

According to PSR-7 it is not a requirement to attempt to rewind the stream after the data has been read by __to_string(). PSR-7 only requires the stream to be rewound before the data is read.

If the stream is not rewindable, then there's no guarantee that the data at the beginning is anywhere in memory now, so I don't see any advantage to copying to seekable stream?

At the end of the day, I think that if you're using a non-rewindable string in your ServerRequest, then you're incompatible with getParsedBody() and won't need or want this middleware.

Well most often you will see something like $streamFactory->createStreamFromFile('php://input') when creating the body stream.
The problem is that there is no guarantee php://input is seekable.
So the only real solution if the stream is not seekable and the raw data needs to access more than once is to copy the stream.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well most often you will see something like $streamFactory->createStreamFromFile('php://input') when creating the body stream.
The problem is that there is no guarantee php://input is seekable.
So the only real solution if the stream is not seekable and the raw data needs to access more than once is to copy the stream.

Right. I see what you're saying now. The problem is that we can't copy the stream as there's no method in StreamInterface that provide access to the underlying stream object.

Thoughts?

Copy link
Member Author

@akrabat akrabat Aug 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A further note. It looks like this has to be solved at the PSR-7 implementation level. e.g. Diactoros's PhpInputStream which is then used by ServerRequest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anfly0 I'm confused trying to do this.

If the stream is not seeable, then we can't rewind it and hence can't parse it in this middleware. Have I missed something?

Copy link
Member Author

@akrabat akrabat Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe I copy the stream if it's not seekable and $stream->tell() is 0?

Copy link

@anfly0 anfly0 Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akrabat I'm no 100 on this, but I think that $stream->tell() can return 0 on a not seekable stream even if some of the data has been read.
Offsets have no real meaning if the stream is not seekable.

One idea is to copy the stream if it's not seekable and then compare the size of the new stream with what's in the Content-Length header.
If the match it's reasonably safe to assume that all data was copied and if they don't some error handling can be done.
And then parse the contents.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we get a non-seeable stream into a ServerRequest?

Copy link

@anfly0 anfly0 Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the question.
The PSR-7 implementation used is free to use whatever resource or method to create the stream.
It is an edge case, but it is entirely possible for the stream not to be seekable.

$parsed = $this->bodyParsers[$mediaType]($body);

if (!is_null($parsed) && !is_object($parsed) && !is_array($parsed)) {
throw new RuntimeException(
'Request body media type parser return value must be an array, an object, or null'
);
}

return $parsed;
}

return null;
}

/**
* @param ServerRequestInterface $request
* @return string|null The serverRequest media type, minus content-type params
*/
protected function getMediaType(ServerRequestInterface $request): ?string
{
$contentType = $request->getHeader('Content-Type')[0] ?? null;

if (is_string($contentType) && trim($contentType) != '') {
$contentTypeParts = explode(';', $contentType);
return strtolower(trim($contentTypeParts[0]));
}

return null;
}
}
2 changes: 2 additions & 0 deletions composer.json
Expand Up @@ -73,6 +73,8 @@
"phpstan": "php -d memory_limit=-1 vendor/bin/phpstan analyse Slim"
},
"suggest": {
"ext-simplexml": "Needed to support XML format in BodyParsingMiddleware",
"ext-xml": "Needed to support XML format in BodyParsingMiddleware",
"slim/psr7": "Slim PSR-7 implementation. See http://www.slimframework.com/docs/v4/start/installation.html for more information."
}
}
31 changes: 31 additions & 0 deletions tests/AppTest.php
Expand Up @@ -28,6 +28,7 @@
use Slim\Interfaces\RouteCollectorInterface;
use Slim\Interfaces\RouteCollectorProxyInterface;
use Slim\Interfaces\RouteParserInterface;
use Slim\Middleware\BodyParsingMiddleware;
use Slim\Middleware\ErrorMiddleware;
use Slim\Middleware\RoutingMiddleware;
use Slim\MiddlewareDispatcher;
Expand Down Expand Up @@ -714,6 +715,36 @@ public function testAddErrorMiddleware()
$this->assertInstanceOf(ErrorMiddleware::class, $errorMiddleware);
}

public function testAddBodyParsingMiddleware()
{
/** @var ResponseFactoryInterface $responseFactory */
$responseFactory = $this->prophesize(ResponseFactoryInterface::class)->reveal();

// Create the app.
$app = new App($responseFactory);

// Add the error middleware.
$bodyParsingMiddleware = $app->addBodyParsingMiddleware();

// Check that the body parsing middleware really has been added to the tip of the app middleware stack.
$middlewareDispatcherProperty = new \ReflectionProperty(App::class, 'middlewareDispatcher');
$middlewareDispatcherProperty->setAccessible(true);
/** @var MiddlewareDispatcher $middlewareDispatcher */
$middlewareDispatcher = $middlewareDispatcherProperty->getValue($app);

$tipProperty = new \ReflectionProperty(MiddlewareDispatcher::class, 'tip');
$tipProperty->setAccessible(true);
/** @var RequestHandlerInterface $tip */
$tip = $tipProperty->getValue($middlewareDispatcher);

$reflection = new \ReflectionClass($tip);
$middlewareProperty = $reflection->getProperty('middleware');
$middlewareProperty->setAccessible(true);

$this->assertSame($bodyParsingMiddleware, $middlewareProperty->getValue($tip));
$this->assertInstanceOf(BodyParsingMiddleware::class, $bodyParsingMiddleware);
}

public function testAddMiddlewareOnRoute()
{
$output = '';
Expand Down