Updating a Model with custom validation bug

[gsqrt2]

Issue Creation Checklist

• I have read the contribution guidelines

Bug Description

When trying to Model.update() a model with custom validation, an error is thrown as the custom validator does not have access to instance fields.

SSCCE

**Here is the link to the SSCCE for this issue: https://github.com/gsqrt2/sequelize-update-validation-error

What do you expect to happen?

In regard to the SSCCE:

Connection.update( {
        approved: true
    }, {
    where: {
        approved: false
    }
})

should update Connections where approved===false to approved: true

What is actually happening?

The custom validator throws, as instance dataValues are undefined

ValidationError [SequelizeValidationError]:  Validation error: Cannot connect to self.

Additional context

The SSCCE is extremely basic, just a dummy Connection with a custom validator that ensures that the two user ids are not equal. Commenting out the validation restores normal functionality.

Environment

• Sequelize version: 6.6.5
• Node.js version: v12.19.0

Bug Report Checklist

How does this problem relate to dialects?

• I think this problem happens regardless of the dialect.
• I think this problem happens only for the following dialect(s):
• I don't know, I was using PUT-YOUR-DIALECT-HERE, with connector library version XXX and database version XXX

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.

[cathykc]

Also encountering this problem! Currently working around by using update on instance.

There's an old closed issue that seems related to this: #4447

[Kraicheck]

There's a related open issue as well: #12666

[github-actions]

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

[github-actions]

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

[gsqrt2]

Still an issue

[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. 🙂

[gsqrt2]

Still an issue

[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. 🙂

[gsqrt2]

If this is still an issue, just leave a comment or remove the "stale" label.

Is there a way I can remove the "stale" label myself instead of repeatedly commenting that it's still an issue?

[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. 🙂

[mypipsen]

Still relevant

[ephys]

This doesn't sound like a bug. Custom validators will be called every time a model will be updated - including when calling bulk updates.

Simply check whether the values your validator depend on are present before using them.

cannotConnectToSelf() {
  if (this.user1id === undefined || this.user2id === undefined) {
    return;
  }

  if (this.user1id === this.user2id) {
    throw new Error('Cannot connect to self.');
  }
},

You'd have the same issue if you selected a subset of your model's attributes in find then called save on the returned instance.

[Kraicheck]

@ephys, I don't understand how you can suggest simply not validating because what you're trying to validate isn't available, not because the values don't exist, but because Sequelize made an internal choice not to retrieve them?
What use are validators that don't always validate or come to wrong conclusions because of incomplete knowledge?

[ephys]

Because that would completely destroy your application's performance. It would require running a select during your bulk update, which could select a very large amount of data.

If your constraint validation depends on more than one field, using check constraints would be much safer and efficient.

If you really want to select data before updating it (which is going to have a large performance impact), you can also use the individualHooks option.

[Kraicheck]

individualHooks indeed seems like it would do the trick. Thanks!
I know this will impact performance but that's not really an issue in my application.

[Kraicheck]

@ephys, individualHooks actually doesn't have an impact on this.
I've tried and verified in the source code; validation happens on the generic version with the default values regardless of the value for indivualHooks.

[ephys]

I can't verify this right now but: is it possible that it's running the validation twice? Once on the object that's passed to Model.update and once for each model being updated?

[Kraicheck]

No, validation only runs once; for the object built from the passed properties.
Afterwards the individual objects are retrieved, the individual hooks are run and if those have a different impact for the different objects, update is run on those individual objects but validation is disabled as it's assumed to have already run.

[ephys]

I see. That could have been intended but if indivualHooks is used, I think it makes more sense to run validation on each instance one by one.

Changing it in v6 could break some users but we're actively working on v7 right now so if someone wants to open a PR to change the behavior in v7 (main branch), we can include it before the first stable release

[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. 🙂

[WikiRik]

@ephys can you add a label to this so the bot won't close this?