fix: Preserve UTF-8 GraphQL upload filenames with latest graphql-upload - #10478
fix: Preserve UTF-8 GraphQL upload filenames with latest graphql-upload#10478coratgerl wants to merge 14 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughgraphql-upload is upgraded to v17. GraphQL upload handling now uses cached dynamic imports and deferred schema initialization. File names and paths are NFC-normalized and validated across controllers, routers, and GridFS operations, with expanded Unicode, upload, and traversal tests. ChangesGraphQL Upload ESM Migration and Unicode Support
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10478 +/- ##
==========================================
+ Coverage 92.66% 92.73% +0.06%
==========================================
Files 193 194 +1
Lines 16981 17084 +103
Branches 248 248
==========================================
+ Hits 15736 15843 +107
+ 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>
|
@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>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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
|
Could you check codecov? |
|
@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
|
@coratgerl I've restarted the Node 22 CI job, I think it timed out; if it times out again, we should check whether this PR increases the CI run time and why that is. @Moumouls does the PR look good to you? |
|
@mtrezza we will do another round i think here to polish some minor details with @coratgerl |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Controllers/FilesController.js (1)
139-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis
validateFilepathis a pure pass-through wrapper — the exact no-op pattern that was already removed fornormalizeFilename.Don't wave this through. Unlike
validateFilename(Lines 124-137), which legitimately consultsthis.adapter.validateFilenamefirst before falling back to the util, this method adds zero behavior:validateFilepath(filepath) { return validateFilepath(filepath); }. The only caller isFilesRouter, which already importsnormalizeFilenamedirectly fromFilesAdapter— it can importvalidateFilepaththe same way. Keeping this wrapper reintroduces precisely the indirection Moumouls flagged and you agreed to drop.Pick one and commit to it:
- If no adapter override is intended, delete this method and have
FilesRoutercall the importedvalidateFilepathdirectly (consistent with how it usesnormalizeFilename).- If you do want an extension point, make it actually delegate, mirroring
validateFilename.♻️ Option A — drop the no-op wrapper
- validateFilepath(filepath) { - return validateFilepath(filepath); - } }Then in
src/Routers/FilesRouter.js, import and call the util directly:-import { RESERVED_FILEPATH_SEGMENTS, normalizeFilename } from '../Adapters/Files/FilesAdapter'; +import { RESERVED_FILEPATH_SEGMENTS, normalizeFilename, validateFilepath } from '../Adapters/Files/FilesAdapter';🤖 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 `@src/Controllers/FilesController.js` around lines 139 - 141, Remove the no-op FilesController.validateFilepath wrapper because it only returns validateFilepath(filepath) without adding any adapter behavior, unlike validateFilename which first checks this.adapter.validateFilename; either delete the method and update FilesRouter to import and call validateFilepath directly alongside normalizeFilename, or if an override is intended, change FilesController.validateFilepath to delegate through this.adapter.validateFilepath before falling back to the util so it is a real extension point.
🤖 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.
Nitpick comments:
In `@src/Controllers/FilesController.js`:
- Around line 139-141: Remove the no-op FilesController.validateFilepath wrapper
because it only returns validateFilepath(filepath) without adding any adapter
behavior, unlike validateFilename which first checks
this.adapter.validateFilename; either delete the method and update FilesRouter
to import and call validateFilepath directly alongside normalizeFilename, or if
an override is intended, change FilesController.validateFilepath to delegate
through this.adapter.validateFilepath before falling back to the util so it is a
real extension point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbc33d18-f3f1-4692-8924-afa93e6ee7d6
📒 Files selected for processing (4)
spec/FilesController.spec.jssrc/Adapters/Files/FilesAdapter.jssrc/Controllers/FilesController.jssrc/Routers/FilesRouter.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Adapters/Files/FilesAdapter.js
- spec/FilesController.spec.js
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Controllers/FilesController.js (1)
139-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
validateFilepathreintroduces the no-op wrapper pattern already removed fornormalizeFilename.
validateFilepath(filepath) { return validateFilepath(filepath); }is a pure pass-through with nothis.adapter.validateFilepathcheck — unlikevalidateFilename(lines 124-137), which explicitly checks for an adapter override before falling back to the imported util. This is the exact anti-pattern flagged earlier in this PR fornormalizeFilename("weird to proxy an util why it's needed"), which was resolved by removing the wrapper and calling the util directly at call sites. Either add adapter-override support for consistency withvalidateFilename, or drop this method and call the importedvalidateFilepathdirectly fromFilesRouter.js.♻️ Option A: support adapter override (consistent with validateFilename)
validateFilepath(filepath) { - return validateFilepath(filepath); + if (typeof this.adapter.validateFilepath === 'function') { + return this.adapter.validateFilepath(filepath); + } + return validateFilepath(filepath); }🤖 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 `@src/Controllers/FilesController.js` around lines 139 - 141, Remove the pass-through validateFilepath method from FilesController and update FilesRouter.js call sites to invoke the imported validateFilepath utility directly, matching the existing normalizeFilename approach. Do not add adapter override behavior unless intentionally choosing consistency with validateFilename instead.
🤖 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.
Nitpick comments:
In `@src/Controllers/FilesController.js`:
- Around line 139-141: Remove the pass-through validateFilepath method from
FilesController and update FilesRouter.js call sites to invoke the imported
validateFilepath utility directly, matching the existing normalizeFilename
approach. Do not add adapter override behavior unless intentionally choosing
consistency with validateFilename instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f917d42b-a8a8-4f48-9305-18583562a3a5
📒 Files selected for processing (17)
package-lock.jsonpackage.jsonspec/FileNameNormalization.spec.jsspec/FilesController.spec.jsspec/ParseFile.spec.jsspec/ParseGraphQLSchema.spec.jsspec/ParseGraphQLServer.spec.jsspec/graphqlUpload.spec.jssrc/Adapters/Files/FilesAdapter.jssrc/Adapters/Files/GridFSBucketAdapter.jssrc/Controllers/FilesController.jssrc/GraphQL/ParseGraphQLSchema.jssrc/GraphQL/ParseGraphQLServer.jssrc/GraphQL/helpers/graphqlUpload.jssrc/GraphQL/loaders/defaultGraphQLTypes.jssrc/GraphQL/loaders/filesMutations.jssrc/Routers/FilesRouter.js
3 participants
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.