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

implement "sugar" wrapper method for findAll() followed by count() #533

Merged
merged 9 commits into from Jun 21, 2013

Conversation

5 participants
@iamjochem
Contributor

iamjochem commented Apr 10, 2013

in response to: #531

@RobGonda

This comment has been minimized.

RobGonda commented Apr 10, 2013

@iamjochem code looking good, had to reset the order option too since it was breaking the count

// no limit or offset for the options given to count()
copts.offset = 0;
copts.limit  = 0;
copts.order  = '';
@iamjochem

This comment has been minimized.

Contributor

iamjochem commented Apr 10, 2013

@RobGonda - fixed and added a test

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 10, 2013

Wonder if it should actually be findAllAndCount, for consistency with findAll, but that would be one ugly function name.

@iamjochem

This comment has been minimized.

Contributor

iamjochem commented Apr 10, 2013

I was struggling with a good name as well ... personally I don't agree with find() ... to me it implies 'find whatever matches' (i.e. what findAll() does). personally I would deprecate find() in favor of findOne().

maybe this function should be called findAndCountAll() ... less ugly?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 10, 2013

It's being discussed having find be findAll and findOne be find. But it would be a 2.0 thing in any case :)

Yea, findAndCountAll looks nicer

@iamjochem

This comment has been minimized.

Contributor

iamjochem commented Apr 10, 2013

with regard to the 2.0 discussion regarding find() ... I would not do the rename, it is tempting to do I imagine but would probably cause endless confusion ... better to just drop it (or start logging deprecated warnings) ... just a thought.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 10, 2013

Weren't you just advocating that current findAll should be find and the current find should be findOne?
Or are you saying move to having findAll and findOne and no find?

@iamjochem

This comment has been minimized.

Contributor

iamjochem commented Apr 10, 2013

if sequelize was a blank slate I'd opt for find() being an alias of findAll(), but given that find() already exists in the wild and currentl means findOne() I would opt to deprecate find() and just use findOne() and findAll() ... I think the typing of 3 extra chars ('All') is much less of a pain than having to explain 100's of times that find() was changed from being an alias of findOne() to findAll() ... you'd mostly be explaining that to existing users of the module - and they would probably be frustrated somewhat by it.

@RobGonda

This comment has been minimized.

RobGonda commented Apr 11, 2013

One more, have to reset 'include' too

copts.include  = null;
@iamjochem

This comment has been minimized.

Contributor

iamjochem commented Apr 11, 2013

got anymore @RobGonda ? :) ... I'm still getting to grips with sequelize, I'm learning as I stumble along.

@RobGonda

This comment has been minimized.

RobGonda commented Apr 11, 2013

Same here - not an expert, but I've been need tons of features so I'm adding them along ...

improve option handling ("deep" clone of the *where* property) in fin…
…dAndCountAll() so that generating the "count" query does not break the generated "select" query (due to the query generator overwriting the options *where* property, e.g. from `["name LIKE ?", "foo%"]` to `["foo%"]`)
})
/*
// at time of writing (v1.6.0) Sequelize does not seem to support 'offset' on it's own consistently (goes wrong for PostGRES and SQLite)

This comment has been minimized.

@janmeier

janmeier Jun 5, 2013

Member

It is hard to support consistently since mysql/sqlite and PG handles it two different ways - Mysql cannot do an offset without limit

To retrieve all rows from a certain offset up to the end of the result set, you can use some large number for the second parameter.

So i guess the right way to make this work across dialects would be something like
{ offset: 1, limit: 9007199254740992 }

This comment has been minimized.

@iamjochem

iamjochem Jun 6, 2013

Contributor

hi @janmeier - I wasn't aware of the MySQL issue. I feel that the limit hack for MySQL (when a limit is not specified) should be stuffed into the MySQL dialect generator.

an alternative could be to require that limit and offset always be specified as a pair - this avoids having to implement the hack and means you'll never get that "Sequelize refuses to return trillionth+1 record when only specifying 'offset'" issue ;-).

other ORM/DBAL projects have the run into the same issues, e.g.:

https://github.com/doctrine/dbal/pull/261/files

note that there they chose for your 'big number' hackin their equivelent of the 'dialect' ... I am wondering whether the big number hack needs to take account of the size of 'MAX LONG' or should we opt to define the 'big number' as a string?

oh and do you need me to add a fix for this problem in this PR? :)

This comment has been minimized.

@janmeier

janmeier Jun 6, 2013

Member

From the MySQL docs

There is a limit of 2^32 (~4.295E+09) rows in a MyISAM table. If you build MySQL with the --with-big-tables option, the row limitation is increased to (2^32)^2 (1.844E+19) rows. See Section 2.16.2, “Typical configure Options”. Binary distributions for Unix and Linux are built with this option.

So 18446744073709552000 as a limit should be fine

Up to you, feel free to extend this PR or create a new one :)

This comment has been minimized.

@iamjochem

iamjochem Jun 6, 2013

Contributor

I thought about it - I'm of the opinion that this is not the place to fix it the problem is in the underlying code (findAndCountALL() is effectively a wrapper around findAll())

This comment has been minimized.

@janmeier

janmeier Jun 21, 2013

Member

I added another issues that the offset issue should be fixed :)

v = Utils._.clone(v);
if (Utils._.isObject(v.where))
v.where = Utils._.clone(v.where);

This comment has been minimized.

@janmeier

janmeier Jun 6, 2013

Member

lo-dash (which we use in place of underscore) has a cloneDeep function ;)

.error(emit.err)
.success(function(cnt) {
if (cnt === 0)
return emit.okay(cnt); // no records, no need for another query

This comment has been minimized.

@janmeier

janmeier Jun 6, 2013

Member

This should be emit.okay(cnt, [])

This comment has been minimized.

@iamjochem

iamjochem Jun 6, 2013

Contributor

I made it so that the custom event emitter that is 'proxying' is responsible for making sure the count is always a number and rows is always an array - that's why the 'internal' emit.okay() calls don't bother to set 'superfluous' args.

*/
it("handles limit", function() {
Helpers.async(function(done) {
User.findAndCountAll({limit: 1})/*.on('sql', console.log)*/.success(function(info) {

This comment has been minimized.

@janmeier

janmeier Jun 6, 2013

Member

Please remove the on sql

@janmeier

This comment has been minimized.

Member

janmeier commented Jun 6, 2013

When we've resolved the comments above and the semi-colons are gone this should be good to merge

@iamjochem

This comment has been minimized.

Contributor

iamjochem commented Jun 6, 2013

I merged in latest sequelize/master, replaced the crufty deep-clone hack with lo-dash deepClone(), murdered some semi-colons, removed the debug-cruft.

whatI did not do:

  1. change the emit.*() related code - the 'proxying' event emitter is where the contract for what findAndCountAll() generated is upheld ... I intentionally only coerce the values for count and row properties there and no where else (i.e. the code intention is that regardless of whatever happens under the hood, assume no actual 'error' event, you will recieve an object with an int and an array in the generated object ... so even if the semantics of findAll() change this method will continue to behave the same), can you accept it as is?
  2. the issue related to offset/limit, it's a 'problem' in the underlying functionality (i.e. the guts of findAll()) ... I guess you only mentioned it because you noticed my commented out test ... given that the 'problem' still exists leaving the commented out test might not be such a bad thing ... then again I doubt many people ever run into the "cant do an offset-with-limit-query" problem ... in practice people use both together ... should I remove the commented out test or not?
return new Utils.CustomEventEmitter(function (emitter) {
var emit = {
err : function(e) { // emit error

This comment has been minimized.

@sdepold

This comment has been minimized.

@iamjochem

iamjochem Jun 7, 2013

Contributor

I thought so too but the proxy() method only allows for straight pass through - if you wish to do any with the incoming result before emitting an event on the proxied emitter then you have to do the proxying 'manually' ... in this case I need to compose the object containing the count and rows object (in the success event handler).

it is not possible to tell the proxy() method that you only want to proxy certain events (i.e. in this case 'error' and 'sql')

This comment has been minimized.

@sdepold

sdepold Jun 7, 2013

Member

well you could just extend the method :D like emitter.proxy(anotherEmitter, ['error', 'sql])

This comment has been minimized.

@iamjochem

iamjochem Jun 7, 2013

Contributor

true but that would go against the idea of keeping PRs self-contained and as simple as possible, also I am not sure it would really make the code more understandable in this case.

just for laughs here is what I think an extended proxy() method should look like (maybe something for another PR):

  CustomEventEmitter.prototype.proxy = function(emitter, events) {
    var isa = events         ? Utils._.isArray(events)  : false
      , iso = !isa && events ? Utils._.isObject(events) : false

    proxyEventKeys.forEach(function (eventKey) {
      var fn

      if (isa && events.indexOf(eventKey) === -1) {
        return
      }

      if (iso) {
        if (!events.hasOwnProperty(eventKey))
          return

        if (Utils._.isFunction(events[eventKey]))
          fn = events[eventKey]
      }

      this.on(eventKey, fn || function(result) { emitter.emit(eventKey, result) })
    }.bind(this))
  }
@iamjochem

This comment has been minimized.

Contributor

iamjochem commented Jun 20, 2013

ping - any chance this is good enough to go in? (see my previous comment about the last changes/improvements I made based on feedback/comments)

janmeier added a commit that referenced this pull request Jun 21, 2013

Merge pull request #533 from iamjochem/feature/findAndCountSugar
implement "sugar" wrapper method for findAll() followed by count()

@janmeier janmeier merged commit 0cd2d14 into sequelize:master Jun 21, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment