-
-
Notifications
You must be signed in to change notification settings - Fork 958
feat(doctrine): uuid filter #7628
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
base: main
Are you sure you want to change the base?
Conversation
f80b922 to
7f6964d
Compare
soyuka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, also note I'm working on a range filter that can be used with many different types (soyuka@c74675d)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes totally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build was successful previously because Doctrine v3 was installed in the Symfony lowest step. Maybe, because doctrine/dbal": "^4.0" is required
https://github.com/api-platform/core/blob/4.2/composer.json#L129C10-L129C33 and doctrine v2 is not compatible with doctrine dbal v4
|
|
||
| public function getSchema(Parameter $parameter): array | ||
| { | ||
| return self::UUID_SCHEMA; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Platform not automatically adds a Symfony Uuid/Ulid constraint, I could create another PR for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (!\is_array($values)) { | ||
| $values = [$values]; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doctrine handles type conversion automatically when you set the third argument in setParameter(). However, if the type is not explicitly set, UUIDs are treated as strings by default. Depending on the database platform, this can cause issues because if the platform natively supports UUIDs, the value should be converted to a binary representation.
Doctrine Documentation:
Calling setParameter() automatically infers which type you are setting as value.
This works for integers, arrays of strings/integers, DateTime instances, and for managed entities. If you want to set a type explicitly, you can call the third argument to setParameter() explicitly. It accepts either a DBALDoctrine\DBAL\ParameterType::*or a DBAL Type name for conversion.
Symfony Documentation:
When using built-in Doctrine repository methods (e.g.
findOneBy()), Doctrine knows how to convert these UUID types to build the SQL query. However, when using DQL queries or building the query yourself, you'll need to setuuidas the type of the UUID parameters.
In the application of fixture:
We have the configuration in config_common.yml:
https://github.com/aaa2000/core/blob/b8397aeec444d94d34e3631961e693560622a933/tests/Fixtures/app/config/config_common.yml#L12
doctrine:
dbal:
driver: 'pdo_sqlite'
charset: 'UTF8'
types:
uuid: Ramsey\Uuid\Doctrine\UuidType
uuid_binary: Ramsey\Uuid\Doctrine\UuidBinaryType
symfony_uuid: Symfony\Bridge\Doctrine\Types\UuidTypeSo, the third argument should be uuid or uuid_binary or symfony_uuid, and the array of uuid strings should be transformed into an array of objects (Symfony uuid, Ramsey uuid...). Note: we can use the convertToPHPValue method of the Doctrine type for this https://github.com/symfony/symfony/blob/8.0/src/Symfony/Bridge/Doctrine/Types/AbstractUidType.php#L43
For WHERE ... IN() clauses, we cannot set the parameter type with uuid or uuid_binary or symfony_uuid. ArrayParameterType only supports INTEGER, STRING, ASCII, and BINARY.
UUIDs are stored as CHAR(36) by default, or in a platform-specific datatype if the database supports native UUIDs. PostgreSQL and SQL Server have native UUID support: PostgreSQLPlatform, SQLServerPlatform See also Doctrine DBAL: Mapping Matrix
For this reason, it seemed simpler to convert the UUIDs into their database representation.
Note that Symfony does the same thing in ORMQueryBuilderLoader https://github.com/symfony/symfony/blob/f31baa789d552e6cd3b349ea35aad6b67508a599/src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php#L70
|
|
||
| $values = (array) $value; | ||
| $associations = []; | ||
| if ($this->isPropertyNested($property, $resourceClass)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
b8397ae to
5cb992d
Compare
soyuka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you target main ?
| } | ||
|
|
||
| $values = $this->convertValuesToTheDatabaseRepresentationIfNecessary($queryBuilder, $doctrineTypeField, $values); | ||
| $this->addWhere($queryBuilder, $queryNameGenerator, $associationAlias, $associationField, $values); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| $this->addWhere($queryBuilder, $queryNameGenerator, $associationAlias, $associationField, $values); | |
| if ($values) { | |
| $this->addWhere($queryBuilder, $queryNameGenerator, $associationAlias, $associationField, $values); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cannot be done because if there are multiple invalid UUIDs, there is no filter and all resources are fetched. So that doesn't seem okay to me.
If I add automatically a Symfony Uuid/Ulid constraint in ParameterValidationConstraints or if I rethrow the ConversionException, I could add it.
@see testSearchFilterOnManyToOneRelationByInvalidUuids
5cb992d to
80c9d45
Compare
80c9d45 to
a6b7796
Compare
| "illuminate/support": "^11.0 || ^12.0", | ||
| "jangregor/phpstan-prophecy": "^2.1.11", | ||
| "justinrainbow/json-schema": "^5.2.11", | ||
| "justinrainbow/json-schema": "^6.5.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cherry pick of #7619 to fix the behat tests on main branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've merged 4.2 onto main just now
4733b7d to
3719b08
Compare
soyuka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! a few small comments, let me know if you think its fine like this, I'm concerned about the addWhere being called with a null value.
| ]); | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there's an issue here, I've a suggestion: let's throw an invalid exception here when catching a conversion exception
| $associationField = $associationFieldIdentifier; | ||
| } | ||
|
|
||
| $value = $this->convertValuesToTheDatabaseRepresentation($queryBuilder, $doctrineTypeField, $value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's catch the InvalidArgumentException and log in here so that we never call addWhere with an invalid value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem ok to me because in the case, we will fetch all the items. Is it really necessary to catch the exception and log ? Or, do you want also to rethrow the exception ?
| $this->logger->notice('Invalid value conversion to database representation', [ | ||
| 'exception' => $e, | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd look better to just throw an invalid exception but I'm not sure about the wanted behavior (if one value is bad should we still filter with other values or just ignore everything?)
this comment makes the pair with my other comment below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay to throw an exception. If one value is bad, I think we can throw a 400 error. This case should not occur if the PR #7649 is also merged
| style: 'form', | ||
| explode: false | ||
| ), | ||
| new OpenApiParameter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should check the Parameter definition, as if the user has set this to not be an array we should not document it as such
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I modify the getSchema method ?
public function getSchema(Parameter $parameter): array
{
if ($parameter->getSchema() !== null) {
return $parameter->getSchema();
}
return [
'oneOf' => [
self::UUID_SCHEMA,
[
'type' => 'array',
'items' => self::UUID_SCHEMA,
],
],
];
}
and modify getOpenApiParameters to enabling/disabling the ìd̀ parameter or the id[] parameter according to the schema ? I don't see an example in the existing code
Exemple:
if the user set the schema
'schema' => [
'type' => 'array',
'items' => [
'type' => 'string',
'format' => 'uuid',
],
]
I enable only the id[] parameter
110cbaf to
860bbed
Compare
|
@soyuka I updated the PR, I throw an InvalidArgumentException if there is a conversion exception. When the uuid is invalid, the response is now a bad request response instead of a success response with 0 items. |
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: