-
-
Notifications
You must be signed in to change notification settings - Fork 865
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
[RFC] Native UID support in search filter #5618
base: main
Are you sure you want to change the base?
[RFC] Native UID support in search filter #5618
Conversation
That's good news, but there's a final boss to fight: the CI! 😱 |
yes, the behat failure on abstract tests is related to symfony/symfony#50480 waiting for it to be released... The test on the Maker also needs a fix. Just add your test and only test yours :) |
Sorry for acting as a Behat newbie (I am one), but I can't get the test to pass. It looks like my
|
@createSchema | ||
Scenario: Get collection by ulid 01H2ZS93NBKJW5W4Y01S8TZ43M | ||
Given there is a UidBasedId resource with id "01H2ZS93NBKJW5W4Y01S8TZ43M" | ||
When I send a "GET" request to "/uid-based-ids?id=/uid-based-ids/01H2ZS93NBKJW5W4Y01S8TZ43M" |
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.
Shouldn't it just be
When I send a "GET" request to "/uid-based-ids?id=/uid-based-ids/01H2ZS93NBKJW5W4Y01S8TZ43M" | |
When I send a "GET" request to "/uid-based-ids?id=01H2ZS93NBKJW5W4Y01S8TZ43M" |
?
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.
Passing just the id doesn't work on my side for some reason I have to figure out - anyway passing the IRI should work, or at least return an empty collection with a 200 status
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.
almost there
@createSchema | ||
Scenario: Get collection by ulid 01H2ZS93NBKJW5W4Y01S8TZ43M | ||
Given there is a UidBasedId resource with id "01H2ZS93NBKJW5W4Y01S8TZ43M" | ||
When I send a "GET" request to "/uid-based-ids?id=/uid-based-ids/01H2ZS93NBKJW5W4Y01S8TZ43M" |
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.
When I send a "GET" request to "/uid-based-ids?id=/uid-based-ids/01H2ZS93NBKJW5W4Y01S8TZ43M" | |
When I send a "GET" request to "/uid_based_ids?id=/uid-based-ids/01H2ZS93NBKJW5W4Y01S8TZ43M" |
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.
Thanks for spotting this! I've been using kebab-case for years, I didn't remember it was not the standard 🙃
your commit could be: |
c1fe7bb
to
61be694
Compare
Well, should work better now. Just 1 annoying thing: when using UIDs, you cannot use the SearchFilter with an ID, only an IRI. This method is only aware of the raw value. Ideally it should know:
Sounds relatively complex to set up - unless you know a better approach? |
Yes its a hard problem :/ especially because we don't have a query parameter sanitizer, it'll be better if you knew it was an IRI or not for starters :/. I wish we could have the identifier Type in that method, and we'd have to find out wheter its an IRI or not before getting its value. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Forgot this PR...does this fix your issue? #5771 |
Sorry still didn't take the time to review this - will have a look ASAP! |
Sorry, i misunderstood your use case - the UID is actually the orm id for you, which i didn't account for in my PR! I guees both are actually relevant, and I even might have to copy what you're doing here in some part of mine. |
One thing I'm not sure about : wouldn't using `type: 'symfony_uuid' in your doctrine column definition help? You're using 'ulid' in your test, but I remember having to use symfony_uuid in mine for this to work. |
Hello @bpolaszek @mrossard @soyuka , Do you have any updates about this PR? Facing this problem in a project that is migrating from ids to UUIDs. |
I haven't been able to work on this for the last few weeks, sorry. |
that could help make things clearer, but that would break a lot of things, wouldn't it? |
Thank you very much for your answers and your work @soyuka and @mrossard . Hope we can see those changes soon. Meanwhile, in case some other people end up here after a Google Search as I did, what is the suggested way to implement now a Search Filter on a foreign key that is stored as a binary UUID? Typical case of filtering a Book entity by Author UUID fr example. Thanks in advance! |
Here you go: <?php
declare(strict_types=1);
namespace App\State\Filter;
use ApiPlatform\Doctrine\Orm\Filter\AbstractFilter;
use ApiPlatform\Doctrine\Orm\Util\QueryNameGeneratorInterface;
use ApiPlatform\Metadata\Operation;
use Doctrine\ORM\QueryBuilder;
use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
use Symfony\Component\Uid\Ulid;
use Throwable;
use function array_map;
use function is_array;
use function sprintf;
use function str_ends_with;
use function Symfony\Component\String\u;
final class UlidFilter extends AbstractFilter
{
/**
* @param string|string[]|null $value
*/
protected function filterProperty(
string $property,
mixed $value,
QueryBuilder $queryBuilder,
QueryNameGeneratorInterface $queryNameGenerator,
string $resourceClass,
Operation $operation = null,
array $context = [],
): void {
if (
null === $value
|| !$this->isPropertyEnabled($property, $resourceClass)
|| !$this->isPropertyMapped($property, $resourceClass, true)
) {
return;
}
$alias = $queryBuilder->getRootAliases()[0];
$valueParameter = ':' . $queryNameGenerator->generateParameterName($property);
$aliasedField = sprintf('%s.%s', $alias, $property);
if (is_array($value)) {
$values = array_map(
fn (string $value) => $this->convertUlidFromStringToBinary($property, $value),
$value
);
$queryBuilder
->andWhere($queryBuilder->expr()->in($aliasedField, $valueParameter))
->setParameter($valueParameter, $values);
return;
}
$value = $this->convertUlidFromStringToBinary($property, $value);
$queryBuilder
->andWhere($queryBuilder->expr()->eq($aliasedField, $valueParameter))
->setParameter($valueParameter, $value);
}
private function convertUlidFromStringToBinary(string $property, string $value): string
{
if ('/' === ($value[0] ?? '')) {
$value = (string) u($value)->afterLast('/');
}
try {
return Ulid::fromString($value)->toBinary();
} catch (Throwable) {
throw new BadRequestHttpException("`{$property}`: Unable to parse Ulid `{$value}`");
}
}
/**
* @return array<string, array<string, mixed>>
*
* @codeCoverageIgnore
*/
public function getDescription(string $resourceClass): array
{
if (!$this->properties) {
return [];
}
$description = [];
foreach ($this->properties as $property => $strategy) {
if (
!$this->isPropertyEnabled($property, $resourceClass)
|| !$this->isPropertyMapped($property, $resourceClass, true)
) {
continue;
}
/** @var string[] $filterParameterNames */
$filterParameterNames = [$property, "{$property}[]"];
foreach ($filterParameterNames as $filterParameterName) {
$description[$filterParameterName] = [
'property' => $property,
'type' => 'string',
'required' => false,
'is_collection' => str_ends_with($filterParameterName, '[]'),
];
}
}
return $description;
}
} Usage: #[ApiFilter(UlidFilter::class, properties: ['author' => null])] |
This is awesome, I would like this to be part of API Platform but I'd also like for it not to be added to the Search filter... It's not possible right now I'll work on finding a way to do so. |
Thanks a lot @bpolaszek ! |
@bpolaszek can you update this? I would like to merge that in the end. |
I won't have the time to tackle this until a week or two, I'll do my best. |
61be694
to
477f322
Compare
I tried a quick rebase let's see. |
477f322
to
5a29594
Compare
5a29594
to
6c085ee
Compare
PostgreSQL and MongoDB have issues with the fix, if someone is willing to take a look we'll gladly merge this. |
Hello there,
Given:
symfony/uid
is natively supported in API-Platform as of Add Symfony Uid support #3715SearchFilter
currently doesn't work with Ulids as they're serialized as strings instead of binaries when used in DQL queriesSearchFilter
, specifically for handling Ulids, which is tedious and has to be done for every project, without any kind of standardizationI know there have been lots of dicussions, even PRs in the past regarding this, at a time when the UID component was more or less experimental and not a lot of people used it, but I think things have changed since then.
Is it something you could reconsider?