Skip to content

Update GridFS exmaple code #900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

harryadel
Copy link
Contributor

@harryadel harryadel commented Mar 28, 2025

Hello this is a problem I ran into during our migration and I thought it'd be nice to document so others could avoid this pitfall.

First off, the internal API for objectId got changed. Meteor even caught on this and updated their code.

Second, there was a silent fail with this piece of code:

.on('finish', async (ver) => {
	const property = `versions.${versionName}.meta.gridFsFileId`
	await self.collection.updateAsync(file._id, {
	    $set: {
	        [property]: ver._id.toHexString(),
	    },
})

ver is undefined so it fails to associate the new gridFS instance. Maybe due to internal mongo versions problem. The code had to be modified so the the gridFS id is stored in the return value of openUploadStream

const uploadStream = bucket
        .openUploadStream(file.name, {
            contentType: file.type || 'binary/octet-stream',
            metadata,
        })

@dr-dimitru dr-dimitru self-requested a review March 28, 2025 19:02
Copy link
Member

@dr-dimitru dr-dimitru left a comment

Choose a reason for hiding this comment

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

@harryadel thank you for spotting this issue! Please see my questions regarding implementation

Comment on lines 86 to 90
const uploadStream = bucket
.openUploadStream(file.name, {
contentType: file.type || 'binary/octet-stream',
metadata,
})
Copy link
Member

Choose a reason for hiding this comment

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

Can we create uploadStream above readStream? Outside open event?

Comment on lines 95 to 100
uploadStream
.on('error', (err) => {
console.error(err);
self.unlink(await this.collection.findOneAsync(file._id), versionName);
})
.on('finish', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we register these events when uploadStream variable created and before passing it to pipe ?

[property]: uploadStream.id.toHexString(),
},
})
self.unlink(await this.collection.findOneAsync(file._id), versionName);
Copy link
Member

Choose a reason for hiding this comment

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

Got to update to new await unlinkAsync

@dr-dimitru
Copy link
Member

@harryadel lmk if we can push this one through, I'm finishing with docs

@harryadel
Copy link
Contributor Author

@dr-dimitru Ok, I'll look at today so we can finalize it. 👍

@harryadel
Copy link
Contributor Author

Hey @dr-dimitru, please re-check

I took time because I wanted to do this in an example app to verify that our changes are indeed correct and not fix some imaginary code. The test repo is here in case you wanna check it out or even include as an example.

@dr-dimitru dr-dimitru merged commit 1ed12d1 into veliovgroup:migration-branch/3.0 Apr 8, 2025
2 checks passed
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.

2 participants