search

geddy / model

Datastore-agnostic ORM in JavaScript
265 stars 55 forks source link

Empty requests crash the adapter #193

Open danfinlay opened 10 years ago

danfinlay commented 10 years ago

Right now if you say: geddy.model.Something.all({id:[ ]}, function(err, results){ // anything })

You'll crash the Postgres adapter, every time. The log is hard to identify, it looks like this:

error: syntax error at or near ")"
    at Connection.parseE

It refers to making a postgres request with an empty array. This should really be handled better. I've gotten good at this, but it was a real obstacle the first time I encountered it.

benatkin commented 10 years ago

@flyswatter what do you think it should do? Should it return zero results?

danfinlay commented 10 years ago

I do think the best choice is to return an empty array, since it performs the action the user requested.

I guess you could throw an error instead, but I don't think it's an error if you're already handling it.

benatkin commented 10 years ago

@flyswatter that's what I think, too. I asked because I'm considering making a pull request to fix this, sounds like a good first code contribution to model.

danfinlay commented 10 years ago

Awesome! I think this will spare many people future grief!

mde commented 10 years ago

Would love a PR for this!

ben-ng commented 10 years ago

@benatkin @mde what do you think about fixing this in the query transformer? Seems like a query like this should be reduced to false and match nothing.

benatkin commented 10 years ago

@ben-ng I like that idea.

mde commented 10 years ago

Sounds good. Would love a PR. :D

benatkin commented 10 years ago

I started looking at it but didn't finish. I saw something that said "please return null on empty operations" that sounds like it's supposed to solve this problem.

I'm going to be busy for the next few days, so if one of you gets the opportunity to work on it, please go ahead!

ben-ng commented 10 years ago

@benatkin I put that in there. There was an earlier bug where something like Posts.all({}, cb) would fail in the same way.

Fixing this now.

ben-ng commented 10 years ago

Lines 120 and 136 already handle this. It looks like I did actually fix it in 33e353a9c45b86656939c24f9b89d43eb6eb5dbc

@flyswatter are you on the latest version of model? I just added a test for this to see if maybe it fails in some other adapter. I've verified that its working with postgres.

ben-ng commented 10 years ago

Ok, this was still breaking in the memory adapter, but I just fixed that. @flyswatter if you are up-to-date and still seeing breakage in postgres.. that's weird! We'll need more info to debug this.