Skip to content

Support using null as ObjectId for GridStore #933

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

Closed
wants to merge 1 commit into from

Conversation

Reggino
Copy link

@Reggino Reggino commented Apr 7, 2013

Regarding the GridStore constructor:

When null is supplied as ObjectId and an extra filename is passed, the GridStore will not try to query by filename.

This patch fixes this.

@christkv
Copy link
Contributor

christkv commented Apr 8, 2013

are you trying to save a file using a filename then you just do

new GridStore(db, "filename", "w")

not

new GridStore(db, null, "filename", "w")

@Reggino
Copy link
Author

Reggino commented Apr 8, 2013

I got that now, but that wasn't obvious for me by looking at the code and/or the method signature: I thought passing an explicit null-value for the objectId would trigger a filename-based lookup. And besides: the typos in the code make it even more 'unclear'.

@christkv
Copy link
Contributor

christkv commented Apr 8, 2013

yeah it's the hubris of legacy code. the second option is for setting a filename as metadata not as the id itself. The best options is usually to pass in a id and a filename.

@Reggino
Copy link
Author

Reggino commented Apr 8, 2013

The reason i came with this issue is this issue in gridfs-stream:

aheckmann/gridfs-stream#11

A 12-lettered (or 24 lettered) string/filename COULD internally be casted to an ObjectId (but this is not the case for GridStore). To prevent this possible mixup, i think it is a good idea to make it a bit more explicit whether a 'filename' or 'ObjectId' is passed

@christkv
Copy link
Contributor

christkv commented Apr 8, 2013

yeah I know but I can't break backward compatibility and this pull request would as it would mange the last options object as you pointed out by modifying a test to make it pass.

@Reggino
Copy link
Author

Reggino commented Apr 8, 2013

That test was broken before i changed anything ;-) I think the change IS backwards compatible!

@Reggino
Copy link
Author

Reggino commented Apr 8, 2013

(i should not have added the test-change in the same pull request, sry)

@christkv
Copy link
Contributor

christkv commented Apr 8, 2013

passes here an on travis without the changes so I'm not sure why it does not pass on you system

@Reggino
Copy link
Author

Reggino commented Apr 8, 2013

Reverted the (unrelated) test change

@christkv
Copy link
Contributor

Closing as not going to support

@christkv christkv closed this Jun 25, 2013
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