fix: feedback applied

Author: Agnes Lin

packages/repository-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts

@@ -187,84 +187,86 @@ export function hasManyInclusionResolverAcceptance(
       expect(toJSON(result)).to.deepEqual(toJSON(expected));
     });
 
-    describe('checks save() and replaceById()', () => {
-      /* istanbul ignore next */
-      (features.hasRevisionToken ? it.skip : it)(
-        'skips the tests for Cloudant as it needs the token',
-        async () => {
-          it('returns inclusions after running save() operation', async () => {
-            // this shows save() works well with func ensurePersistable and ObjectId
-            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));
-          });
-
-          it('returns inclusions after running replaceById() operation', async () => {
-            // this shows replaceById() works well with func ensurePersistable and ObjectId
-            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.replaceById(thorOrder.id, pizza);
-            const odinPizza = await orderRepo.findById(thorOrder.id);
-
-            const result = await customerRepo.find({
-              include: [{relation: 'orders'}],
-            });
-            const expected = [
-              {...thor, parentId: features.emptyValue},
+    skipIf(
+      !features.hasRevisionToken,
+      it,
+      'returns inclusions after running save() operation',
+      async () => {
+        // this shows save() works well with func ensurePersistable and ObjectId
+        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
+        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;
+
+        await orderRepo.replaceById(thorOrder.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: [
               {
-                ...odin,
-                parentId: features.emptyValue,
-                orders: [
-                  {
-                    ...odinPizza,
-                    isShipped: features.emptyValue,
-                    // eslint-disable-next-line @typescript-eslint/camelcase
-                    shipment_id: features.emptyValue,
-                  },
-                ],
+                ...odinPizza,
+                isShipped: features.emptyValue,
+                // eslint-disable-next-line @typescript-eslint/camelcase
+                shipment_id: features.emptyValue,
               },
-            ];
-            expect(toJSON(result)).to.deepEqual(toJSON(expected));
-          });
-        },
-      );
-    });
+            ],
+          },
+        ];
+        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

packages/repository-tests/src/crud/relations/acceptance/has-many.relation.acceptance.ts

@@ -247,6 +247,23 @@ export function hasManyRelationAcceptance(
       ).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', () => {
       it('gets the parent entity through the child entity', async () => {
         const parent = await customerRepo.create({name: 'parent customer'});

packages/repository-tests/src/crud/relations/helpers.ts

@@ -30,6 +30,9 @@ export function givenBoundCrudRepositories(
   Order.definition.properties.id.type = features.idType;
   Address.definition.properties.id.type = features.idType;
   Customer.definition.properties.id.type = features.idType;
+  Customer.definition.properties.id.mongodb = {
+    dataType: 'ObjectID',
+  };
   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.

packages/repository/src/repositories/legacy-juggler-bridge.ts

@@ -420,7 +420,8 @@ export class DefaultCrudRepository<
     return this.updateById(entity.getId(), entity, options);
   }
 
-  delete(entity: T, options?: Options): Promise<void> {
+  async delete(entity: T, options?: Options): Promise<void> {
+    this.ensurePersistable(entity);
     return this.deleteById(entity.getId(), options);
   }
 
@@ -456,7 +457,7 @@ export class DefaultCrudRepository<
     options?: Options,
   ): Promise<void> {
     try {
-      const payload = this.ensurePersistable(data, {replacement: true});
+      const payload = this.ensurePersistable(data);
       await ensurePromise(this.modelClass.replaceById(id, payload, options));
     } catch (err) {
       if (err.statusCode === 404) {
@@ -536,15 +537,13 @@ export class DefaultCrudRepository<
 
   /** Converts an entity object to a JSON object to check if it contains navigational property.
    * Throws an error if `entity` contains navigational property.
-   * Also removed id property  for replaceById operation (mongo use case).
    *
    * @param entity
-   * @param options when attributw replacement is set to true, delete the id property to operate
-   * replaceById method.
+   * @param options
    */
   protected ensurePersistable<R extends T>(
     entity: R | DataObject<R>,
-    options: {replacement?: boolean; idProperty?: string} = {},
+    options: {},
   ): legacy.ModelData<legacy.PersistedModel> {
     // FIXME(bajtos) Ideally, we should call toJSON() to convert R to data object
     // Unfortunately that breaks replaceById for MongoDB connector, where we
@@ -569,12 +568,12 @@ export class DefaultCrudRepository<
         );
       }
     }
-    if (options.replacement === true) {
-      const idProperty = this.entityClass.getIdProperties()[0];
-      if (idProperty) {
-        delete data[idProperty];
-      }
-    }
+    // if (options.replacement === true) {
+    //   const idProperty = this.entityClass.getIdProperties()[0];
+    //   if (!data[idProperty] === undefined) {
+    //     throw new Error(`id property should not be included in ${data}`);
+    //   }
+    // }
     return data;
   }