From 94ef99888e947a9967c5bd1eb97738b997b07f5b Mon Sep 17 00:00:00 2001 From: "Tyler W. Walch" Date: Fri, 17 May 2024 12:19:50 -0400 Subject: [PATCH] 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 --- CHANGELOG.md | 6 +- package.json | 2 +- src/entity.js | 2 +- test/ts_connected.entity.spec.ts | 255 +++++++++++++++++++++++++++++++ 4 files changed, 262 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4d56da3..65b5cf3c 100644 --- 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). \ No newline at end of file +- 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 index 72d6b02b..93fb23c5 100644 --- 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 index f77f786e..242cb1f7 100644 --- 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 index 19edd137..f8cab081 100644 --- 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' } + }); + }); + }); });