blazejkustra / dynamode

Dynamode is a modeling tool for Amazon's DynamoDB
MIT License
62 stars 2 forks source link

This is not a bug but rather a question about update vs put #22

Closed gurkerl83 closed 8 months ago

gurkerl83 commented 8 months ago

Executing a update operation it seems knowing which field to update is the way to go right e.g.

// Appends a single collaborator to the existing list of collaborators
export async function addCollaborator(experimentId: string, collaboratorId: string) {
  // Prepare update properties to append a collaborator to the list
  const updateProps = {
    listAppend: {
      collaborators: [collaboratorId],
    },
  };

  // Perform the update operation
  await ExperimentEntityManager.update({ id: experimentId }, updateProps);
}

On the other hand we have situations where it is unclear what fields to update, one or more, maybe values are undefined.

export const updateExperiment = async (experiment: Experiment) => {
  console.log('updateExperiment', experiment);
  try {
    const updatedExperiment = await ExperimentEntityManager.put(experiment as ExperimentEntity);
    return updatedExperiment;
  } catch (err) {
    const dbError = new DatabaseError('Experiment', 'update', experiment.id, { error: err as Error });
    console.log(dbError);
    throw dbError;
  }
};

Is my understanding correct that we can leverage the put operation then? Digging a bit the code I found the behavior of put uses the following helper.

For reference see the option for marshal removeUndefinedValues: true https://github.com/blazejkustra/dynamode/blob/08d4ae449ae74e038f7e6cd9e8e4555f556ddbba/lib/utils/converter.ts#L5

For explanation see https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-aws-sdk-util-dynamodb/Interface/marshallOptions/

Thx!

blazejkustra commented 8 months ago

Hi! Under the hood these methods fire different dynamodb commands:

Above are fundamentally different but yes, both should get the job done if you want to update a certain item

First of all I think you shouldn't use assertion (as ExperimentEntity) in your code, instead provide a valid type for the experiment argument

export const updateExperiment = async (experiment: typeof ExperimentEntity) => {
  console.log('updateExperiment', experiment);
  try {
    const updatedExperiment = await ExperimentEntityManager.put(experiment);
    return updatedExperiment;
  } catch (err) {
    const dbError = new DatabaseError('Experiment', 'update', experiment.id, { error: err as Error });
    console.log(dbError);
    throw dbError;
  }
};

And call this function like this:

updateExperiment(new ExperimentEntity({...experimentValues}))
blazejkustra commented 8 months ago

You have to know that updating an item with EntityManager.put might be a bad idea if you are updating experiments frequently as this may lead to some race condition and you may lose data.

Let's say you have a function that updates this experiment and pushes new collaborators to it, and there are two updates running at the same time:

initial data:

Experiment: {id: 1, collaborators: ["0"] }

Running threads:

THREAD1: Experiment.get()
THREAD2: Experiment.get()
THREAD1: Experiment.put(new ExperimentEntity({ ...initialData, collaborators: [...initialData, "1"] }))
THREAD2: Experiment.put(new ExperimentEntity({ ...initialData, collaborators: [...initialData, "2"] }))

The item will end up as:

Experiment: {id: 1, collaborators: ["0", "2"] }

While the correct value should be:

Experiment: {id: 1, collaborators: ["0", "1", "2"] }

With update there is no such problem as you don't need to fetch the item first

blazejkustra commented 8 months ago

You can update fields dynamically with update, I did this in one of my project:

if (status !== oldStatus) {
  await ListManager.update(List.getPrimaryKey(listId), {
    increment: {
      [`progress.${status}`]: 1,
    },
    decrement: {
      [`progress.${oldStatus}`]: 1,
    },
  });
}

Where List is defined as following:

type ListStatus = 'active' | 'deleted';

type ListProgress = {
  unchecked: number;
  checked: number;
  unavailable: number;
};

type ListProps = UserTableProps & {
  name: string;
  status: ListStatus;
  ownerUsername: string;
  progress: ListProgress;
};

export default class List extends UserTable {
  @attribute.string()
  listId: string;

  @attribute.string()
  name: string;

  @attribute.string()
  ownerUsername: string;

  @attribute.object()
  progress: ListProgress;

  @attribute.string()
  status: ListStatus;

  constructor(props: ListProps) {
    super(props);

    this.listId = props.pk;
    this.name = props.name;
    this.ownerUsername = props.ownerUsername;
    this.status = props.status;
    this.progress = props.progress;
  }

  static getPrimaryKey(listId: string): UserTablePrimaryKey {
    return {
      pk: listId,
      sk: List.name,
    };
  }
}
blazejkustra commented 8 months ago

@gurkerl83 I hope this helps!

gurkerl83 commented 8 months ago

@blazejkustra Hi thx for the quick response, for now we want to keep it really simple, so although put is not recommended updating frequently, we could use it when the object to update has potential undefined values on it right? The marshal option used in a helper of the put operation removeUndefinedValues: true will create a new object / expression without the keys which are currently undefined right?

blazejkustra commented 8 months ago

Yes it should strip down undefined values. Should be good enough for your application 👍 (just keep in mind that with each put you will completely overwrite existing experiment in the database)

gurkerl83 commented 8 months ago

@blazejkustra This is a stack trace of a collaborator. The code is using the put operation, there are undefined values should be handled. I do not really understand the stack trace in full. The message is kind of misleading

Pass options.removeUndefinedValues=true to remove undefined values from map/array/set.

It seems the option removeUndefinedValues is somehow not picked up.

We are running the following dynamo-db version

"@aws-sdk/client-dynamodb": "3.474.0",
"@aws-sdk/util-dynamodb": "3.474.0"

MicrosoftTeams-image

Maybe you have an idea, what is causing the problem.

Thx!

blazejkustra commented 8 months ago

Can you post a minimal code to reproduce it? Model and the way you call put method should be enough, thanks!

gurkerl83 commented 8 months ago

I will provide something tomorrow. Thx!

blazejkustra commented 8 months ago

I can reproduce it using this code (model code):

  const userUpdate = await UserManager.update(
    { partitionKey: 'pk1', sortKey: 'sk1' },
    {
      set: {
        age: undefined,
      },
      add: {},
    },
  );

Most likely he used update method instead of put: image

gurkerl83 commented 8 months ago

@blazejkustra Hi, sry for the confusion. The put operation works as expected, perfectly handling undefined values. I am so sorry, this was a problem on our side. Many thanks!