tywalch / electrodb

A DynamoDB library to ease the use of modeling complex hierarchical relationships and implementing a Single Table Design while keeping your query code readable.
MIT License
956 stars 58 forks source link

Invalid params generated for patch/update where table/GSI specified with Composite Attribute Templates #343

Open rogersillito opened 6 months ago

rogersillito commented 6 months ago

Describe the bug We need to do an update to a field "status" (that is itself the primary key of a GSI) where that update includes a ConditionExpression to ensure the status has the expected value before applying the update. Our index definitions use attribute templates (rationale below!). When we try to patch (or update) in this way, the params that are generated result in an update expression where setting of the status field is duplicated twice - this results in a DynamoDB error when the patch/update is executed.

image

If I remove the GSI, then the patch/upate params are generated correctly, albeit we can't then benefit from that index (Playground demo of this).

ElectroDB Version 2.12.2

ElectroDB Playground Link ElectroDB Playground Example "patch" mutation

Entity/Service Definitions

{
    model: {
      entity: 'activeApplication',
      version: '1',
      service: 'internalAppPortal',
    },
    attributes: {
      username: {
        type: 'string',
        field: 'un',
        required: true,
      },
      applicationKey: {
        type: 'string',
        field: 'ak',
        required: true,
      },
      status: {
        type: [
          'SPAWN_REQUESTED',
          'SPAWNING_CODEBUILD',
          'SPAWNING_READINESS_CHECK',
          'SPAWNED',
          'REMOVE_REQUESTED',
          'REMOVING_CODEBUILD',
          'FAILED',
        ] as const,
        field: 'st',
        required: true,
      },
      hostName: {
        type: 'string',
        field: 'hn',
        readOnly: true,
        required: true,
      },
      spawnId: {
        type: 'string',
        field: 'si',
      },
      removeId: {
        type: 'string',
        field: 'ri',
      },
    },
    indexes: {
      byUsername: {
        pk: {
          field: 'un',
          composite: ['username'],
          template: '${username}',
          casing: 'none',
        },
        sk: {
          field: 'ak',
          composite: ['applicationKey'],
          template: '${applicationKey}',
          casing: 'none',
        },
      },
      byStatus: {
        index: 'active-applications-by-status',
        pk: {
          field: 'st',
          composite: ['status'],
          template: '${status}',
          casing: 'none',
        },
      },
    },
  }

Expected behavior The patch params should be generated without the duplicated setting of "status".

Errors

{
  "message": "Error thrown by DynamoDB client: \"Invalid UpdateExpression: Two document paths overlap with each other; must remove or rewrite one of these paths; path one: [st], path two: [__edb_e__]\" - For more detail on this error reference: https://electrodb.dev/en/reference/errors/#aws-error",
  "name": "ElectroError",
  "ref": {
    "code": 4001,
    "section": "aws-error",
    "name": "AWSError"
  },
  "code": 4001,
  "date": 1704387127434,
  "isElectroError": true
}

stack trace:

    at Entity.go (C:\DEV\internal-app-portal\admin-app\node_modules\electrodb\src\entity.js:497:20)
    at Object.action (C:\DEV\internal-app-portal\admin-app\node_modules\electrodb\src\clauses.js:1240:23)
    at current.<computed> [as go] (C:\DEV\internal-app-portal\admin-app\node_modules\electrodb\src\clauses.js:1343:41)
    at executeDbOps (webpack-internal:///(api)/./electrodbtest.ts:199:71)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Additional context We have a very simple use-case where our app has only a single entity and a single DynamoDB table needed to store it in. Our keys and access patterns naturally align with the primary key/sort key requirement of DynamoDB. For this reason we would prefer not to use the "default", Single Table Design orientated behaviour of electrodb around indexes which embeds the additional attribute metadata within the pk/sk values. This is why we are going against your recommendation and making use of index attribute templates. If there's an alternative way we could get to the same place and avoid the issue we seem to have hit, that would be good to know?

tywalch commented 6 months ago

Hi @rogersillito 👋

Oh darn, thank you for raising this, I will take a look and get back to you soon!