[UUID] Bug with Ramsey\UUID uuid_binary Doctrine type

[dkarlovi]

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"
}

[theofidry]

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.

[meyerbaptiste]

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)...

[dkarlovi]

@meyerbaptiste a proper way to fix this might be to use the serializer, yes. Especially since the instance supports it.

[soyuka]

@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)...

In fact, this is called from the ItemDataProvider which is linked to Doctrine. Indeed, it checks for the Doctrine identifiers to match them with the query parameter.

The type transformation was added when fixing types. Though I don't see a reason to transform an id to a Doctrine Type. Also, you've a point where the data doesn't come from the database.

We're sending a raw query attribute from the ReadListener.
Maybe we should denormalize it before hitting DataProviders?

[teohhanhui]

convertToPHPValue is indeed what we want - the identifier in the correct PHP type.

Can you help us understand why the conversion is failing?

EDIT: Uh, okay. I see what you mean now...

[dkarlovi]

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

[teohhanhui]

We're sending a raw query attribute from the ReadListener.
Maybe we should denormalize it before hitting DataProviders?

That sounds reasonable. But I don't know if we have enough information to do it at that layer...

[dkarlovi]

BTW this is tagged as a question, but it's a bug.

[dkarlovi]

Does anyone have an idea how to move forward here? I might send a PR to fix it if it's not too complex.

[soyuka]

@dkarlovi as I see it we need some kind of URLIdentiferDenormalization of the URL query parameters before they reach the ReadListener (see my comment above). Meaning that identifiers should be transformed to their correct type (in this case uuid, but there's also composite identifier, numbers, dates - can a date be an identifier? etc.).

[dkarlovi]

@soyuka I see, so would it use the serializer or have custom strategies? Or both?

[soyuka]

I don't think we can use the serializer it's not meant for that. I'm just using the same wording because it's a kind of identifier normalization. Currently it's done in src/Bridge/Doctrine/Orm/Util/IdentifierManagerTrait.php. Still, not sure if it's the best thing that this is related to Doctrine.

I mean transforming the query parameter from foo=1;bar=2 to ['foo' => 1, 'bar' => 2] should definitely not be part of doctrine scope but should be available for any data provider.

Maybe we should do what we can before it reaches our data providers, here:

core/src/EventListener/ReadListener.php

Line 103 in 1835269

$id = $request->attributes->get('id');

This also reminds me of #1376 (comment) (still not sure how I can put this).