fix(policy): update fails for model using both @password and @@validate
#2005
Conversation
|
Warning Rate limit exceeded@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request updates the schema validation logic within the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant P as PolicyUtil
C->>P: Call getZodSchema(model, excludePasswordFields, kind)
P->>P: Check if zodSchemas exists
alt Schema Not Present
P-->>C: Return undefined
else Schema Present
P->>P: If excludePasswordFields is true, override password fields
P->>P: Reapply any schema refinements if necessary
P-->>C: Return modified schema
end
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 (5)
packages/runtime/src/enhancements/node/policy/policy-utils.ts (3)
1370-1373: Graceful handling of missing schemas.
Returningundefinedif no Zod schema is present avoids runtime errors during policy checks. You might consider logging whenthis.zodSchemasis not found, in order to aid debugging.
1390-1391: Helper function naming.
The inline definition foroverridePasswordFieldsis reasonable, but consider extracting it out or renaming it to something more descriptive, liketransformPasswordFieldsToPlainString(), for better clarity.
1414-1428: Reapplying refinement logic after overrides.
Reinvoking the refinement function is crucial for maintaining consistency. Just ensure that no constraints re-impose the strict password rules you intended to relax.tests/regression/tests/issue-2000.test.ts (2)
5-56: Comprehensive schema coverage.
Including an abstract model, enum, extended models, and field annotations provides good complexity for regression testing. Consider adding an explicit negative test for password constraints (e.g., invalid or empty password) to ensure coverage of the@passwordvalidations.
58-68: Robust test of create/update and policy validation.
Ensuring correct password setting and policy errors for invalid updates (e.g., missingnamebut active) demonstrates thorough coverage. You might add a test specifically verifying that ignoring@passwordfields is never allowed in create or update contexts, just to confirm correct handling ofexcludePasswordFields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime/src/enhancements/node/policy/policy-utils.ts(2 hunks)tests/regression/tests/issue-2000.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: OSSAR-Scan
- GitHub Check: build-test (20.x)
- GitHub Check: dependency-review
- GitHub Check: build-test (20.x)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build-test (20.x)
🔇 Additional comments (10)
packages/runtime/src/enhancements/node/policy/policy-utils.ts (7)
829-830: Nicely documented usage.
Adding a note that this method is only for mutation operations makes it clear where and how the function should be used.
1380-1381: Conditional password-field exclusion.
IntroducingexcludePasswordFieldsis a neat strategy to skip password validation under certain contexts. Make sure password fields receive proper validation flows elsewhere (e.g., during create/update) to avoid unintended bypasses.
1398-1400: Clarify optional vs. nullish schema.
UsingpwFieldSchema.nullish()suggests you're acceptingnullvalues. If you only intend to allow the field to be omitted (but not explicitly set tonull), use.optional()instead.
1401-1401: Merge strategy is appropriate.
Merging a plain string schema for@passwordfields effectively overrides their stricter validation. This approach seems correct for partial schema usage.
1405-1405: Exit point is straightforward.
Returning the merged schema cleanly concludes the override logic.
1408-1413: Retrieving the “WithoutRefineSchema”.
Fetching the pre-refinement schema key is a solid approach when re-applying custom validations. Make sure to handle cases when theWithoutRefineSchemakey might not be generated (e.g., older schema versions).
1429-1430: Clean return of the canonical schema.
IfexcludePasswordFieldsis false, returning the ordinary schema is standard.tests/regression/tests/issue-2000.test.ts (3)
1-2: Import statement is clear.
ImportingloadSchemafrom@zenstackhq/testtoolsis straightforward.
3-4: Test structure setup.
Usingdescribeanditorganizes the regression scenario well.
69-69: End of test block.
No issues.
fixes #2000