perf(orm): batch many-to-many relation manipulation#2486
Conversation
📝 WalkthroughWalkthroughThe ORM client's many-to-many relation handler was refactored from per-item single-entity processing to batch operations. Changes include parameter signature updates (single entity to array), return type modification (UpdateResult/DeleteResult to void), bulk insert/delete logic replacing iterative operations, and introduction of a batch ID-fetching helper method. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
Claude finished @ymc9's task —— View job Code Review: Performance optimization for many-to-many relation manipulationReview Tasks
SummaryThis PR introduces a significant performance optimization for many-to-many relation operations by batching database queries instead of executing them individually. The changes are well-implemented and follow good practices. ✅ StrengthsPerformance Improvements
Code Quality
🔍 Technical AnalysisKey Changes
Security Considerations✅ No security concerns identified
🎯 Specific Feedback
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/orm/src/client/crud/operations/base.ts (1)
2048-2061:⚠️ Potential issue | 🟠 MajorPerformance issue:
disconnect: trueon M2M fetches all entities unnecessarily.When
data === true,disconnectConditionsbecomes[true], andgetEntitiesIds(model, [true])attempts to fetch entities matching{ OR: [true] }. This likely fetches all entities from the model, which is:
- Potentially very slow for large tables
- Semantically incorrect - "disconnect all" should only affect entities connected to this parent, not query all entities in the model
The existing
resetManyToManyRelationmethod (lines 710-722) already handles this case efficiently by deleting directly from the join table without fetching entity IDs.💡 Suggested fix
if (m2m) { + // Handle "disconnect all" case efficiently + if (disconnectConditions.length === 1 && disconnectConditions[0] === true) { + await this.resetManyToManyRelation(kysely, fromRelation.model, fromRelation.field, fromRelation.ids); + return; + } + // handle many-to-many relation: batch fetch all entity IDs in one query const allIds = await this.getEntitiesIds(kysely, model, disconnectConditions);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/crud/operations/base.ts` around lines 2048 - 2061, The current M2M disconnect branch should avoid calling getEntitiesIds when data === true (or disconnectConditions indicate "disconnect all"); instead detect that case and use the existing resetManyToManyRelation logic to remove join-table links directly (so only relationships tied to the parent are affected and you avoid scanning the whole target table). Update the branch that currently calls getEntitiesIds(...) and handleManyToManyRelation(...) to short-circuit to resetManyToManyRelation(...) when fromRelation.data === true (or when disconnectConditions is the "all" sentinel), leaving the existing path for filtered disconnects that still need getEntitiesIds; reference getEntitiesIds, handleManyToManyRelation, and resetManyToManyRelation to locate and apply the change.
🤖 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/orm/src/client/crud/operations/base.ts`:
- Around line 2145-2161: The length check after fetching IDs can falsely trigger
createNotFoundError when _data contains duplicate filters; before calling
getEntitiesIds and comparing lengths, deduplicate the input filters used for
lookup (the _data array) or derive a unique key set from _data so you compare
the number of unique lookup inputs to the returned allIds; update the block
around getEntitiesIds and the subsequent allIds.length !== _data.length check in
the same scope where getEntitiesIds, handleManyToManyRelation, and fromRelation
are used (i.e., in the method containing getEntitiesIds and
handleManyToManyRelation) so you validate against unique inputs rather than raw
_data duplicates.
- Around line 1921-1936: The length check using allIds vs _data can fail when
_data contains duplicate filters because getEntitiesIds returns unique IDs;
update the logic around getEntitiesIds, allIds and the comparison to account for
duplicate filters by either deduplicating _data (or its filter keys/values)
before calling getEntitiesIds or by comparing allIds.length to the count of
unique filters instead of _data.length, and only call createNotFoundError if
there are missing unique entities; adjust the flow around getEntitiesIds,
allIds, createNotFoundError and subsequent handleManyToManyRelation accordingly
so duplicates are handled consistently (or explicitly reject duplicates with a
clear error).
---
Outside diff comments:
In `@packages/orm/src/client/crud/operations/base.ts`:
- Around line 2048-2061: The current M2M disconnect branch should avoid calling
getEntitiesIds when data === true (or disconnectConditions indicate "disconnect
all"); instead detect that case and use the existing resetManyToManyRelation
logic to remove join-table links directly (so only relationships tied to the
parent are affected and you avoid scanning the whole target table). Update the
branch that currently calls getEntitiesIds(...) and
handleManyToManyRelation(...) to short-circuit to resetManyToManyRelation(...)
when fromRelation.data === true (or when disconnectConditions is the "all"
sentinel), leaving the existing path for filtered disconnects that still need
getEntitiesIds; reference getEntitiesIds, handleManyToManyRelation, and
resetManyToManyRelation to locate and apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55ed4aef-1afc-440d-ad84-24a0699b14a1
📒 Files selected for processing (1)
packages/orm/src/client/crud/operations/base.ts
| // handle many-to-many relation: batch fetch all entity IDs in one query | ||
| const allIds = await this.getEntitiesIds(kysely, model, _data); | ||
| if (allIds.length !== _data.length) { | ||
| throw createNotFoundError(model); | ||
| } | ||
| await this.handleManyToManyRelation( | ||
| kysely, | ||
| 'connect', | ||
| fromRelation.model, | ||
| fromRelation.field, | ||
| fromRelation.ids, | ||
| m2m.otherModel!, | ||
| m2m.otherField!, | ||
| allIds, | ||
| m2m.joinTable, | ||
| ); |
There was a problem hiding this comment.
Duplicate filters may cause false NotFoundError.
The validation at line 1923 compares allIds.length !== _data.length, but getEntitiesIds uses an OR query that returns unique entities. If _data contains duplicate filters pointing to the same entity, the count won't match even though all entities exist, causing a spurious NotFoundError.
Consider deduplicating _data before the comparison, or comparing against unique filter count:
💡 Suggested approach
if (m2m) {
// handle many-to-many relation: batch fetch all entity IDs in one query
const allIds = await this.getEntitiesIds(kysely, model, _data);
- if (allIds.length !== _data.length) {
+ // Note: OR query returns unique results, so duplicates in _data are implicitly deduplicated
+ // We should validate against the unique count
+ const uniqueFilterCount = new Set(_data.map((d) => JSON.stringify(d))).size;
+ if (allIds.length !== uniqueFilterCount) {
throw createNotFoundError(model);
}Alternatively, if duplicates in the input should be rejected, consider throwing a more descriptive error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // handle many-to-many relation: batch fetch all entity IDs in one query | |
| const allIds = await this.getEntitiesIds(kysely, model, _data); | |
| if (allIds.length !== _data.length) { | |
| throw createNotFoundError(model); | |
| } | |
| await this.handleManyToManyRelation( | |
| kysely, | |
| 'connect', | |
| fromRelation.model, | |
| fromRelation.field, | |
| fromRelation.ids, | |
| m2m.otherModel!, | |
| m2m.otherField!, | |
| allIds, | |
| m2m.joinTable, | |
| ); | |
| // handle many-to-many relation: batch fetch all entity IDs in one query | |
| const allIds = await this.getEntitiesIds(kysely, model, _data); | |
| // Note: OR query returns unique results, so duplicates in _data are implicitly deduplicated | |
| // We should validate against the unique count | |
| const uniqueFilterCount = new Set(_data.map((d) => JSON.stringify(d))).size; | |
| if (allIds.length !== uniqueFilterCount) { | |
| throw createNotFoundError(model); | |
| } | |
| await this.handleManyToManyRelation( | |
| kysely, | |
| 'connect', | |
| fromRelation.model, | |
| fromRelation.field, | |
| fromRelation.ids, | |
| m2m.otherModel!, | |
| m2m.otherField!, | |
| allIds, | |
| m2m.joinTable, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/orm/src/client/crud/operations/base.ts` around lines 1921 - 1936,
The length check using allIds vs _data can fail when _data contains duplicate
filters because getEntitiesIds returns unique IDs; update the logic around
getEntitiesIds, allIds and the comparison to account for duplicate filters by
either deduplicating _data (or its filter keys/values) before calling
getEntitiesIds or by comparing allIds.length to the count of unique filters
instead of _data.length, and only call createNotFoundError if there are missing
unique entities; adjust the flow around getEntitiesIds, allIds,
createNotFoundError and subsequent handleManyToManyRelation accordingly so
duplicates are handled consistently (or explicitly reject duplicates with a
clear error).
| if (_data.length > 0) { | ||
| // batch fetch all entity IDs in one query | ||
| const allIds = await this.getEntitiesIds(kysely, model, _data); | ||
| if (allIds.length !== _data.length) { | ||
| throw createNotFoundError(model); | ||
| } | ||
| results.push( | ||
| await this.handleManyToManyRelation( | ||
| kysely, | ||
| 'connect', | ||
| fromRelation.model, | ||
| fromRelation.field, | ||
| fromRelation.ids, | ||
| m2m.otherModel, | ||
| m2m.otherField, | ||
| ids, | ||
| m2m.joinTable, | ||
| ), | ||
| await this.handleManyToManyRelation( | ||
| kysely, | ||
| 'connect', | ||
| fromRelation.model, | ||
| fromRelation.field, | ||
| fromRelation.ids, | ||
| m2m.otherModel, | ||
| m2m.otherField, | ||
| allIds, | ||
| m2m.joinTable, | ||
| ); |
There was a problem hiding this comment.
Same duplicate filter issue as connectRelation.
The validation at line 2148 (allIds.length !== _data.length) has the same potential issue with duplicate filters causing false NotFoundError. Apply the same fix as suggested for connectRelation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/orm/src/client/crud/operations/base.ts` around lines 2145 - 2161,
The length check after fetching IDs can falsely trigger createNotFoundError when
_data contains duplicate filters; before calling getEntitiesIds and comparing
lengths, deduplicate the input filters used for lookup (the _data array) or
derive a unique key set from _data so you compare the number of unique lookup
inputs to the returned allIds; update the block around getEntitiesIds and the
subsequent allIds.length !== _data.length check in the same scope where
getEntitiesIds, handleManyToManyRelation, and fromRelation are used (i.e., in
the method containing getEntitiesIds and handleManyToManyRelation) so you
validate against unique inputs rather than raw _data duplicates.
1 participant
Summary by CodeRabbit