fix(client): Ensure all client methods require valid ids (#3661)

Author: daffl

packages/rest-client/src/base.ts

@@ -1,6 +1,6 @@
 import qs from 'qs'
 import { Params, Id, Query, NullableId, ServiceInterface } from '@feathersjs/feathers'
-import { Unavailable, convert } from '@feathersjs/errors'
+import { BadRequest, Unavailable, convert } from '@feathersjs/errors'
 import { _, stripSlashes } from '@feathersjs/commons'
 
 function toError(error: Error & { code: string }) {
@@ -113,8 +113,8 @@ export abstract class Base<
   }
 
   _get(id: Id, params?: P) {
-    if (typeof id === 'undefined') {
-      return Promise.reject(new Error("id for 'get' can not be undefined"))
+    if (id === undefined || id === '') {
+      return Promise.reject(new BadRequest('id for .get is required'))
     }
 
     return this.request(
@@ -148,10 +148,8 @@ export abstract class Base<
   }
 
   _update(id: NullableId, data: D, params?: P) {
-    if (typeof id === 'undefined') {
-      return Promise.reject(
-        new Error("id for 'update' can not be undefined, only 'null' when updating multiple entries")
-      )
+    if (id === undefined || id === '') {
+      return Promise.reject(new BadRequest('id for .update is required'))
     }
 
     return this.request(
@@ -170,10 +168,8 @@ export abstract class Base<
   }
 
   _patch(id: NullableId, data: D, params?: P) {
-    if (typeof id === 'undefined') {
-      return Promise.reject(
-        new Error("id for 'patch' can not be undefined, only 'null' when updating multiple entries")
-      )
+    if (id === undefined || id === '') {
+      return Promise.reject(new BadRequest('id for .patch is required'))
     }
 
     return this.request(
@@ -192,10 +188,8 @@ export abstract class Base<
   }
 
   _remove(id: NullableId, params?: P) {
-    if (typeof id === 'undefined') {
-      return Promise.reject(
-        new Error("id for 'remove' can not be undefined, only 'null' when removing multiple entries")
-      )
+    if (id === undefined || id === '') {
+      return Promise.reject(new BadRequest('id for .remove is required'))
     }
 
     return this.request(

packages/rest-client/test/index.test.ts

@@ -46,25 +46,41 @@ describe('REST client tests', function () {
     }
   })
 
-  it('errors when id property for get, patch, update or remove is undefined', async () => {
+  it('errors when id property for get, patch, update or remove is undefined or empty string', async () => {
     const app = feathers().configure(init('http://localhost:8889').fetch(fetch))
 
     const service = app.service('todos')
 
     await assert.rejects(() => service.get(undefined), {
-      message: "id for 'get' can not be undefined"
+      message: 'id for .get is required'
+    })
+
+    await assert.rejects(() => service.get(''), {
+      message: 'id for .get is required'
     })
 
     await assert.rejects(() => service.remove(undefined), {
-      message: "id for 'remove' can not be undefined, only 'null' when removing multiple entries"
+      message: 'id for .remove is required'
+    })
+
+    await assert.rejects(() => service.remove(''), {
+      message: 'id for .remove is required'
     })
 
     await assert.rejects(() => service.update(undefined, {}), {
-      message: "id for 'update' can not be undefined, only 'null' when updating multiple entries"
+      message: 'id for .update is required'
+    })
+
+    await assert.rejects(() => service.update('', {}), {
+      message: 'id for .update is required'
     })
 
     await assert.rejects(() => service.patch(undefined, {}), {
-      message: "id for 'patch' can not be undefined, only 'null' when updating multiple entries"
+      message: 'id for .patch is required'
+    })
+
+    await assert.rejects(() => service.patch('', {}), {
+      message: 'id for .patch is required'
     })
   })
 

packages/transport-commons/src/client.ts

@@ -1,5 +1,5 @@
 /* eslint-disable @typescript-eslint/ban-ts-comment */
-import { convert } from '@feathersjs/errors'
+import { BadRequest, convert } from '@feathersjs/errors'
 import { createDebug } from '@feathersjs/commons'
 import { Id, NullableId, Params, ServiceInterface } from '@feathersjs/feathers'
 
@@ -128,6 +128,10 @@ export class Service<T = any, D = Partial<T>, P extends Params = Params> impleme
   }
 
   _get(id: Id, params: Params = {}) {
+    if (id === undefined || id === '') {
+      return Promise.reject(new BadRequest('id for .get is required'))
+    }
+
     return this.send<T>('get', id, params.query || {}, params.route || {})
   }
 
@@ -144,17 +148,26 @@ export class Service<T = any, D = Partial<T>, P extends Params = Params> impleme
   }
 
   _update(id: NullableId, data: D, params: Params = {}) {
-    if (typeof id === 'undefined') {
-      return Promise.reject(new Error("id for 'update' can not be undefined"))
+    if (id === undefined || id === '') {
+      return Promise.reject(new BadRequest('id for .update is required'))
     }
+
     return this.send<T>('update', id, data, params.query || {}, params.route || {})
   }
 
   update(id: NullableId, data: D, params: Params = {}) {
+    if (id === undefined || id === '') {
+      return Promise.reject(new BadRequest('id for .update is required'))
+    }
+
     return this._update(id, data, params)
   }
 
   _patch(id: NullableId, data: D, params: Params = {}) {
+    if (id === undefined || id === '') {
+      return Promise.reject(new BadRequest('id for .patch is required'))
+    }
+
     return this.send<T | T[]>('patch', id, data, params.query || {}, params.route || {})
   }
 
@@ -163,6 +176,10 @@ export class Service<T = any, D = Partial<T>, P extends Params = Params> impleme
   }
 
   _remove(id: NullableId, params: Params = {}) {
+    if (id === undefined || id === '') {
+      return Promise.reject(new BadRequest('id for .remove is required'))
+    }
+
     return this.send<T | T[]>('remove', id, params.query || {}, params.route || {})
   }
 

packages/transport-commons/test/client.test.ts

@@ -27,6 +27,64 @@ describe('client', () => {
     assert.ok(service.events)
   })
 
+  it('throws for .get, .update, .patch and .remove with undefined or empty string id', async () => {
+    try {
+      await service.get(undefined)
+      assert.ok(false, 'Should never get here')
+    } catch (e: any) {
+      assert.strictEqual(e.message, 'id for .get is required')
+    }
+
+    try {
+      await service.get('')
+      assert.ok(false, 'Should never get here')
+    } catch (e: any) {
+      assert.strictEqual(e.message, 'id for .get is required')
+    }
+
+    try {
+      await service.update(undefined, testData)
+      assert.ok(false, 'Should never get here')
+    } catch (e: any) {
+      assert.strictEqual(e.message, 'id for .update is required')
+    }
+
+    try {
+      await service.update('', testData)
+      assert.ok(false, 'Should never get here')
+    } catch (e: any) {
+      assert.strictEqual(e.message, 'id for .update is required')
+    }
+
+    try {
+      await service.patch(undefined, testData)
+      assert.ok(false, 'Should never get here')
+    } catch (e: any) {
+      assert.strictEqual(e.message, 'id for .patch is required')
+    }
+
+    try {
+      await service.patch('', testData)
+      assert.ok(false, 'Should never get here')
+    } catch (e: any) {
+      assert.strictEqual(e.message, 'id for .patch is required')
+    }
+
+    try {
+      await service.remove(undefined)
+      assert.ok(false, 'Should never get here')
+    } catch (e: any) {
+      assert.strictEqual(e.message, 'id for .remove is required')
+    }
+
+    try {
+      await service.remove('')
+      assert.ok(false, 'Should never get here')
+    } catch (e: any) {
+      assert.strictEqual(e.message, 'id for .remove is required')
+    }
+  })
+
   it('throws an error when the emitter does not have the method', () => {
     const clientService = new Service({
       name: 'todos',