From 96d1847fd2d7a20b581e8627ac0795486d723984 Mon Sep 17 00:00:00 2001 From: "Ronald A. Richardson" Date: Wed, 16 Sep 2026 12:07:39 +0800 Subject: [PATCH 1/2] Let observer refusals reach the caller on update and bulk delete An observer that refuses a write by throwing FleetbaseRequestValidationException had its explanation discarded on two paths. The write was still refused, but the caller saw "Invalid request" or "Error occurred while trying to update a X" instead of the message the observer wrote. Both sites already had a handler for the exception; each was made unreachable by a catch block one frame deeper. updateRecordFromRequest() wrapped every exception from $record->update() in a plain \Exception, so the controller's dedicated catch and the global handler's getErrors() rendering never saw the original. It now rethrows FleetbaseRequestValidationException untouched and keeps wrapping everything else, so database and internal errors still do not leak. bulkDelete() listed catch (\Exception) before its QueryException and FleetbaseRequestValidationException catches, leaving both dead behind a @codeCoverageIgnore block that acknowledged as much. The catches are now ordered most specific first and the ignore block is gone because the branches are reachable and covered. Create and single delete were already correct and are unchanged. Fixes #256 --- src/Traits/HasApiControllerBehavior.php | 12 ++---- src/Traits/HasApiModelBehavior.php | 5 +++ .../Traits/HasApiControllerBehaviorTest.php | 32 ++++++++++++++++ tests/Unit/Traits/HasApiModelBehaviorTest.php | 38 +++++++++++++++++++ 4 files changed, 79 insertions(+), 8 deletions(-) diff --git a/src/Traits/HasApiControllerBehavior.php b/src/Traits/HasApiControllerBehavior.php index a6167cbd..79f80331 100644 --- a/src/Traits/HasApiControllerBehavior.php +++ b/src/Traits/HasApiControllerBehavior.php @@ -627,17 +627,13 @@ public function bulkDelete(BulkDeleteRequest $request) try { $count = $this->model->bulkRemove($ids); - } catch (\Exception $e) { - return response()->error($e->getMessage()); - // QueryException and FleetbaseRequestValidationException are covered by - // the preceding Exception catch in PHP's current catch order. - // @codeCoverageIgnoreStart - } catch (QueryException $e) { - return response()->error($e->getMessage()); } catch (FleetbaseRequestValidationException $e) { return response()->error($e->getErrors()); + } catch (QueryException $e) { + return response()->error($e->getMessage()); + } catch (\Exception $e) { + return response()->error($e->getMessage()); } - // @codeCoverageIgnoreEnd return response()->json( [ diff --git a/src/Traits/HasApiModelBehavior.php b/src/Traits/HasApiModelBehavior.php index e1a8bc89..2161f7cf 100644 --- a/src/Traits/HasApiModelBehavior.php +++ b/src/Traits/HasApiModelBehavior.php @@ -2,6 +2,7 @@ namespace Fleetbase\Traits; +use Fleetbase\Exceptions\FleetbaseRequestValidationException; use Fleetbase\Support\ApiModelCache; use Fleetbase\Support\Auth; use Fleetbase\Support\Http; @@ -416,6 +417,10 @@ public function updateRecordFromRequest(Request $request, $id, ?callable $onBefo $input = Arr::except($input, ['uuid', 'public_id', 'deleted_at', 'updated_at', 'created_at']); try { $record->update($input); + } catch (FleetbaseRequestValidationException $e) { + // An observer refusing the write is user-facing feedback, not an internal failure. + // Let it through untouched so the controller and global handler can render getErrors(). + throw $e; } catch (\Exception $e) { throw new \Exception(app()->hasDebugModeEnabled() ? $e->getMessage() : 'Failed to update ' . $this->getApiHumanReadableName()); } diff --git a/tests/Unit/Traits/HasApiControllerBehaviorTest.php b/tests/Unit/Traits/HasApiControllerBehaviorTest.php index 31837c30..8ef2feef 100644 --- a/tests/Unit/Traits/HasApiControllerBehaviorTest.php +++ b/tests/Unit/Traits/HasApiControllerBehaviorTest.php @@ -1,5 +1,6 @@ and($updateQueryFailure->getData(true))->toBe(['errors' => ['Error occurred while trying to update a Widget']]); }); +test('api controller behavior surfaces observer refusals on update and bulk delete', function () { + $refusingModel = new class extends HasApiControllerBehaviorModel { + public function updateRecordFromRequest(Request $request, $id, ?callable $onBefore = null, ?callable $onAfter = null): self + { + throw new FleetbaseRequestValidationException(['Widget is locked and cannot be changed.']); + } + + public function bulkRemove(array $ids): int + { + throw new FleetbaseRequestValidationException(['Widget is locked and cannot be deleted.']); + } + }; + $queryFailingModel = new class extends HasApiControllerBehaviorModel { + public function bulkRemove(array $ids): int + { + throw new QueryException('mysql', 'delete from widgets', [], new RuntimeException('database unavailable')); + } + }; + + $refusingController = new HasApiControllerBehaviorController($refusingModel); + $updateRefusal = $refusingController->updateRecord(has_api_controller_behavior_request('/v1/widgets/widget-1', 'PATCH', ['name' => 'Nope']), 'widget-1'); + $bulkDeleteRefusal = $refusingController->bulkDelete(BulkDeleteRequest::create('/v1/widgets/bulk-delete', 'DELETE', ['ids' => ['widget-1']])); + $bulkDeleteQuery = (new HasApiControllerBehaviorController($queryFailingModel))->bulkDelete( + BulkDeleteRequest::create('/v1/widgets/bulk-delete', 'DELETE', ['ids' => ['widget-1']]) + ); + + expect($updateRefusal->getData(true))->toBe(['errors' => ['Widget is locked and cannot be changed.']]) + ->and($bulkDeleteRefusal->getData(true))->toBe(['errors' => ['Widget is locked and cannot be deleted.']]) + ->and($bulkDeleteQuery->getData(true)['errors'][0])->toContain('database unavailable'); +}); + test('api controller behavior validates fallback rule contracts before writing', function () { $controller = new HasApiControllerBehaviorController(); $controller->rules = ['name' => ['required']]; diff --git a/tests/Unit/Traits/HasApiModelBehaviorTest.php b/tests/Unit/Traits/HasApiModelBehaviorTest.php index d034aaff..f3b4f071 100644 --- a/tests/Unit/Traits/HasApiModelBehaviorTest.php +++ b/tests/Unit/Traits/HasApiModelBehaviorTest.php @@ -1,5 +1,6 @@ and(fn () => (new HasApiModelBehaviorRecord())->remove('record_alpha'))->toThrow(Exception::class); }); +test('api model behavior propagates observer update refusals without rewrapping them', function () { + $capsule = has_api_model_behavior_database(); + has_api_model_behavior_seed_records($capsule); + session(['company' => 'company-a']); + + $refusals = []; + foreach ([true, false] as $debug) { + config(['app.debug' => $debug]); + + try { + (new HasApiModelBehaviorRefusingUpdateRecord())->updateRecordFromRequest( + has_api_model_behavior_request(['name' => 'Refused update'], method: 'PATCH'), + 'record_alpha' + ); + } catch (Exception $exception) { + $refusals[] = $exception; + } + } + + expect($refusals)->toHaveCount(2) + ->and($refusals[0])->toBeInstanceOf(FleetbaseRequestValidationException::class) + ->and($refusals[0]->getErrors())->toBe(['Record is locked and cannot be updated.']) + ->and($refusals[1])->toBeInstanceOf(FleetbaseRequestValidationException::class) + ->and($refusals[1]->getErrors())->toBe(['Record is locked and cannot be updated.']) + ->and($capsule->getConnection('mysql')->table('api_model_behavior_records')->where('public_id', 'record_alpha')->value('name'))->toBe('Alpha Dispatch'); +}); + test('api model behavior exposes default searchable fields options and no-op query branches', function () { $capsule = has_api_model_behavior_database(); has_api_model_behavior_seed_records($capsule); From 09af8646d7c7b5cc36aee104d79a9e73d7bb5232 Mon Sep 17 00:00:00 2001 From: "Ronald A. Richardson" Date: Wed, 16 Sep 2026 12:07:59 +0800 Subject: [PATCH 2/2] Note the observer refusal fix in the v1.6.62 release notes --- RELEASE.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/RELEASE.md b/RELEASE.md index 03da73fc..254b05d3 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -5,6 +5,10 @@ - Give every custom field a public id, so an API that hands one out names it the way the rest of the platform names a resource rather than exposing an internal uuid. `CustomField` takes `HasPublicId` with the `custom_field` prefix, and `public_id` becomes fillable. - Mint an id on the one path that would otherwise miss it: `HasCustomFields::setCustomField()` saves a field it creates on the fly with `saveQuietly()`, which skips the hook that assigns the id. +## Fixes + +- Let an observer's refusal reach the caller on the update and bulk-delete paths. An observer that refused a write by throwing `FleetbaseRequestValidationException` had its explanation discarded: `HasApiModelBehavior::updateRecordFromRequest()` rewrapped every exception from the save as a plain `\Exception`, and `HasApiControllerBehavior::bulkDelete()` caught `\Exception` ahead of its dedicated handler, so callers saw `Invalid request` or a generic update error instead of the message the observer wrote. The exception now passes through untouched on both paths and is rendered with `getErrors()`, as it already was on create and single delete. Every other exception is wrapped exactly as before. Reported in [#256](https://github.com/fleetbase/core-api/issues/256). + ## Reliability - Backfill existing rows in the migration, and add the column as nullable and indexed rather than unique-and-required, so it is safe on an already-populated `custom_fields` table. @@ -14,4 +18,4 @@ This is platform-wide: every custom field gains a public id, not only those used A database migration is required. No configuration change is needed. -Changes: [#254](https://github.com/fleetbase/core-api/pull/254). +Changes: [#254](https://github.com/fleetbase/core-api/pull/254), [#259](https://github.com/fleetbase/core-api/pull/259).