Skip to content

Commit

Permalink
perf(bulk-actions): improve performance when calling getIdsFromRequest (
Browse files Browse the repository at this point in the history 
#797)
  • Loading branch information
@adriguy
adriguy authored Sep 2, 2021
1 parent 6bf374c commit 95ff02d
Show file tree
Hide file tree
Showing 6 changed files with 91 additions and 25 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
"@babel/runtime": "7.10.1",
"bluebird": "2.9.25",
"core-js": "3.6.5",
"forest-express": "9.2.0",
"forest-express": "9.2.2",
"http-errors": "1.6.1",
"lodash": "4.17.21",
"moment": "2.19.4",
Expand Down
2 changes: 1 addition & 1 deletion src/services/has-many-getter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class HasManyGetter extends ResourcesGetter {
as: associationName,
scope: false,
required: !!buildOptions.forCount, // Why?
...pick(options, ['where', 'include']),
...pick(options, ['attributes', 'where', 'include']),
}],
});

Expand Down
16 changes: 15 additions & 1 deletion src/services/query-options.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,15 @@ class QueryOptions {
options.limit = this._limit;
}

if (this._restrictFieldsOnRootModel && this._requestedFields.size) {
// Restricting loaded fields on the root model is opt-in with sequelize to avoid
// side-effects as this was not supported historically and it would probably break
// smart fields.
// @see https://github.com/ForestAdmin/forest-express-sequelize/blob/7d7ad0/src/services/resources-getter.js#L142

options.attributes = [...this._requestedFields].filter((s) => !s.includes('.'));
}

return SequelizeCompatibility.postProcess(this._model, options);
}

Expand Down Expand Up @@ -93,6 +102,7 @@ class QueryOptions {
this._options = options;

// Used to compute relations that will go in the final 'include'
this._restrictFieldsOnRootModel = false;
this._requestedFields = new Set();
this._neededFields = new Set();
this._scopes = []; // @see sequelizeScopes getter
Expand All @@ -115,11 +125,15 @@ class QueryOptions {
* Add the required includes from a list of field names.
* @param {string[]} fields Fields of HasOne and BelongTo relations are
* accepted (ie. 'book.name').
* @param {string[]} fields the output of the extractRequestedFields() util function
* @param {boolean} applyOnRootModel restrict fetched fields also on the root
*/
async requireFields(fields) {
async requireFields(fields, applyOnRootModel = false) {
if (fields) {
fields.forEach((field) => this._requestedFields.add(field));
}

this._restrictFieldsOnRootModel = Boolean(applyOnRootModel);
}

/**
Expand Down
7 changes: 4 additions & 3 deletions src/services/resources-getter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,15 @@ class ResourcesGetter {
async _buildQueryOptions(buildOptions = {}) {
const { forCount, tableAlias } = buildOptions;
const {
fields, filters, search, searchExtended, segment, segmentQuery, timezone,
fields, filters, restrictFieldsOnRootModel,
search, searchExtended, segment, segmentQuery, timezone,
} = this._params;

const requestedFields = extractRequestedFields(fields, this._model, Schemas.schemas);
const scopeFilters = await scopeManager.getScopeForUser(this._user, this._model.name, true);

const requestedFields = extractRequestedFields(fields, this._model, Schemas.schemas);
const queryOptions = new QueryOptions(this._model, { tableAlias });
await queryOptions.requireFields(requestedFields);
await queryOptions.requireFields(requestedFields, restrictFieldsOnRootModel);
await queryOptions.search(search, searchExtended);
await queryOptions.filterByConditionTree(filters, timezone);
await queryOptions.filterByConditionTree(scopeFilters, timezone);
Expand Down
81 changes: 66 additions & 15 deletions test/databases.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ const user = { renderingId: 1 };
time_range: null,
filters: null,
}, options, user).perform();
expect(stat.value).toHaveLength(0);
expect(stat.value).toBeEmpty();
} finally {
spy.mockRestore();
connectionManager.closeConnection();
Expand Down Expand Up @@ -1230,7 +1230,7 @@ const user = { renderingId: 1 };
};
try {
const result = await new ResourcesGetter(models.user, null, params, user).perform();
expect(result[0]).toHaveLength(0);
expect(result[0]).toBeEmpty();
} finally {
spy.mockRestore();
connectionManager.closeConnection();
Expand Down Expand Up @@ -1526,7 +1526,7 @@ const user = { renderingId: 1 };
};
try {
const result = await new ResourcesGetter(models.bike, null, params, user).perform();
expect(result[0]).toHaveLength(0);
expect(result[0]).toBeEmpty();
} finally {
spy.mockRestore();
connectionManager.closeConnection();
Expand Down Expand Up @@ -1602,7 +1602,7 @@ const user = { renderingId: 1 };
const result = await new ResourcesGetter(
models.georegion, null, params, user,
).perform();
expect(result[0]).toHaveLength(0);
expect(result[0]).toBeEmpty();
} finally {
spy.mockRestore();
connectionManager.closeConnection();
Expand Down Expand Up @@ -1687,7 +1687,7 @@ const user = { renderingId: 1 };
};
try {
const result = await new ResourcesGetter(models.bird, null, params, user).perform();
expect(result[0]).toHaveLength(0);
expect(result[0]).toBeEmpty();
} finally {
spy.mockRestore();
connectionManager.closeConnection();
Expand Down Expand Up @@ -2512,7 +2512,7 @@ const user = { renderingId: 1 };

try {
const result = await new ResourcesGetter(models.user, null, params, user).perform();
expect(result[0]).toHaveLength(0);
expect(result[0]).toBeEmpty();
} finally {
spy.mockRestore();
connectionManager.closeConnection();
Expand Down Expand Up @@ -2699,7 +2699,7 @@ const user = { renderingId: 1 };

try {
const result = await new ResourcesGetter(models.user, null, params, user).perform();
expect(result[0]).toHaveLength(0);
expect(result[0]).toBeEmpty();
} finally {
spy.mockRestore();
connectionManager.closeConnection();
Expand Down Expand Up @@ -2784,7 +2784,7 @@ const user = { renderingId: 1 };

try {
const result = await new ResourcesGetter(models.user, null, params, user).perform();
expect(result[0]).toHaveLength(0);
expect(result[0]).toBeEmpty();
} finally {
spy.mockRestore();
connectionManager.closeConnection();
Expand Down Expand Up @@ -2852,7 +2852,7 @@ const user = { renderingId: 1 };
});

describe('request on the resources getter with a smart field', () => {
it('should only retrieve requested fields when only DB fields are used', async () => {
it('should only retrieve requested fields on relations when only DB fields are used', async () => {
expect.assertions(5);
const { models } = initializeSequelize();
const spy = jest.spyOn(scopeManager, 'getScopeForUser').mockReturnValue(null);
Expand All @@ -2864,7 +2864,7 @@ const user = { renderingId: 1 };

try {
const result = await new ResourcesGetter(models.address, null, params).perform();
expect(result[0]).not.toHaveLength(0);
expect(result[0]).not.toBeEmpty();
expect(result[0][0]).toHaveProperty('user');
expect(result[0][0].user.dataValues).toHaveProperty('firstName');
expect(result[0][0].user.dataValues).toHaveProperty('id');
Expand All @@ -2875,7 +2875,7 @@ const user = { renderingId: 1 };
}
});

it('should retrieve all fields when a smart field is requested', async () => {
it('should retrieve all fields on relations when a smart field is requested', async () => {
expect.assertions(5);
const { models } = initializeSequelize();
const spy = jest.spyOn(scopeManager, 'getScopeForUser').mockReturnValue(null);
Expand All @@ -2887,7 +2887,7 @@ const user = { renderingId: 1 };

try {
const result = await new ResourcesGetter(models.address, null, params).perform();
expect(result[0]).not.toHaveLength(0);
expect(result[0]).not.toBeEmpty();
expect(result[0][0]).toHaveProperty('user');
expect(result[0][0].user.dataValues).toHaveProperty('firstName');
expect(result[0][0].user.dataValues).toHaveProperty('id');
Expand All @@ -2898,6 +2898,57 @@ const user = { renderingId: 1 };
}
});
});

describe('request on the resources getter with restrictFieldsOnRootModel flag', () => {
it('should only retrieve requested fields with the flag', async () => {
expect.assertions(6);
const { models } = initializeSequelize();
const spy = jest.spyOn(scopeManager, 'getScopeForUser').mockReturnValue(null);
const params = {
...baseParams,
fields: { address: 'id,line,zipCode' },
page: { number: '1' },
restrictFieldsOnRootModel: true,
};

try {
const result = await new ResourcesGetter(models.address, null, params).perform();
expect(result[0]).not.toBeEmpty();
expect(result[0][0].dataValues).toHaveProperty('id');
expect(result[0][0].dataValues).toHaveProperty('line');
expect(result[0][0].dataValues).toHaveProperty('zipCode');
expect(result[0][0].dataValues).not.toHaveProperty('city');
expect(result[0][0].dataValues).not.toHaveProperty('country');
} finally {
spy.mockRestore();
connectionManager.closeConnection();
}
});

it('should ignore requested fields without the flag', async () => {
expect.assertions(6);
const { models } = initializeSequelize();
const spy = jest.spyOn(scopeManager, 'getScopeForUser').mockReturnValue(null);
const params = {
...baseParams,
fields: { address: 'id,line,zipCode' },
page: { number: '1' },
};

try {
const result = await new ResourcesGetter(models.address, null, params).perform();
expect(result[0]).not.toBeEmpty();
expect(result[0][0].dataValues).toHaveProperty('id');
expect(result[0][0].dataValues).toHaveProperty('line');
expect(result[0][0].dataValues).toHaveProperty('zipCode');
expect(result[0][0].dataValues).toHaveProperty('city');
expect(result[0][0].dataValues).toHaveProperty('country');
} finally {
spy.mockRestore();
connectionManager.closeConnection();
}
});
});
});

describe('hasmany > has-many-getter', () => {
Expand Down Expand Up @@ -2977,7 +3028,7 @@ const user = { renderingId: 1 };
params,
user,
).perform();
expect(result[0]).not.toHaveLength(0);
expect(result[0]).not.toBeEmpty();

const firstEntry = result[0][0];
expect(Object.keys(firstEntry.user.dataValues)).toStrictEqual(['id']);
Expand Down Expand Up @@ -3144,7 +3195,7 @@ const user = { renderingId: 1 };
params,
user,
).perform();
expect(result[0]).not.toHaveLength(0);
expect(result[0]).not.toBeEmpty();
expect(result[0][0].user.dataValues).toHaveProperty('id');
expect(result[0][0].user.dataValues).toHaveProperty('firstName');
expect(result[0][0].user.dataValues).toHaveProperty('lastName');
Expand Down Expand Up @@ -3173,7 +3224,7 @@ const user = { renderingId: 1 };
params,
user,
).perform();
expect(result[0]).not.toHaveLength(0);
expect(result[0]).not.toBeEmpty();
expect(result[0][0]).toHaveProperty('user');
expect(result[0][0].user.dataValues).toHaveProperty('firstName');
expect(result[0][0].user.dataValues).toHaveProperty('id');
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5067,10 +5067,10 @@ for-in@^1.0.2:
resolved "https://registry.yarnpkg.com/for-in/-/for-in-1.0.2.tgz#81068d295a8142ec0ac726c6e2200c30fb6d5e80"
integrity sha1-gQaNKVqBQuwKxybG4iAMMPttXoA=

[email protected].0:
version "9.2.0"
resolved "https://registry.yarnpkg.com/forest-express/-/forest-express-9.2.0.tgz#c7e22b66d93536984a4197c09067b97dc20f5e8f"
integrity sha512-e9LCoplxXCsLuznLShuMT+oyxBA/+6R+Ib1sazpg4U8/Xe37AgBt2XKefsc1cyUh5EATJKQCNXm95518dij6Ew==
[email protected].2:
version "9.2.2"
resolved "https://registry.yarnpkg.com/forest-express/-/forest-express-9.2.2.tgz#0e5e7310568a6e20dfe74f6cf3768d06997464e0"
integrity sha512-8Sk0ckIWVDGzPpPik36Gd/jTcCn63eeRR5VLaE1QWzzkKzfPo4WeSyvggI/3HyWVhzmy1Nlizwbof/ki9bRIfQ==
dependencies:
"@babel/runtime" "7.10.1"
base32-encode "1.1.1"
Expand Down

0 comments on commit 95ff02d

Please sign in to comment.