Fixes issue 530 (#531)

* Fixes issue 530
Fixed issue where ElectroDB would attempt to set a table index field's value on `upsert` if an index key used an attribute's name as it's field name. This would cause the `upsert` to fail because DyanmoDB prevents set operations on table index fields.

* Fixes table initialization

Author: tywalch

CHANGELOG.md

@@ -598,4 +598,8 @@ All notable changes to this project will be documented in this file. Breaking ch
 
 ## [3.4.6]
 ### Fixed
-- [Issue #343]((https://github.com/tywalch/electrodb/issues/343)); Fixed issue where ElectroDB would attempt to double set an attribute's value on `upsert` if an index key used the attribute's name as it's field name. This would cause the `upsert` to fail because DyanmoDB prevents duplicate set operations to the same field.
+- [Issue #343]((https://github.com/tywalch/electrodb/issues/343)); Fixed issue where ElectroDB would attempt to double set an attribute's value on `upsert` if an index key used the attribute's name as it's field name. This would cause the `upsert` to fail because DyanmoDB prevents duplicate set operations to the same field.
+
+## [3.4.7]
+### Fixed
+- [Issue #530]((https://github.com/tywalch/electrodb/issues/530)); Fixed issue where ElectroDB would attempt to set a table index field's value on `upsert` if an index key used an attribute's name as it's field name. This would cause the `upsert` to fail because DyanmoDB prevents set operations on table index fields.

package.json

@@ -1,6 +1,6 @@
 {
   "name": "electrodb",
-  "version": "3.4.6",
+  "version": "3.4.7",
   "description": "A library to more easily create and interact with multiple entities and heretical relationships in dynamodb",
   "main": "index.js",
   "scripts": {

src/clauses.js

@@ -15,7 +15,6 @@ const {
   AttributeOperationProxy,
   UpdateOperations,
   FilterOperationNames,
-  UpdateOperationNames,
 } = require("./operations");
 const { UpdateExpression } = require("./update");
 const { FilterExpression } = require("./where");
@@ -472,6 +471,8 @@ let clauses = {
               const value = updatedKeys[key];
               if (indexKey[key] === undefined) {
                 setFields[key] = value;
+              } else {
+                delete setFields[key];
               }
             }
 

test/connected.issues.spec.ts

@@ -1,9 +1,10 @@
-// @ts-nocheck
+// @ts-expect-error
 process.env.AWS_NODEJS_CONNECTION_REUSE_ENABLED = 1;
 import DynamoDB from "aws-sdk/clients/dynamodb";
 import { v4 as uuid } from "uuid";
 import { expect } from "chai";
 import { Entity } from "../";
+import { putTable } from "./table";
 const table = "electro";
 const client = new DynamoDB.DocumentClient({
   region: "us-east-1",
@@ -14,6 +15,15 @@ const client = new DynamoDB.DocumentClient({
   },
 });
 
+const dynamodb = new DynamoDB({
+  region: "us-east-1",
+  endpoint: "http://localhost:8000",
+  credentials: {
+    accessKeyId: "test",
+    secretAccessKey: "test",
+  },
+});
+
 describe("Issue #343", () => {
   function createEntity() {
     return new Entity({
@@ -96,18 +106,18 @@ describe("Issue #343", () => {
           "#gsi1sk": "gsi1sk",
       },
       "ExpressionAttributeValues": {
-          ":__edb_e___u0": inventory.model.entity,
-          ":__edb_v___u0": inventory.model.version,
+          ":__edb_e___u0": inventory.schema.model.entity,
+          ":__edb_v___u0": inventory.schema.model.version,
           ":accountId_u0": accountId,
           ":transactionId_u0": transactionId,
           ":date_u0": date,
           ":value_u0": value,
-          ":gsi1pk_u0": `$${inventory.model.service}#transactionid_${transactionId}`,
-          ":gsi1sk_u0": `$${inventory.model.entity}_${inventory.model.version}`
+          ":gsi1pk_u0": `$${inventory.schema.model.service}#transactionid_${transactionId}`,
+          ":gsi1sk_u0": `$${inventory.schema.model.entity}_${inventory.schema.model.version}`
       },
       "Key": {
-          "pk": `$${inventory.model.service}#accountid_${accountId}`,
-          "sk": `$${inventory.model.entity}_${inventory.model.version}#date_${date}#transactionid_${transactionId}`
+          "pk": `$${inventory.schema.model.service}#accountid_${accountId}`,
+          "sk": `$${inventory.schema.model.entity}_${inventory.schema.model.version}#date_${date}#transactionid_${transactionId}`
       }   
     });
 
@@ -135,16 +145,16 @@ describe("Issue #343", () => {
         "#value": "value",
       },
       "ExpressionAttributeValues": {
-        ":__edb_e___u0": inventory.model.entity,
-        ":__edb_v___u0": inventory.model.version,
+        ":__edb_e___u0": inventory.schema.model.entity,
+        ":__edb_v___u0": inventory.schema.model.version,
         ":accountId_u0": accountId,
         ":transactionId_u0": transactionId,
         ":date_u0": date,
         ":value_u0": value,
       },
       "Key": {
-        "pk": `$${inventory.model.service}#accountid_${accountId}`,
-        "sk": `$${inventory.model.entity}_${inventory.model.version}#date_${date}#transactionid_${transactionId}`
+        "pk": `$${inventory.schema.model.service}#accountid_${accountId}`,
+        "sk": `$${inventory.schema.model.entity}_${inventory.schema.model.version}#date_${date}#transactionid_${transactionId}`
       }
     });
   });
@@ -178,18 +188,89 @@ describe("Issue #343", () => {
         "#sk": "sk",
       },
       "ExpressionAttributeValues": {
-        ":__edb_e___u0": inventory.model.entity,
-        ":__edb_v___u0": inventory.model.version,
+        ":__edb_e___u0": inventory.schema.model.entity,
+        ":__edb_v___u0": inventory.schema.model.version,
         ":accountId_u0": accountId,
         ":transactionId_u0": transactionId,
         ":date_u0": date,
         ":value_u0": value,
       },
       "ConditionExpression": "attribute_exists(#pk) AND attribute_exists(#sk)",
       "Key": {
-        "pk": `$${inventory.model.service}#accountid_${accountId}`,
-        "sk": `$${inventory.model.entity}_${inventory.model.version}#date_${date}#transactionid_${transactionId}`
+        "pk": `$${inventory.schema.model.service}#accountid_${accountId}`,
+        "sk": `$${inventory.schema.model.entity}_${inventory.schema.model.version}#date_${date}#transactionid_${transactionId}`
       }
     });
   });
-});
+});
+
+describe("Issue #530", () => {
+  it("should upsert item without apply set to attribute key name", async () => {
+    const table = `issue530`;
+    const Log = new Entity(
+      {
+        model: {
+          entity: "log_record",
+          version: "1",
+          service: "log",
+        },
+        attributes: {
+          ip_addr: {
+            type: "string",
+            required: true,
+            readOnly: true,
+          },
+          expires_ttl: {
+            type: "number",
+          },
+          app: {
+            type: "string",
+          },
+          click_id: {
+            type: "string",
+          },
+          timestamp: {
+            type: "string",
+          },
+        },
+        indexes: {
+          byIpAddr: {
+            pk: {
+              field: "ip_addr",
+              composite: ["ip_addr"],
+            },
+          },
+        },
+      },
+      { table, client }
+    );
+
+    const record = {
+      ip_addr: "127.0.0.1",
+      app: "something",
+    };
+
+    const params = Log.upsert(record).params({});
+    expect(params).to.deep.equal({
+      TableName: table,
+      UpdateExpression: 'SET #__edb_e__ = :__edb_e___u0, #__edb_v__ = :__edb_v___u0, #app = :app_u0',
+      ExpressionAttributeNames: {
+        '#__edb_e__': '__edb_e__',
+        '#__edb_v__': '__edb_v__',
+        '#app': 'app'
+      },
+      ExpressionAttributeValues: {
+        ':__edb_e___u0': 'log_record',
+        ':__edb_v___u0': '1',
+        ':app_u0': 'something'
+      },
+      Key: {
+        ip_addr: '127.0.0.1'
+      },
+    });
+
+    await Log.upsert(record).where((attr, op) => op.notExists(attr.ip_addr)).go()
+    const result = await Log.get({ ip_addr: record.ip_addr }).go();
+    expect(result.data).to.deep.equal(record);
+  })
+})

test/definitions/issue530.json

@@ -0,0 +1,16 @@
+{
+  "TableName": "issue530",
+  "KeySchema": [
+    {
+      "AttributeName": "ip_addr",
+      "KeyType": "HASH"
+    }
+  ],
+  "AttributeDefinitions": [
+    {
+      "AttributeName": "ip_addr",
+      "AttributeType": "S"
+    }
+  ],
+  "BillingMode": "PAY_PER_REQUEST"
+}

test/init.js

@@ -11,6 +11,7 @@ const localSecondaryIndexes = require("./definitions/localsecondaryindexes.json"
 const keysOnly = require("./definitions/keysonly.json");
 const castKeys = require("./definitions/castkeys.json");
 const reverseIndex = require("./definitions/reverseindex.json");
+const issue530 = require("./definitions/issue530.json");
 const shouldDestroy = process.argv.includes("--recreate");
 
 if (
@@ -84,6 +85,7 @@ async function main() {
     createTable(dynamodb, "electro_keysonly", keysOnly),
     createTable(dynamodb, "electro_castkeys", castKeys),
     createTable(dynamodb, "electro_reverseindex", reverseIndex),
+    createTable(dynamodb, "issue530", issue530),
   ]);
 }
 

test/table.ts

@@ -0,0 +1,32 @@
+import type { DynamoDB } from "aws-sdk";
+
+export interface TableManager {
+  exists(): Promise<boolean>;
+  drop(): Promise<void>;
+  create(definition: DynamoDB.CreateTableInput): Promise<void>;
+}
+
+export function createTableManager(dynamodb: DynamoDB, table: string): TableManager {
+  return {
+    async exists() {
+      let tables = await dynamodb.listTables().promise();
+      return (tables.TableNames || []).includes(table);
+    },
+    async drop() {
+      await dynamodb.deleteTable({ TableName: table }).promise();
+    },
+    async create(definition: DynamoDB.CreateTableInput) {
+      await dynamodb
+        .createTable({ ...definition, TableName: table })
+        .promise();
+    },
+  };
+}
+
+export async function putTable(dynamodb: DynamoDB, definition: DynamoDB.CreateTableInput) {
+  const table = createTableManager(dynamodb, definition.TableName);
+  if (await table.exists()) {
+    await table.drop();
+  }
+  return await table.create(definition);
+}