changed() improperly resets after save() if the value was changed while the INSERT INTO or UPDATE was still running

[TobiasWehrum]

Issue Creation Checklist

• I have read the contribution guidelines

Bug Description

Calling instance.save() will set the changed() status of all fields (if the save() triggered an INSERT INTO) or all changed fields (if the save() triggered an UPDATE) to false. If one of those field values is changed while instance.save() is still running, instance.save() will have save the old value, but will still set changed() to false after running, leaving an instance that is a) different from the database but b) has changed() === false, so it the change will be ignored on subsequent save() calls.

Example:

1. Create a new model instance.
2. instance.name = "initial name";
3. const insertIntoPromise = instance.save(); (without awaiting)
4. await waitUntilNextFrame();
5. instance.name = "new name";
6. await insertIntoPromise;

After the save() from step 3 completes we have the following state:

• Database: name === "initial name"
• instance: name === "new name", changed("name") === false (despite the name mismatch between instance and database)

SSCCE

Here is the link to the SSCCE for this issue: sequelize/sequelize-sscce#228

import { DataTypes, Model } from 'sequelize';
import { createSequelize6Instance } from '../setup/create-sequelize-instance';
import { expect } from 'chai';
import sinon from 'sinon';

// if your issue is dialect specific, remove the dialects you don't need to test on.
export const testingOnDialects = new Set(['mssql', 'sqlite', 'mysql', 'mariadb', 'postgres', 'postgres-native']);

// You can delete this file if you don't want your SSCCE to be tested against Sequelize 6

// Your SSCCE goes inside this function.
export async function run() {
  // This function should be used instead of `new Sequelize()`.
  // It applies the config for your SSCCE to work on CI.
  const sequelize = createSequelize6Instance({
    logQueryParameters: true,
    benchmark: true,
    define: {
      // For less clutter in the SSCCE
      timestamps: false,
    },
  });

  class Foo extends Model {
    declare name: string;
  }

  Foo.init({
    name: DataTypes.TEXT,
  }, {
    sequelize,
    modelName: 'Foo',
  });

  // You can use sinon and chai assertions directly in your SSCCE.
  const spy = sinon.spy();
  sequelize.afterBulkSync(() => spy());
  await sequelize.sync({ force: true });
  expect(spy).to.have.been.called;

  // Create a new instance with "initial name"
  const instance = new Foo({ name: "initial name" });

  // Start an INSERT INTO, but don't await for it yet
  const insertIntoPromise = instance.save();

  // Wait until next frame when the INSERT INTO has properly started (but not finished)
  await new Promise<void>(resolve => {
    setTimeout(() => resolve(), 0);
  });

  // Set the name to "new name" while the INSERT INTO is still running
  instance.name = "new name";

  // As expected: instance.changed("name") === true
  console.log({
    'instance.name': instance.name,
    'instance.changed("name")': instance.changed("name")
  });

  // Now we wait until the previous INSERT INTO is finished
  await insertIntoPromise;

  // Unexpectedly: instance.changed("name") === false (despite it being "new name" in this instance and "initial name" in the database)
  console.log({
    'instance.name': instance.name,
    'instance.changed("name")': instance.changed("name")
  });

  // Try to UPDATE with "new name" - but this doesn't do anything, because instance.changed("name") === false
  await instance.save();

  // The instance in memory has "new name"
  expect(instance.name).to.equal("new name");

  // The instance in the database unexpectedly still has "initial name"
  expect((await Foo.findOne())?.name).to.equal("new name"); // AssertionError: expected 'initial name' to equal 'new name'
}

What do you expect to happen?

Refering to my example above, I'd expect the following state after save() from step 3 completes:

• Database: name === "initial name"
• instance: name === "new name", changed("name") === true

What is actually happening?

After the save() from step 3 completes we have the following state:

• Database: name === "initial name"
• instance: name === "new name", changed("name") === false (despite the name mismatch between instance and database)

Additional context

Our app consists of a single server that has a lot of objects that are:

• very frequently edited
• expensive to read from a database because some contain very complex JSON object strings that have to be parsed and converted into objects via https://mobx-keystone.js.org

Because of these requirements, we opted to keep the whole server state in memory and frequently persist everything into the database via if (instance.changed()) { instance.save(); } for every object. That works rather well, but it also means that sometimes a change happens to an instance while instance.save() is running, triggering the bug that is described here and leading to that change never being written to the database.

Environment

• Sequelize version: 6.19.0
• Node.js version: 14.17.1
• Database & Version: MariaDB 10.4.18
• Connector library & Version: mysql2@2.2.5

Would you be willing to resolve this issue by submitting a Pull Request?

• Yes, I have the time and I know how to start.
• Yes, I have the time but I don't know how to start, I would need guidance.
• No, I don't have the time, although I believe I could do it if I had the time...
• No, I don't have the time and I wouldn't even know how to start.

I have checked the save() method in the sequelize source and I don't think I understand it well enough to properly fix this.

[ephys]

Sequelize resets the 'changed' state of its attributes after the query has completed (https://github.com/sequelize/sequelize/blob/main/src/model.js#L4165).

We could create a copy of dataValues at the start of the query then only reset the changed state, and data values returned from the database, of the ones that have not changed in the meantime.

This could also be fixed by the 'Detecting changes' part of this RFC.

A PR for this is welcomed

[github-actions]

This issue has been automatically marked as stale because it has been open for 14 days without activity. It will be closed if no further activity occurs within the next 14 days. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

[TobiasWehrum]

Yeah, creating a copy was also my idea, but I couldn't get it to work with all tests. Your "and data values returned from the database" gave me another idea I'd like to try. I'm not completely confident, considering how intricate the save() method is, but maybe it's worth getting looked over in a PR if I get the tests to go through!