From 1d5bf8edecaeb41e29a2881c44c9801d625f47a4 Mon Sep 17 00:00:00 2001 From: tywalch Date: Mon, 29 Apr 2024 12:50:02 -0400 Subject: [PATCH] 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 () => {