core: [UUID] Bug with Ramsey\UUID uuid_binary Doctrine type

If using Ramsey Doctrine UUID uuid_binary datatype, identifiers in motion are classic UUID strings (ie. 5e619188-b5c8-4797-905e-05f4ba6dfc67) but they’re binary at rest (ie. 0xFEFDF85691AE48E2888D39C3EBEC352F).

In IdentifierManagerTrait, a statement tries to convert the user-supplied identifier to a PHP value, which means it tries to convert the input as if it’s binary, NOT string. This fails, obviously. If I change it to convertToDatabaseValue(), it works as expected.

This brings up another question, if the ID provided is not decodable (not a valid UUID, for example), how should that be treated? I guess as a 404, not as a 500.

/cc @teohhanhui

TLDR

GET http://127.0.0.1:8000/api/images/5e619188-b5c8-4797-905e-05f4ba6dfc67

{
    "type": "https://tools.ietf.org/html/rfc2616#section-10",
    "title": "An error occurred",
    "detail": "Could not convert database value \"5e619188-b5c8-4797-9...\" to Doctrine Type uuid_binary"
}

About this issue

Original URL
State: closed
Created 7 years ago
Comments: 33 (26 by maintainers)

Most upvoted comments

Please stop using my fork also 😃

soyuka on Apr 6, 2018

if the ID provided is not decodable (not a valid UUID, for example), how should that be treated? I guess as a 404, not as a 500

For a URL, if the ID is not a valid UUID or the entity of that ID doesn’t exists it should be a 404.

theofidry on Aug 7, 2017

master is for 2.3 so it will not land in any 2.2.x version.

antograssiot on Jun 16, 2018

I see this is not included in 2.2.7 either. Any chance it will be included in 2.2.8? I don’t feel that confident using dev-master. Thanks!

@dunglas will release a beta for 2.3 soon!

soyuka on Jun 16, 2018

JK, I think @dunglas was a great gatekeeper for this feature.

Thank you for sticking with this. ❤️

dkarlovi on Apr 6, 2018

@teohhanhui convertToPHPValue expects the input to be in database format which it isn’t if’s coming from the user.

dkarlovi on Aug 14, 2017

Hi @michaelfeinbier yes, it’s still not in 2.2.6 release, use "api-platform/core": "dev-master" - it’s working

Perf on Jun 6, 2018

@soyuka amazing to get this merged, looked like @dunglas doesn’t like you very much in that PR 😄

😆 why you say that, it’s a really sensitive part of the code therefore it needed to be reviewed a lot. Also I’ve simplified it a lot thanks to the reviewers 😃.

soyuka on Apr 6, 2018

@soyuka amazing to get this merged, looked like @dunglas doesn’t like you very much in that PR 😄

dkarlovi on Apr 6, 2018

I’m not familiar with this part of the core but this bug makes sense to me.

Type::convertToPHPValue() method allows Doctrine to normalize the data in PHP from the output of the database, therefore it expects in our case to receive many bytes. So, IMO, we don’t use properly this method because we should not use it to normalize data from a client.

@api-platform/core-team why don’t we use the serializer to denormalize IDs here? For example in our case, this could allow us to write an UuidBinaryNormalizer to transform our stringified uuid to an Uuid instance. But maybe I’m totally wrong (because I admit it, it’s weird to denormalize a value that comes from the URI)…

meyerbaptiste on Aug 7, 2017