Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inconsistent index key formatting with optional attributes #445

Closed
anatolzak opened this issue Nov 2, 2024 · 2 comments
Closed

inconsistent index key formatting with optional attributes #445

anatolzak opened this issue Nov 2, 2024 · 2 comments

Comments

@anatolzak
Copy link

Describe the bug
Everything I am going to describe revolves around how to handle key formatting with composite attributes are not required at the schema level. And I believe there are some inconsistencies with how it currently works.

  1. Take the following example Playground
const users = new Entity({
  ...
  attributes: {
    id: { type: 'string', required: true },
    role: { type: 'string' }
  },
  indexes: {
    byId: { pk: { field: 'PK', composite: ['id'] } },
    byRole: {
      index: 'GSI2',
      pk: { field: 'GSI2_PK', composite: ['role'] },
      sk: { field: 'GSI2_SK', composite: ['id'] },
    },
  },
}, { table: 'table' });

running users.create({ id: '123' }).go() will create the following dynamo params:

{
    "Item": {
        "id": "123",
        "PK": "$service$users_1#id_123",
        "GSI2_PK": "$service#role_",
        "GSI2_SK": "$users_1#id_123",
        "__edb_e__": "users",
        "__edb_v__": "1"
    },
    "TableName": "table",
    "ConditionExpression": "attribute_not_exists(#PK)",
    "ExpressionAttributeNames": {
        "#PK": "PK"
    }
}

Note that Electro created the GSI keys using an empty string for the role attribute. I find this interesting but not necessarily problematic.

  1. Taking the previous example, if I were to add another attribute to the composite array that contains role and if this attribute happens to be a required attribute at the schema level, if we were to create an item without passing the role attribute but did pass the other attribute in that composite array, Electro will throw an error.

playground

const users = new Entity({
  ...
  attributes: {
    id: { type: 'string', required: true },
    role: { type: 'string' },
    birth_date: { type: 'string', required: true }
  },
  indexes: {
    byId: { pk: { field: 'PK', composite: ['id'] } },
    byRole: {
      index: 'GSI2',
      pk: {
        field: 'GSI2_PK',
        composite: ['role', 'birth_date'],
      },
      sk: { field: 'GSI2_SK', composite: ['id'] },
    },
  },
}, { table: 'table' });

running users.create({ id: '123', birth_date: '2024-01-01' }).go() will throw

Incomplete composite attributes: Without the composite attributes "role" the following access patterns cannot be updated: "byRole". 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.

ElectroDB Version
4.0.1

Expected behavior
To be honest, I am not sure what the expected behavior should be. Allowing optional attributes to be specified in the composite field poses some challenges like during partial updates.

For example, let's say we were to allow optional attributes in indexes. And let's say that if an optional field is not passed during creation, we don't format that key (which is not how it works currently).

So taking the second example, we would create a user without passing a role and passing birth_date. Following what I said, we won't format the GSI key.

What is the user's expectation when we do a partial update on a user and only update the birth_date attribute without specifying the role attribute?
do we throw an error because we didn't provide all the composite attributes? but what if the role does not exist on the saved item and we want to keep it as such?

Not sure how to come up with a solution that solves all the use cases.

What I am doing for now is only using required attributes in index composite fields and passing an empty string for example for fields that would normally be optional.

@tywalch
Copy link
Owner

tywalch commented Nov 2, 2024

Can you read my reply here and let me know if it clears up some things for you? #433 (comment)

@anatolzak
Copy link
Author

@tywalch thanks for the context!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants