E5EF fix(client): Ensure all client methods require valid ids by daffl · Pull Request #3661 · feathersjs/feathers · GitHub
[go: up one dir, main page]

Skip to content
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
24 changes: 9 additions & 15 deletions packages/rest-client/src/base.ts
Original file line number Diff line number Diff line change
@@ -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 }) {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand Down
26 changes: 21 additions & 5 deletions packages/rest-client/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
< FBC8 /td> })

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'
})
})

Expand Down
23 changes: 20 additions & 3 deletions packages/transport-commons/src/client.ts
Original file line number Diff line number Diff line change
@@ -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'

Expand Down Expand Up @@ -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 || {})
}

Expand All @@ -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 || {})
}

Expand All @@ -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 || {})
}

Expand Down
58 changes: 58 additions & 0 deletions packages/transport-commons/test/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
0