consecutive query#find issue #1176

Closed
aleclofabbro opened this Issue Oct 29, 2012 · 12 comments

Projects

None yet

2 participants

@aleclofabbro

I think i found a little bug,
if trying to apply consecutive find on a query there's a particular conditions where it fails ..

query.find({field:{$lte:'someterm' }});
query.find({field:'someotherterm'});

throws this on the 2nd:

TypeError: Object.keys called on non-object
at Function.keys (native)
at merge (/usr/lib/node_modules/mongoose/lib/utils.js:383:21)
at merge (/usr/lib/node_modules/mongoose/lib/utils.js:392:7)
at Query.find (/usr/lib/node_modules/mongoose/lib/query.js:204:5)
...
...

it tries to sub-merge the object {$lte:'someterm' } into the string 'someotherterm' and fails.

if i invert the order of the calls to find

query.find({field:'someotherterm'});
query.find({field:{$lte:'someterm' }});

no error is thrown and the query can execute, but i didn't dig if the queries merge ok

@aleclofabbro

update:
it fails also with objects, if they are homogeneous:

query.find({field:{$lte:'someterm' }});
query.find({field:{$lte:'someotherterm' }});

@aheckmann
Collaborator

thanks for reporting this. we'll get it fixed.

@aheckmann aheckmann closed this in 168f32e Oct 30, 2012
@aheckmann
Collaborator

fixed. assigning a new value to an argument using find will overwrite the previous value.

use the .gt(), .lt() etc helper methods for merging.

@aleclofabbro

good!
but.. overwriting shuts the door to a feature that i consider important..
this may be a PR..
in the system i'm writing there's a security function-chain in wich each one adds some query restrictions consecutively, with no back/for-ward visibility..
if a function adds a restriction it shouldn't overwrite the previous...
it could likely be an optional argument to find and all the helper methods.. like:
Query#find([criteria], [callback], [merge])
Query#equals(val, [merge])
Query#gt(path, val, [merge])
....
....

btw why do you say "use the .gt(), .lt() etc helper methods for merging."?
do those methods merge unlike find?

thanks, aaron!
Alec

@aleclofabbro

I tryed:

query.where('field').gte('A')
.where('field').gte('L')
this retrieve fileds gte L

query.where('field').gte('L')
.where('field').gte('A')
this retrieve fileds gte A

so it overwrites... or makes an "OR"
..
before i was talking about a [merge] optional argument.. it could be an [and/or] optional argument

@aheckmann
Collaborator

gt() lt() etc are able specify more conditions for a specific field vs find() which will overwrite the previous condition.

@aleclofabbro

Aaron,
as i pointed out above, lt() gt() etc also overwrite the previous if referred to the same field...

I was looking now at the query.js code..
there's a Query.prototype.or and a Query.prototype.nor...
there could be a Query.prototype.and as well.. coded exactly the same as the others above..
why it has been omitted? it's useful..

@aleclofabbro

this function is s just a copy of Query.prototype.or and Query.prototype.nor..
I haven't test it deeeeply, but it seems to work well as it should:

Query.prototype.and = function and (array) {
var and = this._conditions.$and || (this._conditions.$and = []);
if (!Array.isArray(array)) array = [array];
and.push.apply(and, array);
return this;
}

is there any cons on implement it

..
..
btw ... I MUST learn Git / GitHub !!! :(

@aheckmann
Collaborator

@aleclofabbro submit this as a pull request and I'll accept it. its a good opportunity for you to get your feet wet with git / github / and mongoose :)

@aleclofabbro

I don't need paternity for this, i just did nothing..
but i'd like to go through the procedure so i can get introduced into this github, and possibly contribute better in the future...
please gimmeahand to submit a PR ..
should i clone mongoose in my account and modify it .. or make a branch here, or what?
but maybe it's better and faster if you just get the snippet and do it yourself w/out the PR..
and later when i'll be a master of github we'll hook up..

@aheckmann
Collaborator

It's a bit confusing at first but you'll be glad you did it.

The general idea is that you "fork" the project you'd like to contribute to, clone your fork, make changes in your fork, then submit your changes as a "pull request" which can then be reviewed / merged by the project.

Here are some github fork instructions: they are little out of date with regards to screen shots of the github ui but its pretty close: https://help.github.com/articles/fork-a-repo

all about how pull requests work: https://help.github.com/articles/using-pull-requests/

guidelines for code submitted to this project: https://github.com/LearnBoost/mongoose/blob/master/CONTRIBUTING.md

@aleclofabbro

thanks Aaron for help!
at least i sweat and did it..
unfortunately i'm working on a project quite hard right now so i'll not have much time to dig into in git..
but gimme an advice..
i'd like to use my mongoose fork in my dev environment,
is there some gotcha and best practices to avoid messups?

@aheckmann aheckmann added a commit that referenced this issue Nov 10, 2012
@aheckmann aheckmann fixed; overwriting of query arguments
closes #1176
948ae25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment