Skip to content

Commit

Permalink
feat(scopes): unpack Op.and at the top most level if not needed
Browse files Browse the repository at this point in the history
  • Loading branch information
@EtienneTurc
EtienneTurc committed Feb 28, 2022
1 parent b83c88a commit c980457
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 84 deletions.
67 changes: 53 additions & 14 deletions src/model.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -861,20 +861,7 @@ class Model {
}

if (['where', 'having'].includes(key)) {
let objKeysToKeep;
if (objValue) {
const objKeys = Object.getOwnPropertyNames(objValue).concat(Object.getOwnPropertySymbols(objValue));
objKeysToKeep = _.filter(objKeys, key => key !== Op.and);
}

return {
[Op.and]: _.compact([
...(objValue?.[Op.and] ?? []),
!_.isEmpty(objKeysToKeep) && _.pick(objValue, objKeysToKeep),
srcValue,
]),
};

return combineWheresWithAnd(objValue, srcValue);
} else if (key === 'attributes' && _.isPlainObject(objValue) && _.isPlainObject(srcValue)) {
return _.assignWith(objValue, srcValue, (objValue, srcValue) => {
if (Array.isArray(objValue) && Array.isArray(srcValue)) {
Expand Down Expand Up @@ -4772,6 +4759,58 @@ class Model {
static belongsTo(target, options) {}
}

/**
* Unpacks an object that only contains a single Op.and key to the value of Op.and
*
* Internal method used by {@link combineWheresWithAnd}
*
* @param {WhereOptions} where The object to unpack
* @example `{ [Op.and]: [a, b] }` becomes `[a, b]`
* @example `{ [Op.and]: { key: val } }` becomes `{ key: val }`
* @example `{ [Op.or]: [a, b] }` remains as `{ [Op.or]: [a, b] }`
* @example `{ [Op.and]: [a, b], key: c }` remains as `{ [Op.and]: [a, b], key: c }`
* @private
*/
function unpackAnd(where) {
if (!_.isObject(where)) {
return where;
}

const keys = Utils.getComplexKeys(where);

// object is empty, remove it.
if (keys.length === 0) {
return;
}

// we have more than just Op.and, keep as-is
if (keys.length !== 1 || keys[0] !== Op.and) {
return where;
}

const andParts = where[Op.and];

return andParts;
}

function combineWheresWithAnd(whereA, whereB) {
const unpackedA = unpackAnd(whereA);

if (unpackedA === undefined) {
return whereB;
}

const unpackedB = unpackAnd(whereB);

if (unpackedB === undefined) {
return whereA;
}

return {
[Op.and]: [unpackedA, unpackedB].flat(),
};
}

Object.assign(Model, associationsMixin);
Hooks.applyTo(Model, true);

Expand Down
8 changes: 7 additions & 1 deletion test/support.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const fs = require('fs');
const path = require('path');
const { isDeepStrictEqual } = require('util');
const { inspect, isDeepStrictEqual } = require('util');
const _ = require('lodash');

const Sequelize = require('@sequelize/core');
Expand All @@ -18,6 +18,12 @@ chai.use(require('chai-datetime'));
chai.use(require('chai-as-promised'));
chai.use(require('sinon-chai'));

// Using util.inspect to correctly assert objects with symbols
// Because expect.deep.equal does not test non iterator keys such as symbols (https://github.com/chaijs/chai/issues/1054)
chai.Assertion.addMethod('deepEqual', function (expected, depth = 5) {
expect(inspect(this._obj, { depth })).to.deep.equal(inspect(expected, { depth }));
});

chai.config.includeStack = true;
chai.should();

Expand Down
23 changes: 7 additions & 16 deletions test/unit/model/include.test.js
View file
Original file line number Diff line number Diff line change
@@ -1,23 +1,14 @@
'use strict';

const chai = require('chai');
const util = require('util');

const expect = chai.expect;
const Support = require('../support');
const Sequelize = require('@sequelize/core');

const Op = Sequelize.Op;
const { Sequelize, Op } = require('@sequelize/core');

const current = Support.sequelize;

describe(Support.getTestDialectTeaser('Model'), () => {
// Using util.inspect to correctly assert objects with symbols
// Because expect.deep.equal does not test non iterator keys such as symbols (https://github.com/chaijs/chai/issues/1054)
chai.Assertion.addMethod('deepEquals', function (expected, depth = 5) {
expect(util.inspect(this._obj, { depth })).to.deep.equal(util.inspect(expected, { depth }));
});

describe('all', () => {
const Referral = current.define('referal');

Expand Down Expand Up @@ -188,7 +179,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
include: [{ model: this.Project }],
});

expect(options.include[0]).to.have.property('where').which.deepEquals({ [Op.and]: [{ active: true }] });
expect(options.include[0]).to.have.property('where').which.deepEqual({ active: true });
});

it('adds the where from a scoped model', function () {
Expand All @@ -197,7 +188,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
include: [{ model: this.Project.scope('that') }],
});

expect(options.include[0]).to.have.property('where').which.deepEquals({ [Op.and]: [{ that: false }] });
expect(options.include[0]).to.have.property('where').which.deepEqual({ that: false });
expect(options.include[0]).to.have.property('limit').which.equals(12);
});

Expand All @@ -207,7 +198,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
include: [{ model: this.Project.scope('attr') }],
});

expect(options.include[0]).to.have.property('attributes').which.deepEquals(['baz']);
expect(options.include[0]).to.have.property('attributes').which.deepEqual(['baz']);
});

it('merges where with the where from a scoped model', function () {
Expand All @@ -216,7 +207,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
include: [{ where: { active: false }, model: this.Project.scope('that') }],
});

expect(options.include[0]).to.have.property('where').which.deepEquals({ [Op.and]: [{ that: false }, { active: false }] });
expect(options.include[0]).to.have.property('where').which.deepEqual({ [Op.and]: [{ that: false }, { active: false }] });
});

it('add the where from a scoped associated model', function () {
Expand All @@ -225,7 +216,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
include: [{ model: this.Project, as: 'thisProject' }],
});

expect(options.include[0]).to.have.property('where').which.deepEquals({ [Op.and]: [{ this: true }] });
expect(options.include[0]).to.have.property('where').which.deepEqual({ this: true });
});

it('handles a scope with an aliased column (.field)', function () {
Expand All @@ -234,7 +225,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
include: [{ model: this.Project.scope('foobar') }],
});

expect(options.include[0]).to.have.property('where').which.deepEquals({ [Op.and]: [{ foo: 42 }] });
expect(options.include[0]).to.have.property('where').which.deepEqual({ foo: 42 });
});
});

Expand Down
88 changes: 35 additions & 53 deletions test/unit/model/scope.test.js
View file
Original file line number Diff line number Diff line change
@@ -1,24 +1,16 @@
'use strict';

const chai = require('chai');
const util = require('util');

const expect = chai.expect;
const Sequelize = require('@sequelize/core');
const { Sequelize, Op } = require('@sequelize/core');

const Op = Sequelize.Op;
const Support = require('../support');
const DataTypes = require('@sequelize/core/lib/data-types');

const current = Support.sequelize;

describe(Support.getTestDialectTeaser('Model'), () => {
// Using util.inspect to correctly assert objects with symbols
// Because expect.deep.equal does not test non iterator keys such as symbols (https://github.com/chaijs/chai/issues/1054)
chai.Assertion.addMethod('deepEqual', function (expected, depth = 5) {
expect(util.inspect(this._obj, { depth })).to.deep.equal(util.inspect(expected, { depth }));
});

const Project = current.define('project');
const User = current.define('user');

Expand Down Expand Up @@ -167,35 +159,28 @@ describe(Support.getTestDialectTeaser('Model'), () => {

expect(scoped1._scope).to.deepEqual({
where: {
[Op.and]: [
{ something: true, somethingElse: 42 },
],
something: true,
somethingElse: 42,
},
limit: 5,
});
expect(scoped2._scope).to.deepEqual({
where: {
[Op.and]: [
{ something: false },
],
something: false,
},
});
});

it('should work with function scopes', () => {
expect(Company.scope({ method: ['actualValue', 11] })._scope).to.deepEqual({
where: {
[Op.and]: [
{ other_value: 11 },
],
other_value: 11,
},
});

expect(Company.scope('noArgs')._scope).to.deepEqual({
where: {
[Op.and]: [
{ other_value: 7 },
],
other_value: 7,
},
});
});
Expand All @@ -204,17 +189,13 @@ describe(Support.getTestDialectTeaser('Model'), () => {
const scope = { method: ['actualValue', 11] };
expect(Company.scope(scope)._scope).to.deepEqual({
where: {
[Op.and]: [
{ other_value: 11 },
],
other_value: 11,
},
});

expect(Company.scope(scope)._scope).to.deepEqual({
where: {
[Op.and]: [
{ other_value: 11 },
],
other_value: 11,
},
});
});
Expand Down Expand Up @@ -244,7 +225,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
expect(Company.scope('projects', 'users', 'alsoUsers')._scope).to.deepEqual({
include: [
{ model: Project },
{ model: User, where: { [Op.and]: [{ something: 42 }] } },
{ model: User, where: { something: 42 } },
],
});

Expand All @@ -264,9 +245,8 @@ describe(Support.getTestDialectTeaser('Model'), () => {
it('should be able to override the default scope', () => {
expect(Company.scope('somethingTrue')._scope).to.deepEqual({
where: {
[Op.and]: [
{ something: true, somethingElse: 42 },
],
something: true,
somethingElse: 42,
},
limit: 5,
});
Expand Down Expand Up @@ -397,8 +377,10 @@ describe(Support.getTestDialectTeaser('Model'), () => {
const expected = {
where: {
[Op.and]: [
{ [Op.and]: [{ field: 1 }, { field: 1 }] },
{ [Op.and]: [{ field: 2 }, { field: 2 }] },
{ field: 1 },
{ field: 1 },
{ field: 2 },
{ field: 2 },
],
},
};
Expand All @@ -410,9 +392,12 @@ describe(Support.getTestDialectTeaser('Model'), () => {
const expected = {
where: {
[Op.and]: [
{ [Op.and]: [{ field: 1 }, { field: 1 }] },
{ [Op.and]: [{ field: 2 }, { field: 2 }] },
{ [Op.and]: [{ field: 3 }, { field: 3 }] },
{ field: 1 },
{ field: 1 },
{ field: 2 },
{ field: 2 },
{ field: 3 },
{ field: 3 },
],
},
};
Expand Down Expand Up @@ -455,8 +440,10 @@ describe(Support.getTestDialectTeaser('Model'), () => {
[Op.and]: [
{ [Op.or]: [{ field: 1 }, { field: 1 }] },
{ [Op.or]: [{ field: 2 }, { field: 2 }] },
{ [Op.and]: [{ field: 1 }, { field: 1 }] },
{ [Op.and]: [{ field: 2 }, { field: 2 }] },
{ field: 1 },
{ field: 1 },
{ field: 2 },
{ field: 2 },
],
},
};
Expand All @@ -468,8 +455,10 @@ describe(Support.getTestDialectTeaser('Model'), () => {
const expected = {
where: {
[Op.and]: [
{ [Op.and]: [{ field: 1 }, { field: 1 }] },
{ [Op.and]: [{ field: 2 }, { field: 2 }] },
{ field: 1 },
{ field: 1 },
{ field: 2 },
{ field: 2 },
{ [Op.or]: [{ field: 1 }, { field: 1 }] },
{ [Op.or]: [{ field: 2 }, { field: 2 }] },
],
Expand Down Expand Up @@ -499,7 +488,8 @@ describe(Support.getTestDialectTeaser('Model'), () => {
where: {
[Op.and]: [
{ field: 1 },
{ [Op.and]: [{ field: 1 }, { field: 1 }] },
{ field: 1 },
{ field: 1 },
{ [Op.or]: [{ field: 1 }, { field: 1 }] },
Sequelize.where('field', Op.is, 1),
],
Expand All @@ -512,11 +502,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {

it('should be able to use raw queries', () => {
expect(Company.scope([{ method: ['complexFunction', 'qux'] }])._scope).to.deepEqual({
where: {
[Op.and]: [
['qux IN (SELECT foobar FROM some_sql_function(foo.bar))'],
],
},
where: ['qux IN (SELECT foobar FROM some_sql_function(foo.bar))'],
});
});

Expand All @@ -526,7 +512,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
where: {
[Op.and]: [
{ active: true },
['qux IN (SELECT foobar FROM some_sql_function(foo.bar))'],
'qux IN (SELECT foobar FROM some_sql_function(foo.bar))',
],
},
});
Expand Down Expand Up @@ -569,9 +555,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {

expect(Company.scope('newScope')._scope).to.deepEqual({
where: {
[Op.and]: [
{ this: 'that' },
],
this: 'that',
},
include: [{ model: Project }],
});
Expand All @@ -592,9 +576,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {

expect(Company.scope('somethingTrue')._scope).to.deepEqual({
where: {
[Op.and]: [
{ something: false },
],
something: false,
},
});
});
Expand Down

0 comments on commit c980457

Please sign in to comment.