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
997 stars 63 forks source link

Incorrect key generated when using composite operation on patch/update #372

Closed JosephSuyam closed 5 months ago

JosephSuyam commented 5 months ago

Describe the bug Incorrect key is generated for pk and sk when adding .composite on patch/update. I've also added the 2 scenarios on the playground link.

ElectroDB Version 2.13.1

ElectroDB Playground Link ElectroDB Playground

Entity/Service Definitions Include your entity model (or a model that sufficiently recreates your issue) to help troubleshoot.

{
    model: {
      entity: 'posts',
      version: '1',
      service: 'cms',
    },
    attributes: {
      userId: {
        type: 'string',
        required: true,
        // default: () => uuid(),
      },
      type: {
        type: Object.values(PostType),
        required: true,
      },
      title: {
        type: 'string',
        required: true,
      },
      description: {
        type: 'string',
        required: true,
      },
      attachments: {
        type: 'string',
      },
      template: {
        type: 'string',
      },
      status: {
        type: Object.values(PostStatus),
        default: PostStatus.ACTIVE,
        required: true,
      },
      createdAt: {
        type: 'string',
        default: () => date_time,
        readOnly: true,
      },
      updatedAt: {
        type: 'string',
        watch: '*',
        set: () => date_time,
        readOnly: true,
      },
      expireAt: {
        type: 'number',
        watch: ['status'],
        set: (_, { status }) => {
          if (status === PostStatus.INACTIVE) getUnixTime(addDays(new Date(), 30));
        },
        readOnly: true,
      },
    },
    indexes: {
      post: {
        collection: 'postList',
        pk: {
          composite: ['userId'],
          field: 'pk',
        },
        sk: {
          composite: ['type', 'title'],
          field: 'sk',
        },
      },
      title: {
        index: 'gsi1pk-gsi1sk-index',
        pk: {
          field: 'gsi1pk',
          composite: ['title'],
        },
        sk: {
          field: 'gsi1sk',
          composite: ['userId'],
        },
      },
    },
  }

Expected behavior To be able to update the values of composite keys -- title and type for my model.

Errors

{
  _message: undefined,
  message: 'Error thrown by DynamoDB client: "The conditional request failed" - 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',
    sym: Symbol(error-code)
  },
  code: 4001,
  date: 1712645861976,
  isElectroError: true
}

Additional context Not sure if there is just missing on my usage on composite key.

tywalch commented 5 months ago

Hi @JosephSuyam 👋

I took a look at your example and I created a playground to help explain it a bit, but I also do that here.

The composite() method exists to help provide additional context to an update or patch to ensure indexes can be built completely. For example, if you set a single attribute that is involved in a >1 composite index ElectroDB will recognize this action will cause inconsistencies between the index key and the individual attributes and throw. ElectroDB treats consistency between a composite index and it's individual attribute values as critical, and limitations with DynamoDB require these values all provided at the same time.

Going back to composite() this is present because some values in a composite can be readOnly, so they cannot be provided via set. This method allows you set the mutable parts of an index and provided readOnly parts to keep your indexes consistent with the underlying values.

In the example you provided, unfortunately there is a logical issue. The post index (your main table index) is defined with the composite attributes userId, type, and title. Because these attributes together form the primary key of your item, they become readOnly by default. In both of your examples, the values for those attributes differ between what is passed to the patch method and what is passed the the composite method.

// `patch()` gets the values from `post`
Posts.patch({ userId, type: post.type, title: post.type })
  .set({
    description: post.description,
    attachments: post?.attachments,
    template: post?.template,
  })
// `composite()` gets different values
  .composite({ userId, type, title })
  .go();
// `patch()` gets the top level values
Posts.patch({ userId, type, title })
  .set({
    description: post.description,
    attachments: post?.attachments,
    template: post?.template,
  })
  // `composite()` gets the values from `post`
  .composite({ userId, type: post.type, title: post.type })
  .go();

The issue here is that this would result your title index (gsi1pk-gsi1sk-index) having different values than your attribute and main table index.

ElectroDB relies on the addition of this Conditional Expression to ensure that values provided via composite are truely the values on the attribute: "(attribute_exists(#pk) AND attribute_exists(#sk) AND #userId = :userId0) AND #title = :title0". In your cause they wouldn't so both will throw the exact error you provided.

I hope this helps, please let me know if I can clarify further in any way! I appreciate you creating the issue, it helps me understand the areas of the library the can use more clarification, attention, and documentation!

JosephSuyam commented 5 months ago

Hi @tywalch, so basically composite keys are immutable and composite() is pretty much a 'where()' for composite keys right?

I initially thought that composite() would enable the update on the composite keys. Thanks for clarifying, I'll close this issue now. Ohh and can you confirm if this is correct.

so basically composite keys are immutable and composite() is pretty much a 'where()' for composite keys right?

tywalch commented 5 months ago

Yes, I'd say you pretty much got it 👍

When the attributes being set on an update() call would only provide ElectroDB with enough data to make a partial key, ElectroDB throws to prevent consistency issues. The composite() method is helpful when you can't use set on a composite attribute because it's readOnly, but without it ElectroDB would throw because it is missing. Additionally, to further enforce consistency, ElectroDB also adds where conditions on the attributes passed to composite().