Resolves #69 - in which or(where, whereOpt(None)) creates a bad query #71

Merged
merged 2 commits into from Dec 26, 2012

Conversation

Projects
None yet
2 participants
Contributor

ktoso commented Dec 25, 2012

Hello again,
It's been some time since I found #69, but since it's now Christmas 🎄 I found some time and came back to this and fixed the issue. :-)

Reminding the problem again:

// what was the problem:
Venue.where(_.or(_.where(_.email eqs "   "), _.where(None)(_.neverMind eqs _)))

// we got:
{"$or": [{"email": "   "}, {  } ]}
                 ^ false     ^ always true => we fetch the entire collection

// what we get with this fix:
{"$or": [ {"email": "   "} ]} // as expected

There are tests for both cases attached (Some/None) and a fix.

I have added a method to AndCondition, if that's something you'd rather want to keep without methods please let me know where I should move this isEmpty method (or don't do it as a method?).

Thanks again for the great library,
Looking forward to the 2.0 / 2.10 releases!

Konrad

ktoso added some commits Dec 25, 2012

@ktoso ktoso failing test for "or + where + whereOpt" edge case, issue #69 20eb932
@ktoso ktoso resolves #69, a blank `{}` should not be included in or queries
This solves an edge case when you have an or(_.where, _.whereOpt(None))
which would before leave a blank `{}` in the query string, making misbehave.

I've added `isEmpty` to `AndCondition`, please let me know if you'd
rather keep it method-less, and we can find a better place for this
method (or don't introduce one at all?)
b979c66

@jliszka jliszka added a commit that referenced this pull request Dec 26, 2012

@jliszka jliszka Merge pull request #71 from ktoso/master
Resolves #69 - in which or(where, whereOpt(None)) creates a bad query
3b6c91e

@jliszka jliszka merged commit 3b6c91e into foursquare:master Dec 26, 2012

Collaborator

jliszka commented Dec 26, 2012

Thanks! Sorry, I had put the fix in the v2 branch awhile back, forgot to port it back.

Contributor

ktoso commented Dec 26, 2012

No problem :) Glad to hear it's fixed in 2.0 already.

Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment