Skip to content
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

Add Collection#findOne #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

keithnorm
Copy link

Noticed this method was missing and it was easy to add, so here you go if this is helpful.

Not sure how closely you intend to mirror mongodb, but I'm using zango on the front end and mongodb on the backend in an isomorphic React app, so getting as close as possible to full mongodb compatibility is best for me.

Thanks for a great implementation.

@keithnorm
Copy link
Author

FYI most of the changes are because I compiled with a newer version of babel. Aside from that and regenerating the docs, the real changes are in this commit d74a5fe

@erikolson186
Copy link
Owner

I am glad that you are using ZangoDB and I appreciate the contribution.

I looked at the API documentation for the MongoDB node driver and saw that the findOne method supports a callback: http://mongodb.github.io/node-mongodb-native/2.0/api/Collection.html#findOne

I went ahead and implemented the method for ZangoDB a little differently than you did so as to support a callback: a8bc729

If you could comment on the change to create_next_fn.js I would appreciate it. Is there a benefit to retrieving each next function in the same order as they were created?

Thanks!

@keithnorm
Copy link
Author

Yeah, if you run the find_one.js test suite I added without that change to create_next_fn it fails the should perform disjunction equality test and I traced that failure down to the order of executing next fn's. I don't recall the actual details of what was going wrong now, but I can go through it again later on and get back to you with more details on that if you want.

@erikolson186
Copy link
Owner

Don't worry about going through it, I know what the problem is. What I would like to know, though, is if MongoDB ensures document order with disjunction queries. If so, your change is useful for more reasons than just tests.

@keithnorm
Copy link
Author

keithnorm commented May 12, 2017

I ran through the test case in mongo shell and it does concur with the test expectation:

db.docs.insert([{x:2,g:3}, {x:2,g:8} ,{x:2,g:8,z:10}, {x:3,z:3}, {x:4,k:8}, {x:6,g:9}, {x:10,k:4}, {x:undefined}, {x:null}, {x:[{k:2}, {k:8}]}])
db.docs.findOne({ x: { $in: [2, 4, 8] } })
// => { "_id" : ObjectId("591606893f674ab10d9e9c0a"), "x" : 2, "g" : 3 }

So without that change the test fails with AssertionError: expected { x: 2, g: 3 } to deeply equal { x: 4, k: 8 }

You might want to cherry pick in this commit 1768292 to get the test coverage for your implementation of findOne too.

@erikolson186
Copy link
Owner

Great work. I committed a fix, although iterating in reverse rather than using shift: cd3e377

I am going to go through some files to cherry pick. The conf.json, package.json, .gitignore, and find_one.js files all look promising.

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