Skip to content

[12.x] Return unprocessable response when parameter is not a valid UniqueStringId #54033

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

namespace Illuminate\Database\Eloquent\Concerns;

use Illuminate\Database\Eloquent\ModelNotFoundException;
use Illuminate\Database\Eloquent\InvalidIdFormatException;

trait HasUniqueStringIds
{
Expand Down Expand Up @@ -99,10 +99,10 @@ public function getIncrementing()
* @param string|null $field
* @return never
*
* @throws \Illuminate\Database\Eloquent\ModelNotFoundException
* @throws \Illuminate\Database\Eloquent\InvalidIdFormatException
*/
protected function handleInvalidUniqueId($value, $field)
{
throw (new ModelNotFoundException)->setModel(get_class($this), $value);
throw (new InvalidIdFormatException)->setModel(get_class($this), $value, $field);
}
}
74 changes: 74 additions & 0 deletions src/Illuminate/Database/Eloquent/InvalidIdFormatException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
<?php

namespace Illuminate\Database\Eloquent;

use Illuminate\Support\Arr;

/**
* @template TModel of \Illuminate\Database\Eloquent\Model
*/
class InvalidIdFormatException extends \RuntimeException
Comment on lines +6 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense to extend the exception from \Symfony\Component\HttpKernel\Exception\HttpException then?

Suggested change
/**
* @template TModel of \Illuminate\Database\Eloquent\Model
*/
class InvalidIdFormatException extends \RuntimeException
use Symfony\Component\HttpKernel\Exception\HttpException;
/**
* @template TModel of \Illuminate\Database\Eloquent\Model
*/
class InvalidIdFormatException extends HttpException

{
/**
* Name of the affected Eloquent model.
*
* @var class-string<TModel>
*/
protected $model;

/**
* The affected model IDs.
*
* @var array<int, int|string>
*/
protected $ids;

/**
* Set the affected Eloquent model and instance id.
*
* @param class-string<TModel> $model
* @param array<int, int|string>|int|string $ids
* @param string|null $field
* @return $this
*/
public function setModel($model, $ids = [], ?string $field = null)
{
$this->model = $model;
$this->ids = Arr::wrap($ids);
Comment on lines +34 to +37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you type-hint $ids, you can simply do:

Suggested change
public function setModel($model, $ids = [], ?string $field = null)
{
$this->model = $model;
$this->ids = Arr::wrap($ids);
public function setModel($model, array|int|string $ids = [], ?string $field = null)
{
$this->model = $model;
$this->ids = (array) $ids;


$this->message = 'Invalid key';
if ($field !== null) {
$this->message .= " [{$field}]";
}

$this->message .= " for model [{$model}]";

if (count($this->ids) > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified by omitting the function call and comparing it to an empty scalar value:

Suggested change
if (count($this->ids) > 0) {
if ($this->ids !== []) {

$this->message .= ' '.implode(', ', $this->ids);
}

$this->message .= '.';

return $this;
}

/**
* Get the affected Eloquent model.
*
* @return class-string<TModel>
*/
public function getModel()
{
return $this->model;
}

/**
* Get the affected Eloquent model IDs.
*
* @return array<int, int|string>
*/
public function getIds()
{
return $this->ids;
}
}
3 changes: 3 additions & 0 deletions src/Illuminate/Foundation/Exceptions/Handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Illuminate\Contracts\Debug\ShouldntReport;
use Illuminate\Contracts\Foundation\ExceptionRenderer;
use Illuminate\Contracts\Support\Responsable;
use Illuminate\Database\Eloquent\InvalidIdFormatException;
use Illuminate\Database\Eloquent\ModelNotFoundException;
use Illuminate\Database\MultipleRecordsFoundException;
use Illuminate\Database\RecordNotFoundException;
Expand Down Expand Up @@ -146,6 +147,7 @@ class Handler implements ExceptionHandlerContract
BackedEnumCaseNotFoundException::class,
HttpException::class,
HttpResponseException::class,
InvalidIdFormatException::class,
ModelNotFoundException::class,
MultipleRecordsFoundException::class,
RecordNotFoundException::class,
Expand Down Expand Up @@ -634,6 +636,7 @@ protected function prepareException(Throwable $e)
return match (true) {
$e instanceof BackedEnumCaseNotFoundException => new NotFoundHttpException($e->getMessage(), $e),
$e instanceof ModelNotFoundException => new NotFoundHttpException($e->getMessage(), $e),
$e instanceof InvalidIdFormatException => new HttpException(422, $e->getMessage(), $e),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use \Symfony\Component\HttpFoundation\Request::HTTP_UNPROCESSABLE_ENTITY here

Suggested change
$e instanceof InvalidIdFormatException => new HttpException(422, $e->getMessage(), $e),
return $this->status === Request::HTTP_UNPROCESSABLE_ENTITY;

$e instanceof AuthorizationException && $e->hasStatus() => new HttpException(
$e->status(), $e->response()?->message() ?: (Response::$statusTexts[$e->status()] ?? 'Whoops, looks like something went wrong.'), $e
),
Expand Down
45 changes: 45 additions & 0 deletions tests/Integration/Routing/ImplicitModelRouteBindingTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,32 @@ public function testImplicitRouteBindingChildHasUlids()
$response = $this->getJson("/post/{$post->id}/tag-slug/{$tag->slug}");
$response->assertJsonFragment(['id' => $tag->id]);
}

public function test_invalid_key_returns_unprocessable()
{
$routeResolution = static fn (ImplicitBindingCommentByUuid $comment) => response()->json(['ok' => true]);

Route::middleware(['web'])->group(function () use ($routeResolution) {
Route::get('/comments/{comment}', $routeResolution);
Route::get('/comments-by-uuid/{comment:uuid}', $routeResolution);
});

$response = $this->getJson('comments/not-a-uuid');

$response->assertUnprocessable();
$response->assertJson([
'message' => 'Invalid key for model [Illuminate\\Tests\\Integration\\Routing\\ImplicitBindingCommentByUuid] not-a-uuid.',
]);

// When explicitly setting a route key field
$response = $this->getJson('comments-by-uuid/1234!');

$response->assertUnprocessable();
// Then the key is specified
$response->assertJson([
'message' => 'Invalid key [uuid] for model [Illuminate\\Tests\\Integration\\Routing\\ImplicitBindingCommentByUuid] 1234!.',
]);
}
}

class ImplicitBindingUser extends Model
Expand Down Expand Up @@ -357,6 +383,25 @@ public function getRouteKeyName()
}
}

class ImplicitBindingCommentByUuid extends Model
{
use HasUuids;

public $table = 'comments';

protected $fillable = ['slug', 'user_id'];

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

public function uniqueIds()
{
return ['uuid'];
}
}

class ImplicitBindingComment extends Model
{
use HasUuids;
Expand Down