Skip to content

Conversation

@MaxGhenis
Copy link

@MaxGhenis MaxGhenis commented Jan 4, 2026

Summary

This PR adds error handling around the graphqlUploadExpress() middleware to improve error responses for multipart upload failures.

Problem

When multipart upload parsing fails, errors are returned in Express format ({"error": {...}}) instead of GraphQL format ({"errors": [...]}). This makes it harder for GraphQL clients to parse error responses.

Solution

Wrap the graphqlUploadExpress() middleware with error handling that:

  1. Catches any errors from the middleware
  2. Logs the actual error for debugging
  3. Returns a properly formatted GraphQL error response
  4. Returns HTTP 400 instead of 500 for parsing errors

Testing

Added tests in test/server/routes/graphql.test.ts that verify:

  1. Malformed multipart requests (invalid JSON) return 400 with GraphQL-formatted errors
  2. Valid multipart uploads return GraphQL-formatted responses (not 500)

Note

This improves error formatting. The root cause of the 500 errors reported in opencollective/opencollective#8456 appears to be production-environment specific (see investigation comment).

🤖 Generated with Claude Code

The graphqlUploadExpress middleware was returning 500 errors without
detailed error messages when file upload parsing failed. This wraps
the middleware to catch errors and return proper GraphQL-formatted
error responses with helpful messages.

Fixes #11293

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
MaxGhenis and others added 2 commits January 4, 2026 09:53
The test verifies that malformed multipart requests return errors in
GraphQL format (errors array) rather than Express format (error object).

Without the fix:
- Returns: { error: { message: '...', code: 400 } }
- Test fails: expected undefined to exist (no 'errors' field)

With the fix:
- Returns: { errors: [{ message: 'File upload failed: ...' }] }
- Test passes: GraphQL format is properly returned

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Verifies that valid multipart uploads return GraphQL-formatted responses
(not 500 errors) even when authentication fails.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Comment on lines +313 to +316
const uploadMiddleware = graphqlUploadExpress();
uploadMiddleware(req, res, (err?: Error) => {
if (err) {
logger.error(`GraphQL upload error: ${err.message}`, { error: err });
Copy link

Choose a reason for hiding this comment

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

Bug: The error handling callback for graphqlUploadExpress is never called because the middleware uses next(error) to propagate errors, bypassing the intended logic.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The middleware wrapper for graphqlUploadExpress uses a callback (err) => {...} to handle file upload errors. However, the graphqlUploadExpress middleware follows the standard Express.js error handling pattern by calling next(error) upon failure. This bypasses the custom callback entirely, meaning the error handling logic within it will never be executed. As a result, file upload parsing errors will not be formatted as GraphQL responses and will instead be passed to the global error handler, defeating the purpose of the change.

💡 Suggested Fix

Remove the custom callback. Instead, pass next directly to the uploadMiddleware and let errors propagate to the standard Express error-handling middleware. The graphqlUploadExpress middleware should be invoked as uploadMiddleware(req, res, next); to allow Express to correctly route errors.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: server/routes.ts#L313-L316

Potential issue: The middleware wrapper for `graphqlUploadExpress` uses a callback
`(err) => {...}` to handle file upload errors. However, the `graphqlUploadExpress`
middleware follows the standard Express.js error handling pattern by calling
`next(error)` upon failure. This bypasses the custom callback entirely, meaning the
error handling logic within it will never be executed. As a result, file upload parsing
errors will not be formatted as GraphQL responses and will instead be passed to the
global error handler, defeating the purpose of the change.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8139129

@MaxGhenis
Copy link
Author

Closing this PR - Sentry correctly identified that the callback pattern doesn't work with Express middleware (graphqlUploadExpress uses next(error) which bypasses the callback).

More importantly, the root cause has been identified and fixed in the frontend: opencollective/opencollective-frontend#11773

The frontend GraphQL proxy was corrupting multipart requests by JSON-stringifying them. That PR fixes the proxy to forward multipart bodies as raw Buffers, which resolves the file upload issue without needing changes to the API.

@MaxGhenis MaxGhenis closed this Jan 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant