fix #7465 Add uuid filter #7628
Conversation
aaa2000
force-pushed
the
fix-7465-uuid-filter
branch
from
f80b922 to
7f6964d
Compare
| /** | ||
| * @internal | ||
| */ | ||
| class AbstractUuidFilter extends AbstractFilter implements OpenApiParameterFilterInterface |
There was a problem hiding this comment.
Could we not use the AbstractFilter ? It's really not necessary with QueryParameter and this will help skipping all the logic about:
- property checks
- value transformation (should be done by our parameter value casting)
- documentation (use the new interfaces)
There was a problem hiding this comment.
Fixed, the AbstractUuidFilter no longer uses AbstractFilter
soyuka
left a comment
soyuka
left a comment
There was a problem hiding this comment.
Nice, also note I'm working on a range filter that can be used with many different types (soyuka@c74675d)
aaa2000
force-pushed
the
fix-7465-uuid-filter
branch
from
7f6964d to
f797162
Compare
aaa2000
force-pushed
the
fix-7465-uuid-filter
branch
from
b7e3778 to
100a022
Compare
| "api-platform/metadata": "^4.2", | ||
| "api-platform/state": "^4.2.4", | ||
| "doctrine/orm": "^2.17 || ^3.0" | ||
| "doctrine/orm": "^2.17 || ^3.0.1" |
There was a problem hiding this comment.
It needs allowing ArrayParameterType for uuid binary in QueryBuilder doctrine/orm#11287.
So, Uuid binary filter does not work with Doctrine 2.x. Can I skip the tests if Doctrine 2.x is installed and throws an exception in the UuidBianryFilter ?
|
|
||
| public function getSchema(Parameter $parameter): array | ||
| { | ||
| return self::UUID_SCHEMA; |
There was a problem hiding this comment.
API Platform not automatically adds a Symfony Uuid/Ulid constraint, I could create another PR for that
There was a problem hiding this comment.
| if (1 === \count($values)) { | ||
| $queryBuilder | ||
| ->andWhere($queryBuilder->expr()->eq($aliasedField, $valueParameter)) | ||
| ->setParameter($valueParameter, $values[0], $this->getDoctrineParameterType()); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
| if (1 === \count($values)) { | |
| $queryBuilder | |
| ->andWhere($queryBuilder->expr()->eq($aliasedField, $valueParameter)) | |
| ->setParameter($valueParameter, $values[0], $this->getDoctrineParameterType()); | |
| return; | |
| } | |
| if (!is_array($values)) { | |
| $queryBuilder | |
| ->andWhere($queryBuilder->expr()->eq($aliasedField, $valueParameter)) | |
| ->setParameter($valueParameter, $values[0], $this->getDoctrineParameterType()); | |
| return; | |
| } |
| if (!\is_array($values)) { | ||
| $values = [$values]; | ||
| } |
There was a problem hiding this comment.
| if (!\is_array($values)) { | |
| $values = [$values]; | |
| } |
Type casting is done before reaching the filter (see Parameter::castToArray).
| return $databaseValues; | ||
| } | ||
|
|
||
| return $values; |
There was a problem hiding this comment.
we should not need this this is done by Doctrine when applying parameters no? If this is really needed could you tell me why? Also you could have a guard clause in the callee and reduce the function complexity.
| $this->filterProperty($parameter->getProperty(), $parameter->getValue(), $queryBuilder, $queryNameGenerator, $resourceClass, $operation, $context); | ||
| } | ||
|
|
||
| protected function filterProperty(string $property, mixed $value, QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, ?Operation $operation = null, array $context = []): void |
There was a problem hiding this comment.
does this really need to be protected? I'd prefer if it wasn't.
|
|
||
| $values = (array) $value; | ||
| $associations = []; | ||
| if ($this->isPropertyNested($property, $resourceClass)) { |
There was a problem hiding this comment.
At some point I'd like to share the relation filtering logic into other filters, I'm still wondering how we'd make this good enough.
|
|
||
| // metadata doesn't have the field, nor an association on the field | ||
| if (!$metadata->hasAssociation($field)) { | ||
| return; |
There was a problem hiding this comment.
currently we log when something goes wrong, maybe add a log information here as the filter looks not valid?
2 participants
Add Symfony uuid, Ramsey uuid and Ramsey uuid binary filters.
The PR avoids modifying the search filter to prevent regressions. Furthermore, supporting partial strategies for UUID requires more work. In PostgreSQL, I think we need to cast the UUID to a string in the SQL query.
Possible improvements: