Stop user-data errors leaking to Sentry from migrations#177
Stop user-data errors leaking to Sentry from migrations#177premtsd-code wants to merge 5 commits intomainfrom
Conversation
Two changes in Destinations/Appwrite.php: 1. Per-resource wrap rethrows non-Migration\Exception throwables. User errors still record on the migration document and the loop continues (existing behaviour). Library bugs and infra failures now bubble to the worker boundary so they can be routed to Sentry instead of being silently absorbed. 2. Row-flush match block catches DuplicateException and StructureException from the database adapter and rethrows them as Migration\Exception so the per-resource wrap above recognises them as user-facing failures. This keeps "Document already exists" / invalid structure messages in the migration report only.
Convert per-resource user-error throws into setStatus + addError + return false in the create*() methods (createDatabase, createEntity, createField, createIndex, createRecord). Failures are recorded for the migration report where they're detected, and the import loop continues to the next resource — matching the existing skip-from-cache pattern at line 1015 and the documents-DB skip at line 535. The per-resource wrap at import() simplifies to a pure safety net: record for the user, then rethrow. By construction, anything reaching it is a library bug or infra failure, which the worker boundary routes to Sentry. Row-flush DuplicateException / StructureException are caught at the flush site and recorded inline — same shape as the rest of the create*() methods. Behaviour preserved: user errors continue to appear in the migration report, the loop continues across resources, the migration ends with status=failed when any resource failed. Only routing to Sentry changed.
DB adapters already normalise DuplicateException's message to 'Document already exists', so a multi-type catch with $e->getMessage() collapses both branches into one without changing user-visible behaviour. Trimmed the safety-net comment to a single line.
Same shape as the row-flush combine: multi-type catch with a small ternary for the per-type message. Two pairs collapsed: the single attribute path and the relationship two-way path (which differ only in 'Attribute limit exceeded' vs 'Column limit exceeded').
Greptile SummaryThis PR converts user-facing error sites in
Confidence Score: 3/5Not safe to merge without confirming worker-side exception handling for the new rethrow path. The goal and the bulk of the changes are correct, but the throw $e behavioral change (P1) is not verifiable from this library alone — it requires the Appwrite worker to correctly handle an exception from Destination::run() that previously never escaped. Two additional P2s (batch error attribution, createIndex Sentry gap) lower confidence further. src/Migration/Destinations/Appwrite.php — specifically the import() catch block (throw $e) and createIndex() inner catch. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -344,7 +346,7 @@ protected function import(array $resources, callable $callback): void | |||
| previous: $e | |||
| )); | |||
|
|
|||
| $responseResource = $resource; | |||
| throw $e; | |||
There was a problem hiding this comment.
throw $e aborts the entire batch — callback never called
The new throw $e exits the foreach loop immediately, so any resources after the failing index are never processed and $callback($resources) at line 357 is never invoked. Before this PR the old $responseResource = $resource path swallowed the exception and let the loop (and callback) complete normally.
If the worker-side run() caller doesn't have a catch block that also advances migration state, marks remaining resources, and updates the progress counter, those resources will be stuck in whatever pre-loop status they had. Verify the Appwrite worker wraps Destination::run() in a catch and performs the necessary cleanup before assuming this is safe to merge.
| } catch (DuplicateException $e) { | ||
| $resource->setStatus(Resource::STATUS_ERROR, 'Document already exists'); | ||
| $this->addError(new Exception( | ||
| resourceName: $resource->getName(), | ||
| resourceGroup: $resource->getGroup(), | ||
| resourceId: $resource->getId(), | ||
| message: 'Document already exists', | ||
| previous: $e, | ||
| )); | ||
| return false; | ||
| } catch (StructureException $e) { | ||
| $resource->setStatus(Resource::STATUS_ERROR, $e->getMessage()); | ||
| $this->addError(new Exception( | ||
| resourceName: $resource->getName(), | ||
| resourceGroup: $resource->getGroup(), | ||
| resourceId: $resource->getId(), | ||
| message: $e->getMessage(), | ||
| previous: $e, | ||
| )); | ||
| return false; |
There was a problem hiding this comment.
$resource here is the last document in the $this->rowBuffer batch (the one with $isLast = true), but createDocuments/upsertDocuments operates on the entire buffer. When DuplicateException or StructureException is thrown, the actual culprit could be any document in the buffer. Recording resourceId: $resource->getId() in the error report will mislead users into thinking the last document is the duplicate/malformed one when it could be one of the many earlier buffered rows.
1 participant
Summary
Per-resource user errors thrown by
Destinations/Appwrite.phpwere leaking to Sentry through the migrations worker. This PR fixes the routing at the library layer.throw new Exception(...)user-error sites increate*()methods tosetStatus + addError + return false. No exception bubbles for expected outcomes; the failure is recorded where detected.import()becomes a pure safety net: record + rethrow. Library bugs and infra failures bubble through; user errors stay in the migration report.DuplicateExceptionandStructureExceptionfrom the database adapter and re-throws asMigration\Exceptionso the wrap classifies them as user-facing.Migration\Exceptionis now the library's marker for "user-facing migration error". The Appwrite worker usesinstanceof Migration\Exceptionto decide Sentry routing.Motivation
Errors like "Document already exists", "Attribute already exists", "Cannot set default value for array column", "Invalid document structure: Missing required attribute X" were being thrown with code 0 or PDO codes (e.g. 23000), and the migration worker's
code === 0 || >= 500heuristic re-published every one of them to Sentry. ~80% of the noise on theappwrite-queue-migrationsSentry stream came from this path.After this PR, expected user-data conditions never throw; only bugs/infra do.
Test plan