Fix removing unversioned documents via versioned model, and add warni…

…ng. Add tests for remove versioning.

changelog.md

@@ -1,3 +1,6 @@
+### 1.3.1
+ * Fixes removal of unversioned documents via a versioned model.
+
 ## 1.3.0
  * Adds support for model versioning, with associated schema fragment to name
    the version field. To disable versioning pass options.versioning: false to

lib/model.js

@@ -462,8 +462,8 @@ class BaseModel {
                 } else {
                     throw new Error(`Version error: the model .${schema.idFieldName}="${this[schema.idFieldName]}" was updated by another process between loading and saving.`);
                 }
+                /* c8 ignore next 3 */
             } else {
-                /* c8 ignore next 2 */
                 throw e;
             }
         }
@@ -479,8 +479,8 @@ class BaseModel {
     async #remove(){
         const table = this.constructor[kModelTable],
              schema = this.constructor[kModelSchema];
+        /* c8 ignore next 3 */
         if (!table[kTableIsReady]) {
-            /* c8 ignore next 2 */
             await table.ready();
         }
         const commandArgs = {
@@ -489,13 +489,29 @@ class BaseModel {
         };
         // check that the version field is the same as it was when we loaded this model.
         if (schema.versionFieldName) {
-            commandArgs.ConditionExpression = '#v = :v';
-            commandArgs.ExpressionAttributeNames = { '#v': schema.versionFieldName };
-            commandArgs.ExpressionAttributeValues = { ':v': this[schema.versionFieldName] };
+            if (!this[schema.versionFieldName]) {
+                this.#logger.warn('Removing versioned document .%s="%s" missing version field.', schema.versionFieldName, this.id);
+                commandArgs.ConditionExpression = 'attribute_not_exists(#v)';
+                commandArgs.ExpressionAttributeNames = { '#v': schema.versionFieldName };
+            } else {
+                commandArgs.ConditionExpression = '#v = :v';
+                commandArgs.ExpressionAttributeNames = { '#v': schema.versionFieldName };
+                commandArgs.ExpressionAttributeValues = { ':v': this[schema.versionFieldName] };
+            }
         }
         const command = new DeleteCommand(commandArgs);
         this.#logger.trace({command}, 'remove %s', this.id);
-        const data = await table[kTableDDBClient].send(command);
+        let data;
+        try {
+            data = await table[kTableDDBClient].send(command);
+        } catch (e) {
+            if (e.name === 'ConditionalCheckFailedException') {
+                throw new Error(`Version error: the model .${schema.idFieldName}="${this[schema.idFieldName]}" was updated by another process between loading and removing.`);
+                /* c8 ignore next 3 */
+            } else {
+                throw e;
+            }
+        }
         this.#logger.trace({response: data}, 'remove %s response', this.id);
         return this;
     }
@@ -583,8 +599,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 (!Array.isArray(ids)) {
@@ -640,8 +656,8 @@ class BaseModel {
         const {limit, abortSignal} = options?? {};
         const table = DerivedModel[kModelTable];
         const schema = DerivedModel[kModelSchema];
+        /* c8 ignore next 3 */
         if (!table[kTableIsReady]) {
-            /* c8 ignore next 2 */
             await table.ready();
         }
         const sendOptions = {
@@ -676,8 +692,8 @@ class BaseModel {
         const {limit, abortSignal} = options?? {};
         const table = DerivedModel[kModelTable];
         const schema = DerivedModel[kModelSchema];
+        /* c8 ignore next 3 */
         if (!table[kTableIsReady]) {
-            /* c8 ignore next 2 */
             await table.ready();
         }
         const sendOptions = {
@@ -704,8 +720,8 @@ class BaseModel {
     static async #rawQueryOneId(DerivedModel, rawQuery, options) {
         const table = DerivedModel[kModelTable];
         const schema = DerivedModel[kModelSchema];
+        /* c8 ignore next 3 */
         if (!table[kTableIsReady]) {
-            /* c8 ignore next 2 */
             await table.ready();
         }
         const commandParams = {

test/schemas.js

@@ -698,13 +698,79 @@ t.test('versioning', async t => {
         await a.save();
         const b = await AModel.getById(a.id);
 
-        // should log warning about adding a version field
         const results = t.capture(logger, 'warn');
+
+        // should log warning about adding a version field
         await b.save();
 
         t.match(results(), [{args:['Adding missing version field %s to document %s.', 'v', a.id]}], 'should have called logger.warn with a suitable message');
     });
 
+    t.test('creating, saving, then removing', async t => {
+        const a = new AModel({foo:'remove'});
+        const id = a.id;
+        await a.save();
+
+        await a.remove();
+
+        t.equal(await AModel.getById(id), null, 'document should have been removed');
+    });
+
+    t.test('loading then removing', async t => {
+        const a = new AModel({foo:'load and remove'});
+        const id = a.id;
+        await a.save();
+
+        const b = await AModel.getById(a.id);
+        t.equal(b[ASchema.versionFieldName], 1, 'loaded version should be 1');
+
+        await b.remove();
+        t.equal(await AModel.getById(id), null, 'document should have been removed');
+    });
+
+    t.test('loading, saving, then removing', async t => {
+        const a = new AModel({foo:'bar'});
+        const id = a.id;
+        await a.save();
+
+        const b = await AModel.getById(a.id);
+
+        b.foo = 'loaded';
+        await b.save();
+
+        await b.remove();
+        t.equal(await AModel.getById(id), null, 'document should have been removed');
+    });
+
+    t.test('removing an outdated doc causing version error', async t => {
+        const a = new AModel({foo:''});
+
+        await a.save();
+
+        const b = await AModel.getById(a.id);
+        const c = await AModel.getById(a.id);
+
+        b.foo = 'remove version error';
+
+        await b.save();
+
+        t.rejects(c.remove(), {message:`Version error: the model .id="${a.id}" was updated by another process between loading and removing.`}, 'removing an updated model should cause a version error');
+    });
+
+    t.test('removing unversioned model', async t => {
+        const a = new AModel_unversioned({foo:'unversioned  save'});
+
+        await a.save();
+        const b = await AModel.getById(a.id);
+
+        const results = t.capture(logger, 'warn');
+
+        // should log warning about a missing version field
+        await b.remove();
+
+        t.match(results(), [{args:['Removing versioned document .%s="%s" missing version field.', 'v', a.id]}], 'should have called logger.warn with a suitable message');
+    });
+
     t.test('naming a version field when Schema versioning option is false', async t => {
         const logger = require('pino')({level:'warn'});
         // stop child logger creation so we can intercept messages