fix(core): Model#equals always returns true for instances of different models

[maksimf]

Summary

Fixes #18142.

• Model#equals was comparing this.modelDefinition against itself (copy-paste bug on line 4518), so the model-type guard was always a no-op
• Two instances of completely different models with the same primary key would incorrectly be considered equal
• Fix: change const otherModelDefinition = this.modelDefinition → const otherModelDefinition = other.modelDefinition
• Adds a dedicated unit test suite for Model#equals covering same-model, cross-model, and non-Model comparisons

Regression note

This is a recurring regression in equals() — the same class of bug was previously reported and fixed in:

• Issue fix DAO.prototype.equals #494 — fix DAO.prototype.equals (2013)
• PR Prove that Instance.equals() still doesn't work and fix it #5605 — Prove that Instance.equals() still doesn't work and fix it (merged 2016)

The current regression was introduced when the codebase was refactored to use modelDefinition.

Test plan

• New unit test packages/core/test/unit/instance/equals.test.js covers:
  • Same instance compared to itself → true
  • Two instances of the same model with identical PK → true
  • Two instances of the same model with different PKs → false
  • Instances of different models with the same PK → false (was broken)
  • Comparing to null, undefined, plain object → false

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed instance equality logic so comparisons use the other instance’s model data, ensuring distinct models and records are correctly distinguished and preventing incorrect matches across model types or identical-looking records.

• Tests

  • Added unit tests covering equality cases: same instance, different instances with same/different primary keys, cross-model comparisons, and comparisons against non-model values to prevent regressions.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Fixed a copy‑paste bug in Model#equals: the method now compares other.modelDefinition (not this.modelDefinition) so cross‑model comparisons correctly return false; added unit tests covering self, same‑model, cross‑model, and non‑Model comparisons.

Changes

Cohort / File(s)	Summary
Core logic packages/core/src/model.js	Replace incorrect assignment of otherModelDefinition = this.modelDefinition with other.modelDefinition to enable the cross‑model mismatch guard.
Unit tests packages/core/test/unit/instance/equals.test.ts	Add tests for Model#equals(): self comparison, same‑model equal/different PKs, cross‑model same PK, and comparisons against non‑Model values.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through lines of code at night,
Found a twin that looked just right.
I nudged “other” into its place,
Now equals sees each model's face. 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the bug fix: Model#equals returning true for different models when comparing instances with the same PK.
Linked Issues check	✅ Passed	The PR fully addresses all objectives from #18142: fixes the copy-paste bug by reassigning otherModelDefinition to other.modelDefinition, adds comprehensive unit tests covering all required scenarios.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the Model#equals bug and adding tests; no unrelated modifications are present.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[WikiRik]

Good catch!

[coderabbitai]

🧹 Nitpick comments (1)

packages/core/test/unit/instance/equals.test.ts (1)

8-14: Declare id attribute for TypeScript type safety.

The models don't declare the id attribute, but it's used in build() calls throughout the tests. While Sequelize adds id as the default primary key at runtime, InferAttributes only picks up explicitly declared properties, so TypeScript won't recognize id in the build options.

♻️ Suggested fix

-    class User extends Model<InferAttributes<User>, InferCreationAttributes<User>> {
-      declare username: string;
-    }
-
-    class Project extends Model<InferAttributes<Project>, InferCreationAttributes<Project>> {
-      declare title: string;
-    }
+    class User extends Model<InferAttributes<User>, InferCreationAttributes<User>> {
+      declare id: number;
+      declare username: string;
+    }
+
+    class Project extends Model<InferAttributes<Project>, InferCreationAttributes<Project>> {
+      declare id: number;
+      declare title: string;
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/test/unit/instance/equals.test.ts` around lines 8 - 14, The
tests use build() with an id property but the Model subclasses (User and
Project) don't declare id, so TypeScript won't recognize it; add an explicit id
declaration (e.g. declare id?: number) to both class User and class Project
declarations so InferAttributes includes id for build/create options and other
usages.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/core/test/unit/instance/equals.test.ts`:
- Around line 8-14: The tests use build() with an id property but the Model
subclasses (User and Project) don't declare id, so TypeScript won't recognize
it; add an explicit id declaration (e.g. declare id?: number) to both class User
and class Project declarations so InferAttributes includes id for build/create
options and other usages.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: da004d41-58aa-4b28-a11b-f4a05fad6fdb

📥 Commits

Reviewing files that changed from the base of the PR and between 69517b8 and a69f5ce.

📒 Files selected for processing (1)

• packages/core/test/unit/instance/equals.test.ts

[WikiRik]

I approved too early, can you fix the CI issues? For the failing test; Model#equals is used in the following snippet

sequelize/packages/core/src/associations/has-one.ts

Lines 298 to 304 in 9a972bb

	const alreadyAssociated =
	!oldInstance || !associatedInstanceOrPk
	? false
	: associatedInstanceOrPk instanceof Model
	? associatedInstanceOrPk.equals(oldInstance)
	: // @ts-expect-error -- TODO: what if the target has no primary key?
	oldInstance.get(this.target.primaryKeyAttribute) === associatedInstanceOrPk;

That also seems to be the only place that we use it internally. Model#equalsOneOf isn't used

[maksimf]

I approved too early, can you fix the CI issues? For the failing test; Model#equals is used in the following snippet

sequelize/packages/core/src/associations/has-one.ts

Lines 298 to 304 in 9a972bb

	const alreadyAssociated =
	!oldInstance || !associatedInstanceOrPk
	? false
	: associatedInstanceOrPk instanceof Model
	? associatedInstanceOrPk.equals(oldInstance)
	: // @ts-expect-error -- TODO: what if the target has no primary key?
	oldInstance.get(this.target.primaryKeyAttribute) === associatedInstanceOrPk;

That also seems to be the only place that we use it internally. Model#equalsOneOf isn't used

Sure, tried to fix 'em on the go, but id didn't fly. I will do it properly once I'm back to my computer and push 🫡