michaeldyrynda / laravel-model-uuid

This package allows you to easily work with UUIDs in your Laravel models
MIT License
451 stars 47 forks source link

`scopeWhereUuid` returns an error when passed an invalid UUID when using PostgresSQL's `uuid` data type #114

Closed MrThePlague closed 2 years ago

MrThePlague commented 2 years ago

If using this package on a base Laravel application, a query using the scopeWhereUuid will return null if regardless of whether the uuid parameter(s) passed to it are valid UUIDs, which means that a ModelNotFoundException is returned when using the findOrFail() method. e.g.:

MariaDB/MySQL base

Post::whereUuid('this-is-not-valid')->first();
> null

Post::whereUuid('this-is-not-valid')->firstOrFail();
> Illuminate\Database\Eloquent\ModelNotFoundException

However, if you're using the native uuid data type in Postgres, this instead returns a QueryException with either the first() or firstOrFail() methods:

PostgresSQL UUID

Post::whereUuid('this-is-not-valid')->first();
> Illuminate\Database\QueryException with message 'SQLSTATE[22P02]: Invalid text representation: 7 ERROR:  invalid input syntax for type uuid: "this-is-not-valid" (SQL: select * from "recipes" where "recipes"."uuid" in (this-is-not-valid) limit 1)'

A similar behaviour exists if you're using the EfficientUuid, but instead you get an nvalidUuidStringException.

MariaDB/MySQL w/ EfficientUuid

Post::whereUuid('this-is-not-valid')->first();
> Ramsey\Uuid\Exception\InvalidUuidStringException with message 'Invalid UUID string: this-is-not-valid

BindOnUuid

Normally this behaviour isn't a huge deal; however, when you're using the BindsOnUuid trait these last two examples return a 500, instead of what you'd (at least I'd?) expect, a 404.

I'm happy to PR a quick Uuid::isValid() check into the Trait itself, but I wonder if it might be better to move that logic into the scope so that the *orFail() methods work as intended? That being said, I'm not 100% sure what the best way of tackling the latter would be. Thoughts?

michaeldyrynda commented 2 years ago

I think it makes sense to have a consistent handling here.

It might be worth starting by wrapping this array_map (or extracting a new normaliseUuid method or similar) in an array_filter that uses Uuid::isValid to remove any invalid UUIDs.

One thing you'll need to consider is the implications of that array being empty, as Post::whereUuid([])->first() is undesirable.

MrThePlague commented 2 years ago

One thing you'll need to consider is the implications of that array being empty, as Post::whereUuid([])->first() is undesirable.

Thanks for the reply! Are you able to help me understand why this would be undesirable? I think would have the desired effect, but I'm probably missing something.

michaeldyrynda commented 2 years ago

Actually, perhaps it will work out ok. Once upon a time, this would have just returned the first record in the database.

Seems now it will run select * from <table> where 0 = 1, so you shouldn't get any unexpected records back 👍