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). 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);