fix: Prevent set function to remove saved data when failing to update

[jctaveras]

Pull Request check-list

Please make sure to review and check all of these items:

• Does npm run test or npm run test-DIALECT pass with this change (including linting)?
• Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
• Have you added new tests to prevent regressions?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
• Did you update the typescript typings accordingly (if applicable)?
• Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Closes: #13149

These changes prevent the set function to delete the saved data when failing to update with, plush when receiving null it will return destroy the saved data without going through the whole function.

Tests cases

const user = await User.findByPk(1);
user.setTasks([1, 2]);
const tasks = await user.getTasks();
console.log(tasks); // Task[{ id: 1, ... }, { id: 2, ... }]

const user = await User.findByPk(1);
user.addTask(1);
user.setTasks([2]);
const tasks = await user.getTasks();
console.log(tasks); // Task[{ id: 2, ... }]

const user = await User.findByPk(1);
user.setTasks(null);
const tasks = await user.getTasks();
console.log(tasks); // Task[]

const user = await User.findByPk(1);
user.setTasks([1, 2]);
user.setTasks([UNKOWN_TASK_ID]);
const tasks = await user.getTasks();
console.log(tasks); // Task[{ id: 1, ... }, { id: 2, ... }]

[closingin]

Hello @jctaveras !

Sorry for being so late in testing this PR, but I can confirm that it fixes #13149. Thanks a lot for the investigation and the fix 👍

Considering that the issue can generate data loss, I think it would be worth creating a test for it to avoid future regression. What do you think?

[closingin]

Hey @allawesome497 @papb , would any of you be available to take over this PR ? 🙂

[jctaveras]

Hey @closingin sorry I didn't reply to your comment

Sorry for being so late in testing this PR, but I can confirm that it fixes #13149. Thanks a lot for the investigation and the fix +1 Considering that the issue can generate data loss, I think it would be worth creating a test for it to avoid future regression. What do you think?

I'll take a look at those test soon 😉

[closingin]

Hello @jctaveras , any updates on this ? 🙂

[jctaveras]

@closingin sorry for the long delay.

I was checking at the comment you left regarding test on data loss, but I don't think I quite get it 🤔 as far as I can see everything is covered with the current tests, plus I make the expected changes to the tests so they go according the changes that I've introduced.

[closingin]

Well if it's good for you then it's good for me! We need someone to review the PR though.