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

Mquery Integration #1562

Closed
wants to merge 53 commits into from
Closed

Conversation

ebensing
Copy link
Contributor

@ebensing ebensing commented Jul 1, 2013

This is Mongoose working with the mquery library instead of its own. All unit tests are passing.

basic operations seem to work, have not done unit testing though.
More complicated stuff such as mapreduce have not been re-impemented
also, need to reorganized the mongoosequery.js code
there were a couple bug fixes in the actual tests, as well as updates to
MongooseQuery from Query. Most bug fixes were in the MongoosQuery and
Model methods though.
had to stop using mquery's _findAndModify due to casting issues
wrapped mquery's exec() to make sure it returns a promise
needed to be using the _mongooseOptions, not the mquery options
fixed a lot of bugs related the populate() options, all of those tests
now passing
all of these tests are now passing. A couple updates were made to the
tests themselves for this in order to make them consistent with the new
semantics in mquery. (namely, that _updateArgs is now _update and if you
pass no arguments to the update functions, _update is undefined, not an
empty object)
all of these tests are now passing.
fixed an issue where improper values were being passed to count() in
certain situations. also, deleted some trailing whitespace in
mode.test.js
fixed a bug where if a single document was passed into find(), it would
try and merge the whole thing instead of just grabbing the attributes
needed to add the stream method to MongooseQuery and update a couple
small things in MongooseStream to make sure the appropriate options were
being used.
All tests pass now except for the ones in query.test.js which
specifically target the Query object and not the MongooseQuery object.
Probably need to actually port most of those tests to MongooseQuery
Had to make some changes to make sure appropriate things were happening
when whole documents were passed into findOne() and update(). Also,
updated a test because it seemed wrong... not sure why it ever passed.
In the process of updating this to work with mquery
All tests except certain within() without a where() tests are passing.
still a couple tests that need to be fixed / adjusted
Sans the couple within() queries that need to be worked out, all tests
in query.test.js are now updated to use MongooseQuery and they pass
instead of stuff like within.box(..) you now use the mquery syntax of
where(...).within().box(...). All tests are passing
need to make sure to keep the 'mongodb://' portion of the url at the
start of the replica set uri
});
}

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be able to ditch getVersion now right?

query.near('checkin', { center : [40, -72]});
assert.deepEqual(query._conditions, {checkin: {$near: [40, -72]}});
done();
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

duplicate?

Now, mongoose will check the version of the database and use $within or
$geoWithin appropriately based off of what version of the db is running.
Tests have also been updated to reflect this.
Mquery is not compatible with the old syntax.
Mquery and Mongoose have different arg orders for .distinct() this patch
fixes it such that things will behave appropriately when passed to
mquery in certain cases.
This is mainly a workaround for a driver error that has been patched in
a future release. The driver was not checking the results it got back
from geoSearch and just passing the results straight back instead of
creating an error and passing that back.
This now defaults to geoWithin and must be explicitly set by the user if
they want to use $within
mquery's

and updated tests to use the property appropriately
readPreference tests are failing though because of instaceof conflicts
There was an issue with the tests not properly detecting that read
options were actually readPreference objects. I believe this is because
the readPreference objects were now imported out of mquery, so I just
setup mquery to export the reference to the readPreference it uses.
Added back a specific test on sort(), which was mistakenly removed.
Also, updated the populate tests to use the new, supported syntax for
sorting.
their return values

essentially, they just executed the method instead of returning it, this
has been fixed, and now the wrapped collection should act just like a
normal collection.
Not checking for value of res before using it, fixed now
should now be able to safely remove query.js from the repo
@aheckmann
Copy link
Collaborator

merged!

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.

None yet

2 participants