fix(model): make aggregate merge attributes rather than override (#10662)

This was broken by:
https://github.com/sequelize/sequelize/commit/88a340dacec0b1b27d4f67866e2b416482ce7b85#diff-a140b60b6a99a8c69f4b5d0d1f1f2bfaR1964

So cc @u9r52sld 

CONTRIBUTING.md

@@ -25,10 +25,9 @@ We're glad to get pull request if any functionality is missing or something is b
 * Add some tests for your new functionality or a test exhibiting the bug you are solving. Ideally all new tests should not pass _without_ your changes.
 * Add some tests for your new functionality or a test exhibiting the bug you are solving. Ideally all new tests should not pass _without_ your changes.
   - Use [promise style](http://bluebirdjs.com/docs/why-promises.html) in all new tests. Specifically this means:
   - Use [promise style](http://bluebirdjs.com/docs/why-promises.html) in all new tests. Specifically this means:
     - don't use `EventEmitter`, `QueryChainer` or the `success`, `done` and `error` events
     - don't use `EventEmitter`, `QueryChainer` or the `success`, `done` and `error` events
-    - don't use nested callbacks (use [Promise.bind](http://bluebirdjs.com/docs/api/promise.bind.html) to maintain context in promise chains)
     - don't use a done callback in your test, just return the promise chain.
     - don't use a done callback in your test, just return the promise chain.
-  - Small bugfixes and direct backports to the 1.7 branch are accepted without tests.
-* If you are adding to / changing the public API, remember to add API docs, in the form of [JSDoc style](http://usejsdoc.org/about-getting-started.html) comments. See [section 4a](#4a-check-the-documentation  ) for the specifics.
+  - Small bugfixes and direct backports to the 4.x branch are accepted without tests.
+* If you are adding to / changing the public API, remember to add API docs, in the form of [JSDoc style](http://usejsdoc.org/about-getting-started.html) comments. See [section 4a](#4a-check-the-documentation) for the specifics.
 
 
 Interested? Coolio! Here is how to get started:
 Interested? Coolio! Here is how to get started:
 
 
@@ -99,7 +98,7 @@ $ docker-compose up postgres-95 mysql-57 mssql
 ### 4. Running tests
 ### 4. Running tests
 
 
 All tests are located in the `test` folder (which contains the
 All tests are located in the `test` folder (which contains the
-lovely [Mocha](http://visionmedia.github.io/mocha/) tests).
+lovely [Mocha](https://mochajs.org/) tests).
 
 
 ```sh
 ```sh
 $ npm run test-all || test-mysql || test-sqlite || test-mssql || test-postgres || test-postgres-native
 $ npm run test-all || test-mysql || test-sqlite || test-mssql || test-postgres || test-postgres-native

lib/model.js

@@ -18,6 +18,7 @@ const DataTypes = require('./data-types');
 const Hooks = require('./hooks');
 const Hooks = require('./hooks');
 const associationsMixin = require('./associations/mixin');
 const associationsMixin = require('./associations/mixin');
 const Op = require('./operators');
 const Op = require('./operators');
+const { noDoubleNestedGroup } = require('./utils/deprecations');
 
 
 
 
 // This list will quickly become dated, but failing to maintain this list just means
 // This list will quickly become dated, but failing to maintain this list just means
@@ -814,10 +815,9 @@ class Model {
     return this._baseMerge(...args, this._mergeFunction);
     return this._baseMerge(...args, this._mergeFunction);
   }
   }
 
 
-  static _defaultsOptions(...args) {
-    return this._baseMerge(...args, (...cbArgs) => {
-      [cbArgs[0], cbArgs[1]] = [cbArgs[1], cbArgs[0]];
-      return this._mergeFunction(...cbArgs);
+  static _defaultsOptions(target, opts) {
+    return this._baseMerge(target, opts, (srcValue, objValue, key) => {
+      return this._mergeFunction(objValue, srcValue, key);
     });
     });
   }
   }
 
 
@@ -1932,7 +1932,10 @@ class Model {
   static aggregate(attribute, aggregateFunction, options) {
   static aggregate(attribute, aggregateFunction, options) {
     options = Utils.cloneDeep(options);
     options = Utils.cloneDeep(options);
 
 
+    // We need to preserve attributes here as the `injectScope` call would inject non aggregate columns.
+    const prevAttributes = options.attributes;
     this._injectScope(options);
     this._injectScope(options);
+    options.attributes = prevAttributes;
     this._conformIncludes(options, this);
     this._conformIncludes(options, this);
 
 
     if (options.include) {
     if (options.include) {
@@ -1948,8 +1951,17 @@ class Model {
       aggregateColumn = this.sequelize.fn('DISTINCT', aggregateColumn);
       aggregateColumn = this.sequelize.fn('DISTINCT', aggregateColumn);
     }
     }
 
 
-    options.attributes = _.intersection(options.attributes, options.group);
-    options.attributes.push([this.sequelize.fn(aggregateFunction, aggregateColumn), aggregateFunction]);
+    let { group } = options;
+    if (Array.isArray(group) && Array.isArray(group[0])) {
+      noDoubleNestedGroup();
+      group = _.flatten(group);
+    }
+    options.attributes = _.unionBy(
+      options.attributes,
+      group,
+      [[this.sequelize.fn(aggregateFunction, aggregateColumn), aggregateFunction]],
+      a => Array.isArray(a) ? a[1] : a
+    );
 
 
     if (!options.dataType) {
     if (!options.dataType) {
       if (attrOptions) {
       if (attrOptions) {

lib/utils/deprecations.js

@@ -8,3 +8,4 @@ exports.noRawAttributes = deprecate(noop, 'Use sequelize.fn / sequelize.literal
 exports.noTrueLogging = deprecate(noop, 'The logging-option should be either a function or false. Default: console.log', 'SEQUELIZE0002');
 exports.noTrueLogging = deprecate(noop, 'The logging-option should be either a function or false. Default: console.log', 'SEQUELIZE0002');
 exports.noStringOperators = deprecate(noop, 'String based operators are deprecated. Please use Symbol based operators for better security, read more at http://docs.sequelizejs.com/manual/querying.html#operators', 'SEQUELIZE0003');
 exports.noStringOperators = deprecate(noop, 'String based operators are deprecated. Please use Symbol based operators for better security, read more at http://docs.sequelizejs.com/manual/querying.html#operators', 'SEQUELIZE0003');
 exports.noBoolOperatorAliases = deprecate(noop, 'A boolean value was passed to options.operatorsAliases. This is a no-op with v5 and should be removed.', 'SEQUELIZE0004');
 exports.noBoolOperatorAliases = deprecate(noop, 'A boolean value was passed to options.operatorsAliases. This is a no-op with v5 and should be removed.', 'SEQUELIZE0004');
+exports.noDoubleNestedGroup = deprecate(noop, 'Passing a double nested nested array to `group` is unsupported and will be removed in v6.', 'SEQUELIZE0005');

test/integration/model.test.js

@@ -1914,6 +1914,19 @@ describe(Support.getTestDialectTeaser('Model'), () => {
       });
       });
     });
     });
 
 
+    describe('aggregate', () => {
+      if (dialect === 'mssql') {
+        return;
+      }
+      it('allows grouping by aliased attribute', function() {
+        return this.User.aggregate('id', 'count', {
+          attributes: [['id', 'id2']],
+          group: ['id2'],
+          logging: true
+        });
+      });
+    });
+
     describe('options sent to aggregate', () => {
     describe('options sent to aggregate', () => {
       let options, aggregateSpy;
       let options, aggregateSpy;