From d36f632bca487fa3d7b9a0aad16dcbd56de5223b Mon Sep 17 00:00:00 2001 From: fratzinger <22286818+fratzinger@users.noreply.github.com> Date: Sun, 10 Mar 2024 13:38:05 +0100 Subject: [PATCH] fix: rm option returning for single calls, sort for multi patch --- src/adapter.ts | 73 +++++++----------- src/index.ts | 1 + src/utils.ts | 7 +- test/index.test.ts | 183 ++++++++++++++++++++++----------------------- test/utils.test.ts | 35 +++++++++ 5 files changed, 159 insertions(+), 140 deletions(-) diff --git a/src/adapter.ts b/src/adapter.ts index 818d65f..4bce74d 100644 --- a/src/adapter.ts +++ b/src/adapter.ts @@ -306,7 +306,7 @@ export class SequelizeAdapter< .bulkCreate(data as any[], sequelize) .catch(errorHandler); - if (!sequelize.returning) { + if (sequelize.returning === false) { return []; } @@ -337,10 +337,6 @@ export class SequelizeAdapter< .create(data as any, sequelize as CreateOptions) .catch(errorHandler); - if (!sequelize.returning) { - return []; - } - if (sequelize.raw) { return select((result as Model).toJSON()) } @@ -377,33 +373,44 @@ export class SequelizeAdapter< ids = current.map((item: any) => item[this.id]); - sequelize.where = { - [this.id]: ids.length === 1 ? ids[0] : { [this.Op.in]: ids } - } - // @ts-ignore let [, instances] = await Model - .update(values, sequelize as UpdateOptions) + .update(values, { + ...sequelize, + where: { [this.id]: ids.length === 1 ? ids[0] : { [this.Op.in]: ids } } + } as UpdateOptions) .catch(errorHandler) as [number, Model[]?]; - if (!sequelize.returning) { + if (sequelize.returning === false) { return [] } // Returning is only supported in postgres and mssql, and // its a little goofy array order how Sequelize handles it. // https://github.com/sequelize/sequelize/blob/abca55ee52d959f95c98dc7ae8b8162005536d05/packages/core/src/model.js#L3110 - if (sequelize.returning && typeof instances === 'number') { + if (!instances || typeof instances === 'number') { instances = null; } const selected = isPresent(sequelize.attributes); - // TODO: This doesn't support $sort. The results may be in the - // order of the ids array, but its not guaranteed. Its also untested - // because we don't have a Postgres test database. Furthermore, I am - // not sure if these instances have full or partial data properties. if (instances) { + const sortedInstances: Model[] = []; + const unsortedInstances: Model[] = []; + + current.forEach((item: any) => { + const id = item[this.id]; + // @ts-ignore + const instance = instances.find(instance => instance[this.id] === id); + if (instance) { + sortedInstances.push(instance); + } else { + unsortedInstances.push(item); + } + }); + + instances = [...sortedInstances, ...unsortedInstances]; + if (sequelize.raw) { const result = instances.map((instance) => { if (selected) { @@ -425,10 +432,6 @@ export class SequelizeAdapter< return instances as Result[]; } - if (!sequelize.returning) { - return []; - } - const result = await this._find({ ...params, paginate: false, @@ -442,7 +445,7 @@ export class SequelizeAdapter< return result; } - /* TODO: This _get is really only needed to get all of the properties that are not being updated. We can't do similar to _update where we skip the _get (use count instead) and build a fresh instance. With _update we are building ALL the properties and can select/return the right values, with _patch the instance is only partially full. The instance.update does not return the other properties. So, the returned instance wouldn't contain all properties. We have to get a full instance. This could be optimized to maybe skip this _get given some update props and select circumstances */ + /* TODO: This _get is really only needed to get all of the properties that are not being updated. We can't do similar to _update where we skip the _get (use count instead) and build a fresh instance. With _update we are building ALL the properties and can select/return the right values, with _patch the instance is only partially full. The instance.update does not return the other properties. So, the returned instance wouldn't contain all properties. We have to get a full instance. This could be optimized to maybe skip this _get given some update props and select circumstances */ const instance = await this._get(id, { ...params, sequelize: { ...params.sequelize, raw: false } @@ -452,15 +455,8 @@ export class SequelizeAdapter< .update(values, sequelize) .catch(errorHandler); - if (!sequelize.returning) { - return []; - } - if (isPresent(sequelize.include)) { - return this._get(id, { - ...params, - query: { $select: params.query?.$select } - }); + return await this._get(id, params); } if (sequelize.raw) { @@ -511,10 +507,7 @@ export class SequelizeAdapter< .catch(errorHandler); if (isPresent(sequelize.include)) { - return this._get(id, { - ...params, - query: { $select: params.query?.$select } - }); + return await this._get(id, params); } if (isPresent(sequelize.attributes)) { @@ -529,7 +522,7 @@ export class SequelizeAdapter< return instance.toJSON(); } - return instance; + return instance as Result; } async _remove (id: null, params?: ServiceParams): Promise @@ -573,17 +566,9 @@ export class SequelizeAdapter< const result = await this._get(id, params); - const values = (result as Model).toJSON - ? (result as Model).toJSON() - : result; + const instance = result instanceof Model ? result : Model.build(result as any, { isNewRecord: false }); - await Model - .build(values as any, { isNewRecord: false }) - .destroy(sequelize); - - if (!sequelize.returning) { - return []; - } + await instance.destroy(sequelize); return result; } diff --git a/src/index.ts b/src/index.ts index c569b63..a6c0e20 100644 --- a/src/index.ts +++ b/src/index.ts @@ -8,6 +8,7 @@ export * from './declarations' export * from './adapter' export * from './hooks'; export { ERROR, errorHandler } from './utils'; + export class SequelizeService, ServiceParams extends Params = SequelizeAdapterParams, PatchData = Partial> extends SequelizeAdapter implements ServiceMethods, Data, ServiceParams, PatchData> diff --git a/src/utils.ts b/src/utils.ts index a3edf94..8bc109b 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -2,6 +2,7 @@ import type { FeathersError } from '@feathersjs/errors'; import { BadRequest, Forbidden, GeneralError, NotFound, Timeout, Unavailable } from '@feathersjs/errors'; import type { BaseError } from 'sequelize'; export const ERROR = Symbol('feathers-sequelize/error'); + const wrap = (error: FeathersError, original: BaseError) => Object.assign(error, { [ERROR]: original }); export const errorHandler = (error: any) => { @@ -47,11 +48,11 @@ export const getOrder = (sort: Record = {}) => Object.keys(sort).re return order; }, []); -export const isPlainObject = (obj: any) => { - return obj && obj.constructor === {}.constructor; +export const isPlainObject = (obj: any): boolean => { + return !!obj && obj.constructor === {}.constructor; }; -export const isPresent = (obj: any) => { +export const isPresent = (obj: any): boolean => { if (Array.isArray(obj)) { return obj.length > 0; } diff --git a/test/index.test.ts b/test/index.test.ts index 2484987..914742e 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -258,12 +258,8 @@ describe('Feathers Sequelize Service', () => { multi: true })); - before(() => { - return app.service('people').remove(null, { query: { $limit: 1000 } }) - }); - - after(() => app.service('people') - .remove(null, { query: { $limit: 1000 } }) + afterEach(() => app.service('people') + .remove(null, { query: {} }) ); describe('Common functionality', () => { @@ -274,18 +270,16 @@ describe('Feathers Sequelize Service', () => { kirsten = await people.create({ name: 'Kirsten', age: 30 }); }); - afterEach(() => people.remove(kirsten.id).catch(() => {})); - it('allows querying for null values (#45)', async () => { const name = 'Null test'; - const person = await people.create({ name }); + await people.create({ name }); const { data } = await people.find({ query: { age: null } }) as Paginated; + console.log(data); + assert.strictEqual(data.length, 1); assert.strictEqual(data[0].name, name); assert.strictEqual(data[0].age, null); - - await people.remove(person.id); }); it('cleans up the query prototype', async () => { @@ -301,7 +295,7 @@ describe('Feathers Sequelize Service', () => { it('still allows querying with Sequelize operators', async () => { const name = 'Age test'; - const person = await people.create({ name, age: 10 }); + await people.create({ name, age: 10 }); const { data } = await people.find({ query: { age: { [Sequelize.Op.eq]: 10 } } @@ -310,13 +304,11 @@ describe('Feathers Sequelize Service', () => { assert.strictEqual(data.length, 1); assert.strictEqual(data[0].name, name); assert.strictEqual(data[0].age, 10); - - await people.remove(person.id); }); it('$like works', async () => { const name = 'Like test'; - const person = await people.create({ name, age: 10 }); + await people.create({ name, age: 10 }); const { data } = await people.find({ query: { name: { $like: '%ike%' } } @@ -325,8 +317,6 @@ describe('Feathers Sequelize Service', () => { assert.strictEqual(data.length, 1); assert.strictEqual(data[0].name, name); assert.strictEqual(data[0].age, 10); - - await people.remove(person.id); }); it('does not allow raw attribute $select', async () => { @@ -385,6 +375,34 @@ describe('Feathers Sequelize Service', () => { } }); + it('multi patch with correct sort', async () => { + const people = app.service('people'); + const john = await people.create({ name: 'John', age: 30 }); + const jane = await people.create({ name: 'Jane', age: 30 }); + const updated = await people.patch(null, { age: 31 }, { + query: { + id: { $in: [john.id, jane.id] }, + $sort: { name: 1 } + } + }); + + assert.strictEqual(updated.length, 2); + assert.strictEqual(updated[0].id, jane.id); + assert.strictEqual(updated[1].id, john.id); + }); + + it('single patch with $select', async () => { + const updateName = 'Ryan'; + + const result = await people.patch(kirsten.id, { name: updateName }, { query: { $select: ['id'] } }); + + assert.deepStrictEqual(result, { id: kirsten.id }); + + const updatedPerson = await people.get(kirsten.id); + + assert.strictEqual(updatedPerson.name, updateName); + }); + it('filterQuery does not convert dates and symbols', () => { const mySymbol = Symbol('test'); const date = new Date(); @@ -398,17 +416,9 @@ describe('Feathers Sequelize Service', () => { }); it('get works with custom $and', async () => { - let johnId; - try { - const john = await people.create({ name: 'John', age: 30 }); - johnId = john.id; - const foundJohn = await people.get(john.id, { query: { $and: [{ age: 30 }, { status: 'pending' }] } }); - assert.strictEqual(foundJohn.id, john.id); - } finally { - if (johnId) { - await people.remove(johnId); - } - } + const john = await people.create({ name: 'John', age: 30 }); + const foundJohn = await people.get(john.id, { query: { $and: [{ age: 30 }, { status: 'pending' }] } }); + assert.strictEqual(foundJohn.id, john.id); }); }); @@ -435,8 +445,8 @@ describe('Feathers Sequelize Service', () => { }); afterEach(async () => - await orders.remove(null, { query: { $limit: 1000 } }) - .then(() => people.remove(null, { query: { $limit: 1000 } })) + await orders.remove(null, { query: {} }) + .then(() => people.remove(null, { query: {} })) ); it('find() returns correct total when using includes for non-raw requests (#137)', async () => { @@ -479,8 +489,6 @@ describe('Feathers Sequelize Service', () => { const result = await people.remove(kirsten.id, params); expect(result).to.have.property('orders.id'); - - kirsten = await people.create({ name: 'Kirsten', age: 30 }); }); it('can use $dollar.notation$', async () => { @@ -629,15 +637,13 @@ describe('Feathers Sequelize Service', () => { const data = { name: 'Active', status: 'active' }; const SCOPE_TO_ACTIVE = { sequelize: { scope: 'active' } }; const SCOPE_TO_PENDING = { sequelize: { scope: 'pending' } }; - const person = await people.create(data); + await people.create(data); const staPeople = await people.find(SCOPE_TO_ACTIVE) as Paginated; assert.strictEqual(staPeople.data.length, 1); const stpPeople = await people.find(SCOPE_TO_PENDING) as Paginated; assert.strictEqual(stpPeople.data.length, 0); - - await people.remove(person.id); }); }); @@ -667,7 +673,7 @@ describe('Feathers Sequelize Service', () => { david = await people.create({ name: 'David' }); }); - afterEach(() => people.remove(david.id).catch(() => {})); + afterEach(() => people.remove(null, { query: { } }).catch(() => {})); it('find() returns model instances', async () => { const results = await people.find() as any as any[]; @@ -685,8 +691,6 @@ describe('Feathers Sequelize Service', () => { const instance = await people.create({ name: 'Sarah' }); expect(instance).to.be.an.instanceOf(Model); - - await people.remove(instance.id); }); it('bulk create() returns model instances', async () => { @@ -699,23 +703,18 @@ describe('Feathers Sequelize Service', () => { expect(results[0]).to.be.an.instanceOf(Model); assert.ok(results[0].id); assert.ok(results[1].id); - - await people.remove(results[0].id); - await people.remove(results[1].id); }); it('create() with returning=false returns empty array', async () => { - const response1 = await people.create({ name: 'delete' }, { + const responseSingle = await people.create({ name: 'delete' }, { sequelize: { returning: false } }); - const response2 = await people.create([{ name: 'delete' }], { + const responseMulti = await people.create([{ name: 'delete' }], { sequelize: { returning: false } }); - expect(response1).to.deep.equal([]); - expect(response2).to.deep.equal([]); - - await people.remove(null, { query: { name: 'delete' } }); + expect(responseSingle).to.be.an.instanceOf(Model); + expect(responseMulti).to.deep.equal([]); }); it('patch() returns a model instance', async () => { @@ -725,16 +724,16 @@ describe('Feathers Sequelize Service', () => { }); it('patch() with returning=false returns empty array', async () => { - const response1 = await people.patch(david.id, { name: 'Sarah' }, { + const responseSingle = await people.patch(david.id, { name: 'Sarah' }, { sequelize: { returning: false } }); - const response2 = await people.patch(null, { name: 'Sarah' }, { + const responseMulti = await people.patch(null, { name: 'Sarah' }, { query: { name: 'Sarah' }, sequelize: { returning: false } }); - expect(response1).to.deep.equal([]); - expect(response2).to.deep.equal([]); + expect(responseSingle).to.be.an.instanceOf(Model); + expect(responseMulti).to.deep.equal([]); }); it('update() returns a model instance', async () => { @@ -750,17 +749,17 @@ describe('Feathers Sequelize Service', () => { }); it('remove() with returning=false returns empty array', async () => { - const response1 = await people.remove(david.id, { + const responseSingle = await people.remove(david.id, { sequelize: { returning: false } }); david = await people.create({ name: 'David' }); - const response2 = await people.remove(null, { + const responseMulti = await people.remove(null, { query: { name: 'David' }, sequelize: { returning: false } }); - expect(response1).to.deep.equal([]); - expect(response2).to.deep.equal([]); + expect(responseSingle).to.be.an.instanceOf(Model); + expect(responseMulti).to.deep.equal([]); }); }); @@ -772,7 +771,7 @@ describe('Feathers Sequelize Service', () => { david = await rawPeople.create({ name: 'David' }); }); - afterEach(() => rawPeople.remove(david.id).catch(() => {})); + afterEach(() => rawPeople.remove(null, { query: {} }).catch(() => {})); it('`raw: false` works for find()', async () => { const results = await rawPeople.find(NOT_RAW) as any as any[]; @@ -790,8 +789,6 @@ describe('Feathers Sequelize Service', () => { const instance = await rawPeople.create({ name: 'Sarah' }, NOT_RAW); expect(instance).to.be.an.instanceof(Model); - - await rawPeople.remove(instance.id); }); it('`raw: false` works for bulk create()', async () => { @@ -799,8 +796,6 @@ describe('Feathers Sequelize Service', () => { expect(results.length).to.equal(1); expect(results[0]).to.be.an.instanceof(Model); - - await rawPeople.remove(results[0].id); }); it('`raw: false` works for patch()', async () => { @@ -823,19 +818,20 @@ describe('Feathers Sequelize Service', () => { }); }); - describe('ORM functionality with overidden getModel method', () => { + describe('ORM functionality with overridden getModel method', () => { const EXPECTED_ATTRIBUTE_VALUE = 42; function getExtraParams ( - additionalTopLevelParams: Record = {}, - additionalSequelizeParams: Record = {} + params?: Record ) { - return Object.assign({ - sequelize: Object.assign({ + return { + ...params, + sequelize: { expectedAttribute: EXPECTED_ATTRIBUTE_VALUE, - getModelCalls: { count: 0 } - }, additionalSequelizeParams) - }, additionalTopLevelParams); + getModelCalls: { count: 0 }, + ...params?.sequelize + } + } } class ExtendedService extends SequelizeService { @@ -946,28 +942,29 @@ describe('Feathers Sequelize Service', () => { }); it('create() with returning=false returns empty array', async () => { - const params = getExtraParams({}, { returning: false }); - const response1 = await people.create({ name: 'delete' }, params); - const response2 = await people.create([{ name: 'delete' }], params); + const params = getExtraParams({ sequelize: { returning: false } }); + const responseSingle = await people.create({ name: 'delete' }, params); + const responseMulti = await people.create([{ name: 'delete' }], params); - expect(response1).to.deep.equal([]); - expect(response2).to.deep.equal([]); + expect(responseSingle).to.be.an('object').that.has.property('id'); + expect(responseMulti).to.deep.equal([]); await people.remove(null, { ...params, query: { name: 'delete' } }); }); it('patch() with returning=false returns empty array', async () => { - const params = getExtraParams({}, { returning: false }); - const response1 = await people.patch(david.id, { name: 'Sarah' }, + const params = getExtraParams({ sequelize: { returning: false } }); + const responseSingle = await people.patch(david.id, { name: 'Sarah' }, params ); - const response2 = await people.patch(null, { name: 'Sarah' }, { + const responseMulti = await people.patch(null, { name: 'Sarah' }, { + ...params, query: { name: 'Sarah' }, - sequelize: { ...params.sequelize, returning: false } + sequelize: { ...params.sequelize } }); - expect(response1).to.deep.equal([]); - expect(response2).to.deep.equal([]); + expect(responseSingle).to.be.an('object').with.property('id').that.equals(david.id); + expect(responseMulti).to.deep.equal([]); }); it('update() returns a model instance', async () => { @@ -986,16 +983,16 @@ describe('Feathers Sequelize Service', () => { }); it('remove() with returning=false returns empty array', async () => { - const params = getExtraParams({}, { returning: false }); - const response1 = await people.remove(david.id, params); + const params = getExtraParams({ sequelize: { returning: false } }); + const responseSingle = await people.remove(david.id, params); david = await people.create({ name: 'David' }, params); - const response2 = await people.remove(null, { - query: { name: 'David' }, - sequelize: { ...params.sequelize, returning: false } + const responseMulti = await people.remove(null, { + ...params, + query: { name: 'David' } }); - expect(response1).to.deep.equal([]); - expect(response2).to.deep.equal([]); + expect(responseSingle).to.be.an('object').that.has.property('id'); + expect(responseMulti).to.deep.equal([]); }); }); @@ -1011,7 +1008,7 @@ describe('Feathers Sequelize Service', () => { afterEach(() => rawPeople.remove(david.id, getExtraParams()).catch(() => {})); it('`raw: false` works for find()', async () => { - const params = getExtraParams({}, NOT_RAW); + const params = getExtraParams({ sequelize: NOT_RAW }); const results = (await rawPeople.find(params)) as any as any[]; expect(params.sequelize.getModelCalls.count).to.gte(1); @@ -1019,7 +1016,7 @@ describe('Feathers Sequelize Service', () => { }); it('`raw: false` works for get()', async () => { - const params = getExtraParams({}, NOT_RAW); + const params = getExtraParams({ sequelize: NOT_RAW }); const instance = await rawPeople.get(david.id, params); expect(params.sequelize.getModelCalls.count).to.gte(1); @@ -1027,7 +1024,7 @@ describe('Feathers Sequelize Service', () => { }); it('`raw: false` works for create()', async () => { - const params = getExtraParams({}, NOT_RAW); + const params = getExtraParams({ sequelize: NOT_RAW }); const instance = await rawPeople.create({ name: 'Sarah' }, params); expect(params.sequelize.getModelCalls.count).to.gte(1); @@ -1039,7 +1036,7 @@ describe('Feathers Sequelize Service', () => { }); it('`raw: false` works for bulk create()', async () => { - const params = getExtraParams({}, NOT_RAW); + const params = getExtraParams({ sequelize: NOT_RAW }); const results = await rawPeople.create([{ name: 'Sarah' }], params); expect(params.sequelize.getModelCalls.count).to.gte(1); @@ -1052,7 +1049,7 @@ describe('Feathers Sequelize Service', () => { }); it('`raw: false` works for patch()', async () => { - const params = getExtraParams({}, NOT_RAW); + const params = getExtraParams({ sequelize: NOT_RAW }); const instance = await rawPeople.patch(david.id, { name: 'Sarah' }, params); expect(params.sequelize.getModelCalls.count).to.gte(1); @@ -1060,7 +1057,7 @@ describe('Feathers Sequelize Service', () => { }); it('`raw: false` works for update()', async () => { - const params = getExtraParams({}, NOT_RAW); + const params = getExtraParams({ sequelize: NOT_RAW }); const instance = await rawPeople.update(david.id, david, params); expect(params.sequelize.getModelCalls.count).to.gte(1); @@ -1068,7 +1065,7 @@ describe('Feathers Sequelize Service', () => { }); it('`raw: false` works for remove()', async () => { - const params = getExtraParams({}, NOT_RAW); + const params = getExtraParams({ sequelize: NOT_RAW }); const instance = await rawPeople.remove(david.id, params); expect(params.sequelize.getModelCalls.count).to.gte(1); diff --git a/test/utils.test.ts b/test/utils.test.ts index 8777783..f01c412 100644 --- a/test/utils.test.ts +++ b/test/utils.test.ts @@ -148,4 +148,39 @@ describe('Feathers Sequelize Utils', () => { ]); }); }); + + describe('isPlainObject', () => { + it('returns true for plain objects', () => { + expect(utils.isPlainObject({})).to.equal(true); + expect(utils.isPlainObject({ a: 1 })).to.equal(true); + }); + + it('returns false for non-plain objects', () => { + expect(utils.isPlainObject([])).to.equal(false); + expect(utils.isPlainObject(new Date())).to.equal(false); + expect(utils.isPlainObject(null)).to.equal(false); + expect(utils.isPlainObject(undefined)).to.equal(false); + expect(utils.isPlainObject('')).to.equal(false); + expect(utils.isPlainObject(1)).to.equal(false); + expect(utils.isPlainObject(true)).to.equal(false); + }); + }); + + describe('isPresent', () => { + it('returns true for present objects', () => { + expect(utils.isPresent({ a: 1 })).to.equal(true); + expect(utils.isPresent([1])).to.equal(true); + expect(utils.isPresent('a')).to.equal(true); + expect(utils.isPresent(1)).to.equal(true); + expect(utils.isPresent(true)).to.equal(true); + }); + + it('returns false for non-present objects', () => { + expect(utils.isPresent({})).to.equal(false); + expect(utils.isPresent([])).to.equal(false); + expect(utils.isPresent('')).to.equal(false); + expect(utils.isPresent(null)).to.equal(false); + expect(utils.isPresent(undefined)).to.equal(false); + }); + }); });