From c79fabcc288d31cb310a38669e1ef75681b4ec3b Mon Sep 17 00:00:00 2001 From: James Crosby Date: Thu, 25 Apr 2024 12:02:17 +0100 Subject: [PATCH] improve validation of abortSignal option, and add additional tests --- lib/model.js | 22 +++++++++----- lib/shared.js | 24 ++++++++++++++- test/queries.js | 81 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 8 deletions(-) diff --git a/lib/model.js b/lib/model.js index 6d24664..4603313 100644 --- a/lib/model.js +++ b/lib/model.js @@ -147,12 +147,20 @@ class BaseModel { async remove() { return this.#remove(); } async toObject({virtuals=true, ...otherOptions}={}) { return this.#toObject({virtuals, ...otherOptions}); } + // public static methods: // options: {ConsistentRead: true, abortSignal: ...} ... dynamoDB consistent read option (defaults to false), and dynamoDB abortSignal options + static #abortSignalSchema = { + type: 'object', + apiArgument: { + validate: (data) => (typeof data.aborted === 'boolean') && (typeof data.addEventListener === 'function'), + error: 'Must be an AbortController Signal.' + } + }; static #getById_options_validate = ajv.compile({ type: 'object', properties: { - abortSignal: {type:'object'}, + abortSignal: this.#abortSignalSchema, ConsistentRead: {type:'boolean'}, }, additionalProperties: false @@ -173,7 +181,7 @@ class BaseModel { static #getByIds_options_validate = ajv.compile({ type: 'object', properties: { - abortSignal: {type:'object'}, + abortSignal: this.#abortSignalSchema, ConsistentRead: {type:'boolean'}, }, additionalProperties: false @@ -234,7 +242,7 @@ class BaseModel { type: 'object', properties: { limit: {type:'number', const: 1}, - abortSignal: {type:'object'}, + abortSignal: this.#abortSignalSchema, startAfter: {type: 'object'}, rawQueryOptions: this.#rawQueryOptionsSchema, rawFetchOptions: this.#rawFetchOptionsSchema @@ -255,7 +263,7 @@ class BaseModel { type: 'object', properties: { limit: {type:'number', const: 1}, - abortSignal: {type:'object'}, + abortSignal: this.#abortSignalSchema, startAfter: {type: 'object'}, rawQueryOptions: this.#rawQueryOptionsSchema, }, @@ -274,7 +282,7 @@ class BaseModel { type: 'object', properties: { limit: {type:'number', default: 50}, - abortSignal: {type:'object'}, + abortSignal: this.#abortSignalSchema, startAfter: {type: 'object'}, rawQueryOptions: this.#rawQueryOptionsSchema, rawFetchOptions: this.#rawFetchOptionsSchema @@ -336,7 +344,7 @@ class BaseModel { type: 'object', properties: { limit: {type:'number', default:50}, - abortSignal: {type:'object'}, + abortSignal: this.#abortSignalSchema, startAfter: {type: 'object'}, rawQueryOptions: this.#rawQueryOptionsSchema }, @@ -566,8 +574,8 @@ class BaseModel { const { ConsistentRead, abortSignal } = rawOptions ?? {}; const table = DerivedModel[kModelTable]; const schema = DerivedModel[kModelSchema]; + /* c8 ignore next 3 */ if (!table[kTableIsReady]) { - /* c8 ignore next 2 */ await table.ready(); } if ((typeof id !== 'string') || (!id.length)) { diff --git a/lib/shared.js b/lib/shared.js index 76f78dd..8aeae37 100644 --- a/lib/shared.js +++ b/lib/shared.js @@ -6,7 +6,7 @@ const Ajv = require('ajv'); const kExtendedTypeDate = Symbol.for('dynamodm:extendedType:date'); const kExtendedTypeBuffer = Symbol.for('dynamodm:extendedType:buffer'); -// this AJV instance is used for general validation, compiled schemas do not modify data +// this AJV instance is used for general validation (e.g. of API function arguments), compiled schemas do not modify data const ajv = new Ajv({ useDefaults: true }); @@ -54,6 +54,28 @@ const extendedTypeKeyword = { ajv.addKeyword(extendedTypeKeyword); defaultIgnoringAjv.addKeyword(extendedTypeKeyword); +// Keyword for validating that API arguments with a function and friendly error +// message defined in the schema: +const apiArgument = { + keyword: 'apiArgument', + type: 'object', + schemaType: 'object', + metaSchema: { + type: 'object', + properties: { validate:{} , error: {type:'string'} }, + required: ['validate', 'error'], + additionalProperties: false + }, + validate: function apiArgValidate(schema, data) { + if (!schema.validate(data)) { + apiArgValidate.errors = [{ message: schema.error, data }]; + return false; + } + return true; + } +}; +ajv.addKeyword(apiArgument); + // Marshalling of built-in types to dynamodb types: marshallingAjv.addKeyword({ keyword: 'extendedType', diff --git a/test/queries.js b/test/queries.js index 8570e83..4d6cb2e 100644 --- a/test/queries.js +++ b/test/queries.js @@ -273,6 +273,23 @@ t.test('queries:', async t => { t.end(); }); + t.test('aborting getById', async t => { + const ac0 = new AbortController(); + ac0.abort(new Error('my reason 0 ')); + t.rejects(Foo.getById(all_foos[0].id, {abortSignal: ac0.signal}), {name:'AbortError', message:'Request aborted'}, 'getById should be abortable with an AbortController that is already aborted'); + + const ac1 = new AbortController(); + t.rejects(Foo.getById(all_foos[0].id, {abortSignal: ac1.signal}), {name:'AbortError', message:'Request aborted'}, 'getById should be abortable with an AbortController signal immediately'); + ac1.abort(new Error('my reason')); + + const ac2 = new AbortController(); + t.rejects(Foo.getById(all_foos[0].id, {abortSignal: ac2.signal}), {name:'AbortError', message:'Request aborted'}, 'getById should be abortable with an AbortController signal asynchronously'); + setTimeout(() => { + ac2.abort(new Error('my reason 2')); + }, 1); + t.end(); + }); + await t.test('rawQueryManyIds', async t => { t.test('on type index', async t => { const foo_ids = await Foo.rawQueryManyIds({ @@ -347,6 +364,60 @@ t.test('queries:', async t => { t.equal(foo_ids.length, 7, 'should return all N of this type'); t.match(foo_ids, all_foos.slice(0,7).map(f => f.id), 'should return the correct Ids'); }); + await t.test('aborting with already-aborted controller', async t => { + const ac0 = new AbortController(); + ac0.abort(new Error('my reason 0')); + const iter0 = Foo.rawQueryIteratorIds({ + IndexName:'type', + KeyConditionExpression:'#typeFieldName = :type', + ExpressionAttributeValues: { ':type': 'namespace.foo' }, + ExpressionAttributeNames: { '#typeFieldName': 'type' } + }, {abortSignal: ac0.signal}); + await t.rejects(arrayFromAsync(iter0), {name:'AbortError', message:'Request aborted'}, 'rawQueryIteratorIds should be abortable with an AbortController that is already aborted'); + t.end(); + }); + await t.test('aborting with already-aborted controller', async t => { + const ac1 = new AbortController(); + const iter1 = Foo.rawQueryIteratorIds({ + IndexName:'type', + KeyConditionExpression:'#typeFieldName = :type', + ExpressionAttributeValues: { ':type': 'namespace.foo' }, + ExpressionAttributeNames: { '#typeFieldName': 'type' } + }, {abortSignal: ac1.signal}); + const testComplete = t.rejects(arrayFromAsync(iter1), {name:'AbortError', message:'Request aborted'}, 'rawQueryIteratorIds should be abortable with an AbortController signal immediately'); + ac1.abort(new Error('my reason')); + await testComplete; + t.end(); + }); + await t.test('aborting with already-aborted controller', async t => { + const ac2 = new AbortController(); + const iter2 = Foo.rawQueryIteratorIds({ + IndexName:'type', + KeyConditionExpression:'#typeFieldName = :type', + ExpressionAttributeValues: { ':type': 'namespace.foo' }, + ExpressionAttributeNames: { '#typeFieldName': 'type' } + }, {abortSignal: ac2.signal}); + const testComplete = t.rejects(arrayFromAsync(iter2), {name:'AbortError', message:'Request aborted'}, 'rawQueryIteratorIds should be abortable with an AbortController signal asynchronously'); + setTimeout(() => { + ac2.abort(new Error('my reason 2')); + }, 1); + await testComplete; + t.end(); + }); + await t.test('aborting after the fact', async t => { + // check that aborting after completion doesn't do anything bad: + const ac3 = new AbortController(); + const iter3 = Foo.rawQueryIteratorIds({ + IndexName:'type', + KeyConditionExpression:'#typeFieldName = :type', + ExpressionAttributeValues: { ':type': 'namespace.foo' }, + ExpressionAttributeNames: { '#typeFieldName': 'type' } + }, {abortSignal: ac3.signal}); + const foo_ids = await arrayFromAsync(iter3); + ac3.abort(new Error('my reason 3')); + t.equal(foo_ids[0].startsWith('namespace.foo'), true, 'should have still returned foos'); + t.end(); + }); t.end(); }); @@ -506,6 +577,16 @@ t.test('queries:', async t => { t.end(); }); + await t.test('throws with invalid abortSignal', async t => { + // check that accidentally passing the abort controller, instead of its signal, throws an error: + await t.rejects(Foo.queryMany({ type: 'namespace.foo' }, { abortSignal: new AbortController() }), { + message: "Invalid options: [ { message: 'Must be an AbortController Signal.', data: AbortController { signal: AbortSignal { aborted: false } }, instancePath: '/abortSignal', schemaPath: '#/properties/abortSignal/apiArgument" + }, 'passing a signal without an .aborted property should fail'); + await t.rejects(Foo.queryMany({ type: 'namespace.foo' }, { abortSignal: { aborted: false } }), { + message: "Invalid options: [ { message: 'Must be an AbortController Signal.', data: { aborted: false }, instancePath: '/abortSignal', schemaPath: '#/properties/abortSignal/apiArgument" + }, 'passing a signal without an .addEventListener function should fail'); + t.end(); + }); t.end(); });