From 7b0ff3302389487c9c4f41e1294dc82c042e980b Mon Sep 17 00:00:00 2001 From: tywalch Date: Fri, 12 Apr 2024 11:42:11 -0400 Subject: [PATCH 01/14] Initial fix, prior to understanding logical issue with `update`, `patch`, and `upsert`. The operations `update`, `patch`, and `upsert` will need to change their current behavior when encountering a `false` value from `condition`. In that case, these operations will actually need to `delete` the `pk` and `sk` for that index. --- src/entity.js | 77 ++++-- test/offline.options.spec.js | 2 +- test/ts_connected.entity.spec.ts | 410 +++++++++++++++++++++++++------ 3 files changed, 393 insertions(+), 96 deletions(-) diff --git a/src/entity.js b/src/entity.js index e46a78cd..2d8f095d 100644 --- a/src/entity.js +++ b/src/entity.js @@ -2926,11 +2926,13 @@ class Entity { return params; } - _expectIndexFacets(attributes, facets) { + _expectIndexFacets(attributes, facets, utilizeIncludedOnlyIndexes) { let [isIncomplete, { incomplete, complete }] = this._getIndexImpact( attributes, facets, + utilizeIncludedOnlyIndexes, ); + if (isIncomplete) { let incompleteAccessPatterns = incomplete.map( ({ index }) => @@ -2940,6 +2942,7 @@ class Entity { (result, { missing }) => [...result, ...missing], [], ); + throw new e.ElectroError( e.ErrorCodes.IncompleteCompositeAttributes, `Incomplete composite attributes: Without the composite attributes ${u.commaSeparatedString( @@ -3043,10 +3046,13 @@ class Entity { let updateIndex = TableIndex; let keyTranslations = this.model.translations.keys; let keyAttributes = { ...sk, ...pk }; + let completeFacets = this._expectIndexFacets( { ...set }, { ...keyAttributes }, + true, ); + const removedKeyImpact = this._expectIndexFacets( { ...removed }, { ...keyAttributes }, @@ -3059,6 +3065,7 @@ class Entity { sk: "sk", }; } + let composedKeys = this._makeKeysFromAttributes( completeFacets.impactedIndexTypes, { ...set, ...keyAttributes }, @@ -3103,8 +3110,13 @@ class Entity { return { indexKey, updatedKeys, deletedKeys }; } + _indexConditionIsDefined(index) { + const definition = this.model.indexes[this.model.translations.indexes.fromIndexToAccessPattern[index]]; + return definition && definition.conditionDefined; + } + /* istanbul ignore next */ - _getIndexImpact(attributes = {}, included = {}) { + _getIndexImpact(attributes = {}, included = {}, utilizeIncludedOnlyIndexes) { let includedFacets = Object.keys(included); let impactedIndexes = {}; let skippedIndexes = new Set(); @@ -3114,17 +3126,47 @@ class Entity { for (let [attribute, indexes] of Object.entries(this.model.facets.byAttr)) { if (attributes[attribute] !== undefined) { facets[attribute] = attributes[attribute]; - indexes.forEach(({ index, type }) => { + indexes.forEach((definition) => { + const { index, type } = definition; impactedIndexes[index] = impactedIndexes[index] || {}; impactedIndexes[index][type] = impactedIndexes[index][type] || []; impactedIndexes[index][type].push(attribute); impactedIndexTypes[index] = impactedIndexTypes[index] || {}; - impactedIndexTypes[index][type] = - this.model.translations.keys[index][type]; + impactedIndexTypes[index][type] = this.model.translations.keys[index][type]; }); } } + // this function is used to determine key impact for update `set`, update `delete`, and `put`. This block is currently only used by update `set` + if (utilizeIncludedOnlyIndexes) { + for (const [index, { pk, sk }] of Object.entries(this.model.facets.byIndex)) { + // The main table index is handled somewhere else (messy I know), and we only want to do this processing if an index condition is defined for backwards compatibility. Backwards compatibility is not required for this change, but I have paranoid concerns of breaking changes around sparse indexes. + if (index === TableIndex || !this._indexConditionIsDefined(index)) { + continue; + } + + if (pk && pk.length && pk.every(attr => included[attr] !== undefined)) { + pk.forEach((attr) => { + facets[attr] = included[attr]; + }); + impactedIndexes[index] = impactedIndexes[index] || {}; + impactedIndexes[index][KeyTypes.pk] = [...pk]; + impactedIndexTypes[index] = impactedIndexTypes[index] || {}; + impactedIndexTypes[index][KeyTypes.pk] = this.model.translations.keys[index][KeyTypes.pk]; + } + + if (sk && sk.length && sk.every(attr => included[attr] !== undefined)) { + sk.forEach((attr) => { + facets[attr] = included[attr]; + }); + impactedIndexes[index] = impactedIndexes[index] || {}; + impactedIndexes[index][KeyTypes.sk] = [...sk]; + impactedIndexTypes[index] = impactedIndexTypes[index] || {}; + impactedIndexTypes[index][KeyTypes.sk] = this.model.translations.keys[index][KeyTypes.sk]; + } + } + } + for (const indexName in impactedIndexes) { const accessPattern = this.model.translations.indexes.fromIndexToAccessPattern[indexName]; const shouldMakeKeys = this.model.indexes[accessPattern].condition({ ...attributes, ...included }); @@ -3134,7 +3176,8 @@ class Entity { } let incomplete = Object.entries(this.model.facets.byIndex) - .map(([index, { pk, sk }]) => { + .map(([index, definition]) => { + const { pk, sk } = definition; let impacted = impactedIndexes[index]; let impact = { index, @@ -3146,15 +3189,15 @@ class Entity { let missingSk = impacted[KeyTypes.sk] && impacted[KeyTypes.sk].length !== sk.length; if (missingPk) { - impact.missing = [ - ...impact.missing, - ...pk.filter((attr) => { - return ( - !impacted[KeyTypes.pk].includes(attr) && - !includedFacets.includes(attr) - ); - }), - ]; + impact.missing = [ + ...impact.missing, + ...pk.filter((attr) => { + return ( + !impacted[KeyTypes.pk].includes(attr) && + !includedFacets.includes(attr) + ); + }), + ]; } if (missingSk) { impact.missing = [ @@ -3170,6 +3213,7 @@ class Entity { completedIndexes.push(index); } } + return impact; }) .filter(({ missing }) => missing.length); @@ -3944,7 +3988,9 @@ class Entity { `The index option 'condition' is only allowed on secondary indexes`, ); } + let indexCondition = index.condition || (() => true); + let conditionDefined = v.isFunction(index.condition) if (indexType === "clustered") { clusteredIndexes.add(accessPattern); @@ -4054,6 +4100,7 @@ class Entity { index: indexName, scope: indexScope, condition: indexCondition, + conditionDefined: conditionDefined, }; indexHasSubCollections[indexName] = diff --git a/test/offline.options.spec.js b/test/offline.options.spec.js index 3149fd1d..62e500d4 100644 --- a/test/offline.options.spec.js +++ b/test/offline.options.spec.js @@ -488,7 +488,7 @@ describe("Query Options", () => { }, }, { - description: "Should not string numbers that are greater than 0", + description: "Should allow string numbers that are greater than 0", error: false, input: { options: { concurrent: "1" }, diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index fd59a177..fac27cb2 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -3203,90 +3203,89 @@ describe("conditional indexes", () => { } }); - if (index !== 'sparse1') { - it(`${prefix} write index with provided update attributes`, async () => { - const {condition, invocations} = createConditionInvocationCollector(shouldWrite); - const {params, logger} = createParamsCollector(); - const {prop1, prop2, ...props} = createTestEntityData(); - const entity = createTestEntity(condition); - await entity.update({prop1, prop2}).set(props).go({logger}); - // @ts-ignore - const {data} = await entity.query[index]({prop1, prop2, ...props}).go(); - if (shouldWrite) { - expect(data.length).to.equal(1); - expect(data[0]).to.deep.equal({prop1, prop2, ...props}); - } else { - expect(data.length).to.equal(0); - } - for (const invocation of invocations) { - expect(invocation.attr).to.deep.equal({prop1, prop2, ...props}); - } - }); - - it(`${prefix} write index with provided update attributes spread across multiple method calls`, async () => { - const {condition, invocations} = createConditionInvocationCollector(shouldWrite); - const {params, logger} = createParamsCollector(); - const {prop1, prop2, ...props} = createTestEntityData(); - const entity = createTestEntity(condition); - await entity.update({prop1, prop2}).set({ - prop3: props.prop3, - prop4: props.prop4, - }).set({ - prop5: props.prop5, - prop6: props.prop6, - }).set({ - prop7: props.prop7, - prop8: props.prop8, - prop9: props.prop9, - }).go({logger}); - // @ts-ignore - const {data} = await entity.query[index]({prop1, prop2, ...props}).go(); - if (shouldWrite) { - expect(data.length).to.equal(1); - expect(data[0]).to.deep.equal({prop1, prop2, ...props}); - } else { - expect(data.length).to.equal(0); - } + it(`${prefix} write index with provided update attributes`, async () => { + const {condition, invocations} = createConditionInvocationCollector(shouldWrite); + const {params, logger} = createParamsCollector(); + const {prop1, prop2, ...props} = createTestEntityData(); + const entity = createTestEntity(condition); + await entity.update({prop1, prop2}).set(props).go({logger}); + // @ts-ignore + const {data} = await entity.query[index]({prop1, prop2, ...props}).go(); + if (shouldWrite) { + expect(data.length).to.equal(1); + expect(data[0]).to.deep.equal({prop1, prop2, ...props}); + } else { + expect(data.length).to.equal(0); + } - for (const invocation of invocations) { - expect(invocation.attr).to.deep.equal({prop1, prop2, ...props}); - } - }); - - it(`${prefix} write index with provided patch attributes`, async () => { - const {params, logger} = createParamsCollector(); - const {prop1, prop2, ...props} = createTestEntityData(); - let invocations: ConditionArguments[] = []; - let allow = false; - const condition = (args: ConditionArguments) => { - invocations.push(args); - return allow; - } + for (const invocation of invocations) { + expect(invocation.attr).to.deep.equal({prop1, prop2, ...props}); + } + }); - const entity = createTestEntity(condition); - // don't write for sure on put, but then yield to test; patch requires an existing item - await entity.put({prop1, prop2}).go(); - - // reset "allow" and reset "invocations" - allow = shouldWrite; - invocations = []; - - await entity.patch({prop1, prop2}).set(props).go({logger}); - // @ts-ignore - const {data} = await entity.query[index]({prop1, prop2, ...props}).go(); - if (shouldWrite) { - expect(data.length).to.equal(1); - expect(data[0]).to.deep.equal({prop1, prop2, ...props}); - } else { - expect(data.length).to.equal(0); - } + it(`${prefix} write index with provided update attributes spread across multiple method calls`, async () => { + const {condition, invocations} = createConditionInvocationCollector(shouldWrite); + const {params, logger} = createParamsCollector(); + const {prop1, prop2, ...props} = createTestEntityData(); + const entity = createTestEntity(condition); + await entity.update({prop1, prop2}).set({ + prop3: props.prop3, + prop4: props.prop4, + }).set({ + prop5: props.prop5, + prop6: props.prop6, + }).set({ + prop7: props.prop7, + prop8: props.prop8, + prop9: props.prop9, + }).go({logger}); + // @ts-ignore + const {data} = await entity.query[index]({prop1, prop2, ...props}).go(); + if (shouldWrite) { + expect(data.length).to.equal(1); + expect(data[0]).to.deep.equal({prop1, prop2, ...props}); + } else { + expect(data.length).to.equal(0); + } - for (const invocation of invocations) { - expect(invocation.attr).to.deep.equal({prop1, prop2, ...props}); - } - }); - } + for (const invocation of invocations) { + expect(invocation.attr).to.deep.equal({prop1, prop2, ...props}); + } + }); + + it(`${prefix} write index with provided patch attributes`, async () => { + const {params, logger} = createParamsCollector(); + const {prop1, prop2, ...props} = createTestEntityData(); + let invocations: ConditionArguments[] = []; + let allow = false; + const condition = (args: ConditionArguments) => { + invocations.push(args); + return allow; + } + + const entity = createTestEntity(condition); + // don't write for sure on put, but then yield to test; patch requires an existing item + await entity.put({prop1, prop2}).go(); + + // "reset" `allow` and "reset" `invocations` + allow = shouldWrite; + invocations = []; + + await entity.patch({prop1, prop2}).set(props).go({logger}); + // @ts-ignore + const {data} = await entity.query[index]({prop1, prop2, ...props}).go(); + if (shouldWrite) { + expect(data.length).to.equal(1); + expect(data[0]).to.deep.equal({prop1, prop2, ...props}); + } else { + expect(data.length).to.equal(0); + } + + for (const invocation of invocations) { + expect(invocation.attr).to.deep.equal({prop1, prop2, ...props}); + } + }); it(`${prefix} write index with provided batchPut attributes`, async () => { const { condition, invocations } = createConditionInvocationCollector(shouldWrite); @@ -3332,6 +3331,257 @@ describe("conditional indexes", () => { } }); } + + it('should fix gh issue 366', async () => { + const entityName = uuid(); + const updatedAt = new Date().toJSON(); + const createdAt = new Date().toJSON(); + const Thing = new Entity( + { + model: { + service: 'test', + entity: entityName, + version: '1' + }, + attributes: { + id: { + type: 'string', + required: true, + readOnly: true + }, + organizationId: { + type: 'string', + required: true, + readOnly: true + }, + accountId: { + type: 'string' + }, + createdAt: { + type: 'string', + readOnly: true, + required: true, + default: () => createdAt, + set: () => createdAt + }, + updatedAt: { + type: 'string', + watch: '*', + required: true, + default: () => updatedAt, + set: () => updatedAt + }, + settledAt: { + type: 'string', + default: 'n/a' + }, + effectiveAt: { + type: 'string', + default: 'n/a' + }, + type: { + type: 'string', + required: true + }, + category: { + type: 'string', + required: true + }, + amount: { + type: 'string', + required: true + }, + description: { + type: 'string' + } + }, + indexes: { + entries: { + pk: { + field: 'pk', + composite: ['organizationId'] + }, + sk: { + field: 'sk', + composite: ['id'] + } + }, + entriesByAccount: { + index: 'gsi1pk-gsi1sk-index', + pk: { + field: 'gsi1pk', + composite: ['organizationId'] + }, + sk: { + field: 'gsi1sk', + composite: ['accountId', 'id'] + } + }, + entriesBySettledDate: { + index: 'gsi2pk-gsi2sk-index', + condition: (attr) => attr.settledAt !== 'n/a', + pk: { + field: 'gsi2pk', + composite: ['organizationId'] + }, + sk: { + field: 'gsi2sk', + composite: ['settledAt'] + } + }, + entriesByEffectiveDate: { + index: 'gsi3pk-gsi3sk-index', + condition: (attr) => attr.effectiveAt !== 'n/a', + pk: { + field: 'gsi3pk', + composite: ['organizationId'] + }, + sk: { + field: 'gsi3sk', + composite: ['effectiveAt'] + } + } + } + }, + { table, client } + ); + + // with `effectiveAt` set to 'n/a' and `settledAt` set to 'today' the `entriesByEffectiveDate` index should not be written + const params1 = Thing.patch({ id: '123', organizationId: '123' }) + .set({ effectiveAt: 'n/a', accountId: '123', settledAt: 'today' }) + .params(); + + expect(params1).to.deep.equal({ + "UpdateExpression": "SET #effectiveAt = :effectiveAt_u0, #accountId = :accountId_u0, #settledAt = :settledAt_u0, #updatedAt = :updatedAt_u0, #gsi1sk = :gsi1sk_u0, #gsi2pk = :gsi2pk_u0, #gsi2sk = :gsi2sk_u0, #organizationId = :organizationId_u0, #id = :id_u0, #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0", + "ExpressionAttributeNames": { + "#pk": "pk", + "#sk": "sk", + "#accountId": "accountId", + "#settledAt": "settledAt", + "#updatedAt": "updatedAt", + "#effectiveAt": "effectiveAt", + "#gsi1sk": "gsi1sk", + "#gsi2pk": "gsi2pk", + "#gsi2sk": "gsi2sk", + "#organizationId": "organizationId", + "#id": "id", + "#__edb_e__": "__edb_e__", + "#__edb_v__": "__edb_v__" + }, + "ExpressionAttributeValues": { + ":accountId_u0": "123", + ":settledAt_u0": "today", + ":updatedAt_u0": updatedAt, + ":effectiveAt_u0": "n/a", + ":gsi1sk_u0": `$${entityName}_1#accountid_123#id_123`, + // gsi2pk_u0 was not set prior to this fix + ":gsi2pk_u0": "$test#organizationid_123", + ":gsi2sk_u0": `$${entityName}_1#settledat_today`, + ":organizationId_u0": "123", + ":id_u0": "123", + ":__edb_e___u0": `${entityName}`, + ":__edb_v___u0": "1" + }, + "TableName": "electro", + "Key": { + "pk": "$test#organizationid_123", + "sk": `$${entityName}_1#id_123` + }, + "ConditionExpression": "attribute_exists(#pk) AND attribute_exists(#sk)" + }); + + // with `effectiveAt` set to 'today' and `settledAt` set to 'n/a' the `entriesBySettledDate` index should not be written + const params2 = Thing.patch({ id: '123', organizationId: '123' }) + .set({ effectiveAt: 'today', accountId: '123', settledAt: 'n/a' }) + .params(); + + expect(params2).to.deep.equal({ + "UpdateExpression": "SET #effectiveAt = :effectiveAt_u0, #accountId = :accountId_u0, #settledAt = :settledAt_u0, #updatedAt = :updatedAt_u0, #gsi1sk = :gsi1sk_u0, #gsi3pk = :gsi3pk_u0, #gsi3sk = :gsi3sk_u0, #organizationId = :organizationId_u0, #id = :id_u0, #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0", + "ExpressionAttributeNames": { + "#pk": "pk", + "#sk": "sk", + "#effectiveAt": "effectiveAt", + "#settledAt": "settledAt", + "#accountId": "accountId", + "#updatedAt": "updatedAt", + "#gsi1sk": "gsi1sk", + "#gsi3pk": "gsi3pk", + "#gsi3sk": "gsi3sk", + "#organizationId": "organizationId", + "#id": "id", + "#__edb_e__": "__edb_e__", + "#__edb_v__": "__edb_v__" + }, + "ExpressionAttributeValues": { + ":effectiveAt_u0": "today", + ":settledAt_u0": "n/a", + ":accountId_u0": "123", + ":updatedAt_u0": updatedAt, + ":gsi1sk_u0": `$${entityName}_1#accountid_123#id_123`, + // gsi3pk_u0 was not set prior to this fix + ":gsi3pk_u0": "$test#organizationid_123", + ":gsi3sk_u0": `$${entityName}_1#effectiveat_today`, + ":organizationId_u0": "123", + ":id_u0": "123", + ":__edb_e___u0": `${entityName}`, + ":__edb_v___u0": "1" + }, + "TableName": "electro", + "Key": { + "pk": "$test#organizationid_123", + "sk": `$${entityName}_1#id_123` + }, + "ConditionExpression": "attribute_exists(#pk) AND attribute_exists(#sk)" + }); + + const organizationId = uuid(); + const accountId = uuid(); + const id = uuid(); + const type = 'green' + const category = 'liquid' + const amount = '200' + const description = 'a description'; + + await Thing.create({ + organizationId, + accountId, + id, + type, + amount, + category, + description, + settledAt: 'n/a', + effectiveAt: 'n/a' + }).go(); + + // 'gsi1pk-gsi1sk-index' should have been written to + const entriesByAccount = await Thing.query.entriesByAccount({ organizationId, accountId }).go(); + expect(entriesByAccount.data.length).to.equal(1); + expect(entriesByAccount.data[0].id).to.equal(id); + expect(entriesByAccount.data[0].organizationId).to.equal(organizationId); + + // 'gsi2pk-gsi2sk-index' should not have been written to + const entriesBySettledDate = await Thing.query.entriesBySettledDate({ organizationId }).go(); + expect(entriesBySettledDate.data.length).to.equal(0); + + // with settledAt set to 'today', 'gsi2pk-gsi2sk-index' should be written to + await Thing.patch({ id, organizationId }).set({ settledAt: 'today' }).go(); + const entriesBySettledDate2 = await Thing.query.entriesBySettledDate({ organizationId }).go(); + expect(entriesBySettledDate2.data.length).to.equal(1); + expect(entriesBySettledDate2.data[0].id).to.equal(id); + expect(entriesBySettledDate2.data[0].organizationId).to.equal(organizationId); + + // 'gsi3pk-gsi3sk-index' should not have been written to + const entriesByEffectiveDate = await Thing.query.entriesByEffectiveDate({ organizationId }).go(); + expect(entriesByEffectiveDate.data.length).to.equal(0); + + // with effectiveAt set to 'today', 'gsi3pk-gsi3sk-index' should be written to + await Thing.patch({ id, organizationId }).set({ effectiveAt: 'today' }).go(); + const entriesByEffectiveDate2 = await Thing.query.entriesByEffectiveDate({ organizationId }).go(); + expect(entriesByEffectiveDate2.data.length).to.equal(1); + expect(entriesByEffectiveDate2.data[0].id).to.equal(id); + expect(entriesByEffectiveDate2.data[0].organizationId).to.equal(organizationId); + }); }); From 3e8461897b1c3b8247dd91d1af9a576dcd054e8a Mon Sep 17 00:00:00 2001 From: tywalch Date: Fri, 12 Apr 2024 18:09:59 -0400 Subject: [PATCH 02/14] Solidifies new `condition` logic The condition callback will be invoked only when a composite attribute associated with an index is set via an update, patch, or upsert. [existing behavior] The condition callback is provided the attributes being set on that particular operation, including the item's identifying composite attributes. [existing behavior] If the condition callback returns true, ElectroDB will attempt to create the index and all of its associated keys. If an index cannot be created because an update operation only has enough context for a partial key, ElectroDB will throw. [the original issue here, fixed] If the condition callback returns false, the index and all of its associated keys will be removed from the item. [new behavior] Item #1 above is the key to solving the issue you bring up in your first comment, and it's actually what we do currently. This means that condition would only be called when an index must be recalculated. furthermore, as described in #3, ElectroDB will actually throw if your update operation (set and remove) lacks a full composite context and would result in a "partial" key. This would mean that all * -> true transitions are already validated to have all the composite parts necessary to recreate the complete index already. --- src/entity.js | 85 +++++++++++++++++++++++++------- test/offline.entity.spec.js | 25 ++++++++++ test/ts_connected.entity.spec.ts | 8 ++- 3 files changed, 99 insertions(+), 19 deletions(-) diff --git a/src/entity.js b/src/entity.js index 2d8f095d..e741363f 100644 --- a/src/entity.js +++ b/src/entity.js @@ -39,6 +39,11 @@ const u = require("./util"); const e = require("./errors"); const v = require("./validations"); +const ImpactedIndexTypeSource = { + composite: 'composite', + provided: 'provided', +} + class Entity { constructor(model, config = {}) { config = c.normalizeConfig(config); @@ -2926,11 +2931,11 @@ class Entity { return params; } - _expectIndexFacets(attributes, facets, utilizeIncludedOnlyIndexes) { + _expectIndexFacets(attributes, facets, { utilizeIncludedOnlyIndexes, skipConditionCheck } = {}) { let [isIncomplete, { incomplete, complete }] = this._getIndexImpact( attributes, facets, - utilizeIncludedOnlyIndexes, + { utilizeIncludedOnlyIndexes, skipConditionCheck }, ); if (isIncomplete) { @@ -3011,6 +3016,7 @@ class Entity { let completeFacets = this._expectIndexFacets( { ...setAttributes, ...validationAssistance }, { ...keyAttributes }, + { set }, ); // complete facets, only includes impacted facets which likely does not include the updateIndex which then needs to be added here. @@ -3050,12 +3056,13 @@ class Entity { let completeFacets = this._expectIndexFacets( { ...set }, { ...keyAttributes }, - true, + { utilizeIncludedOnlyIndexes: true }, ); const removedKeyImpact = this._expectIndexFacets( { ...removed }, { ...keyAttributes }, + { skipConditionCheck: true } ); // complete facets, only includes impacted facets which likely does not include the updateIndex which then needs to be added here. @@ -3074,6 +3081,14 @@ class Entity { let updatedKeys = {}; let deletedKeys = []; let indexKey = {}; + for (const [indexName, condition] of Object.entries(completeFacets.conditions)) { + if (!condition) { + deletedKeys.push(this.model.translations.keys[indexName][KeyTypes.pk]); + if (this.model.translations.keys[indexName][KeyTypes.sk]) { + deletedKeys.push(this.model.translations.keys[indexName][KeyTypes.sk]); + } + } + } for (const keys of Object.values(removedKeyImpact.impactedIndexTypes)) { deletedKeys = deletedKeys.concat(Object.values(keys)); } @@ -3116,11 +3131,13 @@ class Entity { } /* istanbul ignore next */ - _getIndexImpact(attributes = {}, included = {}, utilizeIncludedOnlyIndexes) { + _getIndexImpact(attributes = {}, included = {}, { utilizeIncludedOnlyIndexes, skipConditionCheck } = {}) { let includedFacets = Object.keys(included); let impactedIndexes = {}; + let conditions = {}; let skippedIndexes = new Set(); let impactedIndexTypes = {}; + let impactedIndexTypeSources = {}; let completedIndexes = []; let facets = {}; for (let [attribute, indexes] of Object.entries(this.model.facets.byAttr)) { @@ -3133,6 +3150,9 @@ class Entity { impactedIndexes[index][type].push(attribute); impactedIndexTypes[index] = impactedIndexTypes[index] || {}; impactedIndexTypes[index][type] = this.model.translations.keys[index][type]; + + impactedIndexTypeSources[index] = impactedIndexTypeSources[index] || {}; + impactedIndexTypeSources[index][type] = ImpactedIndexTypeSource.provided; }); } } @@ -3153,25 +3173,56 @@ class Entity { impactedIndexes[index][KeyTypes.pk] = [...pk]; impactedIndexTypes[index] = impactedIndexTypes[index] || {}; impactedIndexTypes[index][KeyTypes.pk] = this.model.translations.keys[index][KeyTypes.pk]; + + // flagging the impactedIndexTypeSource as `composite` means the entire key is only being impacted because + // all composites are in `included`. This will help us determine if we need to evaluate the `condition` + // callback for the index. If both the `sk` and `pk` were impacted because of `included` then we can skip + // the condition check because the index doesn't need to be recalculated; + impactedIndexTypeSources[index] = impactedIndexTypeSources[index] || {}; + impactedIndexTypeSources[index][KeyTypes.pk] = impactedIndexTypeSources[index][KeyTypes.pk] || ImpactedIndexTypeSource.composite; } if (sk && sk.length && sk.every(attr => included[attr] !== undefined)) { - sk.forEach((attr) => { - facets[attr] = included[attr]; - }); - impactedIndexes[index] = impactedIndexes[index] || {}; - impactedIndexes[index][KeyTypes.sk] = [...sk]; - impactedIndexTypes[index] = impactedIndexTypes[index] || {}; - impactedIndexTypes[index][KeyTypes.sk] = this.model.translations.keys[index][KeyTypes.sk]; + if (this.model.translations.keys[index][KeyTypes.sk]) { + sk.forEach((attr) => { + facets[attr] = included[attr]; + }); + impactedIndexes[index] = impactedIndexes[index] || {}; + impactedIndexes[index][KeyTypes.sk] = [...sk]; + impactedIndexTypes[index] = impactedIndexTypes[index] || {}; + impactedIndexTypes[index][KeyTypes.sk] = this.model.translations.keys[index][KeyTypes.sk]; + + // flagging the impactedIndexTypeSource as `composite` means the entire key is only being impacted because + // all composites are in `included`. This will help us determine if we need to evaluate the `condition` + // callback for the index. If both the `sk` and `pk` were impacted because of `included` then we can skip + // the condition check because the index doesn't need to be recalculated; + impactedIndexTypeSources[index] = impactedIndexTypeSources[index] || {}; + impactedIndexTypeSources[index][KeyTypes.sk] = impactedIndexTypeSources[index][KeyTypes.sk] || ImpactedIndexTypeSource.composite; + } } } } - for (const indexName in impactedIndexes) { - const accessPattern = this.model.translations.indexes.fromIndexToAccessPattern[indexName]; - const shouldMakeKeys = this.model.indexes[accessPattern].condition({ ...attributes, ...included }); - if (!shouldMakeKeys) { - skippedIndexes.add(indexName); + // skipConditionCheck is being used by update `remove`. If Attributes are being removed then the condition check is + // meaningless and ElectroDB should uphold its obligation to keep keys and attributes in sync. + if (!skipConditionCheck) { + for (const indexName in impactedIndexes) { + // this condition is a bit complex. The for loop above will note an index as "impacted" if each composite in a + // `pk` or `sk` is in `included` (which is currently being passed main table facets). If the index was only + // marked as "impacted" because of this reason, we don't need to invoke the condition check. This is because + // a "false" value from the condition check will signal an index must be deleted. It would be unintuitive to the + // user to revaluate the condition everytime any update is done; better to only evaluate if they set a value on + // a composite attribute for that index. + if (impactedIndexTypeSources[indexName][KeyTypes.pk] === ImpactedIndexTypeSource.provided || impactedIndexTypeSources[indexName][KeyTypes.sk] === ImpactedIndexTypeSource.provided) { + const accessPattern = this.model.translations.indexes.fromIndexToAccessPattern[indexName]; + let shouldMakeKeys = this.model.indexes[accessPattern].condition({...attributes, ...included}); + if (!shouldMakeKeys) { + skippedIndexes.add(indexName); + } + conditions[indexName] = shouldMakeKeys; + } else { + skippedIndexes.add(indexName); + } } } @@ -3219,7 +3270,7 @@ class Entity { .filter(({ missing }) => missing.length); let isIncomplete = !!incomplete.length; - let complete = { facets, indexes: completedIndexes, impactedIndexTypes }; + let complete = { facets, indexes: completedIndexes, impactedIndexTypes, conditions }; return [isIncomplete, { incomplete, complete }]; } diff --git a/test/offline.entity.spec.js b/test/offline.entity.spec.js index 9555352e..417a9ce3 100644 --- a/test/offline.entity.spec.js +++ b/test/offline.entity.spec.js @@ -1624,6 +1624,12 @@ describe("Entity", () => { sk: "gsi4sk", }, }, + conditions: { + "gsi1pk-gsi1sk-index": true, + "gsi2pk-gsi2sk-index": true, + "gsi3pk-gsi3sk-index": true, + "gsi4pk-gsi4sk-index": true, + } }, }, ]); @@ -1646,6 +1652,9 @@ describe("Entity", () => { sk: "gsi2sk", }, }, + conditions: { + "gsi2pk-gsi2sk-index": true, + } }, }, ]); @@ -1682,6 +1691,12 @@ describe("Entity", () => { sk: "gsi4sk", }, }, + conditions: { + "gsi1pk-gsi1sk-index": true, + "gsi2pk-gsi2sk-index": true, + "gsi3pk-gsi3sk-index": true, + "gsi4pk-gsi4sk-index": true, + }, }, }, ]); @@ -1713,6 +1728,12 @@ describe("Entity", () => { sk: "gsi4sk", }, }, + conditions: { + "gsi1pk-gsi1sk-index": true, + "gsi2pk-gsi2sk-index": true, + "gsi3pk-gsi3sk-index": true, + "gsi4pk-gsi4sk-index": true, + } }, }, ]); @@ -1735,6 +1756,10 @@ describe("Entity", () => { sk: "gsi3sk", }, }, + conditions: { + "gsi2pk-gsi2sk-index": true, + "gsi3pk-gsi3sk-index": true, + } }, }, ]); diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index fac27cb2..859f0117 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -3452,7 +3452,7 @@ describe("conditional indexes", () => { .params(); expect(params1).to.deep.equal({ - "UpdateExpression": "SET #effectiveAt = :effectiveAt_u0, #accountId = :accountId_u0, #settledAt = :settledAt_u0, #updatedAt = :updatedAt_u0, #gsi1sk = :gsi1sk_u0, #gsi2pk = :gsi2pk_u0, #gsi2sk = :gsi2sk_u0, #organizationId = :organizationId_u0, #id = :id_u0, #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0", + "UpdateExpression": "SET #effectiveAt = :effectiveAt_u0, #accountId = :accountId_u0, #settledAt = :settledAt_u0, #updatedAt = :updatedAt_u0, #gsi1sk = :gsi1sk_u0, #gsi2pk = :gsi2pk_u0, #gsi2sk = :gsi2sk_u0, #organizationId = :organizationId_u0, #id = :id_u0, #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0 REMOVE #gsi3pk, #gsi3sk", "ExpressionAttributeNames": { "#pk": "pk", "#sk": "sk", @@ -3463,6 +3463,8 @@ describe("conditional indexes", () => { "#gsi1sk": "gsi1sk", "#gsi2pk": "gsi2pk", "#gsi2sk": "gsi2sk", + "#gsi3pk": "gsi3pk", + "#gsi3sk": "gsi3sk", "#organizationId": "organizationId", "#id": "id", "#__edb_e__": "__edb_e__", @@ -3496,7 +3498,7 @@ describe("conditional indexes", () => { .params(); expect(params2).to.deep.equal({ - "UpdateExpression": "SET #effectiveAt = :effectiveAt_u0, #accountId = :accountId_u0, #settledAt = :settledAt_u0, #updatedAt = :updatedAt_u0, #gsi1sk = :gsi1sk_u0, #gsi3pk = :gsi3pk_u0, #gsi3sk = :gsi3sk_u0, #organizationId = :organizationId_u0, #id = :id_u0, #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0", + "UpdateExpression": "SET #effectiveAt = :effectiveAt_u0, #accountId = :accountId_u0, #settledAt = :settledAt_u0, #updatedAt = :updatedAt_u0, #gsi1sk = :gsi1sk_u0, #gsi3pk = :gsi3pk_u0, #gsi3sk = :gsi3sk_u0, #organizationId = :organizationId_u0, #id = :id_u0, #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0 REMOVE #gsi2pk, #gsi2sk", "ExpressionAttributeNames": { "#pk": "pk", "#sk": "sk", @@ -3505,6 +3507,8 @@ describe("conditional indexes", () => { "#accountId": "accountId", "#updatedAt": "updatedAt", "#gsi1sk": "gsi1sk", + "#gsi2pk": "gsi2pk", + "#gsi2sk": "gsi2sk", "#gsi3pk": "gsi3pk", "#gsi3sk": "gsi3sk", "#organizationId": "organizationId", From b7d5e6f19c664196b2eca69276b51c06a8b7dd2c Mon Sep 17 00:00:00 2001 From: tywalch Date: Fri, 19 Apr 2024 13:04:14 -0400 Subject: [PATCH 03/14] Checkpoint commit Checkpointing initial pass at new condition tests, tests not passing. --- src/clauses.js | 6 +- src/entity.js | 84 ++++-- src/errors.js | 6 + test/ts_connected.entity.spec.ts | 478 ++++++++++++++++++++++++++++++- 4 files changed, 541 insertions(+), 33 deletions(-) diff --git a/src/clauses.js b/src/clauses.js index f9c27472..aaee3261 100644 --- a/src/clauses.js +++ b/src/clauses.js @@ -432,12 +432,16 @@ let clauses = { const { pk } = state.query.keys; const sk = state.query.keys.sk[0]; - const { updatedKeys, setAttributes, indexKey } = entity._getPutKeys( + const { updatedKeys, setAttributes, indexKey, deletedKeys = [] } = entity._getPutKeys( pk, sk && sk.facets, onlySetAppliedData, ); + for (const deletedKey of deletedKeys) { + state.query.update.remove(deletedKey) + } + // calculated here but needs to be used when building the params upsert.indexKey = indexKey; diff --git a/src/entity.js b/src/entity.js index e741363f..446308e6 100644 --- a/src/entity.js +++ b/src/entity.js @@ -3019,6 +3019,16 @@ class Entity { { set }, ); + let deletedKeys = []; + for (const [indexName, condition] of Object.entries(completeFacets.conditions)) { + if (!condition) { + deletedKeys.push(this.model.translations.keys[indexName][KeyTypes.pk]); + if (this.model.translations.keys[indexName][KeyTypes.sk]) { + deletedKeys.push(this.model.translations.keys[indexName][KeyTypes.sk]); + } + } + } + // complete facets, only includes impacted facets which likely does not include the updateIndex which then needs to be added here. if (!completeFacets.indexes.includes(updateIndex)) { completeFacets.indexes.push(updateIndex); @@ -3045,7 +3055,7 @@ class Entity { } } - return { indexKey, updatedKeys, setAttributes }; + return { indexKey, updatedKeys, setAttributes, deletedKeys }; } _getUpdatedKeys(pk, sk, set, removed) { @@ -3089,9 +3099,11 @@ class Entity { } } } + for (const keys of Object.values(removedKeyImpact.impactedIndexTypes)) { deletedKeys = deletedKeys.concat(Object.values(keys)); } + for (let [index, keys] of Object.entries(composedKeys)) { let { pk, sk } = keyTranslations[index]; if (index === updateIndex) { @@ -3203,30 +3215,7 @@ class Entity { } } - // skipConditionCheck is being used by update `remove`. If Attributes are being removed then the condition check is - // meaningless and ElectroDB should uphold its obligation to keep keys and attributes in sync. - if (!skipConditionCheck) { - for (const indexName in impactedIndexes) { - // this condition is a bit complex. The for loop above will note an index as "impacted" if each composite in a - // `pk` or `sk` is in `included` (which is currently being passed main table facets). If the index was only - // marked as "impacted" because of this reason, we don't need to invoke the condition check. This is because - // a "false" value from the condition check will signal an index must be deleted. It would be unintuitive to the - // user to revaluate the condition everytime any update is done; better to only evaluate if they set a value on - // a composite attribute for that index. - if (impactedIndexTypeSources[indexName][KeyTypes.pk] === ImpactedIndexTypeSource.provided || impactedIndexTypeSources[indexName][KeyTypes.sk] === ImpactedIndexTypeSource.provided) { - const accessPattern = this.model.translations.indexes.fromIndexToAccessPattern[indexName]; - let shouldMakeKeys = this.model.indexes[accessPattern].condition({...attributes, ...included}); - if (!shouldMakeKeys) { - skippedIndexes.add(indexName); - } - conditions[indexName] = shouldMakeKeys; - } else { - skippedIndexes.add(indexName); - } - } - } - - let incomplete = Object.entries(this.model.facets.byIndex) + let indexesWithMissingCompsites = Object.entries(this.model.facets.byIndex) .map(([index, definition]) => { const { pk, sk } = definition; let impacted = impactedIndexes[index]; @@ -3234,7 +3223,7 @@ class Entity { index, missing: [] }; - if (impacted && !skippedIndexes.has(index)) { + if (impacted) { let missingPk = impacted[KeyTypes.pk] && impacted[KeyTypes.pk].length !== pk.length; let missingSk = @@ -3266,8 +3255,45 @@ class Entity { } return impact; - }) - .filter(({ missing }) => missing.length); + }); + + const incomplete = []; + for (const { index, missing } of indexesWithMissingCompsites) { + if (!missing.length) { + continue; + } + + const indexConditionIsDefined = this._indexConditionIsDefined(index); + + // - `skipConditionCheck` is being used by update `remove`. If Attributes are being removed then the condition check is + // meaningless and ElectroDB should uphold its obligation to keep keys and attributes in sync. + // - `index === TableIndex` is a special case where we don't need to check the condition because the main table is immutable + // - `!this._indexConditionIsDefined(index)` means the index doesn't have a condition defined, so we can skip the check + if (!skipConditionCheck || index === TableIndex || !indexConditionIsDefined) { + incomplete.push({ index, missing }); + } + + if (impactedIndexTypeSources[index][KeyTypes.pk] === ImpactedIndexTypeSource.provided || impactedIndexTypeSources[index][KeyTypes.sk] === ImpactedIndexTypeSource.provided) { + const missingAreProvidedInAttributesOrIncluded = missing + .every((attr) => attributes[attr] !== undefined || included[attr] !== undefined); + + if (!missingAreProvidedInAttributesOrIncluded) { + throw new e.ElectroError(e.ErrorCodes.IncompleteIndexCompositesAttributesProvided, `Incomplete composite attributes provided for index ${index}. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: ${u.commaSeparatedString(missing)}`); + } + + const accessPattern = this.model.translations.indexes.fromIndexToAccessPattern[index]; + let shouldMakeKeys = !!this.model.indexes[accessPattern].condition({...attributes, ...included}); + + // this helps identify which conditions were checked (key is present) and what the result was (true/false) + conditions[index] = shouldMakeKeys; + if (!shouldMakeKeys) { + continue; + } + } else { + incomplete.push({ index, missing }); + } + } + let isIncomplete = !!incomplete.length; let complete = { facets, indexes: completedIndexes, impactedIndexTypes, conditions }; @@ -4040,8 +4066,8 @@ class Entity { ); } + let conditionDefined = v.isFunction(index.condition); let indexCondition = index.condition || (() => true); - let conditionDefined = v.isFunction(index.condition) if (indexType === "clustered") { clusteredIndexes.add(accessPattern); diff --git a/src/errors.js b/src/errors.js index eee842ef..ba028f61 100644 --- a/src/errors.js +++ b/src/errors.js @@ -217,6 +217,12 @@ const ErrorCodes = { name: "InvalidIndexOption", sym: ErrorCode, }, + IncompleteIndexCompositesAttributesProvided: { + code: 2012, + section: 'invalid-index-composite-attributes-provided', + name: 'IncompleteIndexCompositesAttributesProvided', + sym: ErrorCode, + }, InvalidAttribute: { code: 3001, section: "invalid-attribute", diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index 859f0117..08248788 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -2,6 +2,7 @@ import { DocumentClient, PutItemInput } from "aws-sdk/clients/dynamodb"; import { Entity, EntityRecord, createWriteTransaction, ElectroEvent } from "../"; import { expect } from "chai"; import { v4 as uuid } from "uuid"; +import {listeners} from "cluster"; const u = require("../src/util"); type ConversionTest = { @@ -2953,13 +2954,18 @@ describe("conditional indexes", () => { } function createConditionInvocationCollector(result: boolean) { - const invocations: ConditionArguments[] = []; + let invocations: ConditionArguments[] = []; const condition: TestEntityCondition = (options) => { invocations.push(options); return result; }; + const clear = () => { + invocations.length = 0; + } + return { + clear, condition, invocations, } @@ -2977,6 +2983,472 @@ describe("conditional indexes", () => { } } + function expectMessageIfThrows(fn: () => void, errMessage?: string) { + let error: Error | undefined = undefined; + try { + fn(); + } catch(err: any) { + error = err; + } + + if (errMessage && !error) { + throw new Error(`Expected error message: ${errMessage}`); + } else if (errMessage && error) { + expect(error.message).to.equal(errMessage); + } else if (error) { + throw error; + } + } + + const formatShouldStatement = (should: boolean) => `should${should ? ' ' : ' not '}`; + + describe('when all composite attributes are not provided', () => { + const conditionCases = [ + ['condition is set and returns true', true], + ['condition is set and returns false', false], + ['condition is not set', undefined], + ]; + for (const [variation, setCondition] of conditionCases) { + describe(`when composite attributes are distinct and ${variation}`, () => { + const collector = createConditionInvocationCollector(!!setCondition); + const conditionIsSet = setCondition !== undefined; + const condition = conditionIsSet ? collector.condition : undefined; + afterEach(() => { + collector.clear(); + }); + const entity = new Entity( + { + model: { + entity: uuid(), + service: uuid(), + version: "1", + }, + attributes: { + prop1: { + type: "string", + }, + prop2: { + type: "string", + }, + prop3: { + type: "string" + }, + }, + indexes: { + test: { + collection: 'testing', + pk: { + field: "pk", + composite: ["prop1"], + }, + sk: { + field: "sk", + composite: ["prop2"], + }, + }, + sparse1: { + index: 'gsi1pk-gsi1sk-index', + // @ts-ignore + condition: condition, + pk: { + field: "gsi1pk", + composite: ["prop1"], + }, + sk: { + field: "gsi1sk", + composite: ["prop2"], + }, + } + }, + }, + {table, client} + ); + + it(`should not throw when impacting composite attributes on put`, () => { + expectMessageIfThrows(() => { + entity.put({ + prop1: uuid(), + prop2: uuid(), + prop3: uuid(), + }).params(); + }); + + if (conditionIsSet) { + expect(collector.invocations.length).to.not.equal(0); + } + }); + + it(`should not throw when impacting composite attributes on create`, () => { + expectMessageIfThrows(() => { + entity.create({ + prop1: uuid(), + prop2: uuid(), + prop3: uuid(), + }).params(); + }); + if (conditionIsSet) { + expect(collector.invocations.length).to.not.equal(0); + } + }); + + it(`should not throw when impacting composite attributes on update`, () => { + expectMessageIfThrows(() => { + entity.update({ prop1: uuid(), prop2: uuid() }) + .set({prop3: uuid()}) + .params(); + }); + if (conditionIsSet) { + expect(collector.invocations.length).to.not.equal(0); + } + }); + + it(`should not throw when impacting composite attributes on patch`, () => { + expectMessageIfThrows(() => { + entity.patch({ prop1: uuid(), prop2: uuid() }) + .set({ prop3: uuid() }) + .params(); + }); + if (conditionIsSet) { + expect(collector.invocations.length).to.not.equal(0); + } + }); + + it(`should not throw when impacting composite attributes on upsert`, () => { + expectMessageIfThrows(() => { + entity.upsert({ + prop1: uuid(), + prop2: uuid(), + prop3: uuid(), + }).params(); + }); + if (conditionIsSet) { + expect(collector.invocations.length).to.not.equal(0); + } + }); + }); + + describe(`when composite attributes share main table index composites and ${variation}`, () => { + const collector = createConditionInvocationCollector(!!setCondition); + const conditionIsSet = setCondition !== undefined; + const condition = conditionIsSet ? collector.condition : undefined; + const entity = new Entity( + { + model: { + entity: uuid(), + service: uuid(), + version: "1", + }, + attributes: { + prop1: { + type: "string", + }, + prop2: { + type: "string", + }, + prop3: { + type: "string" + }, + prop4: { + type: "string" + }, + prop5: { + type: "string" + }, + prop6: { + type: "string" + }, + }, + indexes: { + test: { + collection: 'testing', + pk: { + field: "pk", + composite: ["prop1"], + }, + sk: { + field: "sk", + composite: ["prop2"], + }, + }, + sparse2: { + index: 'gsi2pk-gsi2sk-index', + // @ts-ignore + condition: condition, + pk: { + field: 'gsi2pk', + composite: ['prop2', 'prop3'] + }, + sk: { + field: 'gsi2sk', + composite: ['prop1', 'prop4', 'prop5'] + } + }, + }, + }, + {table, client} + ); + + beforeEach(() => { + collector.clear(); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on put`, () => { + const message = conditionIsSet ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop3" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' : undefined; + expectMessageIfThrows(() => { + entity.put({ + prop1: uuid(), + prop2: uuid(), + prop4: uuid(), + prop5: uuid(), + }).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when missing composite attributes on put`, () => { + const message = conditionIsSet ? 'Oops!' : undefined; + expectMessageIfThrows(() => { + entity.put({ + prop1: uuid(), + prop2: uuid(), + prop6: uuid(), + }).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on create`, () => { + const message = conditionIsSet ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop4", "prop5" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' : undefined; + expectMessageIfThrows(() => { + entity.create({ + prop1: uuid(), + prop2: uuid(), + prop3: uuid(), + }).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when missing composite attributes on create`, () => { + const message = conditionIsSet ? 'Oops!' : undefined; + expectMessageIfThrows(() => { + entity.create({ + prop1: uuid(), + prop2: uuid(), + prop6: uuid(), + }).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when missing composite attributes on update`, () => { + const message = conditionIsSet ? 'Oops!' : undefined; + expectMessageIfThrows(() => { + entity.update({ + prop1: uuid(), + prop2: uuid(), + }).set({ prop6: uuid() }).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on update`, () => { + const message = conditionIsSet ? 'missing prop3' : undefined; + expectMessageIfThrows(() => { + entity.update({ + prop1: uuid(), + prop2: uuid(), + }).set({ prop4: uuid(), prop5: uuid() }).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on patch`, () => { + const message = conditionIsSet ? 'missing prop4 and prop5' : undefined; + expectMessageIfThrows(() => { + entity.patch({ + prop1: uuid(), + prop2: uuid(), + }).set({prop3: uuid()}).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when missing composite attributes on patch`, () => { + const message = conditionIsSet ? 'Oops!' : undefined; + expectMessageIfThrows(() => { + entity.patch({ + prop1: uuid(), + prop2: uuid(), + }).set({prop6: uuid()}).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on upsert`, () => { + const message = conditionIsSet ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop3" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' : undefined; + expectMessageIfThrows(() => { + entity.upsert({ + prop1: uuid(), + prop2: uuid(), + prop5: uuid(), + prop4: uuid(), + }).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when missing composite attributes on upsert`, () => { + const message = conditionIsSet ? 'Oops!' : undefined; + expectMessageIfThrows(() => { + entity.upsert({ + prop1: uuid(), + prop2: uuid(), + prop6: uuid(), + }).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + }); + + describe(`when composite attributes are identical to main table index and ${variation}`, () => { + const collector = createConditionInvocationCollector(!!setCondition); + const conditionIsSet = setCondition !== undefined; + const condition = conditionIsSet ? collector.condition : undefined; + const entity = new Entity( + { + model: { + entity: uuid(), + service: uuid(), + version: "1", + }, + attributes: { + prop1: { + type: "string", + }, + prop2: { + type: "string", + }, + prop3: { + type: "string" + }, + prop4: { + type: "string" + }, + prop5: { + type: "string" + }, + prop6: { + type: "string" + }, + prop7: { + type: "string" + }, + prop8: { + type: "string" + }, + prop9: { + type: "string" + } + }, + indexes: { + test: { + collection: 'testing', + pk: { + field: "pk", + composite: ["prop1"], + }, + sk: { + field: "sk", + composite: ["prop2"], + }, + }, + sparse3: { + index: 'gsi3pk-gsi3sk-index', + // @ts-ignore + condition: condition, + pk: { + field: 'gsi3pk', + composite: ['prop6', 'prop7'] + }, + sk: { + field: 'gsi3sk', + composite: ['prop8', 'prop9'] + } + } + }, + }, + {table, client} + ); + + beforeEach(() => { + collector.clear(); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on put`, () => { + const message = conditionIsSet ? 'missing prop8 and prop9' : undefined; + expectMessageIfThrows(() => { + entity.put({ + prop1: uuid(), + prop2: uuid(), + prop7: uuid(), + prop6: uuid(), + }).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on create`, () => { + const message = conditionIsSet ? 'missing prop6 and prop7' : undefined; + expectMessageIfThrows(() => { + entity.create({ + prop1: uuid(), + prop2: uuid(), + prop8: uuid(), + prop9: uuid(), + }).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on update`, () => { + const message = conditionIsSet ? 'missing prop8 and prop9' : undefined; + expectMessageIfThrows(() => { + entity.update({ + prop1: uuid(), + prop2: uuid(), + }).set({prop6: uuid(), prop7: uuid()}).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on patch`, () => { + const message = conditionIsSet ? 'missing prop6 and prop7' : undefined; + expectMessageIfThrows(() => { + entity.patch({ + prop1: uuid(), + prop2: uuid(), + }).set({prop8: uuid(), prop9: uuid()}).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + + it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on upsert`, () => { + const message = conditionIsSet ? 'missing prop8 and prop9' : undefined; + expectMessageIfThrows(() => { + entity.upsert({ + prop1: uuid(), + prop2: uuid(), + prop6: uuid(), + prop7: uuid(), + }).params(); + }, message); + expect(collector.invocations.length).to.equal(0); + }); + }); + } + }); + it('should throw if condition is added to the main table index', () => { expect(() => new Entity({ model: { @@ -3096,8 +3568,8 @@ describe("conditional indexes", () => { type TestCase = [description: string, index: keyof (ReturnType['query'])] const tests: TestCase[] = [ - ["an index with identical pk and sk composite attributes as the main table", 'sparse1'], - ["an index with at least the pk and sk composite attributes as the main table", 'sparse2'], + ["an index with identical pk and sk composite attributes as the main table index", 'sparse1'], + ["an index with at least the pk and sk composite attributes as the main table index", 'sparse2'], ["an index with distinct composite attributes", "sparse3"], ]; for (const [description, index] of tests) { From 8bb7bac941bf8f183cbcf3dd1482eb660910ba8a Mon Sep 17 00:00:00 2001 From: tywalch Date: Sun, 28 Apr 2024 11:42:13 -0400 Subject: [PATCH 04/14] All current tests working --- package.json | 2 +- src/entity.js | 114 +++++++++------ test/offline.entity.spec.js | 1 + test/ts_connected.entity.spec.ts | 198 ++++++++++++++++++++------ www/src/pages/en/modeling/indexes.mdx | 4 +- www/src/pages/en/mutations/patch.mdx | 1 + www/src/pages/en/mutations/update.mdx | 1 + www/src/pages/en/reference/errors.mdx | 10 ++ 8 files changed, 248 insertions(+), 83 deletions(-) diff --git a/package.json b/package.json index d51c2b09..72d6b02b 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "electrodb", - "version": "2.13.1", + "version": "2.14.0", "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 446308e6..e1bbd078 100644 --- a/src/entity.js +++ b/src/entity.js @@ -2359,14 +2359,14 @@ class Entity { // change, and we also don't want to trigger the setters of any attributes watching these facets because that // should only happen when an attribute is changed. const attributesAndComposites = { - ...update.composites, + // ...update.composites, ...preparedUpdateValues, }; const { indexKey, updatedKeys, deletedKeys = [], - } = this._getUpdatedKeys(pk, sk, attributesAndComposites, removed); + } = this._getUpdatedKeys(pk, sk, attributesAndComposites, removed, update.composites); const accessPattern = this.model.translations.indexes.fromIndexToAccessPattern[TableIndex]; for (const path of Object.keys(preparedUpdateValues)) { @@ -2961,11 +2961,11 @@ class Entity { return complete; } - _makeKeysFromAttributes(indexes, attributes) { + _makeKeysFromAttributes(indexes, attributes, conditions) { let indexKeys = {}; for (let [index, keyTypes] of Object.entries(indexes)) { - const shouldMakeKeys = this.model.indexes[this.model.translations.indexes.fromIndexToAccessPattern[index]].condition(attributes); - if (!shouldMakeKeys) { + const shouldMakeKeys = !this._indexConditionIsDefined(index) || conditions[index]; + if (!shouldMakeKeys && index !== TableIndex) { continue; } @@ -3058,14 +3058,14 @@ class Entity { return { indexKey, updatedKeys, setAttributes, deletedKeys }; } - _getUpdatedKeys(pk, sk, set, removed) { + _getUpdatedKeys(pk, sk, set, removed, composite = {}) { let updateIndex = TableIndex; let keyTranslations = this.model.translations.keys; let keyAttributes = { ...sk, ...pk }; let completeFacets = this._expectIndexFacets( { ...set }, - { ...keyAttributes }, + { ...composite, ...keyAttributes }, { utilizeIncludedOnlyIndexes: true }, ); @@ -3085,7 +3085,8 @@ class Entity { let composedKeys = this._makeKeysFromAttributes( completeFacets.impactedIndexTypes, - { ...set, ...keyAttributes }, + { ...composite, ...set, ...keyAttributes }, + completeFacets.conditions, ); let updatedKeys = {}; @@ -3144,10 +3145,11 @@ class Entity { /* istanbul ignore next */ _getIndexImpact(attributes = {}, included = {}, { utilizeIncludedOnlyIndexes, skipConditionCheck } = {}) { + // beware: this entire algorithm stinks and needs to be completely refactored. It does redundant loops and fights + // itself the whole way through. I'm sorry. let includedFacets = Object.keys(included); let impactedIndexes = {}; let conditions = {}; - let skippedIndexes = new Set(); let impactedIndexTypes = {}; let impactedIndexTypeSources = {}; let completedIndexes = []; @@ -3172,7 +3174,9 @@ class Entity { // this function is used to determine key impact for update `set`, update `delete`, and `put`. This block is currently only used by update `set` if (utilizeIncludedOnlyIndexes) { for (const [index, { pk, sk }] of Object.entries(this.model.facets.byIndex)) { - // The main table index is handled somewhere else (messy I know), and we only want to do this processing if an index condition is defined for backwards compatibility. Backwards compatibility is not required for this change, but I have paranoid concerns of breaking changes around sparse indexes. + // The main table index is handled somewhere else (messy I know), and we only want to do this processing if an + // index condition is defined for backwards compatibility. Backwards compatibility is not required for this + // change, but I have paranoid concerns of breaking changes around sparse indexes. if (index === TableIndex || !this._indexConditionIsDefined(index)) { continue; } @@ -3215,12 +3219,13 @@ class Entity { } } - let indexesWithMissingCompsites = Object.entries(this.model.facets.byIndex) + let indexesWithMissingComposites = Object.entries(this.model.facets.byIndex) .map(([index, definition]) => { const { pk, sk } = definition; let impacted = impactedIndexes[index]; let impact = { index, + definition, missing: [] }; if (impacted) { @@ -3257,43 +3262,72 @@ class Entity { return impact; }); - const incomplete = []; - for (const { index, missing } of indexesWithMissingCompsites) { - if (!missing.length) { - continue; - } - - const indexConditionIsDefined = this._indexConditionIsDefined(index); + let incomplete = []; + for (const { index, missing, definition } of indexesWithMissingComposites) { + const indexConditionIsDefined = this._indexConditionIsDefined(index); - // - `skipConditionCheck` is being used by update `remove`. If Attributes are being removed then the condition check is - // meaningless and ElectroDB should uphold its obligation to keep keys and attributes in sync. - // - `index === TableIndex` is a special case where we don't need to check the condition because the main table is immutable - // - `!this._indexConditionIsDefined(index)` means the index doesn't have a condition defined, so we can skip the check - if (!skipConditionCheck || index === TableIndex || !indexConditionIsDefined) { - incomplete.push({ index, missing }); - } - if (impactedIndexTypeSources[index][KeyTypes.pk] === ImpactedIndexTypeSource.provided || impactedIndexTypeSources[index][KeyTypes.sk] === ImpactedIndexTypeSource.provided) { - const missingAreProvidedInAttributesOrIncluded = missing - .every((attr) => attributes[attr] !== undefined || included[attr] !== undefined); + // `skipConditionCheck` is being used by update `remove`. If Attributes are being removed then the condition check + // is meaningless and ElectroDB should uphold its obligation to keep keys and attributes in sync. + // `index === TableIndex` is a special case where we don't need to check the condition because the main table is immutable + // `!this._indexConditionIsDefined(index)` means the index doesn't have a condition defined, so we can skip the check + if (skipConditionCheck || index === TableIndex || !indexConditionIsDefined) { + incomplete.push({ index, missing }); + conditions[index] = true; + continue; + } - if (!missingAreProvidedInAttributesOrIncluded) { - throw new e.ElectroError(e.ErrorCodes.IncompleteIndexCompositesAttributesProvided, `Incomplete composite attributes provided for index ${index}. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: ${u.commaSeparatedString(missing)}`); - } + const memberAttributeIsImpacted = impactedIndexTypeSources[index] && (impactedIndexTypeSources[index][KeyTypes.pk] === ImpactedIndexTypeSource.provided || impactedIndexTypeSources[index][KeyTypes.sk] === ImpactedIndexTypeSource.provided); + const allMemberAttributesAreIncluded = definition.all.every(({name}) => included[name] !== undefined); + + // if any of the indexes were impacted because attributes were provided, then we need to check the condition. + // Indexes that were impacted only because of `included` don't need to be checked. An example of this is would be + // a secondary index that shares composite attributes with the main table index. These cases don't demonstrate an + // intent to recalculate the index. This assumes `included` is really only used by callers to denote values that + // are not changing. + // if (impactedIndexTypeSources[index] && (allMemberAttributesAreIncluded || impactedIndexTypeSources[index][KeyTypes.pk] === ImpactedIndexTypeSource.provided || impactedIndexTypeSources[index][KeyTypes.sk] === ImpactedIndexTypeSource.provided)) { + // there is truth somewhere in the middle between the above and below conditions + if (memberAttributeIsImpacted || allMemberAttributesAreIncluded) { + // We want to make sure that there is actually at least one member attribute is impacted + // const memberAttributeIsBeingMutated = definition.all.find(({name}) => attributes[name] !== undefined); + + // 5:38pm I just commented this out because we _should_ reevaluate the condition if all the attributes are in included + // if (memberAttributeIsImpacted || allMemberAttributesAreIncluded) { + // incomplete.push({ index, missing }); + // conditions[index] = true; + // continue; + // } + + // 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)) + .map(({name}) => name) + // const missingAreProvidedInAttributesOrIncluded = missing.every((attr) => attributes[attr] !== undefined || included[attr] !== undefined); + // const missingPk = impactedIndexTypeSources[index][KeyTypes.pk] === undefined; + // const hasSkToMiss = !definition.hasSortKeys || (definition.sk && definition.sk.length > 0); + // const missingSk = hasSkToMiss && impactedIndexTypeSources[index][KeyTypes.sk] === undefined; + + // if (!missingAreProvidedInAttributesOrIncluded || missingPk || missingSk) { + if (missingAttributes.length) { + // const missingAttributes = missing.length ? missing : definition.all.filter(({name}) => !attributes[name] || !included[name]); + throw new e.ElectroError(e.ErrorCodes.IncompleteIndexCompositesAttributesProvided, `Incomplete composite attributes provided for index ${index}. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: ${u.commaSeparatedString(missingAttributes)}`); + } - const accessPattern = this.model.translations.indexes.fromIndexToAccessPattern[index]; - let shouldMakeKeys = !!this.model.indexes[accessPattern].condition({...attributes, ...included}); + const accessPattern = this.model.translations.indexes.fromIndexToAccessPattern[index]; + let shouldMakeKeys = !!this.model.indexes[accessPattern].condition({...attributes, ...included}); - // this helps identify which conditions were checked (key is present) and what the result was (true/false) - conditions[index] = shouldMakeKeys; - if (!shouldMakeKeys) { - continue; - } - } else { - incomplete.push({ index, missing }); + // this helps identify which conditions were checked (key is present) and what the result was (true/false) + conditions[index] = shouldMakeKeys; + if (!shouldMakeKeys) { + continue; } + } else { + incomplete.push({ index, missing }); } + } + incomplete = incomplete.filter(({ missing }) => missing.length); let isIncomplete = !!incomplete.length; let complete = { facets, indexes: completedIndexes, impactedIndexTypes, conditions }; diff --git a/test/offline.entity.spec.js b/test/offline.entity.spec.js index 417a9ce3..10344864 100644 --- a/test/offline.entity.spec.js +++ b/test/offline.entity.spec.js @@ -1625,6 +1625,7 @@ describe("Entity", () => { }, }, conditions: { + "": true, "gsi1pk-gsi1sk-index": true, "gsi2pk-gsi2sk-index": true, "gsi3pk-gsi3sk-index": true, diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index 08248788..664dea2b 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -2,7 +2,6 @@ import { DocumentClient, PutItemInput } from "aws-sdk/clients/dynamodb"; import { Entity, EntityRecord, createWriteTransaction, ElectroEvent } from "../"; import { expect } from "chai"; import { v4 as uuid } from "uuid"; -import {listeners} from "cluster"; const u = require("../src/util"); type ConversionTest = { @@ -2835,7 +2834,7 @@ describe('index scope', () => { }); }); -describe("conditional indexes", () => { +describe("index condition", () => { type IndexName = 'sparse1' | 'sparse2' | 'sparse3'; type ConditionArguments = { index: IndexName; @@ -2908,7 +2907,7 @@ describe("conditional indexes", () => { sparse2: { index: 'gsi2pk-gsi2sk-index', condition: (attr) => { - return fn({index: 'sparse2', attr}); + return fn({ index: 'sparse2', attr }); }, pk: { field: 'gsi2pk', @@ -2958,7 +2957,7 @@ describe("conditional indexes", () => { const condition: TestEntityCondition = (options) => { invocations.push(options); return result; - }; + } const clear = () => { invocations.length = 0; @@ -3003,19 +3002,21 @@ describe("conditional indexes", () => { const formatShouldStatement = (should: boolean) => `should${should ? ' ' : ' not '}`; describe('when all composite attributes are not provided', () => { - const conditionCases = [ - ['condition is set and returns true', true], - ['condition is set and returns false', false], - ['condition is not set', undefined], - ]; + const conditionCases = [ + ['index condition is set and returns true', true], + ['index condition is set and returns false', false], + ['index condition is not set', undefined], + ] as const; for (const [variation, setCondition] of conditionCases) { describe(`when composite attributes are distinct and ${variation}`, () => { const collector = createConditionInvocationCollector(!!setCondition); const conditionIsSet = setCondition !== undefined; const condition = conditionIsSet ? collector.condition : undefined; + afterEach(() => { collector.clear(); }); + const entity = new Entity( { model: { @@ -3086,6 +3087,7 @@ describe("conditional indexes", () => { prop3: uuid(), }).params(); }); + if (conditionIsSet) { expect(collector.invocations.length).to.not.equal(0); } @@ -3094,10 +3096,12 @@ describe("conditional indexes", () => { it(`should not throw when impacting composite attributes on update`, () => { expectMessageIfThrows(() => { entity.update({ prop1: uuid(), prop2: uuid() }) - .set({prop3: uuid()}) + .set({ prop3: uuid() }) .params(); }); + if (conditionIsSet) { + // when condition is "fixed" this should be changed to "to.not.equal(0)" expect(collector.invocations.length).to.not.equal(0); } }); @@ -3108,7 +3112,9 @@ describe("conditional indexes", () => { .set({ prop3: uuid() }) .params(); }); + if (conditionIsSet) { + // when condition is "fixed" this should be changed to "to.not.equal(0)" expect(collector.invocations.length).to.not.equal(0); } }); @@ -3121,6 +3127,7 @@ describe("conditional indexes", () => { prop3: uuid(), }).params(); }); + if (conditionIsSet) { expect(collector.invocations.length).to.not.equal(0); } @@ -3131,6 +3138,7 @@ describe("conditional indexes", () => { const collector = createConditionInvocationCollector(!!setCondition); const conditionIsSet = setCondition !== undefined; const condition = conditionIsSet ? collector.condition : undefined; + const entity = new Entity( { model: { @@ -3193,7 +3201,10 @@ describe("conditional indexes", () => { }); it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on put`, () => { - const message = conditionIsSet ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop3" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' : undefined; + const message = conditionIsSet + ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop3" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' + : `Incomplete composite attributes: Without the composite attributes "prop3" the following access patterns cannot be updated: "sparse2". If a composite attribute is readOnly and cannot be set, use the 'composite' chain method on update to supply the value for key formatting purposes. - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#incomplete-composite-attributes`; + expectMessageIfThrows(() => { entity.put({ prop1: uuid(), @@ -3202,11 +3213,15 @@ describe("conditional indexes", () => { prop5: uuid(), }).params(); }, message); + expect(collector.invocations.length).to.equal(0); }); it(`${formatShouldStatement(conditionIsSet)}throw when missing composite attributes on put`, () => { - const message = conditionIsSet ? 'Oops!' : undefined; + const message = conditionIsSet + ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop3", "prop4", "prop5" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' + : 'Incomplete composite attributes: Without the composite attributes "prop3", "prop4", "prop5" the following access patterns cannot be updated: "sparse2". If a composite attribute is readOnly and cannot be set, use the \'composite\' chain method on update to supply the value for key formatting purposes. - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#incomplete-composite-attributes'; + expectMessageIfThrows(() => { entity.put({ prop1: uuid(), @@ -3214,11 +3229,15 @@ describe("conditional indexes", () => { prop6: uuid(), }).params(); }, message); + expect(collector.invocations.length).to.equal(0); }); it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on create`, () => { - const message = conditionIsSet ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop4", "prop5" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' : undefined; + const message = conditionIsSet + ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop4", "prop5" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' + : 'Incomplete composite attributes: Without the composite attributes "prop4", "prop5" the following access patterns cannot be updated: "sparse2". If a composite attribute is readOnly and cannot be set, use the \'composite\' chain method on update to supply the value for key formatting purposes. - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#incomplete-composite-attributes'; + expectMessageIfThrows(() => { entity.create({ prop1: uuid(), @@ -3226,11 +3245,15 @@ describe("conditional indexes", () => { prop3: uuid(), }).params(); }, message); + expect(collector.invocations.length).to.equal(0); }); it(`${formatShouldStatement(conditionIsSet)}throw when missing composite attributes on create`, () => { - const message = conditionIsSet ? 'Oops!' : undefined; + const message = conditionIsSet + ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop3", "prop4", "prop5" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' + : `Incomplete composite attributes: Without the composite attributes "prop3", "prop4", "prop5" the following access patterns cannot be updated: "sparse2". If a composite attribute is readOnly and cannot be set, use the 'composite' chain method on update to supply the value for key formatting purposes. - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#incomplete-composite-attributes`; + expectMessageIfThrows(() => { entity.create({ prop1: uuid(), @@ -3238,55 +3261,61 @@ describe("conditional indexes", () => { prop6: uuid(), }).params(); }, message); + expect(collector.invocations.length).to.equal(0); }); it(`${formatShouldStatement(conditionIsSet)}throw when missing composite attributes on update`, () => { - const message = conditionIsSet ? 'Oops!' : undefined; + // Should remain after condition "fix", why throw when the user isn't trying to mutate prop3, prop4, or prop5. + // Throwing would hurt dx because prop1 and prop2 are main table composites (ie immutable) and their presence + // in the GSI would cause an undue burden on EVERY update operation. + const message = undefined; // conditionIsSet ? 'Oops!' : undefined; + expectMessageIfThrows(() => { entity.update({ prop1: uuid(), prop2: uuid(), }).set({ prop6: uuid() }).params(); }, message); + expect(collector.invocations.length).to.equal(0); }); it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on update`, () => { - const message = conditionIsSet ? 'missing prop3' : undefined; + // this should throw when a condition cb is set because this would cause a recalculation but is missing prop3 for the index + const message = conditionIsSet + ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop3" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' + : undefined; + expectMessageIfThrows(() => { entity.update({ prop1: uuid(), prop2: uuid(), }).set({ prop4: uuid(), prop5: uuid() }).params(); }, message); + expect(collector.invocations.length).to.equal(0); }); it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on patch`, () => { - const message = conditionIsSet ? 'missing prop4 and prop5' : undefined; - expectMessageIfThrows(() => { - entity.patch({ - prop1: uuid(), - prop2: uuid(), - }).set({prop3: uuid()}).params(); - }, message); - expect(collector.invocations.length).to.equal(0); - }); + // this should throw when a condition cb is set because this would cause a recalculation but is missing prop4 and prop5 for the index + const message = setCondition === undefined ? undefined : 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop4", "prop5" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided'; - it(`${formatShouldStatement(conditionIsSet)}throw when missing composite attributes on patch`, () => { - const message = conditionIsSet ? 'Oops!' : undefined; expectMessageIfThrows(() => { entity.patch({ prop1: uuid(), prop2: uuid(), - }).set({prop6: uuid()}).params(); + }).set({ prop3: uuid() }).params(); }, message); + expect(collector.invocations.length).to.equal(0); }); it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on upsert`, () => { - const message = conditionIsSet ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop3" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' : undefined; + const message = conditionIsSet + ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop3" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' + : `Incomplete composite attributes: Without the composite attributes "prop3" the following access patterns cannot be updated: "sparse2". If a composite attribute is readOnly and cannot be set, use the 'composite' chain method on update to supply the value for key formatting purposes. - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#incomplete-composite-attributes`; + expectMessageIfThrows(() => { entity.upsert({ prop1: uuid(), @@ -3295,11 +3324,15 @@ describe("conditional indexes", () => { prop4: uuid(), }).params(); }, message); + expect(collector.invocations.length).to.equal(0); }); it(`${formatShouldStatement(conditionIsSet)}throw when missing composite attributes on upsert`, () => { - const message = conditionIsSet ? 'Oops!' : undefined; + const message = conditionIsSet + ? 'Incomplete composite attributes provided for index gsi2pk-gsi2sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop3", "prop4", "prop5" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' + : 'Incomplete composite attributes: Without the composite attributes "prop3", "prop4", "prop5" the following access patterns cannot be updated: "sparse2". If a composite attribute is readOnly and cannot be set, use the \'composite\' chain method on update to supply the value for key formatting purposes. - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#incomplete-composite-attributes'; + expectMessageIfThrows(() => { entity.upsert({ prop1: uuid(), @@ -3307,6 +3340,7 @@ describe("conditional indexes", () => { prop6: uuid(), }).params(); }, message); + expect(collector.invocations.length).to.equal(0); }); }); @@ -3385,8 +3419,29 @@ describe("conditional indexes", () => { collector.clear(); }); + it('should not throw when providing unused composite attributes on update', () => { + expectMessageIfThrows(() => { + entity.update({ + prop1: uuid(), + prop2: uuid(), + }).composite({ prop6: uuid(), prop7: uuid() }).params(); + expect(collector.invocations.length).to.equal(0); + }); + }); + + it('should not throw when providing unused composite attributes on update', () => { + expectMessageIfThrows(() => { + entity.patch({ + prop1: uuid(), + prop2: uuid(), + }).composite({prop8: uuid(), prop9: uuid()}).params(); + expect(collector.invocations.length).to.equal(0); + }); + }); + it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on put`, () => { - const message = conditionIsSet ? 'missing prop8 and prop9' : undefined; + const message = conditionIsSet ? 'Incomplete composite attributes provided for index gsi3pk-gsi3sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop8", "prop9" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' : undefined; + expectMessageIfThrows(() => { entity.put({ prop1: uuid(), @@ -3395,11 +3450,13 @@ describe("conditional indexes", () => { prop6: uuid(), }).params(); }, message); + expect(collector.invocations.length).to.equal(0); }); it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on create`, () => { - const message = conditionIsSet ? 'missing prop6 and prop7' : undefined; + const message = conditionIsSet ? 'Incomplete composite attributes provided for index gsi3pk-gsi3sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop6", "prop7" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' : undefined; + expectMessageIfThrows(() => { entity.create({ prop1: uuid(), @@ -3408,33 +3465,77 @@ describe("conditional indexes", () => { prop9: uuid(), }).params(); }, message); + expect(collector.invocations.length).to.equal(0); }); it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on update`, () => { - const message = conditionIsSet ? 'missing prop8 and prop9' : undefined; + const message = conditionIsSet ? 'Incomplete composite attributes provided for index gsi3pk-gsi3sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop8", "prop9" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' : undefined; + expectMessageIfThrows(() => { entity.update({ prop1: uuid(), prop2: uuid(), }).set({prop6: uuid(), prop7: uuid()}).params(); }, message); + expect(collector.invocations.length).to.equal(0); + + expectMessageIfThrows(() => { + entity.update({ prop1: uuid(), prop2: uuid() }) + .set({ prop6: uuid(), prop7: uuid() }) + .composite({ prop8: uuid(), prop9: uuid() }) + .params(); + }); + + if (conditionIsSet) { + expect(collector.invocations.length).to.equal(1); + for (let i = 0; i < collector.invocations.length; i++) { + const prev = collector.invocations[i - 1]; + const invocation = collector.invocations[i]; + expect(invocation).to.have.keys('prop1', 'prop2', 'prop6', 'prop7', 'prop8', 'prop9'); + if (prev) { + expect(prev).to.deep.equal(invocation) + } + } + } }); it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on patch`, () => { - const message = conditionIsSet ? 'missing prop6 and prop7' : undefined; + const message = conditionIsSet ? 'Incomplete composite attributes provided for index gsi3pk-gsi3sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop6", "prop7" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' : undefined; + expectMessageIfThrows(() => { entity.patch({ prop1: uuid(), prop2: uuid(), }).set({prop8: uuid(), prop9: uuid()}).params(); }, message); + expect(collector.invocations.length).to.equal(0); + + expectMessageIfThrows(() => { + entity.patch({ prop1: uuid(), prop2: uuid() }) + .set({ prop8: uuid(), prop9: uuid() }) + .composite({ prop6: uuid(), prop7: uuid() }) + .params(); + }); + + if (conditionIsSet) { + expect(collector.invocations.length).to.equal(1); + for (let i = 0; i < collector.invocations.length; i++) { + const prev = collector.invocations[i - 1]; + const invocation = collector.invocations[i]; + expect(invocation).to.have.keys('prop1', 'prop2', 'prop6', 'prop7', 'prop8', 'prop9'); + if (prev) { + expect(prev).to.deep.equal(invocation) + } + } + } }); it(`${formatShouldStatement(conditionIsSet)}throw when partially providing composite attributes on upsert`, () => { - const message = conditionIsSet ? 'missing prop8 and prop9' : undefined; + const message = conditionIsSet ? 'Incomplete composite attributes provided for index gsi3pk-gsi3sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop8", "prop9" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided' : undefined; + expectMessageIfThrows(() => { entity.upsert({ prop1: uuid(), @@ -3443,6 +3544,7 @@ describe("conditional indexes", () => { prop7: uuid(), }).params(); }, message); + expect(collector.invocations.length).to.equal(0); }); }); @@ -3538,10 +3640,10 @@ describe("conditional indexes", () => { const prop5 = uuid(); conditionValue = false; - expect(() => entity.update({prop1, prop2}).set({prop3, prop5}).params()).not.to.throw(); + expect(() => entity.update({prop1, prop2}).set({prop3, prop5}).params()).to.throw('Incomplete composite attributes provided for index gsi1pk-gsi1sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop4" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided'); conditionValue = true; - expect(() => entity.update({prop1, prop2}).set({prop3, prop5}).params()).to.throw('Incomplete composite attributes: Without the composite attributes "prop4" the following access patterns cannot be updated: "secondary". If a composite attribute is readOnly and cannot be set, use the \'composite\' chain method on update to supply the value for key formatting purposes. - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#incomplete-composite-attributes'); + expect(() => entity.update({prop1, prop2}).set({prop3, prop5}).params()).to.throw('Incomplete composite attributes provided for index gsi1pk-gsi1sk-index. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "prop4" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#invalid-index-composite-attributes-provided'); }); it('should check the index condition individually on the subject entity', () => { @@ -3736,26 +3838,40 @@ describe("conditional indexes", () => { return allow; } + + const initialValues = { + prop3: 'val3', + prop4: 'val4', + prop5: 'val5', + }; + const entity = createTestEntity(condition); - // don't write for sure on put, but then yield to test; patch requires an existing item - await entity.put({prop1, prop2}).go(); + + await entity.put({ prop1, prop2, ...initialValues }).go(); // "reset" `allow` and "reset" `invocations` allow = shouldWrite; invocations = []; - await entity.patch({prop1, prop2}).set(props).go({logger}); + // record should exist with temp values + const results = await entity.query[index]({ prop1, prop2, ...initialValues }).go(); + expect(results.data.length).to.equal(1); + expect(results.data[0]).to.deep.equal({ prop1, prop2, ...initialValues }); + + await entity.patch({ prop1, prop2 }).set(props).go({ logger }); // @ts-ignore const {data} = await entity.query[index]({prop1, prop2, ...props}).go(); if (shouldWrite) { + // patch should have added item to index expect(data.length).to.equal(1); expect(data[0]).to.deep.equal({prop1, prop2, ...props}); } else { + // patch should have removed item from index expect(data.length).to.equal(0); } for (const invocation of invocations) { - expect(invocation.attr).to.deep.equal({prop1, prop2, ...props}); + expect(invocation.attr).to.deep.equal({ prop1, prop2, ...props }); } }); diff --git a/www/src/pages/en/modeling/indexes.mdx b/www/src/pages/en/modeling/indexes.mdx index 60e4f7d2..df3f3957 100644 --- a/www/src/pages/en/modeling/indexes.mdx +++ b/www/src/pages/en/modeling/indexes.mdx @@ -430,7 +430,9 @@ In the example below, we are configuring the casing ElectroDB will use individua Sparse indexes are indexes that only contain a subset of the items on your main table. Sparse indexes are useful when you want to reduce the number of records your query must iterate over to find the records you are looking on a secondary index. By having fewer records on a secondary index, you can improve the performance of your queries and reduce the cost of your table. -ElectroDB manages which secondary indexes are written to your DynamoDB table based on the attributes you provide to your query; this includes adding runtime constraints to ensure consistency between your index key values and its constituent composite attributes. If you wish to prevent an index from being written, you can define a `condition` callback on your index. The provided callback will be invoked at query-time, passed all attributes set on that mutation, and if it returns `false` the index will not be written to your DynamoDB table. +ElectroDB manages which secondary indexes are written to your DynamoDB table based on the attributes you provide to your query; this includes adding runtime constraints to ensure consistency between your index key values and its constituent composite attributes. If you wish to prevent an index from being written, you can define a `condition` callback on your index. The provided callback will be invoked at query-time, passed all attributes set on that mutation, and if it returns `false` the keys for that index be deleted -- removing the item's presence from that index. + +> _Note: Beginning with ElectroDB version 2.14.0, the presence of a `condition` callback will add a _new_ runtime validation check on all mutations. When a conditional index is defined, all member attributes of the index must be provided if a mutation operation affects one of the attributes. This is to ensure that the index is kept in sync with the item's attributes. If you are unable to update/patch all member attributes, because some are readOnly, you can also use the [composite](/en/mutations/patch#composite) method on [update](/en/mutations/update#composite) and [patch](/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](/en/reference/errors#invalid-index-composite-attributes-provided)._ #### Example diff --git a/www/src/pages/en/mutations/patch.mdx b/www/src/pages/en/mutations/patch.mdx index b7903d9b..ebe9e0e7 100644 --- a/www/src/pages/en/mutations/patch.mdx +++ b/www/src/pages/en/mutations/patch.mdx @@ -118,6 +118,7 @@ Update Methods are available **_after_** the `update()` or `patch()` method is c | [append](#update-method-append) | `any` `list` | `object` | | [delete](#update-method-delete) | `any` `set` | `object` | | [data](#update-method-data) | `*` | `callback` | +| [composite](#composite) | `number` `string` `boolean` `enum` `map` `list` `set` `any` | `object` | `object` | The methods above can be used (and reused) in a chain to form update parameters, when finished with `.params()` or `.go()` terminal. If your application requires the update method to return values related to the update (e.g. via the `ReturnValues` DocumentClient parameters), you can use the [Execution Option](#execution-options) `{response: "none" | "all_old" | "updated_old" | "all_new" | "updated_new"}` with the value that matches your need. By default, the Update operation returns an empty object when using `.go()`. diff --git a/www/src/pages/en/mutations/update.mdx b/www/src/pages/en/mutations/update.mdx index 038af9d7..b638ff62 100644 --- a/www/src/pages/en/mutations/update.mdx +++ b/www/src/pages/en/mutations/update.mdx @@ -109,6 +109,7 @@ Update Methods are available **_after_** the `update()` or `patch()` method is c | [append](#update-method-append) | `any` `list` | `object` | | [delete](#update-method-delete) | `any` `set` | `object` | | [data](#update-method-data) | `*` | `callback` | +| [composite](#composite) | `number` `string` `boolean` `enum` `map` `list` `set` `any` | `object` | `object` | The methods above can be used (and reused) in a chain to form update parameters, when finished with `.params()` or `.go()` terminal. If your application requires the update method to return values related to the update (e.g. via the `ReturnValues` DocumentClient parameters), you can use the [Execution Option](#execution-options) `{response: "none" | "all_old" | "updated_old" | "all_new" | "updated_new"}` with the value that matches your need. By default, the Update operation returns an empty object when using `.go()`. diff --git a/www/src/pages/en/reference/errors.mdx b/www/src/pages/en/reference/errors.mdx index be6492b4..2e8d7bf6 100644 --- a/www/src/pages/en/reference/errors.mdx +++ b/www/src/pages/en/reference/errors.mdx @@ -371,6 +371,16 @@ When performing a [Query](#building-queries) you can pass an [Execution Option]( **What to do about it:** Expect this error only if you're providing a `limit` option. Double-check the value you are providing is the value you expect to be passing, and that the value passes the tests listed above. +### Invalid Index Composite Attributes Provided + +**Code: 2012** + +**Why this occurred:** +This error occurs on the mutation operations `put`, `create`, `upsert`, `update`, and `patch` when using the [condition callback](/en/modeling/indexes##sparse-indexes) in your Entity's schema. When a conditional index is defined, all member attributes of the index must be provided if a mutation operation affects one of the attributes. This is to ensure that the index is kept in sync with the item's attributes. More information and the discussion around the reasoning behind this change can be found [here](https://github.com/tywalch/electrodb/issues/366). + +**What to do about it:** +If you need to modify a single composite attribute on a multi-composite attribute GSI, you will need to provide new values for all member composite attributes. In the case of `update`/`patch`, if you are unable to update all member attributes (e.g. because some are readOnly) you can also use the [composite](/en/mutations/patch#composite) method on [update](/en/mutations/update#composite) and [patch](/en/mutations/patch#composite). + ## User Defined Validation ### Invalid Attribute From d66962591e23247c00a6242f0897deca45fdb586 Mon Sep 17 00:00:00 2001 From: tywalch Date: Sun, 28 Apr 2024 11:50:53 -0400 Subject: [PATCH 05/14] Clean up and test fix --- src/entity.js | 22 ---------------------- test/ts_connected.entity.spec.ts | 8 ++------ 2 files changed, 2 insertions(+), 28 deletions(-) diff --git a/src/entity.js b/src/entity.js index e1bbd078..b409439c 100644 --- a/src/entity.js +++ b/src/entity.js @@ -3280,35 +3280,13 @@ class Entity { const memberAttributeIsImpacted = impactedIndexTypeSources[index] && (impactedIndexTypeSources[index][KeyTypes.pk] === ImpactedIndexTypeSource.provided || impactedIndexTypeSources[index][KeyTypes.sk] === ImpactedIndexTypeSource.provided); const allMemberAttributesAreIncluded = definition.all.every(({name}) => included[name] !== undefined); - // if any of the indexes were impacted because attributes were provided, then we need to check the condition. - // Indexes that were impacted only because of `included` don't need to be checked. An example of this is would be - // a secondary index that shares composite attributes with the main table index. These cases don't demonstrate an - // intent to recalculate the index. This assumes `included` is really only used by callers to denote values that - // are not changing. - // if (impactedIndexTypeSources[index] && (allMemberAttributesAreIncluded || impactedIndexTypeSources[index][KeyTypes.pk] === ImpactedIndexTypeSource.provided || impactedIndexTypeSources[index][KeyTypes.sk] === ImpactedIndexTypeSource.provided)) { - // there is truth somewhere in the middle between the above and below conditions if (memberAttributeIsImpacted || allMemberAttributesAreIncluded) { - // We want to make sure that there is actually at least one member attribute is impacted - // const memberAttributeIsBeingMutated = definition.all.find(({name}) => attributes[name] !== undefined); - - // 5:38pm I just commented this out because we _should_ reevaluate the condition if all the attributes are in included - // if (memberAttributeIsImpacted || allMemberAttributesAreIncluded) { - // incomplete.push({ index, missing }); - // conditions[index] = true; - // continue; - // } - // 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)) .map(({name}) => name) - // const missingAreProvidedInAttributesOrIncluded = missing.every((attr) => attributes[attr] !== undefined || included[attr] !== undefined); - // const missingPk = impactedIndexTypeSources[index][KeyTypes.pk] === undefined; - // const hasSkToMiss = !definition.hasSortKeys || (definition.sk && definition.sk.length > 0); - // const missingSk = hasSkToMiss && impactedIndexTypeSources[index][KeyTypes.sk] === undefined; - // if (!missingAreProvidedInAttributesOrIncluded || missingPk || missingSk) { if (missingAttributes.length) { // const missingAttributes = missing.length ? missing : definition.all.filter(({name}) => !attributes[name] || !included[name]); throw new e.ElectroError(e.ErrorCodes.IncompleteIndexCompositesAttributesProvided, `Incomplete composite attributes provided for index ${index}. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: ${u.commaSeparatedString(missingAttributes)}`); diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index 664dea2b..02a27ae0 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -3831,6 +3831,7 @@ describe("index condition", () => { it(`${prefix} write index with provided patch attributes`, async () => { const {params, logger} = createParamsCollector(); const {prop1, prop2, ...props} = createTestEntityData(); + const { prop1: _, prop2: __, initialValues } = createTestEntityData(); let invocations: ConditionArguments[] = []; let allow = false; const condition = (args: ConditionArguments) => { @@ -3838,12 +3839,7 @@ describe("index condition", () => { return allow; } - - const initialValues = { - prop3: 'val3', - prop4: 'val4', - prop5: 'val5', - }; + expect(props).to.not.deep.equal(initialValues); const entity = createTestEntity(condition); From 0268faf9020f4c6d89626e5fb38f8272639333a1 Mon Sep 17 00:00:00 2001 From: tywalch Date: Mon, 29 Apr 2024 08:51:57 -0400 Subject: [PATCH 06/14] Clean up and test fix --- test/ts_connected.entity.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index 02a27ae0..40386b77 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -3831,7 +3831,7 @@ describe("index condition", () => { it(`${prefix} write index with provided patch attributes`, async () => { const {params, logger} = createParamsCollector(); const {prop1, prop2, ...props} = createTestEntityData(); - const { prop1: _, prop2: __, initialValues } = createTestEntityData(); + const { prop1: _, prop2: __, ...initialValues } = createTestEntityData(); let invocations: ConditionArguments[] = []; let allow = false; const condition = (args: ConditionArguments) => { From 18554633fb9144dde07a4c269a65c1a1b459f78b Mon Sep 17 00:00:00 2001 From: tywalch Date: Mon, 29 Apr 2024 08:56:19 -0400 Subject: [PATCH 07/14] Clean up and test fix --- test/ts_connected.entity.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index 40386b77..220b2ba6 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -3850,6 +3850,7 @@ describe("index condition", () => { invocations = []; // record should exist with temp values + // @ts-ignore const results = await entity.query[index]({ prop1, prop2, ...initialValues }).go(); expect(results.data.length).to.equal(1); expect(results.data[0]).to.deep.equal({ prop1, prop2, ...initialValues }); From a764a6d96341632765a64e34148074d7e22239c6 Mon Sep 17 00:00:00 2001 From: tywalch Date: Mon, 29 Apr 2024 09:29:11 -0400 Subject: [PATCH 08/14] Clean up and test fix --- test/offline.entity.spec.js | 5 +++++ test/ts_connected.entity.spec.ts | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/test/offline.entity.spec.js b/test/offline.entity.spec.js index 10344864..080da7a1 100644 --- a/test/offline.entity.spec.js +++ b/test/offline.entity.spec.js @@ -1654,7 +1654,11 @@ describe("Entity", () => { }, }, conditions: { + "": true, + "gsi1pk-gsi1sk-index": true, "gsi2pk-gsi2sk-index": true, + "gsi3pk-gsi3sk-index": true, + "gsi4pk-gsi4sk-index": true, } }, }, @@ -1693,6 +1697,7 @@ describe("Entity", () => { }, }, conditions: { + "": true, "gsi1pk-gsi1sk-index": true, "gsi2pk-gsi2sk-index": true, "gsi3pk-gsi3sk-index": true, diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index 220b2ba6..1a5a397b 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -3833,7 +3833,7 @@ describe("index condition", () => { const {prop1, prop2, ...props} = createTestEntityData(); const { prop1: _, prop2: __, ...initialValues } = createTestEntityData(); let invocations: ConditionArguments[] = []; - let allow = false; + let allow = true; const condition = (args: ConditionArguments) => { invocations.push(args); return allow; From 658e6a10790c0bc8a9efaed8d599e432d15df6f8 Mon Sep 17 00:00:00 2001 From: tywalch Date: Mon, 29 Apr 2024 09:48:53 -0400 Subject: [PATCH 09/14] Clean up and test fix --- test/offline.entity.spec.js | 4 ++++ test/ts_connected.entity.spec.ts | 20 +++++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/test/offline.entity.spec.js b/test/offline.entity.spec.js index 080da7a1..472aaa39 100644 --- a/test/offline.entity.spec.js +++ b/test/offline.entity.spec.js @@ -1735,6 +1735,7 @@ describe("Entity", () => { }, }, conditions: { + "": true, "gsi1pk-gsi1sk-index": true, "gsi2pk-gsi2sk-index": true, "gsi3pk-gsi3sk-index": true, @@ -1763,8 +1764,11 @@ describe("Entity", () => { }, }, conditions: { + "": true, + "gsi1pk-gsi1sk-index": true, "gsi2pk-gsi2sk-index": true, "gsi3pk-gsi3sk-index": true, + "gsi4pk-gsi4sk-index": true, } }, }, diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index 1a5a397b..8cd82d24 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -3845,16 +3845,26 @@ describe("index condition", () => { await entity.put({ prop1, prop2, ...initialValues }).go(); - // "reset" `allow` and "reset" `invocations` - allow = shouldWrite; - invocations = []; - - // record should exist with temp values + // record should exist with temp values because `allow=true` // @ts-ignore const results = await entity.query[index]({ prop1, prop2, ...initialValues }).go(); expect(results.data.length).to.equal(1); expect(results.data[0]).to.deep.equal({ prop1, prop2, ...initialValues }); + + allow = false; + // record should no longer exist on GSIs because `allow=false` + await entity.put({ prop1, prop2, ...initialValues }).go(); + // @ts-ignore + const results2 = await entity.query[index]({ prop1, prop2, ...initialValues }).go(); + // the main table index will always contain the item, but not the other GSIs + const expectedLength = index === 'test' ? 1 : 0; + expect(results2.data.length).to.equal(expectedLength); + + // "reset" `allow` and "reset" `invocations` + allow = shouldWrite; + invocations = []; + await entity.patch({ prop1, prop2 }).set(props).go({ logger }); // @ts-ignore const {data} = await entity.query[index]({prop1, prop2, ...props}).go(); From 1d5bf8edecaeb41e29a2881c44c9801d625f47a4 Mon Sep 17 00:00:00 2001 From: tywalch Date: Mon, 29 Apr 2024 12:50:02 -0400 Subject: [PATCH 10/14] Clean up and adds new test cases --- src/entity.js | 11 ++-- test/ts_connected.entity.spec.ts | 98 ++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 7 deletions(-) diff --git a/src/entity.js b/src/entity.js index b409439c..f77f786e 100644 --- a/src/entity.js +++ b/src/entity.js @@ -2359,7 +2359,6 @@ class Entity { // change, and we also don't want to trigger the setters of any attributes watching these facets because that // should only happen when an attribute is changed. const attributesAndComposites = { - // ...update.composites, ...preparedUpdateValues, }; const { @@ -3016,7 +3015,7 @@ class Entity { let completeFacets = this._expectIndexFacets( { ...setAttributes, ...validationAssistance }, { ...keyAttributes }, - { set }, + { set }, ); let deletedKeys = []; @@ -3066,13 +3065,13 @@ class Entity { let completeFacets = this._expectIndexFacets( { ...set }, { ...composite, ...keyAttributes }, - { utilizeIncludedOnlyIndexes: true }, + { utilizeIncludedOnlyIndexes: true }, ); const removedKeyImpact = this._expectIndexFacets( { ...removed }, { ...keyAttributes }, - { skipConditionCheck: true } + { skipConditionCheck: true } ); // complete facets, only includes impacted facets which likely does not include the updateIndex which then needs to be added here. @@ -3146,7 +3145,7 @@ class Entity { /* istanbul ignore next */ _getIndexImpact(attributes = {}, included = {}, { utilizeIncludedOnlyIndexes, skipConditionCheck } = {}) { // beware: this entire algorithm stinks and needs to be completely refactored. It does redundant loops and fights - // itself the whole way through. I'm sorry. + // itself the whole way through. I am sorry. let includedFacets = Object.keys(included); let impactedIndexes = {}; let conditions = {}; @@ -3266,7 +3265,6 @@ class Entity { for (const { index, missing, definition } of indexesWithMissingComposites) { const indexConditionIsDefined = this._indexConditionIsDefined(index); - // `skipConditionCheck` is being used by update `remove`. If Attributes are being removed then the condition check // is meaningless and ElectroDB should uphold its obligation to keep keys and attributes in sync. // `index === TableIndex` is a special case where we don't need to check the condition because the main table is immutable @@ -3288,7 +3286,6 @@ class Entity { .map(({name}) => name) if (missingAttributes.length) { - // const missingAttributes = missing.length ? missing : definition.all.filter(({name}) => !attributes[name] || !included[name]); throw new e.ElectroError(e.ErrorCodes.IncompleteIndexCompositesAttributesProvided, `Incomplete composite attributes provided for index ${index}. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: ${u.commaSeparatedString(missingAttributes)}`); } diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index 8cd82d24..ad8706df 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -3925,6 +3925,104 @@ describe("index condition", () => { }); } }); + + const conditionOptions = [ + [() => true, 'returns true'] as const, + [() => false, 'returns false'] as const, + [undefined, 'is not defined'] as const, + ] as const; + // should not use attributes provided via composite to identify a conditional index requires recalculation + // `when performing an update that impacts an index with a condition that ${descripto}` + describe(`when performing an update set that impacts an index and the provided composite attributes overlap with another index`, () => { + for (const [condition1, description1] of conditionOptions) { + for (const [condition2, description2] of conditionOptions) { + it(`should not throw because it believes additional attributes need to be provided on the overlapped index, when the impacted index has a condition that ${description1} and the overlapped index has a condtion that ${description2}`, () => { + const entityName = uuid(); + const serviceName = uuid(); + + const entity = new Entity({ + model: { + version: '0', + entity: entityName, + service: serviceName, + }, + attributes: { + prop1: { + type: 'string' + }, + prop2: { + type: 'string' + }, + prop3: { + type: 'string' + }, + prop4: { + type: 'string' + }, + prop5: { + type: 'string' + }, + prop6: { + type: 'string' + }, + prop7: { + type: 'string' + }, + prop8: { + type: 'string' + }, + }, + indexes: { + test1: { + pk: { + field: 'pk', + composite: ['prop1'] + }, + sk: { + field: 'sk', + composite: ['prop2'] + } + }, + test2: { + condition: condition1, + index: 'gsi1pk-gsi1sk-index', + pk: { + field: 'gsi1pk', + composite: ['prop3', 'prop5'] + }, + sk: { + field: 'gsi1sk', + composite: ['prop4', 'prop6'] + } + }, + test3: { + condition: condition2, + index: 'gsi2pk-gsi2sk-index', + pk: { + field: 'gsi2pk', + composite: ['prop3', 'prop7'] + }, + sk: { + field: 'gsi2sk', + composite: ['prop4', 'prop8'] + } + } + } + }, {client, table}); + + expect(() => { + entity.update({ prop1: uuid(), prop2: uuid() }) + .set({ prop5: uuid(), prop6: uuid() }) + // prop3 and prop4 overlap on both test1 and test2 indexes, this should not cause the ElectroDB to throw + // because it thinks test2 should have prop7 and prop8 provided. test2 is not trying to be recalculated, + // and doesn't need to be because composite doesn't mutate, so throwing would be horrible DX. + .composite({ prop3: uuid(), prop4: uuid() }) + .params(); + }).to.not.throw(); + }); + } + } + }); } it('should fix gh issue 366', async () => { From 955dc277b53a41a4b4d89bcc33e5395c48af2ebf Mon Sep 17 00:00:00 2001 From: tywalch Date: Mon, 29 Apr 2024 13:12:21 -0400 Subject: [PATCH 11/14] adds new test case --- test/ts_connected.entity.spec.ts | 628 +++++++++++++++++++------------ 1 file changed, 394 insertions(+), 234 deletions(-) diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index ad8706df..5b13171b 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -4025,259 +4025,419 @@ describe("index condition", () => { }); } - it('should fix gh issue 366', async () => { - const entityName = uuid(); - const updatedAt = new Date().toJSON(); - const createdAt = new Date().toJSON(); - const Thing = new Entity( - { - model: { - service: 'test', - entity: entityName, - version: '1' - }, - attributes: { - id: { - type: 'string', - required: true, - readOnly: true - }, - organizationId: { - type: 'string', - required: true, - readOnly: true - }, - accountId: { - type: 'string' - }, - createdAt: { - type: 'string', - readOnly: true, - required: true, - default: () => createdAt, - set: () => createdAt - }, - updatedAt: { - type: 'string', - watch: '*', - required: true, - default: () => updatedAt, - set: () => updatedAt - }, - settledAt: { - type: 'string', - default: 'n/a' - }, - effectiveAt: { - type: 'string', - default: 'n/a' - }, - type: { - type: 'string', - required: true - }, - category: { - type: 'string', - required: true - }, - amount: { - type: 'string', - required: true + describe('github issue 366', () => { + it('should recreate exact model from issue', async () => { + + const table = "your_table_name"; + + const entry = new Entity( + { + model: { + service: 'test', + entity: 'test', + version: '1' }, - description: { - type: 'string' - } - }, - indexes: { - entries: { - pk: { - field: 'pk', - composite: ['organizationId'] + attributes: { + id: { + type: 'string', + required: true, + readOnly: true }, - sk: { - field: 'sk', - composite: ['id'] + organizationId: { + type: 'string', + required: true, + readOnly: true + }, + accountId: { + type: 'string' + }, + createdAt: { + type: 'string', + readOnly: true, + required: true, + default: () => new Date().toJSON(), + set: () => new Date().toJSON() + }, + updatedAt: { + type: 'string', + watch: '*', + required: true, + default: () => new Date().toJSON(), + set: () => new Date().toJSON() + }, + settledAt: { + type: 'string', + default: 'n/a' + }, + effectiveAt: { + type: 'string', + default: 'n/a' + }, + type: { + type: 'string', + required: true + }, + category: { + type: 'string', + required: true + }, + amount: { + type: 'string', + required: true + }, + description: { + type: 'string' } }, - entriesByAccount: { - index: 'gsi1pk-gsi1sk-index', - pk: { - field: 'gsi1pk', - composite: ['organizationId'] + indexes: { + entries: { + pk: { + field: 'pk', + composite: ['organizationId'] + }, + sk: { + field: 'sk', + composite: ['id'] + } }, - sk: { - field: 'gsi1sk', - composite: ['accountId', 'id'] + entriesByAccount: { + index: 'gsi1pk-gsi1sk-index', + pk: { + field: 'gsi1pk', + composite: ['organizationId'] + }, + sk: { + field: 'gsi1sk', + composite: ['accountId', 'id'] + } + }, + entriesBySettledDate: { + index: 'gsi2pk-gsi2sk-index', + condition: (attr) => attr.settledAt !== 'n/a', + pk: { + field: 'gsi2pk', + composite: ['organizationId'] + }, + sk: { + field: 'gsi2sk', + composite: ['accountId', 'settledAt'] + } + }, + entriesByEffectiveDate: { + index: 'gsi3pk-gsi3sk-index', + condition: (attr) => attr.effectiveAt !== 'n/a', + pk: { + field: 'gsi3pk', + composite: ['organizationId'] + }, + sk: { + field: 'gsi3sk', + composite: ['accountId', 'effectiveAt'] + } } + } + }, + { table } + ); + + + const params = entry + .patch({ id: '123', organizationId: '123' }) + .set({ effectiveAt: 'n/a', accountId: "123", settledAt: "today" }) + .params(); + + // params set `gsi1sk` and `gsi2pk` fields and remove `gsi3pk` and `gsi3sk` fields + expect(params).to.deep.equal({ + UpdateExpression: 'SET #effectiveAt = :effectiveAt_u0, #accountId = :accountId_u0, #settledAt = :settledAt_u0, #updatedAt = :updatedAt_u0, #gsi1sk = :gsi1sk_u0, #gsi2pk = :gsi2pk_u0, #gsi2sk = :gsi2sk_u0, #organizationId = :organizationId_u0, #id = :id_u0, #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0 REMOVE #gsi3pk, #gsi3sk', + ExpressionAttributeNames: { + '#pk': 'pk', + '#sk': 'sk', + '#effectiveAt': 'effectiveAt', + '#accountId': 'accountId', + '#settledAt': 'settledAt', + '#updatedAt': 'updatedAt', + '#gsi1sk': 'gsi1sk', + '#gsi2pk': 'gsi2pk', + '#gsi2sk': 'gsi2sk', + '#gsi3pk': 'gsi3pk', + '#gsi3sk': 'gsi3sk', + '#organizationId': 'organizationId', + '#id': 'id', + '#__edb_e__': '__edb_e__', + '#__edb_v__': '__edb_v__' + }, + ExpressionAttributeValues: { + ':effectiveAt_u0': 'n/a', + ':accountId_u0': '123', + ':settledAt_u0': 'today', + ':updatedAt_u0': '2024-04-29T17:09:30.687Z', + ':gsi1sk_u0': '$test_1#accountid_123#id_123', + ':gsi2pk_u0': '$test#organizationid_123', + ':gsi2sk_u0': '$test_1#accountid_123#settledat_today', + ':organizationId_u0': '123', + ':id_u0': '123', + ':__edb_e___u0': 'test', + ':__edb_v___u0': '1' + }, + TableName: 'your_table_name', + Key: { pk: '$test#organizationid_123', sk: '$test_1#id_123' }, + ConditionExpression: 'attribute_exists(#pk) AND attribute_exists(#sk)' + }); + }); + it('should fix gh issue 366', async () => { + const entityName = uuid(); + const updatedAt = new Date().toJSON(); + const createdAt = new Date().toJSON(); + const Thing = new Entity( + { + model: { + service: 'test', + entity: entityName, + version: '1' }, - entriesBySettledDate: { - index: 'gsi2pk-gsi2sk-index', - condition: (attr) => attr.settledAt !== 'n/a', - pk: { - field: 'gsi2pk', - composite: ['organizationId'] + attributes: { + id: { + type: 'string', + required: true, + readOnly: true }, - sk: { - field: 'gsi2sk', - composite: ['settledAt'] + organizationId: { + type: 'string', + required: true, + readOnly: true + }, + accountId: { + type: 'string' + }, + createdAt: { + type: 'string', + readOnly: true, + required: true, + default: () => createdAt, + set: () => createdAt + }, + updatedAt: { + type: 'string', + watch: '*', + required: true, + default: () => updatedAt, + set: () => updatedAt + }, + settledAt: { + type: 'string', + default: 'n/a' + }, + effectiveAt: { + type: 'string', + default: 'n/a' + }, + type: { + type: 'string', + required: true + }, + category: { + type: 'string', + required: true + }, + amount: { + type: 'string', + required: true + }, + description: { + type: 'string' } }, - entriesByEffectiveDate: { - index: 'gsi3pk-gsi3sk-index', - condition: (attr) => attr.effectiveAt !== 'n/a', - pk: { - field: 'gsi3pk', - composite: ['organizationId'] + indexes: { + entries: { + pk: { + field: 'pk', + composite: ['organizationId'] + }, + sk: { + field: 'sk', + composite: ['id'] + } }, - sk: { - field: 'gsi3sk', - composite: ['effectiveAt'] + entriesByAccount: { + index: 'gsi1pk-gsi1sk-index', + pk: { + field: 'gsi1pk', + composite: ['organizationId'] + }, + sk: { + field: 'gsi1sk', + composite: ['accountId', 'id'] + } + }, + entriesBySettledDate: { + index: 'gsi2pk-gsi2sk-index', + condition: (attr) => attr.settledAt !== 'n/a', + pk: { + field: 'gsi2pk', + composite: ['organizationId'] + }, + sk: { + field: 'gsi2sk', + composite: ['settledAt'] + } + }, + entriesByEffectiveDate: { + index: 'gsi3pk-gsi3sk-index', + condition: (attr) => attr.effectiveAt !== 'n/a', + pk: { + field: 'gsi3pk', + composite: ['organizationId'] + }, + sk: { + field: 'gsi3sk', + composite: ['effectiveAt'] + } } } - } + }, + {table, client} + ); + + // with `effectiveAt` set to 'n/a' and `settledAt` set to 'today' the `entriesByEffectiveDate` index should not be written + const params1 = Thing.patch({id: '123', organizationId: '123'}) + .set({effectiveAt: 'n/a', accountId: '123', settledAt: 'today'}) + .params(); + + expect(params1).to.deep.equal({ + "UpdateExpression": "SET #effectiveAt = :effectiveAt_u0, #accountId = :accountId_u0, #settledAt = :settledAt_u0, #updatedAt = :updatedAt_u0, #gsi1sk = :gsi1sk_u0, #gsi2pk = :gsi2pk_u0, #gsi2sk = :gsi2sk_u0, #organizationId = :organizationId_u0, #id = :id_u0, #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0 REMOVE #gsi3pk, #gsi3sk", + "ExpressionAttributeNames": { + "#pk": "pk", + "#sk": "sk", + "#accountId": "accountId", + "#settledAt": "settledAt", + "#updatedAt": "updatedAt", + "#effectiveAt": "effectiveAt", + "#gsi1sk": "gsi1sk", + "#gsi2pk": "gsi2pk", + "#gsi2sk": "gsi2sk", + "#gsi3pk": "gsi3pk", + "#gsi3sk": "gsi3sk", + "#organizationId": "organizationId", + "#id": "id", + "#__edb_e__": "__edb_e__", + "#__edb_v__": "__edb_v__" }, - { table, client } - ); + "ExpressionAttributeValues": { + ":accountId_u0": "123", + ":settledAt_u0": "today", + ":updatedAt_u0": updatedAt, + ":effectiveAt_u0": "n/a", + ":gsi1sk_u0": `$${entityName}_1#accountid_123#id_123`, + // gsi2pk_u0 was not set prior to this fix + ":gsi2pk_u0": "$test#organizationid_123", + ":gsi2sk_u0": `$${entityName}_1#settledat_today`, + ":organizationId_u0": "123", + ":id_u0": "123", + ":__edb_e___u0": `${entityName}`, + ":__edb_v___u0": "1" + }, + "TableName": "electro", + "Key": { + "pk": "$test#organizationid_123", + "sk": `$${entityName}_1#id_123` + }, + "ConditionExpression": "attribute_exists(#pk) AND attribute_exists(#sk)" + }); - // with `effectiveAt` set to 'n/a' and `settledAt` set to 'today' the `entriesByEffectiveDate` index should not be written - const params1 = Thing.patch({ id: '123', organizationId: '123' }) - .set({ effectiveAt: 'n/a', accountId: '123', settledAt: 'today' }) - .params(); + // with `effectiveAt` set to 'today' and `settledAt` set to 'n/a' the `entriesBySettledDate` index should not be written + const params2 = Thing.patch({id: '123', organizationId: '123'}) + .set({effectiveAt: 'today', accountId: '123', settledAt: 'n/a'}) + .params(); - expect(params1).to.deep.equal({ - "UpdateExpression": "SET #effectiveAt = :effectiveAt_u0, #accountId = :accountId_u0, #settledAt = :settledAt_u0, #updatedAt = :updatedAt_u0, #gsi1sk = :gsi1sk_u0, #gsi2pk = :gsi2pk_u0, #gsi2sk = :gsi2sk_u0, #organizationId = :organizationId_u0, #id = :id_u0, #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0 REMOVE #gsi3pk, #gsi3sk", - "ExpressionAttributeNames": { - "#pk": "pk", - "#sk": "sk", - "#accountId": "accountId", - "#settledAt": "settledAt", - "#updatedAt": "updatedAt", - "#effectiveAt": "effectiveAt", - "#gsi1sk": "gsi1sk", - "#gsi2pk": "gsi2pk", - "#gsi2sk": "gsi2sk", - "#gsi3pk": "gsi3pk", - "#gsi3sk": "gsi3sk", - "#organizationId": "organizationId", - "#id": "id", - "#__edb_e__": "__edb_e__", - "#__edb_v__": "__edb_v__" - }, - "ExpressionAttributeValues": { - ":accountId_u0": "123", - ":settledAt_u0": "today", - ":updatedAt_u0": updatedAt, - ":effectiveAt_u0": "n/a", - ":gsi1sk_u0": `$${entityName}_1#accountid_123#id_123`, - // gsi2pk_u0 was not set prior to this fix - ":gsi2pk_u0": "$test#organizationid_123", - ":gsi2sk_u0": `$${entityName}_1#settledat_today`, - ":organizationId_u0": "123", - ":id_u0": "123", - ":__edb_e___u0": `${entityName}`, - ":__edb_v___u0": "1" - }, - "TableName": "electro", - "Key": { - "pk": "$test#organizationid_123", - "sk": `$${entityName}_1#id_123` - }, - "ConditionExpression": "attribute_exists(#pk) AND attribute_exists(#sk)" - }); + expect(params2).to.deep.equal({ + "UpdateExpression": "SET #effectiveAt = :effectiveAt_u0, #accountId = :accountId_u0, #settledAt = :settledAt_u0, #updatedAt = :updatedAt_u0, #gsi1sk = :gsi1sk_u0, #gsi3pk = :gsi3pk_u0, #gsi3sk = :gsi3sk_u0, #organizationId = :organizationId_u0, #id = :id_u0, #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0 REMOVE #gsi2pk, #gsi2sk", + "ExpressionAttributeNames": { + "#pk": "pk", + "#sk": "sk", + "#effectiveAt": "effectiveAt", + "#settledAt": "settledAt", + "#accountId": "accountId", + "#updatedAt": "updatedAt", + "#gsi1sk": "gsi1sk", + "#gsi2pk": "gsi2pk", + "#gsi2sk": "gsi2sk", + "#gsi3pk": "gsi3pk", + "#gsi3sk": "gsi3sk", + "#organizationId": "organizationId", + "#id": "id", + "#__edb_e__": "__edb_e__", + "#__edb_v__": "__edb_v__" + }, + "ExpressionAttributeValues": { + ":effectiveAt_u0": "today", + ":settledAt_u0": "n/a", + ":accountId_u0": "123", + ":updatedAt_u0": updatedAt, + ":gsi1sk_u0": `$${entityName}_1#accountid_123#id_123`, + // gsi3pk_u0 was not set prior to this fix + ":gsi3pk_u0": "$test#organizationid_123", + ":gsi3sk_u0": `$${entityName}_1#effectiveat_today`, + ":organizationId_u0": "123", + ":id_u0": "123", + ":__edb_e___u0": `${entityName}`, + ":__edb_v___u0": "1" + }, + "TableName": "electro", + "Key": { + "pk": "$test#organizationid_123", + "sk": `$${entityName}_1#id_123` + }, + "ConditionExpression": "attribute_exists(#pk) AND attribute_exists(#sk)" + }); - // with `effectiveAt` set to 'today' and `settledAt` set to 'n/a' the `entriesBySettledDate` index should not be written - const params2 = Thing.patch({ id: '123', organizationId: '123' }) - .set({ effectiveAt: 'today', accountId: '123', settledAt: 'n/a' }) - .params(); + const organizationId = uuid(); + const accountId = uuid(); + const id = uuid(); + const type = 'green' + const category = 'liquid' + const amount = '200' + const description = 'a description'; + + await Thing.create({ + organizationId, + accountId, + id, + type, + amount, + category, + description, + settledAt: 'n/a', + effectiveAt: 'n/a' + }).go(); - expect(params2).to.deep.equal({ - "UpdateExpression": "SET #effectiveAt = :effectiveAt_u0, #accountId = :accountId_u0, #settledAt = :settledAt_u0, #updatedAt = :updatedAt_u0, #gsi1sk = :gsi1sk_u0, #gsi3pk = :gsi3pk_u0, #gsi3sk = :gsi3sk_u0, #organizationId = :organizationId_u0, #id = :id_u0, #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0 REMOVE #gsi2pk, #gsi2sk", - "ExpressionAttributeNames": { - "#pk": "pk", - "#sk": "sk", - "#effectiveAt": "effectiveAt", - "#settledAt": "settledAt", - "#accountId": "accountId", - "#updatedAt": "updatedAt", - "#gsi1sk": "gsi1sk", - "#gsi2pk": "gsi2pk", - "#gsi2sk": "gsi2sk", - "#gsi3pk": "gsi3pk", - "#gsi3sk": "gsi3sk", - "#organizationId": "organizationId", - "#id": "id", - "#__edb_e__": "__edb_e__", - "#__edb_v__": "__edb_v__" - }, - "ExpressionAttributeValues": { - ":effectiveAt_u0": "today", - ":settledAt_u0": "n/a", - ":accountId_u0": "123", - ":updatedAt_u0": updatedAt, - ":gsi1sk_u0": `$${entityName}_1#accountid_123#id_123`, - // gsi3pk_u0 was not set prior to this fix - ":gsi3pk_u0": "$test#organizationid_123", - ":gsi3sk_u0": `$${entityName}_1#effectiveat_today`, - ":organizationId_u0": "123", - ":id_u0": "123", - ":__edb_e___u0": `${entityName}`, - ":__edb_v___u0": "1" - }, - "TableName": "electro", - "Key": { - "pk": "$test#organizationid_123", - "sk": `$${entityName}_1#id_123` - }, - "ConditionExpression": "attribute_exists(#pk) AND attribute_exists(#sk)" + // 'gsi1pk-gsi1sk-index' should have been written to + const entriesByAccount = await Thing.query.entriesByAccount({organizationId, accountId}).go(); + expect(entriesByAccount.data.length).to.equal(1); + expect(entriesByAccount.data[0].id).to.equal(id); + expect(entriesByAccount.data[0].organizationId).to.equal(organizationId); + + // 'gsi2pk-gsi2sk-index' should not have been written to + const entriesBySettledDate = await Thing.query.entriesBySettledDate({organizationId}).go(); + expect(entriesBySettledDate.data.length).to.equal(0); + + // with settledAt set to 'today', 'gsi2pk-gsi2sk-index' should be written to + await Thing.patch({id, organizationId}).set({settledAt: 'today'}).go(); + const entriesBySettledDate2 = await Thing.query.entriesBySettledDate({organizationId}).go(); + expect(entriesBySettledDate2.data.length).to.equal(1); + expect(entriesBySettledDate2.data[0].id).to.equal(id); + expect(entriesBySettledDate2.data[0].organizationId).to.equal(organizationId); + + // 'gsi3pk-gsi3sk-index' should not have been written to + const entriesByEffectiveDate = await Thing.query.entriesByEffectiveDate({organizationId}).go(); + expect(entriesByEffectiveDate.data.length).to.equal(0); + + // with effectiveAt set to 'today', 'gsi3pk-gsi3sk-index' should be written to + await Thing.patch({id, organizationId}).set({effectiveAt: 'today'}).go(); + const entriesByEffectiveDate2 = await Thing.query.entriesByEffectiveDate({organizationId}).go(); + expect(entriesByEffectiveDate2.data.length).to.equal(1); + expect(entriesByEffectiveDate2.data[0].id).to.equal(id); + expect(entriesByEffectiveDate2.data[0].organizationId).to.equal(organizationId); }); - - const organizationId = uuid(); - const accountId = uuid(); - const id = uuid(); - const type = 'green' - const category = 'liquid' - const amount = '200' - const description = 'a description'; - - await Thing.create({ - organizationId, - accountId, - id, - type, - amount, - category, - description, - settledAt: 'n/a', - effectiveAt: 'n/a' - }).go(); - - // 'gsi1pk-gsi1sk-index' should have been written to - const entriesByAccount = await Thing.query.entriesByAccount({ organizationId, accountId }).go(); - expect(entriesByAccount.data.length).to.equal(1); - expect(entriesByAccount.data[0].id).to.equal(id); - expect(entriesByAccount.data[0].organizationId).to.equal(organizationId); - - // 'gsi2pk-gsi2sk-index' should not have been written to - const entriesBySettledDate = await Thing.query.entriesBySettledDate({ organizationId }).go(); - expect(entriesBySettledDate.data.length).to.equal(0); - - // with settledAt set to 'today', 'gsi2pk-gsi2sk-index' should be written to - await Thing.patch({ id, organizationId }).set({ settledAt: 'today' }).go(); - const entriesBySettledDate2 = await Thing.query.entriesBySettledDate({ organizationId }).go(); - expect(entriesBySettledDate2.data.length).to.equal(1); - expect(entriesBySettledDate2.data[0].id).to.equal(id); - expect(entriesBySettledDate2.data[0].organizationId).to.equal(organizationId); - - // 'gsi3pk-gsi3sk-index' should not have been written to - const entriesByEffectiveDate = await Thing.query.entriesByEffectiveDate({ organizationId }).go(); - expect(entriesByEffectiveDate.data.length).to.equal(0); - - // with effectiveAt set to 'today', 'gsi3pk-gsi3sk-index' should be written to - await Thing.patch({ id, organizationId }).set({ effectiveAt: 'today' }).go(); - const entriesByEffectiveDate2 = await Thing.query.entriesByEffectiveDate({ organizationId }).go(); - expect(entriesByEffectiveDate2.data.length).to.equal(1); - expect(entriesByEffectiveDate2.data[0].id).to.equal(id); - expect(entriesByEffectiveDate2.data[0].organizationId).to.equal(organizationId); }); }); From 3e47188a5cd816029b6f50ca0a25a3e92b647a24 Mon Sep 17 00:00:00 2001 From: tywalch Date: Mon, 29 Apr 2024 13:35:54 -0400 Subject: [PATCH 12/14] Adds changelog documentation --- CHANGELOG.md | 8 ++++++-- test/ts_connected.entity.spec.ts | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ddf10d0f..a4d56da3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -513,6 +513,10 @@ All notable changes to this project will be documented in this file. Breaking ch ### Added - Adds new query execution option `count` which allows you to specify a specific item count to return from a query. This is useful for cases where you must return a specific/consistent number of items from a query, a deceptively difficult task with DynamoDB and Single Table Design. -## [2.13.1] - 2023-01-23 +## [2.13.1] - 2024-01-23 ### Fixed -- Fixes custom attribute type extraction for union types with RecordItem. Patch provided by GitHub user @wentsul via [PR #346](https://github.com/tywalch/electrodb/pull/346). Thank you for another great addition! \ No newline at end of file +- Fixes custom attribute type extraction for union types with RecordItem. Patch provided by GitHub user @wentsul via [PR #346](https://github.com/tywalch/electrodb/pull/346). Thank you for another great addition! + +## [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 diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index 5b13171b..d0e3eec6 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -4184,6 +4184,7 @@ describe("index condition", () => { ConditionExpression: 'attribute_exists(#pk) AND attribute_exists(#sk)' }); }); + it('should fix gh issue 366', async () => { const entityName = uuid(); const updatedAt = new Date().toJSON(); From e79518f1479a757288086384d01675638b25aa16 Mon Sep 17 00:00:00 2001 From: tywalch Date: Mon, 29 Apr 2024 13:47:22 -0400 Subject: [PATCH 13/14] Fixes test that used dynamic datetime --- test/ts_connected.entity.spec.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index d0e3eec6..82fe598d 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -4055,15 +4055,15 @@ describe("index condition", () => { type: 'string', readOnly: true, required: true, - default: () => new Date().toJSON(), - set: () => new Date().toJSON() + default: () => '2024-02-29', + set: () => '2024-02-29' }, updatedAt: { type: 'string', watch: '*', required: true, - default: () => new Date().toJSON(), - set: () => new Date().toJSON() + default: () => '2024-04-29', + set: () => '2024-04-29' }, settledAt: { type: 'string', @@ -4170,7 +4170,7 @@ describe("index condition", () => { ':effectiveAt_u0': 'n/a', ':accountId_u0': '123', ':settledAt_u0': 'today', - ':updatedAt_u0': '2024-04-29T17:09:30.687Z', + ':updatedAt_u0': '2024-04-29', ':gsi1sk_u0': '$test_1#accountid_123#id_123', ':gsi2pk_u0': '$test#organizationid_123', ':gsi2sk_u0': '$test_1#accountid_123#settledat_today', From 87521c4c15a1c41fb8ff976ae425a6b47f908382 Mon Sep 17 00:00:00 2001 From: tywalch Date: Mon, 29 Apr 2024 15:03:29 -0400 Subject: [PATCH 14/14] Adds additional tests --- test/ts_connected.entity.spec.ts | 173 +++++++++++++++++++++++++++++++ 1 file changed, 173 insertions(+) diff --git a/test/ts_connected.entity.spec.ts b/test/ts_connected.entity.spec.ts index 82fe598d..19edd137 100644 --- a/test/ts_connected.entity.spec.ts +++ b/test/ts_connected.entity.spec.ts @@ -4026,6 +4026,179 @@ describe("index condition", () => { } describe('github issue 366', () => { + it('should recreate exact model from issue but as an upsert', async () => { + + const table = "your_table_name"; + + const entry = new Entity( + { + model: { + service: 'test', + entity: 'test', + version: '1' + }, + attributes: { + id: { + type: 'string', + required: true, + readOnly: true + }, + organizationId: { + type: 'string', + required: true, + readOnly: true + }, + accountId: { + type: 'string' + }, + createdAt: { + type: 'string', + readOnly: true, + required: true, + default: () => '2024-02-29', + set: () => '2024-02-29' + }, + updatedAt: { + type: 'string', + watch: '*', + required: true, + default: () => '2024-04-29', + set: () => '2024-04-29' + }, + settledAt: { + type: 'string', + default: 'n/a' + }, + effectiveAt: { + type: 'string', + default: 'n/a' + }, + type: { + type: 'string', + required: true + }, + category: { + type: 'string', + required: true + }, + amount: { + type: 'string', + required: true + }, + description: { + type: 'string' + } + }, + indexes: { + entries: { + pk: { + field: 'pk', + composite: ['organizationId'] + }, + sk: { + field: 'sk', + composite: ['id'] + } + }, + entriesByAccount: { + index: 'gsi1pk-gsi1sk-index', + pk: { + field: 'gsi1pk', + composite: ['organizationId'] + }, + sk: { + field: 'gsi1sk', + composite: ['accountId', 'id'] + } + }, + entriesBySettledDate: { + index: 'gsi2pk-gsi2sk-index', + condition: (attr) => attr.settledAt !== 'n/a', + pk: { + field: 'gsi2pk', + composite: ['organizationId'] + }, + sk: { + field: 'gsi2sk', + composite: ['accountId', 'settledAt'] + } + }, + entriesByEffectiveDate: { + index: 'gsi3pk-gsi3sk-index', + condition: (attr) => attr.effectiveAt !== 'n/a', + pk: { + field: 'gsi3pk', + composite: ['organizationId'] + }, + sk: { + field: 'gsi3sk', + composite: ['accountId', 'effectiveAt'] + } + } + } + }, + { table } + ); + + const params = entry + .upsert({ + id: '123', + organizationId: '123', + effectiveAt: 'n/a', + accountId: "123", + settledAt: "today", + type: 'test-type', + category: 'test-category', + amount: 'test-amount', + }) + .params(); + + // params set `gsi1sk` and `gsi2pk` fields and remove `gsi3pk` and `gsi3sk` fields + expect(params).to.deep.equal({ + TableName: 'your_table_name', + UpdateExpression: 'SET #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0, #id = :id_u0, #organizationId = :organizationId_u0, #accountId = :accountId_u0, #createdAt = if_not_exists(#createdAt, :createdAt_u0), #updatedAt = :updatedAt_u0, #settledAt = :settledAt_u0, #effectiveAt = :effectiveAt_u0, #type = :type_u0, #category = :category_u0, #amount = :amount_u0, #gsi1pk = :gsi1pk_u0, #gsi1sk = :gsi1sk_u0, #gsi2pk = :gsi2pk_u0, #gsi2sk = :gsi2sk_u0 REMOVE #gsi3pk, #gsi3sk', + ExpressionAttributeNames: { + '#__edb_e__': '__edb_e__', + '#__edb_v__': '__edb_v__', + '#gsi3pk': 'gsi3pk', + '#gsi3sk': 'gsi3sk', + '#id': 'id', + '#organizationId': 'organizationId', + '#accountId': 'accountId', + '#createdAt': 'createdAt', + '#updatedAt': 'updatedAt', + '#settledAt': 'settledAt', + '#effectiveAt': 'effectiveAt', + '#type': 'type', + '#category': 'category', + '#amount': 'amount', + '#gsi1pk': 'gsi1pk', + '#gsi1sk': 'gsi1sk', + '#gsi2pk': 'gsi2pk', + '#gsi2sk': 'gsi2sk' + }, + ExpressionAttributeValues: { + ':__edb_e___u0': 'test', + ':__edb_v___u0': '1', + ':id_u0': '123', + ':organizationId_u0': '123', + ':accountId_u0': '123', + ':createdAt_u0': '2024-02-29', + ':updatedAt_u0': '2024-04-29', + ':settledAt_u0': 'today', + ':effectiveAt_u0': 'n/a', + ':type_u0': 'test-type', + ':category_u0': 'test-category', + ':amount_u0': 'test-amount', + ':gsi1pk_u0': '$test#organizationid_123', + ':gsi1sk_u0': '$test_1#accountid_123#id_123', + ':gsi2pk_u0': '$test#organizationid_123', + ':gsi2sk_u0': '$test_1#accountid_123#settledat_today' + }, + Key: { pk: '$test#organizationid_123', sk: '$test_1#id_123' } + }); + }); + it('should recreate exact model from issue', async () => { const table = "your_table_name";