fix: centralize operation error handling#1071
Conversation
Fixes SNAPSHOT-HUB-35 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR centralizes MySQL queryAsync error handling in src/helpers/mysql.ts and removes per-resolver try/catch blocks (and related logging/Sentry imports) from several GraphQL operation resolvers to reduce duplication.
Changes:
- Override
Pool.prototype.queryAsyncto catch MySQL query errors, log them, optionally report to Sentry, and rethrow a generic"request failed"error. - Adjust Bluebird
promisifyAllto skip promisifyingqueryso the customqueryAsynccan be used. - Remove redundant try/catch +
capture/logimports from multiple GraphQL operation resolvers.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/helpers/mysql.ts | Centralizes queryAsync behavior (custom implementation + Sentry/logging) and alters Bluebird promisification behavior. |
| src/graphql/operations/votes.ts | Removes local try/catch and Sentry/log handling around DB queries. |
| src/graphql/operations/users.ts | Removes local try/catch and Sentry/log handling around DB queries. |
| src/graphql/operations/subscriptions.ts | Removes local try/catch and Sentry/log handling around DB queries. |
| src/graphql/operations/statements.ts | Removes local try/catch and Sentry/log handling around DB queries. |
| src/graphql/operations/statement.ts | Removes local try/catch and Sentry/log handling around DB queries. |
| src/graphql/operations/roles.ts | Removes local try/catch and Sentry/log handling around DB query + mapping. |
| src/graphql/operations/proposals.ts | Removes local try/catch and Sentry/log handling around DB queries. |
| src/graphql/operations/proposal.ts | Removes local try/catch and Sentry/log handling around DB queries. |
| src/graphql/operations/messages.ts | Removes local try/catch and Sentry/log handling around DB queries. |
| src/graphql/operations/leaderboards.ts | Removes local try/catch and Sentry/log handling around DB queries. |
| src/graphql/operations/follows.ts | Removes local try/catch and Sentry/log handling around DB queries. |
| src/graphql/operations/aliases.ts | Removes local try/catch and Sentry/log handling around DB queries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function withErrorHandler(fn) { | ||
| return (...args) => | ||
| fn(...args).catch(e => { | ||
| if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e); | ||
| log.error(`[graphql] ${JSON.stringify(e)}`); | ||
| return Promise.reject(new Error('request failed')); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The withErrorHandler wrapper calls .catch() on all wrapped functions, but some operations (plugins, skins, validations, strategies) are synchronous and return values directly instead of Promises. Calling .catch() on a non-Promise value will cause a runtime error.
To fix this, the wrapper should handle both synchronous and asynchronous functions. Consider wrapping the function call in Promise.resolve() first, or check if the return value is a Promise before calling .catch().
| }; | ||
|
|
||
| export default Object.fromEntries( | ||
| Object.entries(operations).map(([key, fn]) => [key, withErrorHandler(fn)]) |
There was a problem hiding this comment.
The operations space.ts, spaces.ts, ranking.ts, and vote.ts still have their own try/catch blocks with error handling and logging. This means errors in these operations will be logged twice - once in their own try/catch and again in the withErrorHandler wrapper. Additionally, these operations return Error objects (not Promise.reject), which won't trigger the .catch() in the wrapper, so the centralized error handling won't apply to them at all.
Either remove the try/catch blocks from these files to use only the centralized handler, or remove them from the operations object so they aren't wrapped. If these operations need special error handling (like PublicError), that logic should be incorporated into the centralized handler or kept separate from the wrapper.
| Object.entries(operations).map(([key, fn]) => [key, withErrorHandler(fn)]) | |
| Object.entries(operations).map(([key, fn]) => { | |
| const skipWrapper = ['space', 'spaces', 'ranking', 'vote']; | |
| return skipWrapper.includes(key) ? [key, fn] : [key, withErrorHandler(fn)]; | |
| }) |
| function withErrorHandler(fn) { | ||
| return (...args) => | ||
| fn(...args).catch(e => { | ||
| if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e); |
There was a problem hiding this comment.
The centralized error handler no longer captures context information (args, context, info) that was previously passed to capture() in the individual resolvers. This context is valuable for debugging and error tracking in Sentry. The wrapper has access to the arguments passed to each resolver function through the ...args parameter, but these aren't being passed to capture().
Consider capturing the arguments in the error handler to preserve this debugging context.
| if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e); | |
| if (!IGNORED_ERROR_CODES.includes(e.code)) { | |
| const [, resolverArgs, context, info] = args; | |
| capture(e, { args: resolverArgs, context, info }); | |
| } |
There was a problem hiding this comment.
this make sense as well. maybe we need them?
| return (...args) => | ||
| fn(...args).catch(e => { | ||
| if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e); | ||
| log.error(`[graphql] ${JSON.stringify(e)}`); |
There was a problem hiding this comment.
The centralized error logging no longer includes the specific operation name that failed. Previously, each operation logged errors with a specific identifier like [graphql] votes, [graphql] proposals, etc. Now all errors are logged as just [graphql], making it harder to identify which operation failed without examining the full error object.
Consider including the operation name in the log message by adding it as a parameter to the wrapper or extracting it from the function name.
| } catch (e: any) { | ||
| capture(e, { args, context, info }); | ||
| log.error(`[graphql] votes, ${JSON.stringify(e)}`); | ||
| return Promise.reject(new Error('request failed')); | ||
| } |
There was a problem hiding this comment.
Can remove catch on vote.ts also right?
There was a problem hiding this comment.
Also on vp.ts it will ignore the promise.reject messages and will always return request failed?
There was a problem hiding this comment.
vote.ts — try/catch with capture
space.ts — try/catch with PublicError handling
spaces.ts — try/catch with PublicError handling
ranking.ts — try/catch with PublicError handling
options.ts — internal run loop with try/catch
| return (...args) => | ||
| fn(...args).catch(e => { | ||
| if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e); | ||
| log.error(`[graphql] ${JSON.stringify(e)}`); |
There was a problem hiding this comment.
Before we know which operation is failing, copilot is correct here
| fn(...args).catch(e => { | ||
| if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e); | ||
| log.error(`[graphql] ${JSON.stringify(e)}`); | ||
| return Promise.reject(new Error('request failed')); |
There was a problem hiding this comment.
Maybe need to figure out how we should handle custom error messages, for example vp.ts, user.ts
| function withErrorHandler(fn) { | ||
| return (...args) => | ||
| fn(...args).catch(e => { | ||
| if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e); |
There was a problem hiding this comment.
this make sense as well. maybe we need them?
This PR refactors the operations/ files, and centralize the error handling in a single place, for easier filtering out of some error code (and also simplify the code)
Summary
withErrorHandlerwrapper insrc/graphql/operations/index.tsPromise.reject(new Error('request failed'))— no errors bubble upER_QUERY_TIMEOUTerrors are filtered out from Sentry viaIGNORED_ERROR_CODESconstant (still logged)capture/logimports from 12 graphql operation filesRelated
ER_QUERY_TIMEOUTerrors onPOST /graphqlTest plan
'request failed'on error, no raw errors bubble up