From 7031f40f37e74df0bb4ac5349e59d2e8d59779e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Gautier?= Date: Wed, 5 Mar 2014 14:40:39 +0100 Subject: [PATCH] Allowed Constraint to be used on the requirements option on QueryParams and RequestParams --- Controller/Annotations/Param.php | 4 +- Request/ParamFetcher.php | 68 ++++--- Resources/config/request.xml | 1 + Resources/doc/3-listener-support.md | 9 +- Tests/Request/ParamFetcherTest.php | 273 +++++++++++++++++++++++++--- 5 files changed, 308 insertions(+), 47 deletions(-) diff --git a/Controller/Annotations/Param.php b/Controller/Annotations/Param.php index d9399888d..3eb0b4063 100644 --- a/Controller/Annotations/Param.php +++ b/Controller/Annotations/Param.php @@ -23,8 +23,8 @@ abstract class Param public $name; /** @var string */ public $key = null; - /** @var string */ - public $requirements = ''; + /** @var mixed */ + public $requirements = null; /** @var mixed */ public $default = null; /** @var string */ diff --git a/Request/ParamFetcher.php b/Request/ParamFetcher.php index 778448f00..69ac1fd77 100644 --- a/Request/ParamFetcher.php +++ b/Request/ParamFetcher.php @@ -17,6 +17,8 @@ use Doctrine\Common\Util\ClassUtils; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; +use Symfony\Component\Validator\Constraints\Regex; +use Symfony\Component\Validator\ValidatorInterface; /** * Helper to validate parameters of the active request. @@ -48,16 +50,23 @@ class ParamFetcher implements ParamFetcherInterface */ private $controller; + /** + * @var ValidatorInterface + */ + private $validator; + /** * Initializes fetcher. * - * @param ParamReader $paramReader Query param reader - * @param Request $request Active request + * @param ParamReader $paramReader Query param reader + * @param Request $request Active request + * @param ValidatorInterface $validator The validator service */ - public function __construct(ParamReader $paramReader, Request $request) + public function __construct(ParamReader $paramReader, Request $request, ValidatorInterface $validator) { $this->paramReader = $paramReader; - $this->request = $request; + $this->request = $request; + $this->validator = $validator; } /** @@ -103,17 +112,9 @@ public function get($name, $strict = null) } if ($config->array) { - $failMessage = null; - if (!is_array($param)) { - $failMessage = sprintf("Query parameter value of '%s' is not an array", $name); - } elseif (count($param) !== count($param, COUNT_RECURSIVE)) { - $failMessage = sprintf("Query parameter value of '%s' must not have a depth of more than one", $name); - } - - if (null !== $failMessage) { if ($strict) { - throw new BadRequestHttpException($failMessage); + throw new BadRequestHttpException(sprintf("Query parameter value of '%s' is not an array", $name)); } return $default; @@ -159,16 +160,39 @@ public function cleanParamWithRequirements(Param $config, $param, $strict) { $default = $config->default; - if ('' !== $config->requirements - && ($param !== $default || null === $default) - && !preg_match('#^'.$config->requirements.'$#xsu', $param) - ) { - if ($strict) { - $paramType = $config instanceof QueryParam ? 'Query' : 'Request'; + if (null === $config->requirements || ($param === $default && null !== $default)) { + + return $param; + } + + $constraint = $config->requirements; - throw new BadRequestHttpException( - $paramType . " parameter value '$param', does not match requirements '{$config->requirements}'" - ); + if (is_scalar($constraint)) { + if (is_array($param)) { + if ($strict) { + throw new BadRequestHttpException("Query parameter is an array"); + } + + return $default; + } + + $constraint = new Regex(array( + 'pattern' => '#^'.preg_quote($config->requirements).'$#xsu', + 'message' => sprintf( + "%s parameter value '%s', does not match requirements '%s'", + $config instanceof QueryParam ? 'Query' : 'Request', + $param, + $config->requirements + ) + )); + } + + $errors = $this->validator->validateValue($param, $constraint); + + if (0 !== count($errors)) { + + if ($strict) { + throw new BadRequestHttpException((string) $errors); } return null === $default ? '' : $default; diff --git a/Resources/config/request.xml b/Resources/config/request.xml index 507fe440c..a3d182b22 100644 --- a/Resources/config/request.xml +++ b/Resources/config/request.xml @@ -16,6 +16,7 @@ + diff --git a/Resources/doc/3-listener-support.md b/Resources/doc/3-listener-support.md index bd5ce930b..d997cbb83 100644 --- a/Resources/doc/3-listener-support.md +++ b/Resources/doc/3-listener-support.md @@ -421,6 +421,7 @@ fos_rest: use FOS\RestBundle\Request\ParamFetcher; use FOS\RestBundle\Controller\Annotations\RequestParam; use FOS\RestBundle\Controller\Annotations\QueryParam; +use Acme\FooBundle\Validation\Constraints\MyComplexConstraint class FooController extends Controller { @@ -461,7 +462,13 @@ class FooController extends Controller * If ids is not defined, array(1) will be given * * Array must have a single depth or it will return default value. It's difficult to validate with - * preg_match each deeps of array, if you want to deal with that, use another validation system. + * preg_match each deeps of array, if you want to deal with that, you can use a constraint: + * + * @QueryParam(array=true, name="filters", requirements=@MyComplexConstraint, description="List of complex filters") + * + * In this example, the ParamFetcher will validate each value of the array with the constraint, returning the + * default value if you are in safe mode or throw a BadRequestHttpResponse containing the constraint violation + * messages in the message. * * @param ParamFetcher $paramFetcher */ diff --git a/Tests/Request/ParamFetcherTest.php b/Tests/Request/ParamFetcherTest.php index 6885254b8..9e36dcb96 100644 --- a/Tests/Request/ParamFetcherTest.php +++ b/Tests/Request/ParamFetcherTest.php @@ -16,6 +16,9 @@ use FOS\RestBundle\Request\ParamFetcher; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Exception\HttpException; +use Symfony\Component\Validator\Constraints\Regex; +use Symfony\Component\Validator\ConstraintViolation; +use Symfony\Component\Validator\ConstraintViolationList; /** * ParamFetcher test. @@ -25,9 +28,26 @@ */ class ParamFetcherTest extends \PHPUnit_Framework_TestCase { + /** + * @var array + */ private $controller; + + /** + * @var \PHPUnit_Framework_MockObject_MockObject + */ private $paramReader; + /** + * @var \PHPUnit_Framework_MockObject_MockObject + */ + private $validator; + + /** + * @var \PHPUnit_Framework_MockObject_MockObject + */ + private $constraint; + /** * Test setup. */ @@ -39,6 +59,8 @@ public function setup() ->disableOriginalConstructor() ->getMock(); + $this->constraint = $this->getMockForAbstractClass('Symfony\Component\Validator\Constraint'); + $annotations = array(); $annotations['foo'] = new QueryParam; $annotations['foo']->name = 'foo'; @@ -86,6 +108,8 @@ public function setup() ->expects($this->any()) ->method('read') ->will($this->returnValue($annotations)); + + $this->validator = $this->getMock('Symfony\Component\Validator\ValidatorInterface'); } /** @@ -103,24 +127,32 @@ public function getParamFetcher($query = array(), $request = array(), $attribute $req = new Request($query, $request, $attributes); - return new ParamFetcher($this->paramReader, $req); + return new ParamFetcher($this->paramReader, $req, $this->validator); } /** * Test valid parameters. * - * @param string $param which param to test - * @param string $expected Expected query parameter value. - * @param string $expectedAll Expected query parameter values. - * @param array $query Query parameters for the request. - * @param array $request Request parameters for the request. + * @param string $param which param to test + * @param string $expected Expected query parameter value. + * @param string $expectedAll Expected query parameter values. + * @param array $query Query parameters for the request. + * @param array $request Request parameters for the request. + * @param \Closure $callback Callback to be applied on the validator * * @dataProvider validatesConfiguredParamDataProvider */ - public function testValidatesConfiguredParam($param, $expected, $expectedAll, $query, $request) + public function testValidatesConfiguredParam($param, $expected, $expectedAll, $query, $request, \Closure $callback = null) { + if (null !== $callback) { + $self = $this; + $validator = $this->validator; + $callback($validator, $self); + } + $queryFetcher = $this->getParamFetcher($query, $request); $queryFetcher->setController($this->controller); + $this->assertEquals($expected, $queryFetcher->get($param)); $this->assertEquals($expectedAll, $queryFetcher->all()); } @@ -150,21 +182,36 @@ public static function validatesConfiguredParamDataProvider() array( // check that invalid non-strict params take default value 'foo', '1', - array('foo' => '1', 'bar' => '1', 'baz' => '1', 'baz' => '4', 'buzz' => array(1), 'boo' => array(), 'boozz' => null, 'biz' => null), + array('foo' => '1', 'bar' => '1', 'baz' => '4', 'buzz' => array(1), 'boo' => array(), 'boozz' => null, 'biz' => null), array('foo' => 'bar'), array('bar' => '1', 'baz' => '4'), + function(\PHPUnit_Framework_MockObject_MockObject $validator, \PHPUnit_Framework_TestCase $self) { + $errors = new ConstraintViolationList(array( + new ConstraintViolation("expected error", null, array(), null, null, null) + )); + + $validator->expects($self->at(0)) + ->method('validateValue') + ->with('bar', new Regex(array('pattern' => '#^\\\d\+$#xsu', 'message' => "Query parameter value 'bar', does not match requirements '\\d+'")), null) + ->will($self->returnValue($errors)); + $validator->expects($self->at(1)) + ->method('validateValue') + ->with('bar', new Regex(array('pattern' => '#^\\\d\+$#xsu', 'message' => "Query parameter value 'bar', does not match requirements '\\d+'")), null) + ->will($self->returnValue($errors)); + + } ), array( // invalid array 'buzz', array(1), - array('foo' => '1', 'bar' => '1', 'baz' => '1', 'baz' => '4', 'buzz' => array(1), 'boo' => array(), 'boozz' => null, 'biz' => null), + array('foo' => '1', 'bar' => '1', 'baz' => '4', 'buzz' => array(1), 'boo' => array(), 'boozz' => null, 'biz' => null), array('buzz' => 'invaliddata'), array('bar' => '1', 'baz' => '4'), ), array( // invalid array (multiple depth) 'buzz', array(1), - array('foo' => '1', 'bar' => '1', 'baz' => '1', 'baz' => '4', 'buzz' => array(1), 'boo' => array(), 'boozz' => null, 'biz' => null), + array('foo' => '1', 'bar' => '1', 'baz' => '4', 'buzz' => array(1), 'boo' => array(), 'boozz' => null, 'biz' => null), array('buzz' => array(array(1))), array('bar' => '1', 'baz' => '4'), ), @@ -172,49 +219,64 @@ public static function validatesConfiguredParamDataProvider() array( // multiple array 'buzz', array(2, 3, 4), - array('foo' => '1', 'bar' => '1', 'baz' => '1', 'baz' => '4', 'buzz' => array(2, 3, 4), 'boo' => array(), 'boozz' => null, 'biz' => null), + array('foo' => '1', 'bar' => '1', 'baz' => '4', 'buzz' => array(2, 3, 4), 'boo' => array(), 'boozz' => null, 'biz' => null), array('buzz' => array(2, 3, 4)), array('bar' => '1', 'baz' => '4'), ), array( // multiple array with one invalid value 'buzz', array(2, 1, 4), - array('foo' => '1', 'bar' => '1', 'baz' => '1', 'baz' => '4', 'buzz' => array(2, 1, 4), 'boo' => array(), 'boozz' => null, 'biz' => null), + array('foo' => '1', 'bar' => '1', 'baz' => '4', 'buzz' => array(2, 1, 4), 'boo' => array(), 'boozz' => null, 'biz' => null), array('buzz' => array(2, 'invaliddata', 4)), array('bar' => '1', 'baz' => '4'), + function(\PHPUnit_Framework_MockObject_MockObject $validator, \PHPUnit_Framework_TestCase $self) { + $errors = new ConstraintViolationList(array( + new ConstraintViolation("expected error", null, array(), null, null, null) + )); + + $validator->expects($self->at(1)) + ->method('validateValue') + ->with('invaliddata', new Regex(array('pattern' => '#^\\\d\+$#xsu', 'message' => "Query parameter value 'invaliddata', does not match requirements '\\d+'")), null) + ->will($self->returnValue($errors)); + + $validator->expects($self->at(6)) + ->method('validateValue') + ->with('invaliddata', new Regex(array('pattern' => '#^\\\d\+$#xsu', 'message' => "Query parameter value 'invaliddata', does not match requirements '\\d+'")), null) + ->will($self->returnValue($errors)); + } ), array( // Array not provided in GET query 'boo', array(), - array('foo' => '1', 'bar' => '1', 'baz' => '1', 'baz' => '4', 'buzz' => array(2, 3, 4), 'boo' => array(), 'boozz' => null, 'biz' => null), + array('foo' => '1', 'bar' => '1', 'baz' => '4', 'buzz' => array(2, 3, 4), 'boo' => array(), 'boozz' => null, 'biz' => null), array('buzz' => array(2, 3, 4)), array('bar' => '1', 'baz' => '4'), ), array( // QueryParam provided in GET query but as a scalar 'boo', array(), - array('foo' => '1', 'bar' => '1', 'baz' => '1', 'baz' => '4', 'buzz' => array(2, 3, 4), 'boo' => array(), 'boozz' => null, 'biz' => null), + array('foo' => '1', 'bar' => '1', 'baz' => '4', 'buzz' => array(2, 3, 4), 'boo' => array(), 'boozz' => null, 'biz' => null), array('buzz' => array(2, 3, 4), 'boo' => 'scalar'), array('bar' => '1', 'baz' => '4'), ), array( // QueryParam provided in GET query with valid values 'boo', array('1', 'foo', 5), - array('foo' => '1', 'bar' => '1', 'baz' => '1', 'baz' => '4', 'buzz' => array(2, 3, 4), 'boo' => array('1', 'foo', 5), 'boozz' => null, 'biz' => null), + array('foo' => '1', 'bar' => '1', 'baz' => '4', 'buzz' => array(2, 3, 4), 'boo' => array('1', 'foo', 5), 'boozz' => null, 'biz' => null), array('buzz' => array(2, 3, 4), 'boo' => array('1', 'foo', 5)), array('bar' => '1', 'baz' => '4'), ), array( // QueryParam provided in GET query with valid values 'boozz', null, - array('foo' => '1', 'bar' => '1', 'baz' => '1', 'baz' => '4', 'buzz' => array(2, 3, 4), 'boo' => array('1', 'foo', 5), 'boozz' => null, 'biz' => null), + array('foo' => '1', 'bar' => '1', 'baz' => '4', 'buzz' => array(2, 3, 4), 'boo' => array('1', 'foo', 5), 'boozz' => null, 'biz' => null), array('buzz' => array(2, 3, 4), 'boo' => array('1', 'foo', 5)), array('bar' => '1', 'baz' => '4'), ), array( // QueryParam provided in GET query with valid values 'boozz', 5, - array('foo' => '1', 'bar' => '1', 'baz' => '1', 'baz' => '4', 'buzz' => array(2, 3, 4), 'boo' => array('1', 'foo', 5), 'boozz' => 5, 'biz' => null), + array('foo' => '1', 'bar' => '1', 'baz' => '4', 'buzz' => array(2, 3, 4), 'boo' => array('1', 'foo', 5), 'boozz' => 5, 'biz' => null), array('buzz' => array(2, 3, 4), 'boo' => array('1', 'foo', 5), 'boozz' => 5), array('bar' => '1', 'baz' => '4', 'boozz' => 5), ) @@ -243,8 +305,14 @@ public function testValidatesConfiguredParamStrictly() * Throw exception on invalid parameters. * @dataProvider exceptionOnValidatesFailureDataProvider */ - public function testExceptionOnValidatesFailure($query, $request, $param) + public function testExceptionOnValidatesFailure($query, $request, $param, \Closure $callback = null) { + if (null !== $callback) { + $self = $this; + $validator = $this->validator; + $callback($validator, $self); + } + $queryFetcher = $this->getParamFetcher($query, $request); $queryFetcher->setController($this->controller); @@ -274,17 +342,42 @@ public static function exceptionOnValidatesFailureDataProvider() array( // test missing strict param array(), array(), - 'bar' + 'bar', ), array( // test invalid strict param array(), array('bar' => 'foo'), - 'bar' + 'bar', + function(\PHPUnit_Framework_MockObject_MockObject $validator, \PHPUnit_Framework_TestCase $self) { + $errors = new ConstraintViolationList(array( + new ConstraintViolation("expected error", null, array(), null, null, null) + )); + + $validator->expects($self->at(0)) + ->method('validateValue') + ->with('foo', new Regex(array('pattern' => '#^\\\d\+$#xsu', 'message' => "Request parameter value 'foo', does not match requirements '\\d+'")), null) + ->will($self->returnValue($errors)); + + $validator->expects($self->at(1)) + ->method('validateValue') + ->with('foo', new Regex(array('pattern' => '#^\\\d\+$#xsu', 'message' => "Request parameter value 'foo', does not match requirements '\\d+'")), null) + ->will($self->returnValue($errors)); + } ), array( // test missing strict param with lax requirement array(), array('baz' => 'foo'), - 'baz' + 'baz', + function(\PHPUnit_Framework_MockObject_MockObject $validator, \PHPUnit_Framework_TestCase $self) { + $errors = new ConstraintViolationList(array( + new ConstraintViolation("expected error", null, array(), null, null, null) + )); + + $validator->expects($self->at(0)) + ->method('validateValue') + ->with('foo', new Regex(array('pattern' => '#^\\\d\?$#xsu', 'message' => "Request parameter value 'foo', does not match requirements '\\d?'")), null) + ->will($self->returnValue($errors)); + } ), ); } @@ -295,7 +388,7 @@ public static function exceptionOnValidatesFailureDataProvider() */ public function testExceptionOnRequestWithoutController() { - $queryFetcher = new ParamFetcher($this->paramReader, new Request()); + $queryFetcher = new ParamFetcher($this->paramReader, new Request(), $this->validator); $queryFetcher->get('none', '42'); } @@ -338,4 +431,140 @@ public function testKeyPrecedenceOverName() $queryFetcher->setController($this->controller); $this->assertEquals(5, $queryFetcher->get('biz')); } + + /** + * Test an Exception is thrown in strict mode + */ + public function testConstraintThrowExceptionInStrictMode() + { + $errors = new ConstraintViolationList(array( + new ConstraintViolation("expected message 1", null, array(), null, null, null), + new ConstraintViolation("expected message 2", null, array(), null, null, null), + )); + + $this->validator->expects($this->once()) + ->method('validateValue') + ->with('foobar', $this->constraint) + ->will($this->returnValue($errors)); + + $param = new QueryParam(); + $param->name = 'bizoo'; + $param->strict = true; + $param->requirements = $this->constraint; + $param->description = 'A requirements param'; + + $request = new Request(array('bizoo' => 'foobar'), array(), array('_controller' => __CLASS__.'::stubAction')); + $reader = $this->getMockBuilder('FOS\RestBundle\Request\ParamReader') + ->disableOriginalConstructor() + ->getMock(); + + $reader->expects($this->any()) + ->method('read') + ->will($this->returnValue(array('bizoo' => $param))); + + $this->setExpectedException( + "\\Symfony\\Component\\HttpKernel\\Exception\\BadRequestHttpException", + ":\n expected message 1\n:\n expected message 2\n" + ); + + $queryFetcher = new ParamFetcher($reader, $request, $this->validator); + $queryFetcher->setController($this->controller); + $queryFetcher->get('bizoo'); + } + + /** + * Test that the default value is returned in safe mode + */ + public function testConstraintReturnDefaultInSafeMode() + { + $violation1 = $this->getMockBuilder('Symfony\Component\Validator\ConstraintViolation') + ->disableOriginalConstructor() + ->getMock(); + + $violation1->expects($this->never())->method('getMessage'); + + $this->validator->expects($this->once()) + ->method('validateValue') + ->with('foobar', $this->constraint) + ->will($this->returnValue(array($violation1))); + + $param = new QueryParam(); + $param->name = 'bizoo'; + $param->requirements = $this->constraint; + $param->default = 'expected'; + $param->description = 'A requirements param'; + + $request = new Request(array('bizoo' => 'foobar'), array(), array('_controller' => __CLASS__.'::stubAction')); + $reader = $this->getMockBuilder('FOS\RestBundle\Request\ParamReader') + ->disableOriginalConstructor() + ->getMock(); + + $reader->expects($this->any()) + ->method('read') + ->will($this->returnValue(array('bizoo' => $param))); + + $queryFetcher = new ParamFetcher($reader, $request, $this->validator); + $queryFetcher->setController($this->controller); + $this->assertEquals('expected', $queryFetcher->get('bizoo')); + } + + /** + * Test a succesful return with a requirements + */ + public function testConstraintOk() + { + $this->validator->expects($this->once()) + ->method('validateValue') + ->with('foobar', $this->constraint) + ->will($this->returnValue(array())); + + $param = new QueryParam(); + $param->name = 'bizoo'; + $param->requirements = $this->constraint; + $param->default = 'not expected'; + $param->description = 'A requirements param'; + + $request = new Request(array('bizoo' => 'foobar'), array(), array('_controller' => __CLASS__.'::stubAction')); + $reader = $this->getMockBuilder('FOS\RestBundle\Request\ParamReader') + ->disableOriginalConstructor() + ->getMock(); + + $reader->expects($this->any()) + ->method('read') + ->will($this->returnValue(array('bizoo' => $param))); + + $queryFetcher = new ParamFetcher($reader, $request, $this->validator); + $queryFetcher->setController($this->controller); + $this->assertEquals('foobar', $queryFetcher->get('bizoo')); + } + + /** + * Test that we can use deep array structure with a requirements + */ + public function testDeepArrayAllowedWithConstraint() + { + $this->validator->expects($this->once()) + ->method('validateValue') + ->with(array('foo' => array('b', 'a', 'r')), $this->constraint) + ->will($this->returnValue(array())); + + $param = new QueryParam(); + $param->name = 'bizoo'; + $param->requirements = $this->constraint; + $param->default = 'not expected'; + $param->description = 'A requirements param'; + + $request = new Request(array('bizoo' => array('foo' => array('b', 'a', 'r'))), array(), array('_controller' => __CLASS__.'::stubAction')); + $reader = $this->getMockBuilder('FOS\RestBundle\Request\ParamReader') + ->disableOriginalConstructor() + ->getMock(); + + $reader->expects($this->any()) + ->method('read') + ->will($this->returnValue(array('bizoo' => $param))); + + $queryFetcher = new ParamFetcher($reader, $request, $this->validator); + $queryFetcher->setController($this->controller); + $this->assertSame(array('foo' => array('b', 'a', 'r')), $queryFetcher->get('bizoo')); + } }