Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: relax constrain check to allow input containing constrained values #2754

Merged
merged 1 commit into from
Apr 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@

import {expect} from '@loopback/testlab';
import {
FilterBuilder,
Filter,
Where,
constrainFilter,
constrainWhere,
constrainDataObject,
constrainDataObjects,
constrainFilter,
constrainWhere,
Entity,
Filter,
FilterBuilder,
Where,
} from '../../..';

describe('constraint utility functions', () => {
Expand Down Expand Up @@ -87,24 +87,58 @@ describe('constraint utility functions', () => {
});
});

it('throws error when the query changes field in constrain', () => {
it('throws error when the query changes field in constraint', () => {
const input = new Order({id: 1, description: 'order 1'});
const constraint: Partial<Order> = {id: 2};
expect(() => {
constrainDataObject(input, constraint);
}).to.throwError(/Property "id" cannot be changed!/);
});

it('allows constrained fields with the same values', () => {
const input = new Order({id: 2, description: 'order 1'});
const constraint: Partial<Order> = {id: 2};
const result = constrainDataObject(input, constraint);
expect(result).to.deepEqual(
new Order({
id: 2,
description: 'order 1',
}),
);
});
});

describe('constrainDataObjects', () => {
it('constrains array of data objects', () => {
const input = [
new Order({id: 1, description: 'order 1'}),
new Order({id: 2, description: 'order 2'}),
new Order({description: 'order 1'}),
new Order({description: 'order 2'}),
];
const constraint: Partial<Order> = {id: 3};
const result = constrainDataObjects(input, constraint);
expect(result[0]).to.containDeep(Object.assign({}, input[0], constraint));
expect(result[1]).to.containDeep(Object.assign({}, input[1], constraint));
});

it('throws error when the query changes field in constraint', () => {
const input = [new Order({id: 1, description: 'order 1'})];
const constraint: Partial<Order> = {id: 2};
expect(() => {
constrainDataObjects(input, constraint);
}).to.throwError(/Property "id" cannot be changed!/);
});

it('allows constrained fields with the same values', () => {
const input = [new Order({id: 2, description: 'order 1'})];
const constraint: Partial<Order> = {id: 2};
const result = constrainDataObjects(input, constraint);
expect(result).to.deepEqual([
new Order({
id: 2,
description: 'order 1',
}),
]);
});
});

/*---------------HELPERS----------------*/
Expand Down
20 changes: 9 additions & 11 deletions packages/repository/src/repositories/constraint-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {Filter, WhereBuilder, Where, FilterBuilder} from '../query';
import {AnyObject, DataObject} from '../common-types';
import {cloneDeep} from 'lodash';
import {AnyObject, DataObject} from '../common-types';
import {Entity} from '../model';
import {Filter, FilterBuilder, Where, WhereBuilder} from '../query';

/**
* A utility function which takes a filter and enforces constraint(s)
Expand Down Expand Up @@ -55,12 +55,16 @@ export function constrainDataObject<T extends Entity>(
): DataObject<T> {
const constrainedData = cloneDeep(originalData);
for (const c in constraint) {
if (constrainedData.hasOwnProperty(c))
if (constrainedData.hasOwnProperty(c)) {
// Known limitation: === does not work for objects such as ObjectId
if (originalData[c] === constraint[c]) continue;
bajtos marked this conversation as resolved.
Show resolved Hide resolved
throw new Error(`Property "${c}" cannot be changed!`);
}
(constrainedData as AnyObject)[c] = constraint[c];
}
return constrainedData;
}

/**
* A utility function which takes an array of model instance data and
* enforces constraint(s) on it
Expand All @@ -71,13 +75,7 @@ export function constrainDataObject<T extends Entity>(
*/
export function constrainDataObjects<T extends Entity>(
originalData: DataObject<T>[],
constraint: Partial<T>,
constraint: DataObject<T>,
): DataObject<T>[] {
const constrainedData = cloneDeep(originalData);
for (let obj of constrainedData) {
for (let prop in constraint) {
(obj as AnyObject)[prop] = constraint[prop];
}
}
return constrainedData;
return originalData.map(obj => constrainDataObject(obj, constraint));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly changing the behavior when the input data is trying to modify the constrained properties. Before my change, we would silently replace values provided by the user with values enforced by the constraint. With my change in place, we reject requests trying to modify constrained properties.

AFAICT, the function constrainDataObjects (notice the plural name!) is not used anywhere in loopback-next codebase and I would find it surprising if anybody was using it directly from 3rd party projects. I consider this change as a backwards-compatible bug fix (not a breaking change).

}