fix: Preserve UTF-8 GraphQL upload filenames with latest graphql-upload#10478
Open
coratgerl wants to merge 11 commits into
Open
fix: Preserve UTF-8 GraphQL upload filenames with latest graphql-upload#10478coratgerl wants to merge 11 commits into
coratgerl wants to merge 11 commits into
Conversation
Load the latest ESM-only graphql-upload through a small compatibility layer so GraphQL uploads keep accented filenames intact without changing Parse Server's public module model. Co-authored-by: Cursor <cursoragent@cursor.com>
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
Bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughgraphql-upload is upgraded to v17, GraphQL upload handling uses cached dynamic imports, schema loading defers upload type initialization, file paths are normalized and validated across file operations, and tests add accented-filename and path-validation coverage. ChangesGraphQL Upload ESM Migration and Unicode Support
Sequence Diagram(s)sequenceDiagram
participant Client
participant ParseGraphQLServer
participant createGraphQLUploadMiddleware
participant getGraphQLUpload
participant defaultGraphQLTypes
participant FilesController
participant GridFSBucketAdapter
Client->>ParseGraphQLServer: multipart upload request
ParseGraphQLServer->>createGraphQLUploadMiddleware: mount upload middleware
createGraphQLUploadMiddleware->>getGraphQLUpload: load GraphQLUpload
ParseGraphQLServer->>defaultGraphQLTypes: load schema types
ParseGraphQLServer->>FilesController: normalizeFilename / validateFilepath
FilesController->>GridFSBucketAdapter: normalized file operation
GridFSBucketAdapter-->>Client: stored file response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.0)spec/ParseFile.spec.jsThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/GraphQL/loaders/filesMutations.js`:
- Around line 41-53: The response handling treats 3xx as success and uses a
nonexistent Parse.error constant in the catch; change the status check to reject
for any non-2xx (use if (res.statusCode < 200 || res.statusCode >= 300)) and
update the catch to capture the thrown error and reject with a real Parse.Error
code constant (e.g., use Parse.Error.FILE_SAVE_ERROR or another appropriate
Parse.Error.* code) so the reject call uses new
Parse.Error(Parse.Error.FILE_SAVE_ERROR, errorOrData) and includes the caught
error details; locate these changes around the parsedData/res.statusCode
handling and the catch that currently calls reject(new Parse.Error(Parse.error,
data)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a99d2150-8ad0-460f-ac62-c9b90904bbd1
📒 Files selected for processing (9)
package-lock.jsonpackage.jsonspec/ParseGraphQLServer.spec.jssrc/Adapters/Files/FilesAdapter.jssrc/GraphQL/ParseGraphQLSchema.jssrc/GraphQL/ParseGraphQLServer.jssrc/GraphQL/helpers/graphqlUpload.jssrc/GraphQL/loaders/defaultGraphQLTypes.jssrc/GraphQL/loaders/filesMutations.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #10478 +/- ##
==========================================
+ Coverage 92.66% 92.73% +0.06%
==========================================
Files 193 194 +1
Lines 16981 17085 +104
Branches 248 248
==========================================
+ Hits 15736 15844 +108
+ Misses 1224 1220 -4
Partials 21 21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Reject non-2xx responses from the internal /files handoff and use Parse.Error.FILE_SAVE_ERROR when the response body is not valid JSON. Add regression tests for accented filenames in FilesController and Parse.File. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Adapters/Files/FilesAdapter.js`:
- Line 115: Filename normalization is missing: validateFilename only tests the
regex but never normalizes, causing NFC/NFD mismatches across storage and
lookups. Normalize filenames (and each path segment) to a consistent Unicode
form (e.g., NFC) at the point the storage key is finalized and always pass that
normalized value into GridFS operations; update FilesAdapter (add or change
validateFilename to return/produce a normalized filename or add a new
normalizeFilename helper), ensure FilesRouter/FilesController call that
normalizer before upload/download/delete/metadata, and make GridFSBucketAdapter
use the normalized name for openUploadStream(filename),
openDownloadStreamByName, bucket.find({ filename }), delete, and any other
filename lookups.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bfd78a56-4652-4ed2-b00c-ac8ba20f9045
📒 Files selected for processing (11)
package-lock.jsonpackage.jsonspec/FilesController.spec.jsspec/ParseFile.spec.jsspec/ParseGraphQLServer.spec.jssrc/Adapters/Files/FilesAdapter.jssrc/GraphQL/ParseGraphQLSchema.jssrc/GraphQL/ParseGraphQLServer.jssrc/GraphQL/helpers/graphqlUpload.jssrc/GraphQL/loaders/defaultGraphQLTypes.jssrc/GraphQL/loaders/filesMutations.js
Normalize file names to NFC before finalizing storage keys so uploads, reads, metadata lookups, and deletes resolve the same GridFS record across Unicode forms. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Moumouls
suggested changes
May 26, 2026
Reject non-string filenames before normalization, validate multi-segment file paths on download/delete/metadata routes, and drop combining marks from the filename allowlist after NFC normalization. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Adapters/Files/FilesAdapter.js`:
- Around line 149-151: The current validation in FilesAdapter.js rejects any
pathname containing the substring '..' (using normalized.includes('..')), which
wrongly blocks filenames like "a..b.txt"; update validateFilepath (the block
using the normalized variable) to split the normalized path on '/' (and handle
leading/trailing slashes) and reject only when any path segment is exactly '..'
(e.g., segments.some(s => s === '..')), preserving valid filenames that contain
'..' inside a segment; keep existing Parse.Error and message but change the
detection logic accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ffaa707-9a79-4f67-ac8d-b56a7b1ebae8
📒 Files selected for processing (5)
spec/FileNameNormalization.spec.jsspec/FilesController.spec.jssrc/Adapters/Files/FilesAdapter.jssrc/Controllers/FilesController.jssrc/Routers/FilesRouter.js
Member
|
Could you check codecov? |
Member
|
@coratgerl check the failing test on Node 20 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/ParseGraphQLServer.spec.js (1)
7515-7519:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten the GraphQL response guard.
not.toBeNull()still passes whencreateFileisundefined, and the early return then skips the real assertions. That means a malformed empty 200 response can slip through as a passing test.Proposed fix
expect(result.errors).toBeUndefined(); - expect(result.data?.createFile).not.toBeNull(); - if (result.errors || !result.data?.createFile) { + expect(result.data?.createFile).toBeDefined(); + if (!result.data?.createFile) { + fail(`Unexpected GraphQL response: ${JSON.stringify(result)}`); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/ParseGraphQLServer.spec.js` around lines 7515 - 7519, The test currently uses expect(result.data?.createFile).not.toBeNull(), which still allows undefined and lets malformed empty success responses pass; change the assertion to expect(result.data?.createFile).toBeDefined() (or use not.toBeUndefined()) and tighten the early-return guard to check for undefined specifically (e.g., if (result.errors || result.data?.createFile === undefined) return;) so the test fails when createFile is missing or undefined; update references to result and result.data?.createFile accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@spec/ParseGraphQLServer.spec.js`:
- Around line 7515-7519: The test currently uses
expect(result.data?.createFile).not.toBeNull(), which still allows undefined and
lets malformed empty success responses pass; change the assertion to
expect(result.data?.createFile).toBeDefined() (or use not.toBeUndefined()) and
tighten the early-return guard to check for undefined specifically (e.g., if
(result.errors || result.data?.createFile === undefined) return;) so the test
fails when createFile is missing or undefined; update references to result and
result.data?.createFile accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 442ca129-a4df-4728-b3f8-1a921407e744
📒 Files selected for processing (7)
package-lock.jsonpackage.jsonspec/ParseGraphQLSchema.spec.jsspec/ParseGraphQLServer.spec.jssrc/Controllers/FilesController.jssrc/GraphQL/ParseGraphQLSchema.jssrc/Routers/FilesRouter.js
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- package-lock.json
- src/Controllers/FilesController.js
- src/Routers/FilesRouter.js
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
graphql-uploadto the latest ESM-only release while keeping Parse Server on its current public module model via a small private compatibility helper/fileshandoff safely, and allowing accented characters through filename validationcafé.txtuploads and keep existing GraphQL upload coverage greenThis PR was done in TDD.
Test plan
spec/ParseGraphQLServer.spec.jsspec/ParseFile.spec.jsfilename checksnpm run lintnpm testNotes
npm testfinishes with one unrelated pre-existing failure inParse.Query testing order by _updated_atMade with Cursor
Summary by CodeRabbit
graphql-uploadto 17.0.0.