Skip to content
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

fix(ulid): convert Ulid object to binary for ORM search filter #4313

Closed
wants to merge 1 commit into from
Closed

fix(ulid): convert Ulid object to binary for ORM search filter #4313

wants to merge 1 commit into from

Conversation

misaert
Copy link

@misaert misaert commented Jun 9, 2021

Q A
Branch? 2.6
Tickets N/A
License MIT
Doc PR N/A

When I want filter a resource by a relation property defined by an Ulid identifier, it doesn't work. This PR adds the conversion to binary before calling the query builder of Doctrine (as explained on the Symfony documentation: https://symfony.com/doc/current/components/uid.html#storing-ulids-in-databases).

Example:

Given the entity `Fruit` has a relation `ManyToOne` with the entity `Producer`
And the ID for each entity is Ulid type
And I have one producer with Ulid `01F7RK5K7FE9V0WPT8AGXS2C2J` who has a fruit
When I call the API `GET /api/fruits?producer=/api/producers/01F7RK5K7FE9V0WPT8AGXS2C2J`
Then I want to have one fruit as result

Currently, the result is 0 because the query builder uses the string representation that does not correspond to the binary stored in the database.

It is my first contribution. I don't know if I have to add a test for it and how, and I don't know if I have to change the Changelog file right now.

Don't hesitate to help me to improve this PR 🙂

@alanpoulain
Copy link
Member

Hello,
Thanks for the PR but we don't really want to handle binary UUID like this.
We tried to have a general approach (#3774) but it has created a lot of issues and has been reverted (#4134).
As I said in this comment (#4134 (comment)), we don't want to have a whitelist like you added in the search filter and we recommend to create your own search filter instead.

@misaert
Copy link
Author

misaert commented Jun 9, 2021

Hello,
Thanks for the PR but we don't really want to handle binary UUID like this.
We tried to have a general approach (#3774) but it has created a lot of issues and has been reverted (#4134).
As I said in this comment (#4134 (comment)), we don't want to have a whitelist like you added in the search filter and we recommend to create your own search filter instead.

Hello,

Thanks for the quick answer. I understand. I hope a generic solution will be found in the future. For now, I am following your recommandation with my own search filter on my project.

@soyuka
Copy link
Member

soyuka commented Jun 10, 2021

Closing, we'll attempt something to fix this properly but it'll probably invoke using a new filter or a complete rewrite of the SearchFilter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants