-
-
Notifications
You must be signed in to change notification settings - Fork 12
fix: enhance auth type #429
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
Conversation
WalkthroughUpdates ORM type definitions to include relation fields in auth typing by adding an internal recursive Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The target branch was initially set to |
ymc9
left a comment
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.
Hi @genu , thanks for making the fix! Please check my comments.
|
@ymc9 I updated the solution to use |
ymc9
left a comment
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.
LGTM! Thank you!
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: 0
🧹 Nitpick comments (1)
tests/regression/test/issue-422/regression.test.ts (1)
13-16: Consider clarifying test data naming.Lines 13 and 16 use
'user1'as the session ID, which may confuse readers since it suggests a user identifier rather than a session identifier. Consider using names like'session1','session2'for session IDs to improve readability.Example:
- db.$setAuth({ id: 'user1', user: { id: 'user1' } }); + db.$setAuth({ id: 'session2', user: { id: 'user1' } }); - db.$setAuth({ id: 'user1', user: { id: 'user1', profile: { name: 'User1' } } }); + db.$setAuth({ id: 'session3', user: { id: 'user1', profile: { name: 'User1' } } });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/regression/test/issue-422/input.ts(1 hunks)tests/regression/test/issue-422/models.ts(1 hunks)tests/regression/test/issue-422/regression.test.ts(1 hunks)tests/regression/test/issue-422/schema.ts(1 hunks)tests/regression/test/issue-422/schema.zmodel(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.
Applied to files:
tests/regression/test/issue-422/schema.ts
🧬 Code graph analysis (2)
tests/regression/test/issue-422/regression.test.ts (2)
packages/testtools/src/client.ts (1)
createTestClient(95-244)tests/regression/test/issue-422/schema.ts (1)
schema(121-121)
tests/regression/test/issue-422/schema.ts (2)
packages/schema/src/expression-utils.ts (1)
ExpressionUtils(19-123)packages/schema/src/schema.ts (1)
SchemaDef(11-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (2)
tests/regression/test/issue-422/regression.test.ts (1)
5-18: LGTM! Effective compile-time type test for Issue #422.The test correctly verifies that
$setAuthaccepts auth payloads with relations (line 13) and nested relations (line 16), addressing the core issue. The absence of runtime assertions is appropriate since this primarily validates TypeScript type inference.tests/regression/test/issue-422/schema.zmodel (1)
1-25: LGTM! Well-structured test schema for Issue #422.The schema correctly defines the models needed to test auth with relations:
- Session model with
@@authattribute (line 11) serves as the auth type- Proper relations configured between Session → User and User → Profile
- Cascade delete behavior ensures referential integrity in tests
- UUID defaults and unique constraints are appropriate
The schema effectively supports testing scalar fields, single-level relations, and nested relations in auth payloads.
Fixes #422
Summary by CodeRabbit
Refactor
API Changes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.