Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation consistently applies $eq across affected GridFS operations and includes comprehensive specification coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Aligns GridFS file-ID queries with the updated specification, preventing operator-shaped IDs from being interpreted as query operators.
Changes:
- Wraps GridFS ID matches in
$eqfor downloads, deletes, renames, chunk reads, and upload aborts. - Adds unified and prose coverage for query injection scenarios.
- Synchronizes retryable-read fixtures and GridFS documentation.
| File | Description |
|---|---|
src/gridfs/download.ts |
Uses $eq for file and chunk lookups. |
src/gridfs/index.ts |
Uses $eq for download, delete, and rename filters. |
src/gridfs/upload.ts |
Safely matches chunk IDs during abort cleanup. |
test/integration/gridfs/gridfs.prose.test.ts |
Tests abort cleanup with an operator-shaped ID. |
test/spec/gridfs/README.md |
Adds the new prose test specification. |
test/spec/gridfs/queries-use-eq.yml |
Adds YAML unified tests for $eq behavior. |
test/spec/gridfs/queries-use-eq.json |
Adds equivalent JSON unified tests. |
test/spec/retryable-reads/unified/gridfs-download.yml |
Updates expected download command filters. |
test/spec/retryable-reads/unified/gridfs-download.json |
Updates generated JSON expectations. |
test/spec/retryable-reads/unified/gridfs-download-serverErrors.yml |
Updates server-error retry expectations. |
test/spec/retryable-reads/unified/gridfs-download-serverErrors.json |
Updates generated server-error expectations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ? stream.s.filter._id.toString() | ||
| : stream.s.filter.filename; | ||
| const identifier = | ||
| stream.s.filter._id != null ? String(stream.s.filter._id.$eq) : stream.s.filter.filename; |
There was a problem hiding this comment.
What's the reason for switching from .toString() to String()? Unlike before, a nullish value now gives "null"/"undefined" instead of throwing.
There was a problem hiding this comment.
Good question, and you are right, this can be simplified. I actually think we don't have to have this conversion at all since the only place where identifier is used - one line bellow:
const errmsg = `FileNotFound: file ${identifier} was not found`;
which itself convert this value into string.
Our existing check stream.s.filter._id != null is enough to tell which download method was used: by filename or by id. So there are (and there were) no cases when we try to stringify nullish value.
PavelSafronov
left a comment
PavelSafronov
left a comment
There was a problem hiding this comment.
Code changes look good, approving.
Will merge after the release notes are added.
Description
Summary of Changes
Sync spec tests and modify query filter format.
What is the motivation for this change?
Align with updated specification.
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript