Skip to content

Commit 7bbd19d

Browse files
arnaud-moncelarnaud-moncel
authored
fix: fix mssql ordering collection by pk (#734)
1 parent 5250b4a commit 7bbd19d

File tree

6 files changed

+357
-82
lines changed

6 files changed

+357
-82
lines changed

‎package.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,8 @@
6464
"semantic-release-slack-bot": "1.6.2",
6565
"sequelize": "5.21.3",
6666
"sequelize-fixtures": "1.1.1",
67-
"simple-git": "1.65.0"
67+
"simple-git": "1.65.0",
68+
"tedious": "11.0.8"
6869
},
6970
"scripts": {
7071
"build": "babel src --out-dir dist",

‎src/services/has-many-getter.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,9 @@ class HasManyGetter extends ResourcesGetter {
4747
parentOptions.limit = options.limit;
4848

4949
// Order with the relation (https://github.com/sequelize/sequelize/issues/4553)
50-
parentOptions.order = (options.order || []).map((fields) => [associationName, ...fields]);
50+
if (options.order) {
51+
parentOptions.order = options.order.map((fields) => [associationName, ...fields]);
52+
}
5153
}
5254

5355
return parentOptions;

‎src/services/query-options.js‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import PrimaryKeysManager from './primary-keys-manager';
88
import QueryBuilder from './query-builder';
99
import SearchBuilder from './search-builder';
1010
import QueryUtils from '../utils/query';
11+
import { isMSSQL } from '../utils/database';
1112

1213
/**
1314
* Sequelize query options generator which is configured using forest admin concepts (filters,
@@ -23,7 +24,7 @@ class QueryOptions {
2324
const options = {};
2425
if (this._sequelizeWhere) options.where = this._sequelizeWhere;
2526
if (this._sequelizeInclude) options.include = this._sequelizeInclude;
26-
if (this._order.length) options.order = this._order;
27+
if (this._sequelizeOrder.length) options.order = this._sequelizeOrder;
2728
if (this._offset !== undefined && this._limit !== undefined) {
2829
options.offset = this._offset;
2930
options.limit = this._limit;
@@ -65,6 +66,16 @@ class QueryOptions {
6566
return include.length ? include : null;
6667
}
6768

69+
get _sequelizeOrder() {
70+
if (isMSSQL(this._model.sequelize) && this._sequelizeInclude?.length) {
71+
// FIx a sequelize bug linked to this issue: https://github.com/sequelize/sequelize/issues/11258
72+
const primaryKeys = Object.keys(this._model.primaryKeys);
73+
this._order = this._order.filter((order) => !primaryKeys.includes(order[0]));
74+
}
75+
76+
return this._order;
77+
}
78+
6879
/**
6980
* @param {sequelize.model} model Sequelize model that should be targeted
7081
* @param {boolean} options.includeRelations Include BelongsTo and HasOne relations by default

‎test/services/query-options.test.js‎

Lines changed: 45 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4,33 +4,56 @@ import QueryOptions from '../../src/services/query-options';
44

55
describe('services > query-options', () => {
66
describe('order', () => {
7-
const model = {
8-
name: 'actor',
9-
unscoped: () => model,
10-
sequelize: { constructor: Sequelize },
11-
associations: {},
7+
const buildModelMock = (dialect) => {
8+
// Sequelize is created here without connection to a database
9+
const sequelize = new Sequelize({ dialect });
10+
11+
const modelActor = sequelize.define('actor', {});
12+
const modelMovie = sequelize.define('movie', {});
13+
14+
modelActor.belongsTo(modelMovie);
15+
16+
Interface.Schemas = { schemas: { actor: { idField: 'id' } } };
17+
18+
return modelActor;
1219
};
13-
Interface.Schemas = { schemas: { actor: { idField: 'id' } } };
1420

15-
it('should return null if there is no sort param', async () => {
16-
expect.assertions(1);
17-
const options = new QueryOptions(model);
18-
await options.sort();
19-
expect(options.sequelizeOptions.order).toBeUndefined();
20-
});
21+
describe('with mssql', () => {
22+
const model = buildModelMock('mssql');
2123

22-
it('should return should set order ASC by default', async () => {
23-
expect.assertions(1);
24-
const options = new QueryOptions(model);
25-
await options.sort('id');
26-
expect(options.sequelizeOptions.order).toStrictEqual([['id', 'ASC']]);
24+
it('should return null if the sorting params is the primarykey', async () => {
25+
expect.assertions(1);
26+
const options = new QueryOptions(model);
27+
await options.sort('id');
28+
expect(options.sequelizeOptions.order).toBeUndefined();
29+
});
2730
});
2831

29-
it('should return should set order DESC if there is a minus sign', async () => {
30-
expect.assertions(1);
31-
const options = new QueryOptions(model);
32-
await options.sort('-id');
33-
expect(options.sequelizeOptions.order).toStrictEqual([['id', 'DESC']]);
32+
['mysql', 'postgres'].forEach((dialect) => {
33+
describe(`with ${dialect}`, () => {
34+
const model = buildModelMock(dialect);
35+
36+
it('should return null if there is no sort param', async () => {
37+
expect.assertions(1);
38+
const options = new QueryOptions(model);
39+
await options.sort();
40+
expect(options.sequelizeOptions.order).toBeUndefined();
41+
});
42+
43+
it('should set order ASC by default', async () => {
44+
expect.assertions(1);
45+
const options = new QueryOptions(model);
46+
await options.sort('id');
47+
expect(options.sequelizeOptions.order).toStrictEqual([['id', 'ASC']]);
48+
});
49+
50+
it('should set order DESC if there is a minus sign', async () => {
51+
expect.assertions(1);
52+
const options = new QueryOptions(model);
53+
await options.sort('-id');
54+
expect(options.sequelizeOptions.order).toStrictEqual([['id', 'DESC']]);
55+
});
56+
});
3457
});
3558
});
3659
});

‎test/services/resources-remover.test.js‎

Lines changed: 61 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -3,67 +3,76 @@ import ResourcesRemover from '../../src/services/resources-remover';
33
import { InvalidParameterError } from '../../src/services/errors';
44

55
describe('services > resources-remover', () => {
6-
describe('perform', () => {
7-
it('should throw error if ids is not an array or empty', async () => {
8-
expect.assertions(3);
9-
function Actor() {
10-
this.unscoped = () => this;
11-
this.sequelize = { constructor: Sequelize };
12-
}
6+
const buildModelMock = (dialect) => {
7+
// Sequelize is created here without connection to a database
8+
const sequelize = new Sequelize({ dialect });
139

14-
await expect(new ResourcesRemover(new Actor(), []).perform())
15-
.rejects
16-
.toBeInstanceOf(InvalidParameterError);
10+
const Actor = sequelize.define('actor', {});
11+
const Film = sequelize.define('film', {});
12+
const ActorFilm = sequelize.define('ActorFilem', {
13+
actorId: {
14+
type: Sequelize.DataTypes.INTEGER,
15+
primaryKey: true,
16+
},
17+
filmId: {
18+
type: Sequelize.DataTypes.INTEGER,
19+
primaryKey: true,
20+
},
21+
});
1722

18-
await expect(new ResourcesRemover(new Actor(), 'foo').perform())
19-
.rejects
20-
.toBeInstanceOf(InvalidParameterError);
23+
ActorFilm.belongsTo(Actor);
24+
ActorFilm.belongsTo(Film);
2125

22-
await expect(new ResourcesRemover(new Actor(), {}).perform())
23-
.rejects
24-
.toBeInstanceOf(InvalidParameterError);
25-
});
26+
return { Actor, Film, ActorFilm };
27+
};
28+
29+
['mysql', 'mssql', 'postgres'].forEach((dialect) => {
30+
describe(`perform with ${dialect}`, () => {
31+
it('should throw error if ids is not an array or empty', async () => {
32+
expect.assertions(3);
33+
34+
const { Actor } = buildModelMock(dialect);
35+
36+
await expect(new ResourcesRemover(Actor, []).perform())
37+
.rejects
38+
.toBeInstanceOf(InvalidParameterError);
2639

27-
it('should remove resources with a single primary key', async () => {
28-
expect.assertions(1);
40+
await expect(new ResourcesRemover(Actor, 'foo').perform())
41+
.rejects
42+
.toBeInstanceOf(InvalidParameterError);
2943

30-
function Actor() {
31-
this.sequelize = { constructor: Sequelize };
32-
this.name = 'actor';
33-
this.primaryKeys = { id: {} };
34-
this.unscoped = () => this;
35-
this.sequelize = { constructor: Sequelize };
36-
this.associations = {};
37-
this.destroy = (condition) => {
44+
await expect(new ResourcesRemover(Actor, {}).perform())
45+
.rejects
46+
.toBeInstanceOf(InvalidParameterError);
47+
});
48+
49+
it('should remove resources with a single primary key', async () => {
50+
expect.assertions(1);
51+
52+
const { Actor } = buildModelMock(dialect);
53+
jest.spyOn(Actor, 'destroy').mockImplementation((condition) => {
3854
expect(condition).toStrictEqual({ where: { id: ['1', '2'] } });
39-
};
40-
}
55+
});
4156

42-
await new ResourcesRemover(new Actor(), ['1', '2']).perform();
43-
});
57+
await new ResourcesRemover(Actor, ['1', '2']).perform();
58+
});
4459

45-
it('should remove resources with composite keys', async () => {
46-
expect.assertions(1);
47-
function ActorFilm() {
48-
this.sequelize = { constructor: Sequelize };
49-
this.name = 'actorFilm';
50-
this.primaryKeys = { actorId: {}, filmId: {} };
51-
this.unscoped = () => this;
52-
this.sequelize = { constructor: Sequelize };
53-
this.associations = {};
54-
this.destroy = (condition) => {
55-
expect(condition).toStrictEqual({
56-
where: {
57-
[Op.or]: [
58-
{ actorId: '1', filmId: '2' },
59-
{ actorId: '3', filmId: '4' },
60-
],
61-
},
60+
it('should remove resources with composite keys', async () => {
61+
expect.assertions(1);
62+
63+
const { ActorFilm } = buildModelMock(dialect);
64+
jest.spyOn(ActorFilm, 'destroy').mockImplementation((condition) => {
65+
expect(condition.where).toStrictEqual({
66+
[Op.or]: [
67+
{ actorId: '1', filmId: '2' },
68+
{ actorId: '3', filmId: '4' },
69+
],
6270
});
63-
};
64-
}
65-
const sequelizeOptions = { Sequelize };
66-
await new ResourcesRemover(new ActorFilm(), ['1|2', '3|4'], sequelizeOptions).perform();
71+
});
72+
73+
const sequelizeOptions = { Sequelize };
74+
await new ResourcesRemover(ActorFilm, ['1|2', '3|4'], sequelizeOptions).perform();
75+
});
6776
});
6877
});
6978
});

0 commit comments

Comments
 (0)