fix(zmodel): require implicitly used "public" schema to be declared in "schemas" config #492
Conversation
…n "schemas" config
|
Warning Rate limit exceeded@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 14 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)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors datasource schema validation to compute schemas once and enforce: if a default schema is set it must be in the schemas array; if no default is set and any data model or enum lacks an explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
|
Claude finished @ymc9's task —— View job PR Review: PostgreSQL Multi-Schema "public" Schema Declaration FixReview Tasks:
📋 Review SummaryThis PR addresses an important edge case in PostgreSQL multi-schema support by requiring the implicit "public" schema to be declared when using the ✅ StrengthsCode Quality & Implementation
Test Coverage
🎯 Technical AnalysisImplementation DetailsThe core fix is in const hasImplicitPublicSchema = ds.$container.declarations.some(
(d) => (isDataModel(d) || isEnum(d)) && !d.attributes.some((a) => a.decl.$refText === 'schema'),
);This correctly identifies when:
Edge Case HandlingThe logic properly handles the distinction between:
⚡ Performance Considerations
🔒 Security Assessment
📝 Minor Suggestions
🧪 Test Coverage AnalysisThe test additions are thorough:
📊 Overall AssessmentThis is a high-quality fix that:
Recommendation: ✅ Approve - This PR is ready to merge. |
![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 (1)
packages/language/src/validators/datasource-validator.ts (1)
73-94: Consider making the@@schemadetection a bit more robust than$refText.Using
a.decl.$refText === 'schema'will usually work, but checking the resolved symbol name (when available) is less brittle and clearer intent. E.g.a.decl.ref?.name === 'schema' || a.decl.$refText === 'schema'.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/language/src/validators/datasource-validator.ts(2 hunks)tests/e2e/orm/client-api/pg-custom-schema.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/client-api/pg-custom-schema.test.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
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.
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/client-api/pg-custom-schema.test.tspackages/language/src/validators/datasource-validator.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/client-api/pg-custom-schema.test.ts
🧬 Code graph analysis (1)
packages/language/src/validators/datasource-validator.ts (2)
packages/language/src/utils.ts (1)
getStringLiteral(75-77)packages/language/src/generated/ast.ts (2)
isDataModel(391-393)isEnum(432-434)
⏰ 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). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (1)
packages/language/src/validators/datasource-validator.ts (1)
3-3: New AST type-guards import looks appropriate.
There was a problem hiding this comment.
Pull request overview
This PR adds validation to ensure that the implicitly-used "public" schema is declared in the "schemas" configuration for PostgreSQL datasources. When no explicit defaultSchema is specified and models/enums lack a @@schema attribute, they implicitly use the "public" schema, which must be included in the schemas array.
Key Changes:
- Added validation logic to check for implicit "public" schema usage when
defaultSchemais not specified - Added test case to verify the validation triggers when "public" is missing from the schemas array despite being implicitly used
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
packages/language/src/validators/datasource-validator.ts |
Added conditional logic to validate implicit "public" schema usage; imports isDataModel and isEnum for declaration filtering |
tests/e2e/orm/client-api/pg-custom-schema.test.ts |
Added test case covering scenario where enum and model without @@Schema require "public" in schemas array |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
Claude finished @ymc9's task —— View job PR Review: PostgreSQL Multi-Schema "public" Schema Declaration FixReview Tasks:
📋 Review SummaryThis PR addresses an important edge case in PostgreSQL multi-schema support by requiring the implicit "public" schema to be declared when using the ✅ StrengthsCode Quality & Implementation
Test Coverage
🎯 Technical AnalysisImplementation DetailsThe core fix is in const hasImplicitPublicSchema = ds.$container.declarations.some(
(d) => (isDataModel(d) || isEnum(d)) && !d.attributes.some((a) => a.decl.$refText === '@@schema'),
);This correctly identifies when:
Edge Case HandlingThe logic properly handles the distinction between:
⚡ Performance Considerations
🔒 Security Assessment
📝 Minor Suggestions
🧪 Test Coverage AnalysisThe test additions are thorough:
📊 Overall AssessmentThis is a high-quality fix that:
Recommendation: ✅ Approve - This PR is ready to merge. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/language/src/validators/datasource-validator.ts (1)
83-94: LGTM! Implicit public schema detection is implemented correctly.The logic properly detects when models or enums lack an explicit
@@schemaattribute and correctly requires"public"to be in theschemasarray, since it will be implicitly used. The attribute check on line 87 correctly uses'@@schema'with the@@prefix for model-level attributes.Consider making the error message more descriptive to help users understand why:
- accept('error', `"public" must be included in the "schemas" array`, { + accept('error', `"public" must be included in the "schemas" array because it is implicitly used by models or enums without an explicit @@schema attribute`, {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/language/src/validators/datasource-validator.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
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.
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
packages/language/src/validators/datasource-validator.ts
📚 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:
packages/language/src/validators/datasource-validator.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
packages/language/src/validators/datasource-validator.ts
🧬 Code graph analysis (1)
packages/language/src/validators/datasource-validator.ts (2)
packages/language/src/utils.ts (1)
getStringLiteral(75-77)packages/language/src/generated/ast.ts (2)
isDataModel(391-393)isEnum(432-434)
⏰ 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, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (3)
packages/language/src/validators/datasource-validator.ts (3)
3-3: LGTM! Import additions support the new validation.The
isDataModelandisEnumtype guards are correctly imported from the generated AST and are necessary for the implicit public schema detection logic.
73-75: LGTM! Efficient extraction of schemas array.Extracting
schemasArrayonce and reusing it in both validation branches is good for performance and maintainability. The non-null assertion is safe given the prior validation at line 68.
76-82: LGTM! DefaultSchema validation is correct.The logic properly validates that when
defaultSchemais explicitly set, it must be included in theschemasarray. The error message is clear and actionable.
…it @@Schema (#496) * Initial plan * Add test for explicit schema usage without public Co-authored-by: ymc9 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: ymc9 <[email protected]>
|
Claude finished @ymc9's task —— View job PR Review: PostgreSQL Multi-Schema "public" Schema Declaration FixReview Tasks:
📋 Review SummaryThis PR addresses an important edge case in PostgreSQL multi-schema support by requiring the implicit "public" schema to be declared when using the ✅ StrengthsCode Quality & Implementation
Test Coverage
🎯 Technical AnalysisImplementation DetailsThe core fix is in const hasImplicitPublicSchema = ds.$container.declarations.some(
(d) => (isDataModel(d) || isEnum(d)) && !d.attributes.some((a) => a.decl.$refText === '@@schema'),
);This correctly identifies when:
Edge Case HandlingThe logic properly handles the distinction between:
⚡ Performance Considerations
🔒 Security Assessment
📝 Minor Suggestions
🧪 Test Coverage AnalysisThe test additions are thorough:
📊 Overall AssessmentThis is a high-quality fix that:
Recommendation: ✅ Approve - This PR is ready to merge. |
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.