bugfix: graphql-upload error handling #67
Conversation
Updated DTO to keep input file mandatory Added transform option to validation pipe
WalkthroughThe changes in this pull request involve updates to the global validation pipe configuration in the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/api/src/modules/summary/dto/create-summary.dto.ts (1)
Line range hint
1-14: Consider adding file validation constraintsWhile making the file required is good, consider adding additional validation decorators to ensure secure file uploads:
@Field(() => GraphQLUpload) +@IsNotEmpty({ message: 'Input file is required' }) +@ValidatePromise() +@IsObject() inputFile: Promise<FileUpload>;Also consider adding:
- File size validation
- File type validation
- File name sanitization
apps/api/src/main.ts (2)
Line range hint
36-41: Enhance upload middleware error handlingThe current error handling in graphqlUploadExpress could be improved to provide more structured errors:
app.use(graphqlUploadExpress({ maxFileSize: 1000000, maxFiles: 1 }), (err, req, res, next) => { if (err) { - res.status(500).json({ error: err.message }); + const status = err.status || 500; + res.status(status).json({ + status: 'error', + code: err.code || 'UPLOAD_ERROR', + message: err.message, + data: null + }); } else { next(); } });
Line range hint
23-31: Secure upload directory configurationThe upload directory configuration needs additional security measures:
- The current regex for sanitizing path (
[^\w\s/]) might still allow potentially dangerous paths- No validation of absolute path to prevent directory traversal
- Default fallback path is within project directory which isn't ideal for production
Consider:
export const uploadDir = - configService.get('UPLOAD_FOLDER_PATH')?.replace(/[^\w\s/]/g, '') ?? - join(process.cwd(), 'uploads'); + validateUploadPath(configService.get('UPLOAD_FOLDER_PATH')) ?? + join(process.env.UPLOAD_BASE_DIR || '/var/uploads', 'summaries'); +function validateUploadPath(path: string | undefined): string | undefined { + if (!path) return undefined; + const normalized = path.replace(/[^a-zA-Z0-9/_-]/g, ''); + if (normalized.includes('..') || !path.startsWith('/var/uploads')) { + throw new Error('Invalid upload path configuration'); + } + return normalized; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/main.ts(1 hunks)apps/api/src/modules/summary/dto/create-summary.dto.ts(1 hunks)
🔇 Additional comments (1)
apps/api/src/modules/summary/dto/create-summary.dto.ts (1)
14-14: Verify handling of breaking change across the application
Making inputFile required is a breaking change that could affect existing clients. Ensure all GraphQL queries and mutations using this DTO are updated accordingly.
Consider implementing proper error messages in the GraphQL schema to guide clients about the required file upload:
type Mutation {
createSummary(input: CreateSummaryInput!): Summary!
@error(code: "FILE_REQUIRED", message: "Input file is required for summary creation")
}✅ Verification successful
The previous search might have failed because we need to first identify where this DTO is being used. Let's gather more context about its usage patterns in the codebase to verify the impact of making inputFile required.
Breaking change is already properly handled across the application
The verification shows that making inputFile required is safe because:
- The GraphQL mutation in the client code already treats the file as required with
$file: Upload!type - The service implementation already expects and handles the file with proper null checks
- All client implementations (portal, Postman collections, documentation) consistently show the file as a required parameter
The change aligns the DTO with the existing usage patterns in the codebase, making it more type-safe without introducing breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GraphQL operations using CreateSummaryDTO
rg -l "CreateSummaryDTO" | xargs rg "mutation.*summary.*\{" -A 5
Length of output: 64
Script:
#!/bin/bash
# Let's first find all files containing CreateSummaryDTO
rg -l "CreateSummaryDTO"
# Then search for GraphQL mutations in resolver files
fd resolver | xargs rg "createSummary.*\(" -A 5
# Also search for any test files that might be using this DTO
fd "\.spec\.|\.test\." | xargs rg "CreateSummaryDTO"
Length of output: 1112
Script:
#!/bin/bash
# Let's check the implementation in the service to see how it handles the file
rg -A 10 "createSummary.*\(" "apps/api/src/modules/summary/summary.service.ts"
# Let's also check for any GraphQL schema definitions
fd "\.graphql$|schema\.ts$" | xargs rg "createSummary|CreateSummaryInput"
# And check the actual resolver implementation to see the decorator setup
ast-grep --pattern 'class SummaryResolver {
$$$
@Mutation($_)
createSummary($$$) {
$$$
}
$$$
}'
Length of output: 3548
2 participants
Updated DTO to keep input file mandatory
Added transform option to validation pipe
API PR Checklist
Pre-requisites
PR Details
PR details have been updated as per the given format (see below)
feat: add admin login endpoint)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Description:
Related changes:
N/A
Screenshots:
Add any screenshots as required.
Query request and response:
Documentation changes:
Test suite output:
Pending actions:
Additional notes:
Summary by CodeRabbit
New Features
Bug Fixes
inputFileproperty when creating a summary instance.