Framework: Route model binding parameter error passing a string with Postgres

[mariohbrino]

• Laravel Version: 5.7.9
• PHP Version: 7.2.10
• Database Driver & Version: PostgreSQL 10.5

Description:

I noticed that the route model binding exists an error passing a string variable when using the Postgres database, this does not happen with the MySQL database.
When I use the Postgres database, the route model binding if pass a parameter string it don't abort and build the query with a string. When I use the MySQL database it abort and show error 404 the route is not found.

Steps To Reproduce:

Records

id | name
--- | ---
1 | John
2 | Jean
3 | Lian

Routes

Route::get('/user/{user}', 'UserController@show');

UserController

public function show(User $user) {
    return $user;
}

url abort

localhost/user/4

url does not abort

localhost/user/test

Error

SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer: "test" (SQL: select * from "users" where "id" = test limit 1)

[derekmd]

https://laravel.com/docs/5.7/routing#route-parameters "Regular Expression Constraints"

Route::get('/user/{user}', 'UserController@show')->where('user', '[0-9]+');

Or in RouteServiceProvider@boot():

Route::pattern('user', '[0-9]+');

URI user/test wouldn't match any routes so a 404 HTTP status code would be returned.

[dwightwatson]

That would do the trick, sure, but it feels weird that you need to consider it (and litter your routes file with constraints) just because you're using Postgres.

[staudenmeir]

Do you have an idea for a fix?

We could wrap the query in a try/catch block and throw a ModelNotFoundException, but I don't think that's ideal. This makes it harder debug other issues like missing tables or misspelled columns.

Or we use the model's $keyType and check integer keys with ctype_digit() (if getRouteKeyName() === getKeyName()).

[mariohbrino]

I don't know if it's the best solution and where is the right place to validate it. I made a change into the /Iluminate/Routing/ImplicitRouteBinding.php on method resolveForRoute and it work.

This works including when is customized the key name with the method getRouteKeyName()

Solution:

Add after line 33 where have $instance = $container->make($parameter->getClass()->name);

// Abort if the route key name is equal to model key name and if the parameter value is not numeric
if ($instance->getRouteKeyName() === $instance->getKeyName() && ! ctype_digit($parameterValue)) {
      abort(404);
}

[mariohbrino]

I think the right way should be this.

// Throw exception ModelNotFoundException if the route key name is equal to model key name and if the parameter value is not numeric
if ($instance->getRouteKeyName() === $instance->getKeyName() && ! ctype_digit($parameterValue)) {
      throw (new ModelNotFoundException)->setModel(get_class($instance));
}

[staudenmeir]

You also have to check the $keyType for int/integer. Otherwise, this breaks string primary keys.

[mariohbrino]

Should be this the final result?

Thanks.

// Throw exception ModelNotFoundException if the route key name is equal to model key name and if the parameter value is not numeric
if ($instance->getRouteKeyName() === $instance->getKeyName() && ! ctype_digit($parameterValue) && $instance->getKeyType() === 'int') {
      throw (new ModelNotFoundException)->setModel(get_class($instance));
}

[staudenmeir]

I would use in_array($instance->getKeyType(), ['int', 'integer']) to cover both options.

[driesvints]

Feel free to send in a PR when you figure this out.

[dwightwatson]

I don’t think it’s safe to check keyType as that’s the type of the auto-incrementing key, not the routable key. If you’re using a string slug as the routable parameter alongside an auto-incrementing ID then I’m not sure the above solutions would work.

[derekmd]

You'll find Postgres has similar "Invalid text representation" exceptions for UUID columns that don't allow any SQL integer or string value in the SELECT parameter binding. So RouteServiceProvider does turn into a regex fest:

namespace App\Providers;

use Illuminate\Foundation\Support\Providers\RouteServiceProvider as ServiceProvider;
use Illuminate\Support\Facades\Route;

class RouteServiceProvider extends ServiceProvider
{
    protected $namespace = 'App\Http\Controllers';

    protected const MODEL_ID_BINDINGS = [
        'id',
        'notification',
        // ...
    ];

    protected const MODEL_UUID_BINDINGS = [
        'uuid',
        'photo',
        'user',
        // ...
    ];

    public function boot()
    {
        Route::patterns(array_merge(
            array_fill_keys(self::MODEL_ID_BINDINGS, '[0-9]{1,10}'),
            array_fill_keys(self::MODEL_UUID_BINDINGS, '[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}'),
            [
                // ...
            ]
        ));

        parent::boot();
    }

Instead of inferring from Model@getKeyType() A possible model-focused solution is to add getRouteKeyType() to indicate the type or pattern of getRouteKeyName():

public function getRouteKeyName()
{
    return 'uuid';
}

public function getRouteKeyType()
{
    return '[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}';
}

public function getRouteKeyName()
{
    return 'id';
}

public function getRouteKeyType()
{
    return 'int';
}

Now route pattern matching is spread across many model classes instead of all being located in RouteServiceProvider. However it would allow a project base model class to define everything in one spot without having to name the route bindings:

namespace App;

use Illuminate\Database\Eloquent\Model as BaseModel;

class Model extends BaseModel
{
    public function getRouteKeyType()
    {
        switch ($this->getRouteKeyName()) {
            case 'id':
                return '\d+';
            case: 'uuid':
                return '[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}';
            default:
                break; // no pattern enforced
        }
    }
}

I truthfully don't see many problems with the current implementation other than a dozen or so explicit regex pattern lines? Do the docs just need explicit instructions for Postgres columns?

[staudenmeir]

@dwightwatson mariohbrino's suggestion is checking that:

`$instance->getRouteKeyName() === $instance->getKeyName()`

[matthijs]

What about the following. Add a method to eloquent model 'getKeyValidator()' which returns a validator rule and use that for validation?

In this case you have the validation where it belongs and you can make use of any validation that is supported by laravel.

[moosapour-arash]

any news on this ?

[taylorotwell]

Documenting the usage of Route::pattern in your RouteServiceProvider is the solution to this. All other solutions seem fairly weak. Documentation PR's welcome.