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

Removing attributes with upsert #339

Open
mikeyaa opened this issue Dec 21, 2023 · 9 comments
Open

Removing attributes with upsert #339

mikeyaa opened this issue Dec 21, 2023 · 9 comments

Comments

@mikeyaa
Copy link

mikeyaa commented Dec 21, 2023

Is it possible to remove attributes from a record while using upsert? I see update has set() and remove() but how would I go about upserting a record that removes some properties in one go? It seems it will only set attributes but not unset them.

@tywalch
Copy link
Owner

tywalch commented Dec 22, 2023

Definitely, I can't quite remember why I omitted that operation but I will take a look!

@mikeyaa
Copy link
Author

mikeyaa commented Dec 23, 2023

Thank you! Your library has been very helpful to far!

@rangoc
Copy link

rangoc commented May 1, 2024

@tywalch

We've encountered the same issue, just today, as the OP has posted about.

Upon reading the documentation, we first thought that the update() was what we were looking for, as it had remove():

The DynamoDB method update() will create an item if one does not exist. Because updates have reduced attribute validations when compared to put, the practical ramifications of this is that an update can create a record without all the attributes you’d expect from a newly created item.

And also, based on this, we thought that if there is no record in the db, new one will be created. That is true, BUT... it gets created without _created_at field, even though on the entity we have this:

 _created_at: {
            type: 'string',
            required: true,
            readOnly: true,
            set: () => new Date().toISOString(),
            default: () => new Date().toISOString(),
},

So that's where we hit a roadblock.

Then we came across upsert() which does the job, but it lacks the ability to remove fields, as the OP says. And we need to have that ability.

Definitely, I can't quite remember why I omitted that operation but I will take a look!

This brings some hope, are there any updates on this?

@tywalch
Copy link
Owner

tywalch commented May 1, 2024

This brings some hope, are there any updates on this?

Hi @rangoc

I definitely owe this ticket an update; thank you for weighing in as well, it always helps with prioritization to know when multiple experience the same scenario. I apologize @mikeyaa that I have kept you waiting.

I did start to look at this and found there was some hidden complexity with remove, namely when that property is a composite attribute. I see a path to adding it, though I think it's best to wrap this update into another request to be able to drop and reindex indexes because there is a lot of overlapping parts. I also found that currently the remove method needs a little bit of attention in this area when the attribute is a composite attribute to a key, so that will also need to be addressed.

I can't really commit right now to how long this might take, we our first baby in February and that has definitely slowed down my updates in the short term. That said, I fold this need into the drop/reindex addition I think I can make faster progress.

In the meantime, you might be able to get around this by avoiding the readOnly: true constraint at least temporarily and enforcing that within the app code that accesses the database. If that doesn't work maybe we could brainstorm some other way to work around this until a fix is in?

@rangoc
Copy link

rangoc commented May 1, 2024

@tywalch Thanks for such a rapid feedback 🥇 And also congratulations on the baby! :)

Our _created_at is not a part of the composite, so that's one less constraint to worry about for now I guess :)

I'll try with the workaround, and see where it gets me! :) Thanks!

@rangoc
Copy link

rangoc commented May 1, 2024

@tywalch

Hmm... just now tested the workaround, doesn't seem to have any effect.

How we decided to solve this for the time being, is by doing an additional query, querying for an item, and checking whether it already exists.

If it does, we do an update(), if not we do create(). It does cost us that one additional query, but oh well, it provides some additional data safety and more peaceful mind :)

@tywalch
Copy link
Owner

tywalch commented May 1, 2024

I'll also add there is also a patch method which only updates the record if it exists (via a ConditionExpression). If you do move forward with a two-step operation I'd recommend using that method over update.

I don't know your architecture obviously, but if you believe the item exists at the time of write, a patch will ensure those cases are single-step and then you can always "fail back" to a create. Both those options are "safer" because they will enforce your expectations.

@rangoc
Copy link

rangoc commented May 2, 2024

@tywalch Sorry, just now seeing that you've replied. Thanks for trying to help! 🙌🏻

Here's the model:

export const UserDataEntity = makeOnce(
  () =>
    new Entity(
      {
        model: {
          service: 'user',
          entity: 'UserData',
          version: '1',
        },
        attributes: {
          user_id: {
            type: 'string',
            required: true,
          },
          first_name: {
            type: 'string',
            required: true,
          },
          last_name: {
            type: 'string',
            required: true,
          },
          country_id: {
            type: 'number',
            required: true,
          },
          organisation_title: {
            type: 'string',
          },
          job_level_id: {
            type: 'number',
            required: true,
          },
          job_function_id: {
            type: 'number',
            required: true,
          },
          job_industry_id: {
            type: 'number',
          },
          job_title: {
            type: 'string',
          },
          _updated_at: {
            type: 'string',
            readOnly: true,
            watch: '*',
            set: () => new Date().toISOString(),
          },
          _created_at: {
            type: 'string',
            required: true,
            readOnly: true,
            set: () => new Date().toISOString(),
            default: () => new Date().toISOString(),
          },
        },
        indexes: {
          byUserId: {
            pk: {
              field: 'pk',
              composite: ['user_id'],
            },
            // pk: $user#user_id_
            sk: {
              field: 'sk',
              composite: [],
            },
          },
        },
      },
      {
        identifiers: {
          entity: '_type',
          version: '_version',
        },
        // logger: console.log,
        client: ElectroDbClient().client,
        table: ElectroDbClient().tableName,
      } satisfies EntityConfiguration,
    ),
);

Here's the model.

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

3 participants