Further fixes in service of Issue #366 (#384)

* Further fixes in service of Issue #366 A bug was discovered that the logic to validate the presence of an attribute was a simple falsey check instead of a check that the value was `undefined`. This caused empty strings, zero values, and the boolean value `false` to incorrectly be considered missing * Fixes test
tywalch · May 17, 2024 · 94ef998 · 94ef998
1 parent e42fb34
commit 94ef998
Show file tree

Hide file tree

Showing 4 changed files with 262 additions and 3 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -519,4 +519,8 @@ All notable changes to this project will be documented in this file. Breaking ch
 
 ## [2.14.0] - 2024-04-29
 ### Fixed/Changed
-- Addresses [Issue #366](https://github.com/tywalch/electrodb/issues/366) with unexpected outcomes from index `condition` usage. Discussion [inside the issue ticket](https://github.com/tywalch/electrodb/issues/366) revealed complexities associated with the implementation of the `condition` callback. Previously, a callback returning `false` would simply not write the fields associated with an index on update. Through discussion with [@sam3d](https://github.com/sam3d) and [@nonken](https://github.com/nonken), it was revealed that this behavior could lead to inconsistencies between indexes and attributes. Furthermore, this behavior did not align with user expectations/intuitions, which expected a `false` response to trigger the removal of the item from the index. To achieve this, it was discussed that the presence of a `condition` callback should add a _new_ runtime validation check on all mutations to verify all member attributes of the index must be provided if a mutation operation affects one of the attributes. Previously ElectroDB would validate only that composite members of an index field (a partition or sort key) within an index were fully provided; now, when a condition callback is present, it will validate that all members from both fields are provided. If you are unable to update/patch all member attributes, because some are readOnly, you can also use the [composite](https://electrodb.dev/en/mutations/patch#composite) method on [update](https://electrodb.dev/en/mutations/update#composite) and [patch](https://electrodb.dev/en/mutations/patch#composite). More information and the discussion around the reasoning behind this change can be found [here](https://github.com/tywalch/electrodb/issues/366). Failure to provide all attributes will result in an [Invalid Index Composite Attributes Provided Error](https://electrodb.dev/en/reference/errors#invalid-index-composite-attributes-provided).
+- Addresses [Issue #366](https://github.com/tywalch/electrodb/issues/366) with unexpected outcomes from index `condition` usage. Discussion [inside the issue ticket](https://github.com/tywalch/electrodb/issues/366) revealed complexities associated with the implementation of the `condition` callback. Previously, a callback returning `false` would simply not write the fields associated with an index on update. Through discussion with [@sam3d](https://github.com/sam3d) and [@nonken](https://github.com/nonken), it was revealed that this behavior could lead to inconsistencies between indexes and attributes. Furthermore, this behavior did not align with user expectations/intuitions, which expected a `false` response to trigger the removal of the item from the index. To achieve this, it was discussed that the presence of a `condition` callback should add a _new_ runtime validation check on all mutations to verify all member attributes of the index must be provided if a mutation operation affects one of the attributes. Previously ElectroDB would validate only that composite members of an index field (a partition or sort key) within an index were fully provided; now, when a condition callback is present, it will validate that all members from both fields are provided. If you are unable to update/patch all member attributes, because some are readOnly, you can also use the [composite](https://electrodb.dev/en/mutations/patch#composite) method on [update](https://electrodb.dev/en/mutations/update#composite) and [patch](https://electrodb.dev/en/mutations/patch#composite). More information and the discussion around the reasoning behind this change can be found [here](https://github.com/tywalch/electrodb/issues/366). Failure to provide all attributes will result in an [Invalid Index Composite Attributes Provided Error](https://electrodb.dev/en/reference/errors#invalid-index-composite-attributes-provided).
+
+## [2.14.1] - 2024-05-17
+### Fixed
+- Further fixes in service of [Issue #366](https://github.com/tywalch/electrodb/issues/366). A bug was discovered that the logic to validate the presence of an attribute was a simple falsey check instead of a check that the value was `undefined`. This caused empty strings, zero values, and the boolean value `false` to incorrectly be considered missing.  
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "electrodb",
-  "version": "2.14.0",
+  "version": "2.14.1",
   "description": "A library to more easily create and interact with multiple entities and heretical relationships in dynamodb",
   "main": "index.js",
   "scripts": {

diff --git a/src/entity.js b/src/entity.js
@@ -3282,7 +3282,7 @@ class Entity {
         // the `missing` array will contain indexes that are partially provided, but that leaves cases where the pk or
         // sk of an index is complete but not both. Both cases are invalid if `indexConditionIsDefined=true`
         const missingAttributes = definition.all
-            .filter(({name}) => !attributes[name] && !included[name] || missing.includes(name))
+            .filter(({name}) => attributes[name] === undefined && included[name] === undefined || missing.includes(name))
             .map(({name}) => name)
 
         if (missingAttributes.length) {

diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts
@@ -4613,6 +4613,261 @@ describe("index condition", () => {
       expect(entriesByEffectiveDate2.data[0].organizationId).to.equal(organizationId);
     });
   });
+
+  describe('when a falsey value is provided', () => {
+    it('should not consider an attribute with an empty string value to be missing', () => {
+      const entity = new Entity({
+        model: {
+          entity: 'chatConversation',
+          version: '1.0',
+          service: 'chatbot',
+        },
+        attributes: {
+          conversationId: {
+            type: 'string',
+            required: true,
+          },
+          loggedInUserId: {
+            type: 'string',
+            required: false,
+          },
+          messages: {
+            type: 'list',
+            items: {
+              type: 'any',
+            },
+            required: true,
+          },
+        },
+        indexes: {
+          byConversationId: {
+            pk: {
+              field: 'pk',
+              composite: ['conversationId'],
+              casing: 'none',
+            },
+            sk: {
+              field: 'sk',
+              composite: [],
+              casing: 'none',
+            },
+          },
+          byUser: {
+            index: 'gsi1pk-gsi1sk-index',
+            type: 'clustered',
+            condition: (attr: any) => {
+              return !!attr.loggedInUserId;
+            },
+            pk: {
+              field: 'gsi1pk',
+              composite: ['loggedInUserId'],
+              casing: 'none',
+            },
+            sk: {
+              field: 'gsi1sk',
+              composite: ['conversationId'],
+              casing: 'none',
+            },
+          },
+        },
+      }, { table, client });
+
+      const operation = () => {
+        return entity.create({
+          conversationId: '123',
+          loggedInUserId: '', // empty string value
+          messages: []
+        }).params();
+      }
+
+      expect(operation).to.not.throw();
+
+      const params = operation();
+
+      expect(params).to.deep.equal({
+        Item: {
+          conversationId: '123',
+          loggedInUserId: '',
+          messages: [],
+          pk: '$chatbot#conversationId_123',
+          sk: '$chatConversation_1.0',
+          __edb_e__: 'chatConversation',
+          __edb_v__: '1.0'
+        },
+        TableName: table,
+        ConditionExpression: 'attribute_not_exists(#pk) AND attribute_not_exists(#sk)',
+        ExpressionAttributeNames: { '#pk': 'pk', '#sk': 'sk' }
+      });
+    });
+
+    it('should not consider an attribute with a zero value to be missing', () => {
+      const entity = new Entity({
+        model: {
+          entity: 'chatConversation',
+          version: '1.0',
+          service: 'chatbot',
+        },
+        attributes: {
+          conversationId: {
+            type: 'string',
+            required: true,
+          },
+          loggedInUserId: {
+            type: 'number',
+            required: false,
+          },
+          messages: {
+            type: 'list',
+            items: {
+              type: 'any',
+            },
+            required: true,
+          },
+        },
+        indexes: {
+          byConversationId: {
+            pk: {
+              field: 'pk',
+              composite: ['conversationId'],
+              casing: 'none',
+            },
+            sk: {
+              field: 'sk',
+              composite: [],
+              casing: 'none',
+            },
+          },
+          byUser: {
+            index: 'gsi1pk-gsi1sk-index',
+            type: 'clustered',
+            condition: (attr: any) => {
+              return !!attr.loggedInUserId;
+            },
+            pk: {
+              field: 'gsi1pk',
+              composite: ['loggedInUserId'],
+              casing: 'none',
+            },
+            sk: {
+              field: 'gsi1sk',
+              composite: ['conversationId'],
+              casing: 'none',
+            },
+          },
+        },
+      }, { table, client });
+      const operation = () => {
+        return entity.create({
+          conversationId: '123',
+          loggedInUserId: 0, // zero value
+          messages: []
+        }).params();
+      }
+
+      expect(operation).to.not.throw();
+
+      const params = operation();
+
+      expect(params).to.deep.equal({
+        Item: {
+          conversationId: '123',
+          loggedInUserId: 0,
+          messages: [],
+          pk: '$chatbot#conversationId_123',
+          sk: '$chatConversation_1.0',
+          __edb_e__: 'chatConversation',
+          __edb_v__: '1.0'
+        },
+        TableName: table,
+        ConditionExpression: 'attribute_not_exists(#pk) AND attribute_not_exists(#sk)',
+        ExpressionAttributeNames: { '#pk': 'pk', '#sk': 'sk' }
+      });
+    });
+
+    it('should not consider an attribute with a boolean false value to be missing', () => {
+      const entity = new Entity({
+        model: {
+          entity: 'chatConversation',
+          version: '1.0',
+          service: 'chatbot',
+        },
+        attributes: {
+          conversationId: {
+            type: 'string',
+            required: true,
+          },
+          isLoggedIn: {
+            type: 'boolean',
+            required: false,
+          },
+          messages: {
+            type: 'list',
+            items: {
+              type: 'any',
+            },
+            required: true,
+          },
+        },
+        indexes: {
+          byConversationId: {
+            pk: {
+              field: 'pk',
+              composite: ['conversationId'],
+              casing: 'none',
+            },
+            sk: {
+              field: 'sk',
+              composite: [],
+              casing: 'none',
+            },
+          },
+          byUser: {
+            index: 'gsi1pk-gsi1sk-index',
+            type: 'clustered',
+            condition: (attr: any) => {
+              return !!attr.isLoggedIn;
+            },
+            pk: {
+              field: 'gsi1pk',
+              composite: ['isLoggedIn'],
+              casing: 'none',
+            },
+            sk: {
+              field: 'gsi1sk',
+              composite: ['conversationId'],
+              casing: 'none',
+            },
+          },
+        },
+      }, { table, client });
+
+      const operation = () => {
+        return entity.create({
+          conversationId: '123',
+          isLoggedIn: false, // boolean false value
+          messages: []
+        }).params();
+      }
+
+      expect(operation).to.not.throw();
+
+      const params = operation();
+      expect(params).to.deep.equal({
+        Item: {
+          conversationId: '123',
+          isLoggedIn: false,
+          messages: [],
+          pk: '$chatbot#conversationId_123',
+          sk: '$chatConversation_1.0',
+          __edb_e__: 'chatConversation',
+          __edb_v__: '1.0'
+        },
+        TableName: table,
+        ConditionExpression: 'attribute_not_exists(#pk) AND attribute_not_exists(#sk)',
+        ExpressionAttributeNames: { '#pk': 'pk', '#sk': 'sk' }
+      });
+    });
+  });
 });