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

findAndCountAll with scope having attributes #4566

Closed
scboffspring opened this Issue Sep 25, 2015 · 9 comments

Comments

6 participants
@scboffspring

scboffspring commented Sep 25, 2015

When having a defaultScope defined, which contains the attributes options, the count query is messed up.

defaultScope: {
        where: {
          active: true
        },
        attributes: ['id', 'username', 'email', 'active', 'approved', 'company_id']
      }

The following query is generated

SELECT "id", "username", "email", "active", "approved", "company_id", count(*) AS "count" FROM "users" AS "users" WHERE "users"."company_id" = 1 AND "users"."active" = true;

2 errors within the query. The first one, in that case, the "group by" is required. The second error is that it should normally not include the "attributes" for the count query.
If I fix the first one (add the "group" option), the query runs, but "res.count" is actually returning anarray of objects from the query, with the "count" field inside.

Example :

{ count:
   [ { id: 1,
       username: 'Test',
       email: 'test@test,cin',
       active: true,
       approved: true,
       company_id: 1,
       count: '1' },
     { id: 2,
       username: 'Test2',
       email: 'test@test.com',
       active: true,
       approved: true,
       company_id: 1,
       count: '1' } ],
      rows:....
}

The error come from the file https://github.com/sequelize/sequelize/blob/master/lib/model.js

The findAndCountAll strips correctly the attributes options (https://github.com/sequelize/sequelize/blob/master/lib/model.js#L1604)
But then within the count query, the scope is injected with the attributes and all other options that should be omitted (https://github.com/sequelize/sequelize/blob/master/lib/model.js#L1542)

I'd be happy to fix it, but I do not know what would be the best possible option to fix it. I see the following possibilities :

  • Add a parameter to the count function to omit the attributes/... options after the scope is injected
  • Separate the count function in 2, one which will prepare the parameters for the normal count, and the second that actually does the count. This way the findAndCount function would use directly the "count" part, as the option is already ready.

Any toughs on this ?

Thanks

@janmeier

This comment has been minimized.

Member

janmeier commented Oct 4, 2015

I think setting countOptions.attributes = [] would fix the issue as well - that would prevent the attribute scope being injected in count. We still want to inject the where part of the scope, so I don't think splitting the function into two would be a good idea.

PR welcome :)

@valtlfelipe

This comment has been minimized.

valtlfelipe commented Jan 11, 2016

Any update on this? On version 3.17.1 it's not fixed anymore.

@janmeier

This comment has been minimized.

Member

janmeier commented Jan 11, 2016

@valtlfelipe Please show a test case then

@valtlfelipe

This comment has been minimized.

valtlfelipe commented Jan 11, 2016

@janmeier same issue as @scboffspring related. I have attributes defined in my model. And by executing an findAndCountAll it throws an SQL error in Postgres that group by is required for the id column. But, if the attributes is not defined in my model, it generates the correct SQL with DISTINCT.

@valtlfelipe

This comment has been minimized.

valtlfelipe commented Jan 11, 2016

@janmeier more practical example:

defaultScope: {
            where: {
                status: 'active'
            },
            attributes: ['id', 'name', 'user_name', 'email', 'status']
        },

This generates the following query:

SELECT "id", "name", "user_name", "email", "status", count(*) AS "count" FROM "user" AS "ClientUser" WHERE "ClientUser"."status" = 'active';

And Postgres throws the following error:

ERROR:  column "User.id" must appear in the GROUP BY clause or be used in an aggregate function
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 11, 2016

Considering the PR merged introduced a regression which is still active this issue should still be fixed.

@krisr

This comment has been minimized.

krisr commented Jan 11, 2016

The other regression this introduced was that it just ignores the documented attributes parameter that is passed into count, such that we can no longer do group by counts that select additional attributes.

Also, this PR did not address the original issue in Model.prototype.aggregate which presumably suffers from the same issue with default scopes as Model.prototype.count.

I think properly addressing the original issue requires more careful thought. I propose that we revert this fix all together because I suspect it does more harm than good at this point. Alternatively, a compromise may be to set the attributes to those passed in by the caller, instead of setting them to the empty array.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 12, 2016

@krisr That's THE regression as far as i can tell :)

@jay-depot

This comment has been minimized.

jay-depot commented Feb 25, 2017

@mickhansen It seems like the regression mentioned here is alive and well. This is a HUGE problem if your query uses attributes to create virtual columns, something like SELECT some_Function_Of("table"."column") AS property WHERE property = 'foo'; since during the COUNT step, the query bombs out on the error that column property doesn't exist. Which it would, if we weren't stripping off attributes indiscriminately here.

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