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.

Sign up for GitHub

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

feat(repository): rejects CRUD operations for navigational properties #4148

Merged
merged 1 commit into from
Dec 4, 2019
+415 −109
Merged

feat(repository): rejects CRUD operations for navigational properties #4148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
149 changes: 145 additions & 4 deletions ...ry-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,142 @@ export function hasManyInclusionResolverAcceptance(
expect(toJSON(result)).to.deepEqual(toJSON(expected));
});

it('throws when navigational properties are present when updating model instance', async () => {
const created = await customerRepo.create({name: 'customer'});
const customerId = created.id;
skipIf(
features.hasRevisionToken,
it,
'returns inclusions after running save() operation',
Copy link
Member

Choose a reason for hiding this comment

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

Could you please provide more details on why are these new test cases skipped? IIUC, you are running these tests only for CouchDB/Cloudant and skipping them for all other databases (memory, PostgreSQL, MongoDB). The code in the test looks legit to me, I find it suspicious that it does not work on so many database types.

Ideally, I'd like to find a way how to enable these tests for all databases we support.

If that's not possible or if it requires too much effort, then I would like you to add code comments near !features.hasRevisionToken line to explain why each test is skipped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test should run on all databases except Cloudant cause it needs the _rev property. I will add comments to clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

@agnes512 thank you for replying in time, I am running these 2 tests and I think I understand why they fail for Cloudant and mongodb, fixing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bajtos The test case here skips Cloudant connector because the Customer model in fixtures doesn't have _rev property defined.

skipIf(features.hasRevisionToken, testcase){//tests}means when features.hasRevisionToken is true(which only applies for the cloudant connector), the test will be skipped.

I am not very familiar with the shared test case, so agreed with Agnes's solution,
if you want to enable same tests for cloudant, I can add them in a separate PR (within same story).

Copy link
Contributor

@jannyHou jannyHou Nov 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@bajtos The test fails for mongodb because the id type is object not string, therefore fails at the check in https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L2621, hmm...any suggestion on how to fix it?

Update, just talked to Agnes, she points me to the fix code commented in https://github.com/strongloop/loopback-next/blob/49ecf1b86f058576404f28c88d4650651fc31419/packages/repository/src/repositories/legacy-juggler-bridge.ts#L580-L585 :)

Do you agree on the solution? If it is acceptable I will recover this code. (Then I believe all tests will pass)

async () => {
// this shows save() works well with func ensurePersistable and ObjectId
// the test skips for Cloudant as it needs the _rev property for replacement.
// see replace-by-id.suite.ts
const thor = await customerRepo.create({name: 'Thor'});
const odin = await customerRepo.create({name: 'Odin'});

const thorOrder = await orderRepo.create({
customerId: thor.id,
description: 'Pizza',
});

const pizza = await orderRepo.findById(thorOrder.id);
pizza.customerId = odin.id;

await orderRepo.save(pizza);
const odinPizza = await orderRepo.findById(thorOrder.id);

const result = await customerRepo.findById(odin.id, {
include: [{relation: 'orders'}],
});
const expected = {
...odin,
parentId: features.emptyValue,
orders: [
{
...odinPizza,
isShipped: features.emptyValue,
// eslint-disable-next-line @typescript-eslint/camelcase
shipment_id: features.emptyValue,
},
],
};
expect(toJSON(result)).to.containEql(toJSON(expected));
},
);

skipIf(
features.hasRevisionToken,
it,
'returns inclusions after running replaceById() operation',
async () => {
// this shows replaceById() works well with func ensurePersistable and ObjectId
// the test skips for Cloudant as it needs the _rev property for replacement.
// see replace-by-id.suite.ts
const thor = await customerRepo.create({name: 'Thor'});
const odin = await customerRepo.create({name: 'Odin'});

const thorOrder = await orderRepo.create({
customerId: thor.id,
description: 'Pizza',
});

const pizza = await orderRepo.findById(thorOrder.id.toString());
pizza.customerId = odin.id;
// FIXME: [mongo] if pizza obj is converted to JSON obj, it would get an error
// because it tries to convert ObjectId to string type.
// should test with JSON obj once it's fixed.

await orderRepo.replaceById(pizza.id, pizza);
const odinPizza = await orderRepo.findById(thorOrder.id);

const result = await customerRepo.find({
include: [{relation: 'orders'}],
});
const expected = [
{
...thor,
parentId: features.emptyValue,
},
{
...odin,
parentId: features.emptyValue,
orders: [
{
...odinPizza,
isShipped: features.emptyValue,
// eslint-disable-next-line @typescript-eslint/camelcase
shipment_id: features.emptyValue,
},
],
},
];
expect(toJSON(result)).to.deepEqual(toJSON(expected));
},
);

it('returns inclusions after running updateById() operation', async () => {
const thor = await customerRepo.create({name: 'Thor'});
const odin = await customerRepo.create({name: 'Odin'});

const thorOrder = await orderRepo.create({
customerId: thor.id,
description: 'Pizza',
});

const pizza = await orderRepo.findById(thorOrder.id.toString());
pizza.customerId = odin.id;
const reheatedPizza = toJSON(pizza);

await orderRepo.updateById(pizza.id, reheatedPizza);
const odinPizza = await orderRepo.findById(thorOrder.id);

const result = await customerRepo.find({
include: [{relation: 'orders'}],
});
const expected = [
{
...thor,
parentId: features.emptyValue,
},
{
...odin,
parentId: features.emptyValue,
orders: [
{
...odinPizza,
isShipped: features.emptyValue,
// eslint-disable-next-line @typescript-eslint/camelcase
shipment_id: features.emptyValue,
},
],
},
];
expect(toJSON(result)).to.deepEqual(toJSON(expected));
});

it('throws when navigational properties are present when updating model instance with save()', async () => {
// save() calls replaceById so the result will be the same for replaceById
const customer = await customerRepo.create({name: 'customer'});
const anotherCus = await customerRepo.create({name: 'another customer'});
const customerId = customer.id;

await orderRepo.create({
description: 'pizza',
Expand All @@ -201,11 +334,19 @@ export function hasManyInclusionResolverAcceptance(
});
expect(found.orders).to.have.lengthOf(1);

const wrongOrder = await orderRepo.create({
description: 'wrong order',
customerId: anotherCus.id,
});

found.name = 'updated name';
found.orders.push(wrongOrder);
Copy link
Member

Choose a reason for hiding this comment

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

Is it a requirement to modify find.orders array? Shouldn't the check be triggered even when the array remains the same as returned by customerRepo.findById? If the behavior is different depending on whether the array was changed or not, then we should have two tests to cover both variants.


await expect(customerRepo.save(found)).to.be.rejectedWith(
'The `Customer` instance is not valid. Details: `orders` is not defined in the model (value: undefined).',
/Navigational properties are not allowed.*"orders"/,
);
});

// scope for inclusion is not supported yet
it('throws error if the inclusion query contains a non-empty scope', async () => {
const customer = await customerRepo.create({name: 'customer'});
Expand Down
81 changes: 79 additions & 2 deletions packages/repository-tests/src/crud/relations/acceptance/has-many.relation.acceptance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ export function hasManyRelationAcceptance(
);
});

it('does not create an array of the related model', async () => {
it('throws when tries to create() an instance with navigational property', async () => {
await expect(
customerRepo.create({
name: 'a customer',
Expand All @@ -184,7 +184,84 @@ export function hasManyRelationAcceptance(
},
],
}),
).to.be.rejectedWith(/`orders` is not defined/);
).to.be.rejectedWith(
'Navigational properties are not allowed in model data (model "Customer" property "orders")',
);
});

it('throws when tries to createAll() instancese with navigational properties', async () => {
await expect(
customerRepo.createAll([
{
name: 'a customer',
orders: [{description: 'order 1'}],
},
{
name: 'a customer',
address: {street: '1 Amedee Bonnet'},
},
]),
).to.be.rejectedWith(
'Navigational properties are not allowed in model data (model "Customer" property "orders")',
);
});

it('throws when the instance contains navigational property when operates update()', async () => {
const created = await customerRepo.create({name: 'customer'});
await orderRepo.create({
description: 'pizza',
customerId: created.id,
});

const found = await customerRepo.findById(created.id, {
include: [{relation: 'orders'}],
});
expect(found.orders).to.have.lengthOf(1);

found.name = 'updated name';
await expect(customerRepo.update(found)).to.be.rejectedWith(
/Navigational properties are not allowed.*"orders"/,
);
});

it('throws when the instancees contain navigational property when operates updateAll()', async () => {
await customerRepo.create({name: 'Mario'});
await customerRepo.create({name: 'Luigi'});

await expect(
customerRepo.updateAll({
name: 'Nintendo',
orders: [{description: 'Switch'}],
}),
).to.be.rejectedWith(/Navigational properties are not allowed.*"orders"/);
});

it('throws when the instance contains navigational property when operates updateById()', async () => {
const customer = await customerRepo.create({name: 'Mario'});

await expect(
customerRepo.updateById(customer.id, {
name: 'Luigi',
orders: [{description: 'Nintendo'}],
}),
).to.be.rejectedWith(/Navigational properties are not allowed.*"orders"/);
});

it('throws when the instance contains navigational property when operates delete()', async () => {
const customer = await customerRepo.create({name: 'customer'});

await orderRepo.create({
description: 'pizza',
customerId: customer.id,
});

const found = await customerRepo.findById(customer.id, {
include: [{relation: 'orders'}],
});

await expect(customerRepo.delete(found)).to.be.rejectedWith(
'Navigational properties are not allowed in model data (model "Customer" property "orders")',
);
});

context('when targeting the source model', () => {
Expand Down
1 change: 1 addition & 0 deletions packages/repository-tests/src/crud/relations/acceptance/has-one.relation.acceptance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export function hasOneRelationAcceptance(
);

beforeEach(async () => {
await customerRepo.deleteAll();
await addressRepo.deleteAll();
existingCustomerId = (await givenPersistedCustomerInstance()).id;
});
Expand Down
6 changes: 6 additions & 0 deletions packages/repository-tests/src/crud/relations/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ import {CrudFeatures, CrudRepositoryCtor} from '../..';
import {
Address,
AddressRepository,
Customer,
CustomerRepository,
Order,
OrderRepository,
Shipment,
ShipmentRepository,
} from './fixtures/models';
import {
Expand All @@ -25,6 +27,10 @@ export function givenBoundCrudRepositories(
repositoryClass: CrudRepositoryCtor,
features: CrudFeatures,
) {
Order.definition.properties.id.type = features.idType;
Address.definition.properties.id.type = features.idType;
Customer.definition.properties.id.type = features.idType;
Shipment.definition.properties.id.type = features.idType;
// when running the test suite on MongoDB, we don't really need to setup
// this config for mongo connector to pass the test.
// however real-world applications might have such config for MongoDB
Expand Down
13 changes: 8 additions & 5 deletions packages/repository-tests/src/crud/replace-by-id.suite.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,17 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {Entity, model, property} from '@loopback/repository';
import {AnyObject, EntityCrudRepository} from '@loopback/repository';
import {
AnyObject,
Entity,
EntityCrudRepository,
model,
property,
} from '@loopback/repository';
import {expect, toJSON} from '@loopback/testlab';
import {MixedIdType} from '../helpers.repository-tests';
import {
deleteAllModelsInDefaultDataSource,
MixedIdType,
withCrudCtx,
} from '../helpers.repository-tests';
import {
Expand Down Expand Up @@ -72,7 +77,6 @@ export function createSuiteForReplaceById(
// This important! Not all databases allow `patchById` to set
// properties to "undefined", `replaceById` must always work.
created.description = undefined;

await repo.replaceById(created.id, created);

const found = await repo.findById(created.id);
Expand Down Expand Up @@ -112,7 +116,6 @@ export function createSuiteForReplaceById(
// This important! Not all databases allow `patchById` to set
// properties to "undefined", `replaceById` must always work.
created.description = undefined;

await repo.replaceById(created.id, created);

const found = await repo.findById(created.id);
Expand Down
Loading