I was checking out of of our web services the other day, replicating a bug I was fixing, and found the need to guess at what the body content needed to be for a POST I was doing. Our validation is solid, so I could just start with an empty object, and the validation would tell me what I needed to pass. Or, yes, I could RTFM but I couldn't be arsed.
Anyhow, this tactic worked for the first few properties: the validation came back with "you need this property"... "nah, it needs to be an integer"... "that's too low", etc. Cool. But then I passed an empty string for one of the properties, and instead of getting a 400 back with a hint as to what the constraints needed to be; I instead got a 500 server error. F***'s sake.
{
"errors": [
"Internal Server Error"
]
}
This is good and bad. Bad it was a 500 for some reason; good that there's no detail bleeding out. This was part of what I was fixing.
Looking at the logs I was getting this:
{"errors": ["Expected argument of type \"array or Traversable and ArrayAccess\", \"string\" given"]}
Indeed I was passing in a string:
{
"firstName" : "Zachary",
"lastName" : "Cameron Lynch",
"address" : ""
}
(not the actual data in question, but it's work code so I cannot share it). Right so address is supposed to be an object, eg:
{
"firstName" : "Zachary",
"lastName" : "Cameron Lynch",
"address" : {
"street" : "123 Some Street",
"city" : "Our City"
}
}
That sort of thing.
But anyhow, why's the valiation failing? Well not failing - I'd expect / want that - but why is it erroring? I assumed we were doing something wrong: not validating it was an object, and just charging off and using it. I looked into it, and our code seemed legit, so start trying to make a solid repro case so I can ask people out in the world about it.
I know nothing about Symfony's Validator component. I have code-reviewed other people using it, so am vaguely aware of how it works, but I've not really used it. First things first then... the docs.
I concocted this test case from the introductory docs:
use PHPUnit\Framework\TestCase;
use Symfony\Component\Validator\Constraints\Length;
use Symfony\Component\Validator\Constraints\NotBlank;
use Symfony\Component\Validator\Validation;
class StringValidatorTest extends TestCase
{
public function testExampleFromDocumentation()
{
$validator = Validation::createValidator();
$violations = $validator->validate(
'Bernhard',
[
new Length(['min' => 10]),
new NotBlank(),
]
);
$this->assertCount(1, $violations);
$this->assertEquals(
'This value is too short. It should have 10 characters or more.',
$violations[0]->getMessage()
);
}
}
And this worked as expected:
PHPUnit 7.2.6 by Sebastian Bergmann and contributors.
. 1 / 1 (100%)
Time: 218 ms, Memory: 4.00MB
OK (1 test, 2 assertions)
Also this is pretty straight fwd. Create a validator, pass a value and some constraints to a validate, and... job done.
From there I needed to test the behaviour of passing the wrong data type when expected. I decided expecting an integer and then not giving it one would be the easiest test here:
First a class to wrap-up the validator with the constraints in question:
namespace me\adamcameron\general\validation\collectionIssue;
use Symfony\Component\Validator\Constraints\Type;
use Symfony\Component\Validator\Validation;
class IntegerValidator
{
private $validator;
private $constraints;
public function __construct()
{
$this->validator = Validation::createValidator();
$this->setConstraints();
}
private function setConstraints()
{
$this->constraints = [
new Type(['type' => 'integer'])
];
}
public function validate($value)
{
return $this->validator->validate($value, $this->constraints);
}
}
Nothing surprising there. I have a constraints collection which is simply "gotta be an integer". And a method that takes a value and validates it against those constraints.
And then some tests:
<?php
namespace me\adamcameron\general\test\validation\collectionIssue;
use me\adamcameron\general\validation\collectionIssue\IntegerValidator;
use PHPUnit\Framework\TestCase;
class IntegerValidatorTest extends TestCase
{
use ViolationAssertions;
private $validator;
protected function setUp()
{
$this->validator = new IntegerValidator();
}
/** @dataProvider provideCasesForValidateTests */
public function testValidate($validValue)
{
$violations = $this->validator->validate($validValue);
$this->assertHasNoViolations($violations);
}
public function provideCasesForValidateTests()
{
return [
'has value' => ['value' => 42],
'is null' => ['value' => null]
];
}
/** @dataProvider provideCasesForViolationsTests */
public function testValidateReturnsViolationsWhenConstraintsBroken($invalidValue)
{
$violations = $this->validator->validate($invalidValue);
$this->assertHasViolations($violations);
}
public function provideCasesForViolationsTests()
{
return [
'string' => ['value' => 'forty-two'],
'integer string' => ['value' => '42'],
'float' => ['value' => 4.2],
'integer as float' => ['value' => 42.0],
'array' => ['value' => [42]]
];
}
}
PHPUnit 7.2.6 by Sebastian Bergmann and contributors.
....... 7 / 7 (100%)
Time: 204 ms, Memory: 4.00MB
OK (7 tests, 14 assertions)
(Oh, I whipped a coupla custom assertions out into a trait):
namespace me\adamcameron\general\test\validation\collectionIssue;
use Symfony\Component\Validator\ConstraintViolationList;
trait ViolationAssertions
{
public function assertHasNoViolations($violations)
{
$this->assertInstanceOf(ConstraintViolationList::class, $violations);
$this->assertEmpty($violations);
}
public function assertHasViolations($violations)
{
$this->assertInstanceOf(ConstraintViolationList::class, $violations);
$this->assertNotEmpty($violations);
}
}
Bottom line, as you can see from the test results, one can pass an invalid value to a validator, and it won't error; it'll just return you yer violation. Cool.
So what's the go with a collection then? Why are we getting that error?
I concocted a similar CollectionValidator:
namespace me\adamcameron\general\validation\collectionIssue;
use Symfony\Component\Validator\Constraints\Collection;
use Symfony\Component\Validator\Constraints\Type;
use Symfony\Component\Validator\Validation;
class CollectionValidator
{
private $validator;
private $constraints;
public function __construct()
{
$this->validator = Validation::createValidator();
$this->setConstraints();
}
private function setConstraints()
{
$this->constraints = [
new Collection([
'fields' => [
'field1' => [new Type(['type'=>'integer'])]
],
'allowMissingFields' => true,
'allowExtraFields' => true
])
];
}
public function validate($value = null)
{
return $this->validator->validate($value, $this->constraints);
}
}
This time I'm defining a "collection" (read: "array" in this case) which could have a property field1, and if it does; it needs to be an integer. It could have other properties too: don't care. Or indeed field1 could be missing: don't care. But if it's there... it needs to be an integer.
And we test this. I'll break the class down this time so I can discuss:
namespace me\adamcameron\general\test\validation\collectionIssue;
use me\adamcameron\general\validation\collectionIssue\CollectionValidator;
use PHPUnit\Framework\TestCase;
class CollectionValidatorTest extends TestCase
{
use ViolationAssertions;
private $validator;
protected function setUp()
{
$this->validator = new CollectionValidator();
}
/** @dataProvider provideCasesForValidateEmptyCollectionTests */
public function testValidateWithValidButEmptyCollectionTypeIsOk($validValue)
{
$violations = $this->validator->validate($validValue);
$this->assertHasNoViolations($violations);
}
public function provideCasesForValidateEmptyCollectionTests()
{
return [
'array' => ['value' => []],
'iterator' => ['value' => new \ArrayIterator()]
];
}
}
First just a baseline test with some empty (but valid) values. These pass:
PHPUnit 7.2.6 by Sebastian Bergmann and contributors.
.. 2 / 2 (100%)
Time: 187 ms, Memory: 4.00MB
OK (2 tests, 4 assertions)
Next I test a fully-legit collection with a valid integer value for the field1 value.
public function testValidateWithIntegerField1ValueShouldPass()
{
$violations = $this->validator->validate(['field1'=>42]);
$this->assertHasNoViolations($violations);
}
All good:
PHPUnit 7.2.6 by Sebastian Bergmann and contributors.
. 1 / 1 (100%)
Time: 211 ms, Memory: 4.00MB
OK (1 test, 2 assertions)
And now with the correct field1 property, but without an integer. This should have violations
public function testValidateWithNonIntegerField1ValueShouldHaveViolation()
{
$violations = $this->validator->validate(['field1'=>'NOT_AN_INTEGER']);
$this->assertHasViolations($violations);
$this->assertSame('This value should be of type integer.', $violations[0]->getMessage());
}
Results:
PHPUnit 7.2.6 by Sebastian Bergmann and contributors.
. 1 / 1 (100%)
Time: 191 ms, Memory: 4.00MB
OK (1 test, 3 assertions)
And now one without field1, but with another property:
public function testValidateWithOnlyOtherFieldsShouldPass()
{
$violations = $this->validator->validate(['anotherField'=>'another value']);
$this->assertHasNoViolations($violations);
}
PHPUnit 7.2.6 by Sebastian Bergmann and contributors.
. 1 / 1 (100%)
Time: 191 ms, Memory: 4.00MB
OK (1 test, 2 assertions)
And for good measure I chuck it a null and see what happens:
public function testValidateWithNullIsApparentlyOK()
{
$violations = $this->validator->validate(null);
$this->assertHasNoViolations($violations);
}
Not sure this is legit IMO - a null is not a collection as per my constraint definition. But... well... not what I care about right now. It passes:
PHPUnit 7.2.6 by Sebastian Bergmann and contributors.
. 1 / 1 (100%)
Time: 259 ms, Memory: 4.00MB
OK (1 test, 2 assertions)
OK so at this point I reckon I have done my constraints properly as my tests all pass.
And now the test that replicates our problem:
public function testValidateWithNonTraversableShouldCauseViolation()
{
$violations = $this->validator->validate('a string');
$this->assertHasViolations($violations);
}
Note that here I am not passing a collection value at all. I am just passing a string. So this should have a violation, right? Nuh-uh:
PHPUnit 7.2.6 by Sebastian Bergmann and contributors.
E 1 / 1 (100%)
Time: 194 ms, Memory: 4.00MB
There was 1 error:
1) me\adamcameron\general\test\validation\collectionIssue\CollectionValidatorTest::testValidateWithNonTraversableShouldCauseViolation
Symfony\Component\Validator\Exception\UnexpectedTypeException: Expected argument of type "array or Traversable and ArrayAccess", "string" given
C:\src\php\general\vendor\symfony\validator\Constraints\CollectionValidator.php:37
C:\src\php\general\vendor\symfony\validator\Validator\RecursiveContextualValidator.php:829
C:\src\php\general\vendor\symfony\validator\Validator\RecursiveContextualValidator.php:675
C:\src\php\general\vendor\symfony\validator\Validator\RecursiveContextualValidator.php:118
C:\src\php\general\vendor\symfony\validator\Validator\RecursiveValidator.php:100
C:\src\php\general\src\validation\collectionIssue\CollectionValidator.php:36
C:\src\php\general\test\validation\collectionIssue\CollectionValidatorTest.php:63
ERRORS!
Tests: 1, Assertions: 0, Errors: 1.
So let me get this straight. I am passing validation code a value that ought not validate. And what I get is not a "it dun't validate mate" violation, the validation lib just breaks.
Grumble.
Now this just seems to me to be a testing-101-fail on the part of the Validator library. One must always be able to give the validator any value, and the result should just be "yeah all good", or "nup. And here's why". However as the Symfony mob usually do such solid work, I am kinda wondering if I'm messing something up, and just not seeing it?
What do you think? Even if you don't know PHP or Symfony validation, what would your expectations be here? Or can yous ee the glaring error that I can't see from having looked at this too long?
I will presently ask a question on this on Stack Overflow (a heavily-abridged version of this), but wanted to get some more friendly eyes on it first.
Update
This was bugging my colleague so he went in and checked the underlying Symfony code - something I should have done - and the CollectionValidator is very specifically throwing this exception very early in the piece:public function validate($value, Constraint $constraint)
{
if (!$constraint instanceof Collection) {
throw new UnexpectedTypeException($constraint, __NAMESPACE__.'\Collection');
}
if (null === $value) {
return;
}
if (!is_array($value) && !($value instanceof \Traversable && $value instanceof \ArrayAccess)) {
throw new UnexpectedTypeException($value, 'array or Traversable and ArrayAccess');
}
That strikes me as very daft, and contrary to the intend of a validator lib. It's been there since the very first commit of that code, as far as I can tell (back in 2010). It's clearly their intent, but the intent is misguided IMO.
Another update
This has just bitten us on the butt again, and my mate Levi came across this very article when trying to troubleshoot it. Ha. However I didn't post an answer. I know we worked around it somehow... just trying to find the code now.We found a coupla issues on the Symfony bug tracker:
- (#26463) AllValidator should not check for type
- (#12312) [Validator] Decouple type validation from constraint validators
I'll post a follow-up article and cross link to here once we've worked around it.
Cheers.
--
Adam
