feat(core): add Model.findByPks() for batch primary key lookups#18128
feat(core): add Model.findByPks() for batch primary key lookups#18128topmonroe9 wants to merge 10 commits intosequelize:mainfrom
Conversation
Closes sequelize#11297 Add a new static method `findByPks` that accepts an array of primary key values and returns all matching instances. For single-column primary keys it generates a single `WHERE pk IN (...)` clause; for composite keys it builds `WHERE (pk1=v1 AND pk2=v2) OR ...`. This is a convenience wrapper around `findAll` that mirrors `findByPk` but for multiple identifiers at once, similar to Rails' `find` and TypeORM's `findByIds`.
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new static Model.findByPks method with input validation, support for simple and composite primary keys (IN for single-key, OR of ANDs for composite), a helper to build per-PK where clauses, type declarations, and unit tests; delegates to Model.findAll with merged where options. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant FinderMethod as Model.findByPks
participant Finder as Model.findAll
participant DB as Database
Caller->>FinderMethod: call findByPks(identifiers, options)
FinderMethod->>FinderMethod: validate identifiers, determine PK shape
alt single PK
FinderMethod->>Finder: findAll({ where: { pk: { [Op.in]: ids } }, ...options })
else composite PKs
FinderMethod->>Finder: findAll({ where: { [Op.or]: [ {pk1: v1, pk2: v2}, ... ] }, ...options })
end
Finder->>DB: execute query
DB-->>Finder: rows
Finder-->>FinderMethod: instances/raw rows
FinderMethod-->>Caller: return array of results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/test/unit/model/find-by-pks.test.ts (2)
116-127: Consider verifying the merged where clause structure.The test could be strengthened by verifying that the PK condition and the custom
whereclause are properly merged withOp.and.💡 Optional: Add merged where clause verification
await testModel.findByPks([1, 2], { where: { active: true } }); expect(findAllSpy.calledOnce).to.equal(true); + const callArgs = findAllSpy.getCall(0).args[0]; + expect(callArgs.where).to.have.property(Op.and);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/unit/model/find-by-pks.test.ts` around lines 116 - 127, Extend the test to assert that Model.findAll was called with a merged where containing the PK condition and the provided where under an Op.and; after stubbing Model.findAll in the test, check the spy's firstCall.args[0].where equals an object like { [Op.and]: [ { id: [1, 2] }, { active: true } ] } (import Sequelize.Op if needed) so testModel.findByPks and Model.findAll are verified to merge correctly.
49-69: Consider verifying the where clause structure for composite PKs.The test confirms
findAllis called, but doesn't verify the constructed where clause. Adding an assertion would improve test robustness.💡 Optional: Add where clause verification
await testModel.findByPks([ { pk1: 1, pk2: 10 }, { pk1: 2, pk2: 20 }, ]); expect(findAllSpy.calledOnce).to.equal(true); + const callArgs = findAllSpy.getCall(0).args[0]; + expect(callArgs.where).to.have.property(Op.or);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/unit/model/find-by-pks.test.ts` around lines 49 - 69, Add a strict assertion that the stubbed Model.findAll was invoked with the expected where clause for composite keys: inspect findAllSpy.firstCall.args[0].where and assert it equals an Op.or array of the provided PK objects (i.e. [{ pk1:1, pk2:10 }, { pk1:2, pk2:20 }]); reference the tested method testModel.findByPks and the stub findAllSpy, and import/usage of Sequelize.Op (or sequelize.Op) to construct/compare the expected where shape so the test verifies the exact query structure rather than only that findAll was called.
🤖 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/model/find-by-pks.test.ts`:
- Around line 116-127: Extend the test to assert that Model.findAll was called
with a merged where containing the PK condition and the provided where under an
Op.and; after stubbing Model.findAll in the test, check the spy's
firstCall.args[0].where equals an object like { [Op.and]: [ { id: [1, 2] }, {
active: true } ] } (import Sequelize.Op if needed) so testModel.findByPks and
Model.findAll are verified to merge correctly.
- Around line 49-69: Add a strict assertion that the stubbed Model.findAll was
invoked with the expected where clause for composite keys: inspect
findAllSpy.firstCall.args[0].where and assert it equals an Op.or array of the
provided PK objects (i.e. [{ pk1:1, pk2:10 }, { pk1:2, pk2:20 }]); reference the
tested method testModel.findByPks and the stub findAllSpy, and import/usage of
Sequelize.Op (or sequelize.Op) to construct/compare the expected where shape so
the test verifies the exact query structure rather than only that findAll was
called.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/model-typescript.tspackages/core/src/model.d.tspackages/core/test/unit/model/find-by-pks.test.ts
- Fix prettier formatting in test file - Verify Op.or is used in composite PK where clause - Verify Op.and is used when merging with existing where option
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/test/unit/model/find-by-pks.test.ts (1)
11-14: Add DB lifecycle hooks in this test suite.This file only restores Sinon in
afterEach; it does not follow the test lifecycle pattern for syncing and closing Sequelize instances, which can leak state between tests.As per coding guidelines "Use beforeEach/afterEach to sync with { force: true } and close Sequelize instances in tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/unit/model/find-by-pks.test.ts` around lines 11 - 14, Add test lifecycle hooks to ensure Sequelize is reset and closed: add a beforeEach that calls sequelize.sync({ force: true }) (or the test DB instance named in this file) to recreate tables before each test, and update the existing afterEach to first call sinon.restore() and then await sequelize.close() (or the file's DB instance close() method) to fully tear down the connection; keep the hook names as beforeEach/afterEach and await both sync({ force: true }) and close() so tests don't leak state between runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/test/unit/model/find-by-pks.test.ts`:
- Around line 15-22: The test should also assert that no DB call happens when
passing an empty array: stub or spy on the model's findAll (e.g.
sinon.stub(testModel, 'findAll') or sinon.spy) before calling
testModel.findByPks([]), call await testModel.findByPks([]), assert that
testModel.findAll was not called, and finally restore the stub/spy; reference
the testModel.findByPks and testModel.findAll symbols when making the change.
---
Nitpick comments:
In `@packages/core/test/unit/model/find-by-pks.test.ts`:
- Around line 11-14: Add test lifecycle hooks to ensure Sequelize is reset and
closed: add a beforeEach that calls sequelize.sync({ force: true }) (or the test
DB instance named in this file) to recreate tables before each test, and update
the existing afterEach to first call sinon.restore() and then await
sequelize.close() (or the file's DB instance close() method) to fully tear down
the connection; keep the hook names as beforeEach/afterEach and await both
sync({ force: true }) and close() so tests don't leak state between runs.
- Use type-safe patterns to avoid 'possibly undefined' errors on getCall().args indexing across TS 5.5-5.8 - Assert findAll is not called for empty identifiers array
|
I don’t really see a reason why this is needed above where: { Id: [1,2,3]} |
Yeah for a single PK that's basically the same thing. The bigger win is composite primary keys though, where: { id: [1,2,3] } doesn't really work when your PK is two columns. Also you don't have to hardcode the PK field name, so findByPks([1,2,3]) works whether the PK is id, uuid, whatever. Same idea behind why findByPk exists when you could just use findOne Honestly for the simple case it's just a convenience method, but #11297 had a bunch of people asking for it so figured it was worth adding |
|
Alright. I can see that use case, overall looks fine to me. ideally we would refactor the object.create(null) stuff into a function that either returns a created object or throws, so that but yeah. nice work. |
|
Thanks, yeah that made sense. I pulled the validation + where-clause stuff into a shared Pushed it just now |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/model-typescript.ts`:
- Around line 548-564: The findByPks implementation allows null/undefined
entries in the identifiers array which are passed into Op.in and can mask bugs;
update findByPks to mirror findByPk by rejecting nullish identifiers: in the
findByPks method add a guard after the early-empty check that iterates
identifiers (or uses identifiers.some(id => id == null)) and throws a
descriptive Error if any identifier is null or undefined, ensuring you still
reference identifiers, primaryKeyAttributeNames, and the existing Op.in logic
unchanged otherwise.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/test/unit/model/find-by-pks.test.ts (2)
9-13: Align test lifecycle hooks with repository test isolation rules.This suite currently restores Sinon only. Please align setup/teardown with the standard sync/close lifecycle pattern to avoid shared-state leakage across tests.
As per coding guidelines "Use beforeEach/afterEach to sync with { force: true } and close Sequelize instances in tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/unit/model/find-by-pks.test.ts` around lines 9 - 13, The test suite for findByPks currently only calls sinon.restore in afterEach; update the lifecycle to follow the repository test isolation pattern by adding a beforeEach that calls sync({ force: true }) on the Sequelize/model test DB and expanding afterEach to both restore sinon (sinon.restore()) and close the Sequelize connection (await sequelize.close() or the test helper close method). Locate the describe('findByPks', ...) block and modify the hooks around it (add beforeEach to perform sync({ force: true }) and change afterEach to restore sinon and close the Sequelize instance) so each test runs with a fresh DB and cleans up connections.
72-76: Strengthen assertions for generatedwhereclauses.Both tests only assert that
whereexists, so regressions inOp.or/Op.andcomposition could still pass.Proposed assertion tightening
- const where = findAllSpy.firstCall.args[0] as Record<string, unknown> | undefined; - expect(where).to.exist; + findAllSpy.should.have.been.calledWithMatch({ + where: { + [Op.or]: [ + { pk1: 1, pk2: 10 }, + { pk1: 2, pk2: 20 }, + ], + }, + }); @@ - const where = findAllSpy.firstCall.args[0] as Record<string, unknown> | undefined; - expect(where).to.exist; + findAllSpy.should.have.been.calledWithMatch({ + where: { + [Op.and]: [{ active: true }, { id: { [Op.in]: [1, 2] } }], + }, + });Also applies to: 149-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/unit/model/find-by-pks.test.ts` around lines 72 - 76, The test currently only checks that the captured `where` object exists (from `findAllSpy.firstCall.args[0]`), which is weak; update the assertions to verify the expected Op composition and contents explicitly: for single-table PK lookups assert `where[Op.or]` is an array of objects matching each primary key (check lengths and that each element has the expected key/value pairs), and for composite-PK cases assert `where[Op.or]` contains objects with `Op.and` arrays where each `Op.and` element matches each column/value pair; reference and update the assertions around `findAllSpy` / `findAllSpy.firstCall.args[0]` (the `where` variable) in both the block at lines ~72-76 and the similar block at ~149-152 to validate Op structure, array lengths, and exact key/value shapes rather than only existence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/model-typescript.ts`:
- Around line 606-614: The loop that builds pkWhere in model-typescript.ts
currently only checks for undefined and allows nulls (in the block iterating
primaryKeyAttributeNames), so update the guard to reject null as well: change
the check that inspects identifier[attributeName] to treat null and undefined as
missing (e.g., identifier[attributeName] == null or explicit === null || ===
undefined) and throw the same TypeError; ensure this logic is applied in the
same loop that assigns pkWhere[attributeName] = identifier[attributeName] so
composite key parts of null are refused (matching findByPks' nullish guard).
---
Nitpick comments:
In `@packages/core/test/unit/model/find-by-pks.test.ts`:
- Around line 9-13: The test suite for findByPks currently only calls
sinon.restore in afterEach; update the lifecycle to follow the repository test
isolation pattern by adding a beforeEach that calls sync({ force: true }) on the
Sequelize/model test DB and expanding afterEach to both restore sinon
(sinon.restore()) and close the Sequelize connection (await sequelize.close() or
the test helper close method). Locate the describe('findByPks', ...) block and
modify the hooks around it (add beforeEach to perform sync({ force: true }) and
change afterEach to restore sinon and close the Sequelize instance) so each test
runs with a fresh DB and cleans up connections.
- Around line 72-76: The test currently only checks that the captured `where`
object exists (from `findAllSpy.firstCall.args[0]`), which is weak; update the
assertions to verify the expected Op composition and contents explicitly: for
single-table PK lookups assert `where[Op.or]` is an array of objects matching
each primary key (check lengths and that each element has the expected key/value
pairs), and for composite-PK cases assert `where[Op.or]` contains objects with
`Op.and` arrays where each `Op.and` element matches each column/value pair;
reference and update the assertions around `findAllSpy` /
`findAllSpy.firstCall.args[0]` (the `where` variable) in both the block at lines
~72-76 and the similar block at ~149-152 to validate Op structure, array
lengths, and exact key/value shapes rather than only existence.
|
@WikiRik What are you thoughts on this? |
WikiRik
left a comment
There was a problem hiding this comment.
It's been a while since I looked at this code. I think in general this seems good. I have a few initial remarks and will do a better review later
| /** | ||
| * Search for multiple instances by their primary keys. | ||
| * | ||
| * Returns the models with matching primary keys. If a given primary key has no match, | ||
| * it is simply omitted from the result (no error is thrown). | ||
| * | ||
| * @param identifiers An array of primary key values. For composite keys, each element | ||
| * should be an object containing all primary key attributes. | ||
| * @param options find options | ||
| */ | ||
| static findByPks<M extends Model, R = Attributes<M>>( | ||
| this: ModelStatic<M>, | ||
| identifiers: readonly unknown[], | ||
| options: Omit<FindOptions<Attributes<M>>, 'raw'> & { raw: true }, | ||
| ): Promise<R[]>; | ||
| static findByPks<M extends Model>( | ||
| this: ModelStatic<M>, | ||
| identifiers: readonly unknown[], | ||
| options?: FindOptions<Attributes<M>>, | ||
| ): Promise<M[]>; | ||
|
|
There was a problem hiding this comment.
Do we need these if we have defined it in the model-typescript.ts as well?
There was a problem hiding this comment.
Yeah you're right, findByPk doesn't have one either so this was just unnecessary duplication. Removed it.
There was a problem hiding this comment.
We should add a few integration tests as well
There was a problem hiding this comment.
Added integration tests — basic lookups, composite PKs, empty input, and where merging.
| primaryKeyAttributeNames: SetView<string>, | ||
| identifier: unknown, | ||
| ): Record<string, unknown> { | ||
| const pkWhere: Record<string, unknown> = Object.create(null); |
There was a problem hiding this comment.
I'm not sure anymore but don't we have a constant in the utils that's basically a null object? I know we sure we have one for EMPTY_OBJECT
There was a problem hiding this comment.
Good catch, switched it to pojo() from @sequelize/utils.
|
lgtm. Didn't know about pojo(). will be useful for a couple other things! |
Summary
Closes #11297
Model.findByPks(identifiers, options?)— a convenience method for fetching multiple instances by their primary keys in a single queryWHERE pk IN (...)usingOp.inWHERE (pk1=v1 AND pk2=v2) OR (pk1=v3 AND pk2=v4) ...This mirrors conventions from other ORMs (Rails
find, TypeORMfindByIds) and avoids the boilerplate of manually constructingOp.inqueries for a very common use case.Changes
packages/core/src/model-typescript.ts— newfindByPksstatic methodpackages/core/src/model.d.ts— type declarations withrawoverloadpackages/core/test/unit/model/find-by-pks.test.ts— 9 unit tests covering single PK, composite PK, empty input, validation errors, where merging, and no-PK modelsTest plan
DIALECT=postgres yarn mocha "test/unit/model/find-by-pks.test.ts"— 9/9)findByPktests still pass (no regressions)tsc --noEmit)Summary by CodeRabbit
New Features
Tests