fix(delegate): enforcing concrete model policies when read from a delegate base #1726
Conversation
WalkthroughWalkthroughThe changes introduced in this pull request enhance the functionality of the enhancement system within the codebase. Key modifications include the addition of a new function for processing relation payloads, updates to method signatures to improve type specificity, and adjustments to ensure asynchronous operations are properly handled. These changes collectively aim to improve the handling of relation fields and policy enforcement across various components of the system. Changes
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: 11
Outside diff range and nitpick comments (3)
packages/runtime/src/enhancements/node/create-enhancement.ts (2)
51-61: Specify a more precise type forpayloadparameter inprocessIncludeRelationPayload.Currently,
payloadis typed asunknown, which reduces type safety. Consider defining a specific type or interface forpayloadto leverage TypeScript's type checking and improve code clarity.
129-131: Address TODO: Refactor global callback usage for better encapsulation.The TODO comment indicates that sharing
processIncludeRelationPayloadglobally is not ideal. I can assist in proposing a refactored design to improve encapsulation and modularity.Would you like me to provide suggestions on refactoring this implementation or open a new GitHub issue to track this task?
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (1)
465-465: Handle possible undefined properties safelyIn line 465, you're accessing
readAsset1.title, which may beundefined. Although this is part of the test to verify policy enforcement, consider adding explicit checks or comments to clarify the expected behavior.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/runtime/src/enhancements/node/create-enhancement.ts (5 hunks)
- packages/runtime/src/enhancements/node/delegate.ts (15 hunks)
- packages/runtime/src/enhancements/node/policy/index.ts (3 hunks)
- packages/runtime/src/enhancements/node/policy/policy-utils.ts (1 hunks)
- packages/runtime/src/enhancements/node/proxy.ts (3 hunks)
- tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (1 hunks)
Additional context used
Biome
packages/runtime/src/enhancements/node/delegate.ts
[error] 361-361: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (21)
packages/runtime/src/enhancements/node/proxy.ts (3)
72-72: LGTM!The change from
keyof PrismaProxyHandlertoPrismaProxyActionsfor themethodparameter improves type safety and clarity. It aligns with the usage of themethodparameter within the method body.
87-87: LGTM!The change from
keyof PrismaProxyHandlertoPrismaProxyActionsfor themethodparameter improves type safety and clarity. It aligns with the usage of themethodparameter within the method body and is consistent with the change in thewithFluentCallmethod.
213-213: LGTM!The change from
objecttoDbClientContractfor theprismaparameter improves type safety and clarity. It ensures that the passed argument conforms to the expected contract and reduces the likelihood of passing an incompatible object.packages/runtime/src/enhancements/node/policy/policy-utils.ts (1)
1101-1103: Approved: The added logging enhances visibility into the function's behavior.The introduced logging statement provides useful information when the specified model cannot be read back from the database. It enhances the observability of the
readBackfunction by clearly indicating which model encountered an issue.The logging is appropriately guarded by the
shouldLogQueryflag, allowing it to be easily enabled or disabled based on the desired level of verbosity.The log level of 'info' is suitable for this type of informational message.
packages/runtime/src/enhancements/node/policy/index.ts (2)
10-10: Import ofPolicyUtilis appropriately added.The import statement for
PolicyUtilis necessary for the new functionality introduced in the code.
22-22: Verify that the updated type constraint onDbClientdoes not introduce incompatibilities.Changing the generic type parameter from
objecttoDbClientContractin thewithPolicyfunction strengthens type safety. However, this may affect existing code that useswithPolicywith clients not extendingDbClientContract. Please ensure all usages ofwithPolicyremain compatible with this new constraint.Run the following script to identify all usages of
withPolicyand check for type compatibility:packages/runtime/src/enhancements/node/create-enhancement.ts (4)
4-10: Imports updated to include necessary types.The added type imports from
'../../types'ensure all required TypeScript types are available, improving type safety and code clarity.
16-16: Added imports from'./policy'for policy enhancements.Importing
policyProcessIncludeRelationPayloadandwithPolicyfrom'./policy'is necessary for implementing the new policy-related functionality.
74-74: Enhanced type safety increateEnhancementfunction signature.Changing the generic constraint to
DbClient extends DbClientContractensures that theprismaclient provided conforms to the expected contract, enhancing type safety and preventing potential runtime errors.
110-110: AppliedwithDelegateenhancement with appropriate parameters.The
withDelegatefunction is correctly called withresult,options, andcontext, ensuring that delegate enhancements are properly applied when the'delegate'kind is enabled.tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (1)
Line range hint
486-510: Good use of field-level policies in testingThe test case starting at line 486 effectively verifies field-level policies for different user contexts. The use of
@allowand@denydirectives at the field level is correctly implemented and tested.packages/runtime/src/enhancements/node/delegate.ts (10)
506-506: Ensure asynchronous call toinjectSelectIncludeHierarchyindoCreateis handled properlyIn the
doCreatemethod, the call toinjectSelectIncludeHierarchyhas been updated to useawait. Confirm that this change maintains the desired execution order and that any dependent code handles the asynchronous behavior appropriately.
728-728: Verify asynchronous handling after updatinginjectSelectIncludeHierarchyinupsertThe method
injectSelectIncludeHierarchyis now awaited in theupsertfunction. Ensure that this change does not introduce any side effects and that the overall logic remains consistent.
747-747: Confirm correct async behavior indoUpdatewithinjectSelectIncludeHierarchyThe call to
injectSelectIncludeHierarchywithindoUpdateis now awaited. Verify that this modification preserves the method's correctness and that all asynchronous operations are properly managed.
941-941: Handle new async behavior indeletemethod withbuildSelectIncludeHierarchyThe
deletemethod now awaitsbuildSelectIncludeHierarchy. Please ensure that this change integrates seamlessly with the existing logic and that any asynchronous dependencies are handled correctly.
993-993: Validate asynchronous call toinjectSelectIncludeHierarchyindoDeleteIn the
doDeletemethod,injectSelectIncludeHierarchyis now called withawait. Confirm that this addition does not disrupt the deletion process and that all promises are appropriately resolved.
Line range hint
178-232: Confirm correct handling of asynchronousinjectSelectIncludeHierarchyThe method
injectSelectIncludeHierarchyhas been converted to anasyncfunction, and calls to it have been updated withawait. Please ensure that all invocations of this method within the codebase are properly awaited to prevent unhandled promise rejections and maintain correct execution flow.#!/bin/bash # Description: Verify all calls to `injectSelectIncludeHierarchy` are awaited. # Find calls to `injectSelectIncludeHierarchy` that are not preceded by `await`. rg --type ts --pcre2 '^\s*(?!await\s+)(\w+\.)?injectSelectIncludeHierarchy\(' --files-with-matches
Line range hint
236-260: Ensure proper usage of asynchronousbuildSelectIncludeHierarchyThe
buildSelectIncludeHierarchymethod is now asynchronous. Verify that all calls to this method are usingawaitto handle the returned promise correctly.#!/bin/bash # Description: Find all calls to `buildSelectIncludeHierarchy` that might not be awaited. # Search for calls to `buildSelectIncludeHierarchy` not using `await`. rg --type ts --pcre2 '^\s*(?!await\s+)(\w+\.)?buildSelectIncludeHierarchy\(' --files-with-matches
Line range hint
331-365: Check asynchronous handling ininjectConcreteIncludeRecursivelyThe method
injectConcreteIncludeRecursivelyis now anasyncfunction, and internal calls have been updated accordingly. Please confirm that all invocations of this method are properly awaited and that any asynchronous operations within are correctly handled.#!/bin/bash # Description: Verify all calls to `injectConcreteIncludeRecursively` are awaited. # Find calls to `injectConcreteIncludeRecursively` that are not preceded by `await`. rg --type ts --pcre2 '^\s*(?!await\s+)(\w+\.)?injectConcreteIncludeRecursively\(' --files-with-matchesTools
Biome
[error] 361-361: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
42-47: VerifyDelegateProxyHandlerinstantiations includecontextparameterThe constructor of
DelegateProxyHandlernow requires acontextparameter. Ensure all instances whereDelegateProxyHandleris instantiated pass thecontextargument to avoid potential errors.Verification successful
All
DelegateProxyHandlerinstantiations include the requiredcontextparameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instantiations of `DelegateProxyHandler` missing the `context` parameter. # Search for `new DelegateProxyHandler` and check for missing fourth argument. rg --type ts --pcre2 'new\s+DelegateProxyHandler\(([^,]+\s*,\s*[^,]+\s*,\s*[^,]+)(\))' --replace '$1$2' --files-with-matchesLength of output: 207
Script:
#!/bin/bash # Description: Find all instantiations of `DelegateProxyHandler` missing the `context` parameter. # Using ast-grep to match `new DelegateProxyHandler` with exactly three arguments. ast-grep --lang typescript --pattern 'new DelegateProxyHandler($_arg1, $_arg2, $_arg3)' --files-with-matchesLength of output: 346
Script:
#!/bin/bash # Description: Find all instantiations of `DelegateProxyHandler` missing the `context` parameter. # Using ripgrep to search for 'new DelegateProxyHandler' with exactly three arguments. rg --type ts --pcre2 'new\s+DelegateProxyHandler\(\s*[^,]+\s*,\s*[^,]+\s*,\s*[^,]+\s*\)' --files-with-matchesLength of output: 111
25-29: Ensure all calls towithDelegateinclude the newcontextparameterThe function
withDelegatenow accepts an additionalcontextparameter. Please verify that all invocations ofwithDelegatethroughout the codebase have been updated to include this new parameter to prevent runtime errors.
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts
Outdated
Show resolved
Hide resolved
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts
Show resolved
Hide resolved
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts
Show resolved
Hide resolved
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts
Show resolved
Hide resolved
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts
Show resolved
Hide resolved
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts
Show resolved
Hide resolved
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts
Show resolved
Hide resolved
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts
Show resolved
Hide resolved
No description provided.