-
-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(delegate): discriminator fields should be removed from unchecked create/update input types #1804
Conversation
…create/update input types Fixes #1763
📝 WalkthroughWalkthroughThe pull request introduces modifications to 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
tests/regression/tests/issue-1763.test.ts (1)
14-15
: Security consideration: Review permissive access policyThe comment suggests this is for testing purposes, but the
@@allow('all', true)
directive grants unrestricted access. In a real application, this would be a security risk.Consider adding a comment explaining that this permissive policy is specifically for testing:
- // full access by author + // Note: Unrestricted access for testing purposes only. In production, implement proper access controls. @@allow('all', true)packages/schema/src/plugins/enhancer/enhance/index.ts (1)
546-548
: Consider simplifying array flattening.The
flatMap
usage here can be simplified toflat
since you're not transforming the elements.- const concreteModelNames = delegateInfo - .map(([_, concretes]) => concretes.flatMap((c) => c.name)) - .flatMap((name) => name); + const concreteModelNames = delegateInfo + .map(([_, concretes]) => concretes.map((c) => c.name)) + .flat();🧰 Tools
🪛 Biome
[error] 546-548: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.(lint/correctness/noFlatMapIdentity)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/schema/src/plugins/enhancer/enhance/index.ts (2 hunks)
- tests/regression/tests/issue-1763.test.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/schema/src/plugins/enhancer/enhance/index.ts
[error] 546-548: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.(lint/correctness/noFlatMapIdentity)
🔇 Additional comments (4)
tests/regression/tests/issue-1763.test.ts (2)
1-4
: LGTM: Test setup follows best practicesThe test setup uses the appropriate testing utilities and follows the standard describe/it pattern for regression testing.
34-40
: Verify error handling for invalid data scenariosThe test only covers the happy path of creating a
ConcretePost
. Consider adding test cases for:
- Invalid age values
- Duplicate IDs
- Missing required fields
Let's check if there are any other similar test files that might provide coverage for these scenarios:
packages/schema/src/plugins/enhancer/enhance/index.ts (2)
Line range hint
518-533
: LGTM! Parameter name change improves clarity.The renaming from
delegateModels
todelegateInfo
better reflects the data structure being passed, making the code more maintainable.
539-553
: LGTM! Enhanced regex pattern improves field removal accuracy.The improved regex pattern now correctly handles both delegate and concrete model names, effectively addressing the issue where TypeScript erroneously requires delegation fields.
🧰 Tools
🪛 Biome
[error] 546-548: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.(lint/correctness/noFlatMapIdentity)
Fixes #1763