IntegerType returns unexpected results with UUID

[sdustinh]

This is a (multiple allowed):

• bug
• enhancement
• feature-discussion (RFC)

sdustinh@sdustinh:~/github/app$ php --version
PHP 7.1.20-1+ubuntu16.04.1+deb.sury.org+1 (cli) (built: Jul 25 2018 10:06:40) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.1.0, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.1.20-1+ubuntu16.04.1+deb.sury.org+1, Copyright (c) 1999-2018, by Zend Technologies
    with Xdebug v2.6.0, Copyright (c) 2002-2018, by Derick Rethans
sdustinh@sdustinh:~/github/app$ mysql --version
mysql  Ver 14.14 Distrib 5.7.23, for Linux (x86_64) using  EditLine wrapper
sdustinh@sdustinh:~/github/app$ composer show cakephp/*
cakephp/bake                1.6.4  Bake plugin for CakePHP 3
cakephp/cakephp             3.5.17 The CakePHP framework
cakephp/cakephp-codesniffer 3.0.5  CakePHP CodeSniffer Standards
cakephp/chronos             1.2.2  A simple API extension for DateTime.
cakephp/debug_kit           3.14.2 CakePHP Debug Kit
cakephp/elastic-search      0.3.5  An Elastic Search datasource and data mapper for CakePHP 3.0
cakephp/migrations          1.7.2  Database Migration plugin for CakePHP 3.0 based on Phinx
cakephp/plugin-installer    1.1.0  A composer installer for CakePHP 3.0+ plugins.

I understand that this is arguably a bug, but in the course of legitimate usage it has caused some problems so I'm listing it as a bug for this issue. The IntegerType.php has a small but really annoying bug that pops up occasionally in my application and it a pain to track down every time. If you pass a UUID to query conditions where the field is an integer the IntegerType will convert the UUID to an integer and return unexpected results. Consider the following:

// UsersTable.php
public function findUser(Query $query, array $options = []): Query
{
    return $query->where([
        'OR' => [
            [$this->aliasField('id') => $options['id']],
            [$this->aliasField('uuid') => $options['id']],
        ],
    ]);
}

// Using the finder
$users = $this->Users->find('user', ['id' => '6a88accf-a34e-4dd9-ade0-8d255ccaecbe'])->toArray();

In the above example, I use toArray() to illustrate the point but first() would return the incorrect record as well. The following results are returned...

$users = [
    [
        'id' => 6,
        'uuid' => 'c52a67e7-87fa-408d-b819-ccb4033f7372',
    ],
    [
        'id' => 213,
        'uuid' => '6a88accf-a34e-4dd9-ade0-8d255ccaecbe',
    ],
];

This is a real example that has happened up at least 2 times in my application. What happens is when the UUID is converted to an integer by php - if the UUID happens to begin with an integer - PHP is returning numeric values to until it reaches non-numeric values.

$uuid = '6a88accf-a34e-4dd9-ade0-8d255ccaecbe';
(int)$uuid === 6; // true

This causes application code to return very unexpected results. It's actually so obscure that it makes writing a test to check for it very difficult, too. The first time it popped up I just fixed it and moved on, but it is starting to happen more when I want to use id and uuid interchangeably.

I haven't experienced it with any of the other database types, but I imagine this bug might present itself wherever a cast is being performed. For example using on or off in the BoolType could be caught with filter_var($value, FILTER_VALIDATE_BOOLEAN).

If anyone else has this problem, hopefully you've used a finder because the solution can be a little verbose. Alternatively, you can extend the IntegerType and patch it yourself.

public function findUser(Query $query, array $options = []): Query
{
    $conditions = [];

    foreach ((array)$options['id'] as $id) {
        $field = (is_numeric($id) === true) ? 'id' : 'uuid';
        $conditions[] = [$this->aliasField($field) => $id];
    }

    return $query->where(['OR' => $conditions]);
}

[VarCI-bot]

sdustinh, please include the CakePHP version number you are using in your description. It helps us debug your issue.

--
Automated response by Var.CI 🤖

[ADmad]

We could change the check in IntegerType::toDatabase() from is_scalar() to is_numeric() instead, so that an exception is thrown instead of value being implicitly truncated.

[ADmad]

IntegerType::manyToPHP() also does casting to int and could cause the same issue.

[lorenzo]

How do you use both types interchangeably?

[sdustinh]

@lorenzo URLs mostly. Super simple example to illustrate the point without detracting from the issue...

I have records that were created with primary keys that have a UUID (or some other unique string) on them. Then there are printed labels that were intended to be consumed with a barcode scanner (inventory system). This lets me print out a label using a UUID which references a record which could be rotated out at some point in the future (e.g. A piece of hardware dies and needs to be swapped out). The scanner can always hit a single URL like /devices/:uuid. Well, these UUIDs frequently end up in spreadsheets or pasted into searches which check against many fields.

The example initially reported by this issue is really specific to UUID but can be reproduced with any string that PHP would try to cast:

• IPv4: (int)"6.7.8.9"
• MAC Address: (int)"06:41:ea:3a:12:37"
• Serial numbers: (int)"06N2244"

As the application grows and there are more integrations (e.g. Saltstack, customer APIs, developers in other departments, etc) which want to reference the same record in different ways:

// DevicesTable.php
public function findDevice(Query $query, array $options = []): Query
{
    return $query->where([
        'OR' => [
            [$this->aliasField('id IN') => (array)$options['device']],
            [$this->aliasField('uuid IN') => (array)$options['device']],
            [$this->aliasField('label IN') => (array)$options['device']],
            [$this->aliasField('serial_number IN') => (array)$options['device']],
            [$this->aliasField('ip_assignment IN') => (array)$options['device']],
        ],
    ]);
}

// DevicesController.php
public function search()
{
    $queryParams = $this->request->getQueryParams();
    $devices = $this->Devices->find('all', $queryParams)->limit(10);
    $this->set('devices', $devices);
    $this->set('_serialize', ['devices']);
}

// DeviceUuidMiddleware.php
public function __invoke(ServerRequestInterface $request, ResponseInterface $response, callable $next): ResponseInterface
{
    if ((bool)preg_match('^/\/devices\/' . Router::UUID . '/', $request->getRequestTarget()) !== false) {
        $request = $this->replaceDeviceUuid($request); // /devices/6a88accf-a34e-4dd9-ade0-8d255ccaecbe -> /devices/1
    }

    return $next($request, $response);
}

The application itself is only ever concerned with the primary key, but the device can be accessed using any of the other unique references to it in the same way as the primary key.

[lorenzo]

The best we can do is to throw an exception, which by the looks of what you are doing, is definitely not what you want, as it would break your nice finder.

[sdustinh]

Definitely throw the exception - that is what is best for the framework. My application should not dictate what is best for the framework. I'll handle relevant changes on my end.

[markstory]

@sdustinh Any interest in putting together a pull request for this?

[sdustinh]

@markstory Of course. Just the changes discussed? is_scalar() -> is_numeric() okay for everyone?

[sdustinh]

@markstory On the manyToPHP() do you want to continue or throw the exception?

[sdustinh]

I went ahead and just piggy backed the continue. If you want me to change it, let me know and I'll update the PR.