fix(delegate): support _count select of base fields #1937
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the delegate model's query handling, specifically in the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/runtime/src/enhancements/node/delegate.ts (4)
183-196: Improve maintainability of unified selector pattern.This set of array-based selectors for handling normal and count-based selections is clear and modular. As a good-to-have improvement, consider extracting these objects into well-named constants or small helper functions to avoid inlining potentially repetitive logic in other areas.
204-231: Potential improvement for unrecognized fields.Looping through
Object.entries(data)to resolve inherited fields is correct. However, if there is any chance of unexpected or typo-ed fields, consider adding a warning or logging statement whenresolveFieldreturnsundefined. This would help surface subtle data payload issues more proactively.
245-245: Avoid thedeleteoperator for performance reasons.Static analysis warns of performance overhead when using the
deleteoperator. Although semantically correct here, consider setting the property toundefinedor usingReflect.deleteProperty(args[kind], '_count')if truly necessary to remove the property.Below is one possible approach:
- delete args[kind]['_count']; + args[kind]['_count'] = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 245-245: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
310-311: Parameter naming consistency.The parameter
forCountis descriptive, but matching similar naming (e.g.,isCount) might enhance overall code consistency and reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime/src/enhancements/node/delegate.ts(3 hunks)tests/regression/tests/issue-1467.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/runtime/src/enhancements/node/delegate.ts
[error] 245-245: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (6)
packages/runtime/src/enhancements/node/delegate.ts (3)
198-202: Guard checks appear solid.Early returns (skipping processing when the data object doesn't exist) help keep conditional logic concise and prevent errors. This approach looks good.
233-253: Well-handled base-field injection.Conditionally injecting base field selections is a neat approach. The fallback logic ensures that if the field is inherited from a base model, it’s properly routed. The code accomplishes the PR’s objective to support base field counting.
🧰 Tools
🪛 Biome (1.9.4)
[error] 245-245: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
338-351: Robust deepmerge usage.Merging
_countobjects (line 345-348) is careful and ensures multiple_countspecifications are correctly unified. Great job accommodating partial merges.tests/regression/tests/issue-1467.test.ts (3)
3-33: Well-organized schema definition.The inheritance from
DrinkinBeerand usage of@@delegate(type)effectively tests the base field handling. Very clear test structure for verifying delegated model logic.
38-43: Correctly creating test data.Creating a
Beerinstance and linking multipleContainerrecords to that instance thoroughly sets up the scenario needed to test_countbehaviors on inherited models.
45-49: Good validation of the_countfix.Selecting
_counton thebeermodel and verifying the container count addresses the core issue reported in #1467. This ensures that base fields and count logic behave as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (1)
434-438: Optional: Consider verifying additional relation counts.You're verifying both B and C counts at the base level. It might be worth also verifying the
cscount at the A2 level for completeness, though this is not strictly necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build-test (20.x)
- GitHub Check: dependency-review
- GitHub Check: build-test (20.x)
- GitHub Check: OSSAR-Scan
- GitHub Check: build-test (20.x)
🔇 Additional comments (3)
tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (3)
382-418: Schema definitions look clean and consistent.The nested inheritance structure (
A→A1→A2) and multiple delegate definitions (@@delegate(type),@@delegate(type1)) are logically consistent. The presence ofBandCmodels referencingAis well-structured for testing the relation counts.
421-425: Great use of _count in creation.Leveraging Prisma’s
_countselect immediately upon record creation is a solid approach to verify the relation counts (bs) in a single query.
427-432: Effective check of relation counts for nested records.Verifying the count fields (
bs) returned fromfindFirstensures that the_countlogic is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/runtime/src/enhancements/node/delegate.ts (3)
193-210: Good separation of concerns using selector functions.The implementation cleanly separates different selection cases using an array of selector functions, making the code more modular and easier to maintain.
Consider extracting these selectors into a separate constant or class property to improve reusability and testability.
+private readonly selectors = [ + (payload: any) => ({ data: payload.select, kind: 'select' as const, isCount: false }), + // ... other selectors +];
330-331: Clear parameter documentation needed.The
forCountparameter lacks JSDoc documentation explaining its purpose and usage.Add JSDoc documentation for better code maintainability:
private injectBaseFieldSelect( model: string, field: string, value: any, selectInclude: any, context: 'select' | 'include', + /** Indicates if the injection is for a count-based selection */ forCount = false )
358-373: Complex conditional logic for count-based selections.The implementation correctly handles count-based selections but the nested conditional logic could be simplified.
Consider extracting the count-based selection logic into a separate private method for better readability:
+private injectCountBasedSelection( + thisLayer: any, + baseRelationName: string, + context: 'select' | 'include', + field: string, + value: any +) { + if ( + !thisLayer[baseRelationName][context]['_count'] || + typeof thisLayer[baseRelationName][context] !== 'object' + ) { + thisLayer[baseRelationName][context]['_count'] = {}; + } + thisLayer[baseRelationName][context]['_count'] = deepmerge( + thisLayer[baseRelationName][context]['_count'], + { select: { [field]: value } } + ); +}Then use it in the main method:
if (forCount) { - if ( - !thisLayer[baseRelationName][context]['_count'] || - typeof thisLayer[baseRelationName][context] !== 'object' - ) { - thisLayer[baseRelationName][context]['_count'] = {}; - } - thisLayer[baseRelationName][context]['_count'] = deepmerge( - thisLayer[baseRelationName][context]['_count'], - { select: { [field]: value } } - ); + this.injectCountBasedSelection(thisLayer, baseRelationName, context, field, value); } else { thisLayer[baseRelationName][context][field] = value; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime/src/enhancements/node/delegate.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/runtime/src/enhancements/node/delegate.ts
[error] 264-264: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: OSSAR-Scan
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: dependency-review
🔇 Additional comments (3)
packages/runtime/src/enhancements/node/delegate.ts (3)
183-191: Well-documented implementation of polymorphic base hierarchy injection.The comments clearly explain the two cases where base hierarchy injection is needed, making the code more maintainable.
264-264: Performance consideration: Use of delete operator.While Biome flags the use of the delete operator for potential performance impact, in this case, it's the correct approach as we need to completely remove empty _count payloads to maintain proper query structure.
The alternative of setting to undefined would still keep the property in the object, which is not desired here.
🧰 Tools
🪛 Biome (1.9.4)
[error] 264-264: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
262-269: Proper cleanup and merging of count selections.The code correctly handles empty _count payloads and uses deepmerge for combining selections. However, consider the performance implications of deep merging large payloads.
Let's verify the usage of deepmerge in the codebase:
✅ Verification successful
Performance impact of deepmerge usage is acceptable
The codebase shows that deepmerge is used appropriately with small, focused objects during query building. The implementation includes optimizations like custom merge strategies and proper type handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential performance bottlenecks with deepmerge # Look for large payloads being merged # Find all deepmerge usages rg --type ts "deepmerge\(" -A 5 -B 5Length of output: 10107
🧰 Tools
🪛 Biome (1.9.4)
[error] 264-264: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
fixes #1467