bookshelf / bookshelf

A simple Node.js ORM for PostgreSQL, MySQL and SQLite3 built on top of Knex.js
http://bookshelfjs.org
MIT License
6.36k stars 573 forks source link

Model.count() does not parse count if Model.query() is called #882

Closed blah238 closed 9 years ago

blah238 commented 9 years ago

In 0.8.2, Model.count() was added with the following description:

Gets the number of matching records in the database, respecting any previous calls to query. If a column if provided, records with a null value in that column will be excluded from the count.

This works if you use Model.where().count() without calling .query(), but if you call Model.query().where().count() or any other query, the value returned (at least under PostgreSQL) is a weird data structure like:

[ { count: '0' } ]

which has to be parsed with an ugly function like this (I'm using Lodash here):

function parseCount (countQueryResult) {
  return parseInt(_.first(_.values(_.first(countQueryResult))), 10);
}

It would be nice if this was handled automatically, or at least documented. Also the documentation is a little bit misleading in that it says it respects previous calls to query() which isn't quite the case, as if you call query() yourself it just defers to the Knex count().

Side note: under PostgreSQL counts are returned as strings, although this can be changed.

rhys-vdw commented 9 years ago

So what's really happening here is that you're calling two different count() functions. The new method Collection#count, and QueryBuilder#count from Knex.js.

Model#query() or Collection#query() return a QueryBuilder when called without arguments. So you're just calling QueryBuilder.count() in your second example there.

What were you trying to achieve by calling .query() without arguments in the chain?

blah238 commented 9 years ago

Sorry, that was just an oversimplification. The query I have in mind is to count the models that have one of several values, e.g. Model.query().whereIn('value', values).count(). Perhaps whereIn could get the same treatment as Model.where() in Bookshelf?

rhys-vdw commented 9 years ago

Perhaps whereIn could get the same treatment as Model.where() in Bookshelf?

I'm open to that.

For now do this:

Model.query('whereIn', 'value', values).count()
rhys-vdw commented 9 years ago

Also, I wonder if maybe deprecating .query() with no arguments in favor of .queryBuilder() would be helpful to clarify this.

blah238 commented 9 years ago

That works great. What's the difference exactly?

rhys-vdw commented 9 years ago

Calling query without arguments returns the underlying Knex QueryBuilder. I think it's explained in the docs. On Sat, 29 Aug 2015 at 11:58 am blah238 notifications@github.com wrote:

That works great. What's the difference exactly?

— Reply to this email directly or view it on GitHub https://github.com/tgriesser/bookshelf/issues/882#issuecomment-135927305 .

rhys-vdw commented 9 years ago

Leaving this open to consider the option of renaming query() (no arguments) to queryBuilder() to eliminate this point of confusion. Having a consistent contract for query() seems preferable.

sgentile commented 9 years ago

I'm having a difficult time trying to implement count, I looked in the tests and see an example, but when I implement it I get errors.

Examples is:

I need to get the blog and the number of posts

return new Blog({'id': id}).posts().count().where(...).fetch(...)

gives error: TypeError: Uncaught error: (intermediate value).where(...).fetch(...).posts is not a function

in my model I have:

posts: function () { return this.hasMany("Post", "blogId"); }

All works find when I do a withRelated - but I don't want to pull down all the posts, I just want a count ?

rhys-vdw commented 9 years ago

Hi @sgentile. count returns a promise. You must call where before count. You can call count or fetch - not both. Look at the tests again.

In future please create a new issue rather than commenting on closed issues. I think part of the problem is that there is no example in the docs, so it would be good to have a new issue for this if you wouldn't mind.