fix(policy): revers lookup condition to parent entity is not properly built with compound id fields #2053
Conversation
… built with compound id fields fixes #1964
📝 WalkthroughWalkthroughThis pull request updates several classes to require explicit passing of the database context when constructing query conditions. Methods in both the delegate and policy handlers now call an updated asynchronous version of Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant QueryUtils
participant Database
Client->>Handler: Invoke update/delete operation (with db, context)
Handler->>QueryUtils: await buildReversedQuery(db, context, options)
QueryUtils->>Database: Execute query logic with logging
Database-->>QueryUtils: Return query conditions/result
QueryUtils-->>Handler: Provide reversed query
Handler-->>Client: Return operation result
Assessment against linked issues
Possibly related issues
Possibly related PRs
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (2)
packages/runtime/src/enhancements/node/delegate.ts (2)
1029-1029: Consider using undefined assignment instead of delete operator.According to the static analysis hint, using the delete operator can impact performance. Consider replacing
delete context.parent['delete']withcontext.parent['delete'] = undefinedfor better performance.- delete context.parent['delete']; + context.parent['delete'] = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 1029-1029: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
1037-1037: Consider using undefined assignment instead of delete operator.Similar to the previous suggestion, consider replacing
delete context.parent['deleteMany']withcontext.parent['deleteMany'] = undefinedfor better performance.- delete context.parent['deleteMany']; + context.parent['deleteMany'] = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 1037-1037: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/runtime/src/enhancements/node/delegate.ts(2 hunks)packages/runtime/src/enhancements/node/policy/handler.ts(10 hunks)packages/runtime/src/enhancements/node/query-utils.ts(3 hunks)tests/regression/tests/issue-1964.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/runtime/src/enhancements/node/query-utils.ts (4)
packages/runtime/src/enhancements/node/logger.ts (1) (1)
Logger(8-66)packages/runtime/src/types.ts (2) (2)
DbClientContract(91-93)CrudContract(86-86)packages/runtime/src/enhancements/node/create-enhancement.ts (1) (1)
InternalEnhancementOptions(31-64)packages/runtime/src/enhancements/edge/utils.ts (1) (1)
formatObject(9-11)
🪛 Biome (1.9.4)
packages/runtime/src/enhancements/node/delegate.ts
[error] 1029-1029: 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 (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (17)
packages/runtime/src/enhancements/node/query-utils.ts (4)
13-15: Updated dependencies to support new logging functionality.The new import of
Loggerand update to useformatObjectfrom edge are necessary to support the enhanced logging functionality added to this class.
19-23: Added logger property to QueryUtils class.A new
loggerproperty has been added and properly initialized in the constructor. This change allows for consistent logging throughout the class, specifically when executing queries during reverse lookups.
69-74: Modified buildReversedQuery method signature to include database context.The method signature has been updated to:
- Accept a database context parameter (
db: CrudContract)- Make the method asynchronous
- Rename
unsafeOperationtouncheckedOperationfor better clarityThese changes ensure the method can execute database queries when needed to resolve compound ID fields.
116-118: Improved readability of condition check.Renamed
unsafeOperationtouncheckedOperationfor better semantic clarity throughout the codebase. This makes the code more maintainable and easier to understand.packages/runtime/src/enhancements/node/policy/handler.ts (8)
812-813: Updated buildReversedQuery call to pass database context.The method call has been properly updated to pass the database context and await the result, in line with the method signature changes in QueryUtils.
888-889: Correctly awaiting buildReversedQuery with database context.This update ensures that the database context is passed to the buildReversedQuery method when checking for duplicate constraints, which is critical for proper operation with compound ID fields.
939-940: Updated buildReversedQuery call for update operation.Properly updated to pass the database context and await the result when building a unique filter for update operations with nested writing.
1000-1001: Updated buildReversedQuery call for updateMany operation.Updated to pass the database context and properly await the result when handling the reverse query for updateMany operations.
1069-1070: Updated buildReversedQuery call for upsert operation.Correctly passing the database context and awaiting the result when building a unique filter for upsert operations.
1146-1147: Updated buildReversedQuery call for set operation.Database context is now correctly passed and the result awaited when building the reverse query for set operations.
1165-1166: Updated buildReversedQuery call for delete operation.The method call has been updated to pass the database context parameter and properly await the result for delete operations.
1182-1183: Updated buildReversedQuery call for deleteMany operation.Properly updated to include the database context parameter and await the result when building the query for deleteMany operations.
packages/runtime/src/enhancements/node/delegate.ts (3)
962-963: Updated buildReversedQuery call in updateMany handler.The call has been correctly updated to pass the database context and await the result, aligning with the method signature changes in QueryUtils.
1025-1026: Updated buildReversedQuery call in delete handler.Properly passing the database context and awaiting the result when building the where clause for delete operations, ensuring correct behavior with compound ID fields.
1033-1034: Updated buildReversedQuery call in deleteMany handler.The method call now correctly passes the database context and awaits the result when building the where clause for deleteMany operations.
tests/regression/tests/issue-1964.test.ts (2)
1-60: Well-designed regression test for compound ID lookup issue.Excellent test case that verifies the fix for compound ID fields in reverse lookup conditions. This test:
- Creates a schema with
AuthorandPostmodels with a relationship- Defines a compound unique key (
@@unique([orgId, name])) on the Author model- Tests the scenario where an update operation needs to delete related posts
- Uses the compound key
orgId_namein the where clause of the update operationThis effectively tests the scenario where the policy handler needs to correctly build a reverse lookup condition with a compound ID field.
61-128: Comprehensive regression test for nested update operations.This test case provides thorough coverage for:
- Nested updates across multiple levels of relationships (User → Profile → Address)
- Verifies that deleteMany operation works correctly in a nested context
- Validates the result by checking that the Address entity was properly deleted
The test uses
strictUndefinedCheckspreview feature, which likely helps expose the issue with compound ID fields and undefined values during reverse lookups.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/runtime/src/enhancements/node/delegate.ts (1)
905-905: Added null check for safer object handlingThis change properly handles the case when
args.whereis undefined by using the nullish coalescing operator, preventing potential errors when operating on a missing where clause.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/runtime/src/enhancements/node/delegate.ts(4 hunks)packages/runtime/src/enhancements/node/policy/handler.ts(11 hunks)packages/testtools/src/schema.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/runtime/src/enhancements/node/delegate.ts
[error] 1029-1029: 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: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (8)
packages/testtools/src/schema.ts (1)
99-103: Always enabling strictUndefinedChecks makes sense for reliable testingThis change ensures that
strictUndefinedChecksis always enabled in the preview features for test environments when no preview features are explicitly provided. This is a good improvement as it creates more consistent test behavior and helps prevent issues with undefined values in complex queries - which is likely related to the PR's goal of fixing reverse lookup conditions with compound ID fields.packages/runtime/src/enhancements/node/delegate.ts (4)
962-962: Updated to correctly pass database context in buildReversedQueryThe function call now includes the database context parameter and uses
awaitsince the method has been updated to be asynchronous. This ensures proper database context is available when building compound ID field conditions.
1025-1025: Updated to correctly pass database context in delete operationSimilar to the other instances, this ensures the database context is properly passed to the query builder for delete operations.
1033-1033: Updated to correctly pass database context in deleteMany operationThe function call now includes the database context parameter and uses
awaitsince the method is asynchronous, ensuring proper query construction.
1098-1098: Added null check for safer object handling in doDeleteManySimilar to the earlier change, this properly handles undefined values for the
whereparameter by using the nullish coalescing operator.packages/runtime/src/enhancements/node/policy/handler.ts (3)
812-812: Updated to correctly pass database context in policy handlerThe function call now includes the database context parameter and uses
awaitfor the asynchronous method. This aligns with the changes in the delegate handler to ensure proper handling of compound ID fields.
888-889: Consistently updated buildReversedQuery calls to include database contextAll instances of
buildReversedQuerycalls have been updated to include the database context parameter and to useawaitfor handling the asynchronous nature of the method. This comprehensive update ensures consistency throughout the codebase and properly handles compound ID field conditions.Also applies to: 900-900, 940-940, 1000-1000, 1033-1033, 1066-1066, 1146-1146, 1165-1165, 1182-1182
1093-1093: Added null check for safer handling of where clauseThe added nullish coalescing operator ensures that an empty object is used when
args.whereis undefined, preventing potential errors in downstream operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/tests/enhancements/with-policy/unique-as-id.test.ts (1)
199-199: Consider standardizing thelogPrismaQueryoption across testsThis test includes a new option
{ logPrismaQuery: true }which isn't used in other test cases. This might be intentional for debugging purposes, but it creates inconsistency in the test configuration.If this was added for debugging, consider either:
- Removing it before merging if no longer needed
- Adding it to all tests for consistency
- Moving it to a common configuration if you want to enable it globally
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/tests/enhancements/with-policy/unique-as-id.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: OSSAR-Scan
- GitHub Check: build-test (20.x)
🔇 Additional comments (3)
tests/integration/tests/enhancements/with-policy/unique-as-id.test.ts (3)
175-233: Good test case for compound unique keys with one-to-many relationshipsThis test effectively verifies that nested operations on to-many relationships work correctly when using compound unique fields as identifiers. It properly tests both update and delete operations, ensuring that only the targeted records are affected.
The test structure is clear and follows a good pattern:
- Setup data with two parent-child relationships
- Perform operations on one relationship
- Verify both the affected and unaffected relationships
235-296: Good test case for compound unique keys with one-to-one relationshipsThis test effectively verifies that nested operations on to-one relationships work correctly when using compound unique fields as identifiers. It provides good coverage for both update and delete operations.
The test properly validates both the direct effects (deletion/update of the targeted record) and the absence of side effects (other relationships remaining intact).
175-296: Comprehensive regression tests for PR #2053These test cases thoroughly validate the fix for issue #1964 by demonstrating correct behavior for both one-to-many and one-to-one relationships when using compound unique fields as identifiers in parent models.
The tests are well-structured and cover:
- Nested update operations through compound unique keys
- Nested delete operations through compound unique keys
- Verification that operations don't affect unrelated records
This provides good coverage for the "reverse lookup condition to parent entity" issue mentioned in the PR description.
fixes #1964