feat: restrict instance methods if no primary key

[frostzt]

Pull Request Checklist

• Have you added new tests to prevent regressions?
• If a documentation update is necessary, have you opened a PR to the documentation repository?
• Did you update the typescript typings accordingly (if applicable)?
• Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
• Does the name of your PR follow our conventions?

Description Of Change

Instance methods such as save, update, and others will now throw an error if no primaryKey was passed by the user.

Closes: #14402
Closes #15113

Todo

• Integration tests for save and update are failing.

[ephys]

You actually discovered another issues related to PKs. They're closely related but not fully the same. Ok with fixing both in this PR? :)

[frostzt]

@ephys I would love to do it! Thanks for the review lemme get this resolved!

[WikiRik]

Few comments about the tests. There might be valid answers but this is what I noticed

[ephys]

There are failing tests related to upsert in the suite, I'll take a look when I can to see what it is about

[frostzt]

Alright for some reason the save, update, and restore in the integration tests are being fulfilled when I expect them to throw!?

[ephys]

Regarding the failing tests:

Unit Tests

Restore Test

The restore test is failing because it's calling save. Because it's a new model, save uses INSERT instead of UPDATE. Which means it doesn't call .where().

How to fix it:

• restore should call update instead of save
• update should throw if isNewRecord is true (can you add a unit test for that too?)
• The unit test should have isNewRecord set to false

The Upsert tests

I think upsert should be investigated deeper. Its current implementation sounds very strange to me. But let's keep that for its own PR.

In the meantime, instead of calling instance.where(), it could create the "where" object full of primary keys itself. Like what where() does but without throwing. It's a temporary measure until we can investigate upsert further.

Integration Tests

restore, update, save

None of these methods changes any value on the model (model is not deleted, restoring doesn't change the value of deletedAt), update and save don't change anything. The method aborts before it gets to .where().

How to solve this: we should call where() sooner in save() (remember: only in update mode, not in insert mode). I prefer the consistency of it throwing even if there is nothing to save.

[frostzt]

Thanks for the review @ephys I have updated the PR, I don't know if that's exactly what you mentioned, seems like its gonna take me some time understanding the code and the way the repo works haha!

[WikiRik]

@ephys ping

[ephys]

I've pushed a commit that should fix our unit tests. Let's see whether this passes now

@frostzt don't hesitate to review commit c5eb5e4

I added a bunch of TODOs because the way Model.upsert currently works is a bit unclean, but it's unrelated to your work

[frostzt]

Thank you @ephys

[ephys]

The failing test in postgres/mysql/mariadb/sqlite is an easy fix: this line must use create instead of build

The failing DB2/mssql should be fixed by changing this line

sequelize/src/model.js

Line 2583 in 2e8e13f

instance.where(false, true),

to instance.where(false, true) ?? {},

[frostzt]

@ephys I'll get onto this post this week. Actually just got a lot work.

[ephys]

Take your time, I took a month to get back to this 😅

[WikiRik]

Looks good to me. Not the biggest fan of how rejectedWith does not check if you've included the entire error message but that's not a big issue. The error messages we're testing for in this PR are specific enough

[ephys]

I don't mind personally, my goal using rejectedWith is just to check that the right error is being thrown, not an unrelated one.

made changes

[WikiRik]

Yeah, that goal is being fulfilled so we're all good

[WikiRik]

@ephys I'll leave merging this up to you in case you want to wait for the review of @frostzt

[frostzt]

Thank you @ephys and @WikiRik I'll check the commits since I wanna know why this was breaking. Just give me this week post that I'll be back!

[frostzt]

Hey @ephys I got it now, do you think it is good to go?

[ephys]

unless you see anything else, I think it's ready

[frostzt]

Awesome @ephys I don't really notice anything aside the TODOs which I think I can take on in other PRs!

[ephys]

@WikiRik I'm doing the merge (for the breaking changes commit message)