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

Virtual attribute set function can't always get other attributes #348

Open
3 of 6 tasks
JoakimFFCG opened this issue Nov 16, 2022 · 3 comments
Open
3 of 6 tasks

Virtual attribute set function can't always get other attributes #348

JoakimFFCG opened this issue Nov 16, 2022 · 3 comments

Comments

@JoakimFFCG
Copy link

Issue Creation Checklist

  • I understand that my issue will be automatically closed if I don't fill in the requested information
  • I have read the contribution guidelines

Bug Description

In a set function on a virtual attribute you can't always get other attributes.
Depending on how the set is triggered the specifics differ. But for example, with Model.update() the set doesn't work properly when the attributes it uses aren't in the 'values' argument (and only if the attributes are specified before the virtual attribute).
See the examples for some specific cases.

Reproducible Example

Here is the link to the SSCCE for this issue: https://github.com/sequelize/sequelize-sscce
Also, a similar example not based on sequelize-sscce: https://github.com/JoakimFFCG/sequelize-demo

(I first did the example without sequelize-sscce, so might as well link that too. Also, it runs all the test cases, not stopping at the first one that fails.)

Example code from https://github.com/JoakimFFCG/sequelize-demo/blob/main/index.js

// Works fine
user.firstName = 'Firstname';
user.setNote = 'Yet other note';
user = await user.save();
console.log(user.note);       // It's "Firstname: Yet other note"

// Doesn't work
user.setNote = 'Note!';
user.firstName = 'FN';
user = await user.save();
console.log(user.note);       // It's "Firstname: Note!"
console.log(user.firstName);  // It's "FN"

What do you expect to happen?

In a set function on a virtual attribute you should always be able to get other attributes.

What is actually happening?

In a set function on a virtual attribute you can't always get other attributes (they are undefined). See the code examples for specifics.

Environment

  • Sequelize version: 6.25.6 & 7.0.0-alpha.10
  • Node.js version: v18.12.1
  • If TypeScript related: TypeScript version: Not typescript related
  • Database & Version: Shouldn't be database related
  • Connector library & Version: Shouldn't be database related

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 will need guidance.
  • No, I don't have the time, but my company or I are supporting Sequelize through donations on OpenCollective.
  • No, I don't have the time, and I understand that I will need to wait until someone from the community or maintainers is interested in resolving my issue.

Indicate your interest in the resolution of this issue by adding the 👍 reaction. Comments such as "+1" will be removed.

@ephys
Copy link
Member

ephys commented Nov 17, 2022

I'm sorry to say none of these are bugs. It's just how setters work

user.setNote = 'Note!';
user.firstName = 'FN';
user = await user.save();
console.log(user.note);       // It's "Firstname: Note!"
console.log(user.firstName);  // It's "FN"

In this example, when the setNote setter is called, firstName hasn't been updated yet, so the setter can only access the old value of firstName. You'd have the same result with plain JavaScript setters (which these basically are)

await User.update({
  setNote: 'My note'
}, {
  where: { id: user.id }
});
user = await User.findOne();
console.log(user.note);       // It's "undefined: My note"
console.log(user.firstName);  // It's "Another"

This is normal too. Model.update doesn't fetch all matching entities before updating them. That would defeat the purpose of this method, which is to execute a single update query to update one of more rows.
As it doesn't fetch anything, the setter of setNote can only access the other properties that are present in the object (none, in this case).

// Doesn't work
await User.update({
  setNote: 'My note 2',
  firstName: user.firstName,
}, {
  where: { id: user.id }
});

This one is weirder but also "normal".
Properties are assigned to the temporary model one by one. setNote is assigned first, which triggers it's setter. It tries to get firstName but it's not available yet.

Best we could do here is assign virtual properties after everything else. But you'd still have to worry about the execution order of the setters themselves.

Again all of these issues aren't really sequelize issues. They're just the sort of issues that happen when you try to access other properties in a setter (including plain JavaScript setters). You're going to have unintuitive behaviors.

Final thoughts: accessing a property like this in your setter is a bad idea. You should probably avoid it completely, and explore other options to achieve your goal.

@JoakimFFCG
Copy link
Author

@ephys

Final thoughts: accessing a property like this in your setter is a bad idea. You should probably avoid it completely, and explore other options to achieve your goal.

Well, then the documentation probably shouldn't do it:

const User = sequelize.define('user', {
  username: DataTypes.STRING,
  password: {
    type: DataTypes.STRING,
    set(value) {
      // Storing passwords in plaintext in the database is terrible.
      // Hashing the value with an appropriate cryptographic hash function is better.
      // Using the username as a salt is better.
      this.setDataValue('password', hash(this.username + value));
    }
  }
});

If setters really are supposed to work like just plain JavaScript setters, I really think that has to be explained in the documentation.
Highlighting that it's possible to get other attributes in the setter function, and not include a warning about it, implies that other attributes are pretty much always accessible (at least that's how I interpreted it).

@ephys
Copy link
Member

ephys commented Nov 18, 2022

I agree, that must be changed & warned about. It's too bug prone to let people know about it (even worse that it's including an example of it), I'll try to include it in our rewrite

@ephys ephys transferred this issue from sequelize/sequelize Nov 19, 2022
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