Skip to content

Handle binary UUID in SearchFilter #3774

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.

Sign up for GitHub

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

Jump to bottom
Merged
alanpoulain merged 5 commits into from
Feb 19, 2021
Merged

Handle binary UUID in SearchFilter #3774

alanpoulain merged 5 commits into from
Feb 19, 2021

Conversation

@odoucet
Copy link
Contributor

@odoucet odoucet commented Oct 20, 2020 •
edited
Loading

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tickets fixes api-platform/api-platform#1139
License MIT

When searching on a binary UUID field, current behaviour leads to search done on Doctrine with the string UUID and not the binary version of it.
This is because the field type must be provided to Doctrine each time we use QueryBuilder. Why ? Because Doctrine sees string on input, and does not know it needs conversion.

This PR reads $metadata->getTypeOfField() and force field type.

WARNING
Current PR passes the unit tests (so it does not break current behaviour), but does not implement two new needed tests (to make sure it fixes the issue mentioned) :

  • search on a single UUIDBinary
  • search with multiple UUIDBinary values

Test has been done on a local project so I know it works, but better to check this with a test :)

I need some help adding custom type to SearchFilterTestTrait as uuidBinary is not recognized.

@alanpoulain
Copy link
Member

alanpoulain commented Oct 20, 2020

Since it's a bugfix, it should target 2.5.

alanpoulain
alanpoulain reviewed Oct 20, 2020
View reviewed changes
odoucet
odoucet commented Oct 20, 2020
View reviewed changes
Copy link
Contributor Author

@odoucet odoucet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used a less generic parameter name ... Tell me if it's the right way to do.

@odoucet odoucet changed the title WIP - Handle binary UUID in SearchFilter Handle binary UUID in SearchFilter Oct 22, 2020
dunglas
dunglas requested changes Oct 22, 2020
View reviewed changes
@odoucet odoucet requested a review from dunglas October 24, 2020 10:35
@soyuka
Copy link
Member

soyuka commented Nov 8, 2020

Nice patch, please target 2.5

@soyuka soyuka added the Ready label Nov 8, 2020
@odoucet
Copy link
Contributor Author

odoucet commented Nov 9, 2020

Updated to use QueryNameGenerator for variable names.

alanpoulain
alanpoulain reviewed Nov 9, 2020
View reviewed changes
src/Bridge/Doctrine/Orm/Filter/SearchFilter.php Outdated
for ($i = 0; $i < $nbArgs; ++$i) {
$inQuery[] = ':filterproperty'.$i;
$parameters->add(new Parameter('filterproperty'.$i, $caseSensitive ? $values[$i] : strtolower($values[$i]), $type));
$args = [];
Copy link
Member

@alanpoulain alanpoulain Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not always using this conditional branch instead of having an if / else?
It will:

  • make the code easier to read (and maintain),
  • prevent bugs to arise only in one conditional branch.

WDYT @soyuka @dunglas?

Copy link
Contributor Author

@odoucet odoucet Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will require updating some tests expectations because when multiple values are given, they are set as one unique array parameter, and that will become several parameters. See https://github.com/api-platform/core/pull/3774/checks?check_run_id=1379433208#step:14:91

I'm currently blocked at adding UUID tests, I cant fully understand yet how tests are organized and how can I add custom types.

\Doctrine\DBAL\Types\Type::addType('uuid', \Ramsey\Uuid\Doctrine\UuidType::class);

is required to manipulate uuid types.

alanpoulain
alanpoulain reviewed Nov 10, 2020
View reviewed changes
@odoucet
Copy link
Contributor Author

odoucet commented Nov 10, 2020

I've pushed an attempt to add test on UUID field, but the current test failed because SearchFilterTest::testGetDescriptionDefaultFields() failed on $filter->getDescription : it seems the uuid type is never loaded, despite adding it in DoctrineOrmFilterTestCase:setUp().
I need help on this one... or I can update my branch with no additional test and it can be merged right away.

@odoucet odoucet mentioned this pull request Jan 21, 2021
Closed
@soyuka
Copy link
Member

soyuka commented Jan 22, 2021

if you need more help @odoucet ping me on symfony's slack!

Base automatically changed from master to main January 23, 2021 21:59
@odoucet
Copy link
Contributor Author

odoucet commented Feb 8, 2021

I'll update my PR with latest master branch in a few days.
I need help on how to provide test code for this (I did not understand how to create custom doctrine types in unit tests).

@odoucet
Copy link
Contributor Author

odoucet commented Feb 11, 2021

Hello,
I've updated my pull request to be up to date with latest master.
This MR lacks specific unit tests because I was unable to add a binary doctrine type to tests. @soyuka if you can help, it would be great :)
This MR can also be merged as-is, I know it works because I use binary type in a private project.

@alanpoulain alanpoulain changed the base branch from main to 2.6 February 19, 2021 12:34
@alanpoulain alanpoulain force-pushed the uuidbin-search branch 2 times, most recently from 4ded0f9 to 16dd558 Compare February 19, 2021 12:39
@alanpoulain
Copy link
Member

alanpoulain commented Feb 19, 2021

Hello @odoucet,
Adding PHPUnit tests (they are integration tests, not really unit ones) is not possible since the conversion is done at the execution.
I've added Behat tests instead.

@alanpoulain alanpoulain merged commit ff248ae into api-platform:2.6 Feb 19, 2021
@alanpoulain
Copy link
Member

alanpoulain commented Feb 19, 2021

Thank you @odoucet.

@odoucet odoucet deleted the uuidbin-search branch March 2, 2021 09:53
@alanpoulain alanpoulain mentioned this pull request Mar 4, 2021
Closed
@bendavies bendavies mentioned this pull request Mar 12, 2021
Closed
soyuka added a commit to soyuka/core that referenced this pull request Mar 13, 2021
@soyuka
Revert "Handle binary UUID in SearchFilter (api-platform#3774)"
ef89083 
This reverts commit ff248ae.
soyuka added a commit to soyuka/core that referenced this pull request Mar 13, 2021
@soyuka
Revert "Handle binary UUID in SearchFilter (api-platform#3774)"
db90e1c 
This reverts commit ff248ae.
alanpoulain pushed a commit to soyuka/core that referenced this pull request Mar 16, 2021
@soyuka @alanpoulain
Revert "Handle binary UUID in SearchFilter (api-platform#3774)"
b29548d 
This reverts commit ff248ae.
alanpoulain added a commit that referenced this pull request Mar 16, 2021
@soyuka @alanpoulain
SearchFilter: fix regression in 2.6.3 (#4134)
e74bf8e 
* Revert "Handle binary UUID in SearchFilter (#3774)"

This reverts commit ff248ae.

Co-authored-by: Alan Poulain <contact@alanpoulain.eu>
@alanpoulain alanpoulain mentioned this pull request Jun 9, 2021
Closed
@alanpoulain alanpoulain mentioned this pull request Jun 23, 2021
Closed
@soyuka soyuka mentioned this pull request Jun 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alanpoulain alanpoulain alanpoulain left review comments

@dunglas dunglas Awaiting requested review from dunglas

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SearchFilter doesnt transform uuid parameter to binary, instead uses string represenation in sql

4 participants

@odoucet @alanpoulain @soyuka @dunglas