Throw hydrator exceptions when encountering invalid types #2073
Conversation
alcaeus
force-pushed
the
hydrator-type-safety
branch
from
81c6de1
to
69e69bd
Compare
alcaeus
force-pushed
the
hydrator-type-safety
branch
from
69e69bd
to
5d767f7
Compare
| @@ -245,6 +245,11 @@ private function generateHydratorClass(ClassMetadata $class, string $hydratorCla | |||
| /** @ReferenceOne */ | |||
| if (isset(\$data['%1\$s'])) { | |||
| \$reference = \$data['%1\$s']; | |||
|
|
|||
| if (\$this->class->fieldMappings['%2\$s']['storeAs'] !== ClassMetadata::REFERENCE_STORE_AS_ID && ! is_array(\$reference)) { | |||
| throw HydratorException::associationTypeMismatch('%3\$s', '%1\$s', 'array', gettype(\$reference)); | |||
There was a problem hiding this comment.
I was originally confused as to the value of %3\$s, and assumed it was literal PHP code based on the previous usage. Further inspection revealed that this is in a completely separate sprintf() call and it's the class name string, which is also added 18 lines below. 👍
It's a shame we can't use more meaningful labels for these things.
There was a problem hiding this comment.
Yeah, that is very unfortunate. We may be able to improve this using Heredoc, but that's a change for a different release.
| @@ -245,6 +245,11 @@ private function generateHydratorClass(ClassMetadata $class, string $hydratorCla | |||
| /** @ReferenceOne */ | |||
| if (isset(\$data['%1\$s'])) { | |||
| \$reference = \$data['%1\$s']; | |||
|
|
|||
| if (\$this->class->fieldMappings['%2\$s']['storeAs'] !== ClassMetadata::REFERENCE_STORE_AS_ID && ! is_array(\$reference)) { | |||
There was a problem hiding this comment.
If I understand correctly, REFERENCE_STORE_AS_ID is the only strategy that stores an arbitrary value. All others nest the reference in an embedded document (either a DBRef or ODM's newer style that omits the $-prefixed field names), correct?
| if (! $embeddedDocuments) { | ||
| return; | ||
| } | ||
|
|
||
| if ($owner === null) { | ||
| throw PersistentCollectionException::ownerRequiredToLoadCollection(); |
There was a problem hiding this comment.
This exception already existed but just wasn't used here?
There was a problem hiding this comment.
Yep. This was introduced whenever we require the owner to be set (PHPStan found a few instances where we assumed it would be set). Since we now need the owner to generate the correct exception message, we have to also throw it here.
| $sorted = isset($mapping['sort']) && $mapping['sort']; | ||
|
|
||
| foreach ($collection->getMongoData() as $key => $reference) { | ||
| $className = $this->uow->getClassNameForAssociation($mapping, $reference); | ||
| $className = $this->uow->getClassNameForAssociation($mapping, $reference); |
There was a problem hiding this comment.
Noting this is just a whitespace change.
There was a problem hiding this comment.
Yep - assignment operator alignment strikes again. See doctrine/coding-standard#107.
There was a problem hiding this comment.
Oh yeah, I totally forgot about that PR
|
|
||
| $this->dm->getHydratorFactory()->hydrate($user, [ | ||
| '_id' => 1, | ||
| 'embedOne' => 'jon', |
There was a problem hiding this comment.
Glad to see we're still using @jwage as a data fixture.
There was a problem hiding this comment.
The beauty of copy/paste 😅
|
|
||
| $this->dm->getHydratorFactory()->hydrate($user, [ | ||
| '_id' => 1, | ||
| 'referenceOne' => 'jon', |
There was a problem hiding this comment.
Out of curiosity, do we bother testing for malformed DBRef or ODM-style reference objects? I realize that's probably out of scope for this PR.
There was a problem hiding this comment.
I believe we have no tests for such scenarios
There was a problem hiding this comment.
We don't have these, but adding them would probably make sense. Although I'd argue that we may want to be less strict when reading these references (e.g., accept ['id' => 'foo', 'collection' => 'foo']) when reading a DBRef object.
There was a problem hiding this comment.
On one hand it'd be nice but on the other it could give false impression that changing one storage strategy to another works "just like that" which can bite back when using aggregations
Summary
This adds type checks for associations to the hydrator, to prevent errors when receiving invalid data from the database. For most associations, this resulted in a
TypeErrorbeing thrown, but for references this could result in invalid offsets when accessing the reference "array".To fix this problem, there are new
HydratorExceptionmethods to account for these errors. We print the class name, field name of the offending data, as well as the expected and actual types. Since we are in the middle of hydration, we can't reliably print a full field path (we don't know if this is a root hydration or already within an embedded document), which also means that we can't print an identifier for the document being hydrated. However, the exception message better explains the problem than a PHP error does. More importantly, this makes it look less like a bug in ODM and more like an issue with the data (which it actually is).