merge dev to main (v3.0.0-beta.21) #389
Conversation
* fix(zmodel): unique attribute validation issues * update
* feat: postgres custom schema * update
|
Warning Rate limit exceeded@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 42 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 (19)
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. WalkthroughImplements PostgreSQL multi-schema support: activates @@Schema attribute, validates datasource schema/defaultSchema settings, propagates schema names through ORM name-mapping and executors, updates schema generation/providers to carry defaultSchema, adds tests and testtools dbFile support, and bumps package versions to 3.0.0-beta.21. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 for v3.0.0-beta.21I've completed a comprehensive review of this release PR. Here are my findings: ✅ Strong PointsPostgreSQL Custom Schema Support - Excellent Implementation
Code Quality
Test Coverage
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for PostgreSQL custom schemas (multi-schema databases) in ZenStack. It enables users to specify which schema(s) their models belong to using the @@schema attribute and configure default/available schemas in the datasource.
- Added
@@schemaattribute support for models to specify custom PostgreSQL schemas - Implemented
defaultSchemaandschemasconfiguration in the datasource for PostgreSQL - Enhanced query execution to properly qualify table names with schema information
Reviewed Changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/language/res/stdlib.zmodel | Uncommented and documented the @@schema attribute for multi-schema PostgreSQL support |
| packages/language/src/validators/datasource-validator.ts | Added validation for defaultSchema and schemas datasource fields, restricted to PostgreSQL provider |
| packages/language/src/validators/attribute-application-validator.ts | Added validation to ensure @@schema references defined schemas in datasource, updated constraint validation to use getAttributeArg |
| packages/language/src/utils.ts | Added getAttributeArg helper function and updated getAttributeArgLiteral signature to support InternalAttribute |
| packages/schema/src/schema.ts | Extended DataSourceProvider type to include optional defaultSchema field |
| packages/sdk/src/ts-schema-generator.ts | Updated schema generation to extract and include defaultSchema in provider object, fixed constraint field extraction |
| packages/orm/src/client/executor/name-mapper.ts | Implemented schema-qualified table names for PostgreSQL queries |
| packages/orm/src/client/executor/zenstack-query-executor.ts | Enabled name mapper for all PostgreSQL providers to support schema qualification |
| packages/orm/package.json | Moved kysely from peerDependencies to dependencies |
| packages/testtools/src/schema.ts | Added replacePlaceholders function to replace $DB_URL and $PROVIDER placeholders in test schemas |
| packages/testtools/src/client.ts | Added dbFile option to support pre-existing database files, preserved provider properties when overriding |
| tests/e2e/orm/client-api/pg-custom-schema.test.ts | Added comprehensive test suite for PostgreSQL custom schema support |
| tests/regression/test/v2-migrated/issue-2283/regression.test.ts | Added regression test using pre-existing database file |
| tests/regression/test/v2-migrated/issue-2283/.gitignore | Excluded database files from gitignore for test fixtures |
| */package.json | Version bumps across all packages to 3.0.0-beta.21 |
Comments suppressed due to low confidence (1)
packages/language/src/validators/attribute-application-validator.ts:306
- Potential bug: When
getAttributeArgreturns undefined, the error handling tries to accessattr.args[0]!which could be undefined if the attribute has no arguments at all. This would throw a runtime error. Consider usingattras the error node instead, or add a check:node: attr.args[0] ?? attr.
const fields = getAttributeArg(attr, 'fields');
const attrName = attr.decl.ref?.name;
if (!fields) {
accept('error', `expects an array of field references`, {
node: attr.args[0]!,
});
return;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/orm/src/client/executor/name-mapper.ts (1)
311-321: Preserve existing schema annotations when remapping tables.Recreating every
TableNodenow discards any schema Kysely already attached (for example from.withSchema()or a raw table reference). As soon as we run on PostgreSQL, a query likedb.$qb.withSchema('tenant_a').selectFrom('Foo')gets rewritten to hit"public"."Foo"because we overwrite the caller-specified schema. That’s a regression for anyone switching schemas at query time.- const mappedName = this.mapTableName(node.table.identifier.name); - const tableSchema = this.getTableSchema(node.table.identifier.name); + const mappedName = this.mapTableName(node.table.identifier.name); + const originalSchema = node.table.identifier.schema; + const tableSchema = + originalSchema ?? this.getTableSchema(node.table.identifier.name); @@ - const tableSchema = this.getTableSchema(modelName); + const originalSchema = innerNode.table.identifier.schema; + const tableSchema = originalSchema ?? this.getTableSchema(modelName);Please preserve the explicit schema when it’s already present and only fall back to our computed default otherwise.
Also applies to: 365-369
🧹 Nitpick comments (2)
tests/regression/test/v2-migrated/issue-2283/regression.test.ts (2)
5-6: Use a more descriptive test name.The test name
'regression'within a describe block'Regression for issue 2283'is redundant and uninformative. Consider a name that describes what behavior is being tested.- it('regression', async () => { + it('should correctly query nested lab profile with published content, modules, and classes', async () => {
701-701: Document the purpose of this assertion.This assertion verifies that the
modulefield is undefined on aClassinstance, but sincemodulewasn't included in the select clause (lines 666-670), this is expected ORM behavior. If this assertion is specifically testing a bug from issue 2283 where unselected relations were incorrectly populated, please add a comment explaining the context.+ // Verify that unselected relations are not populated (issue 2283) expect(r.lab.content[0].modules[0].classes[0].module).toBeUndefined();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/regression/test/v2-migrated/issue-2283/dev.dbis excluded by!**/*.db
📒 Files selected for processing (34)
package.json(1 hunks)packages/cli/package.json(1 hunks)packages/clients/tanstack-query/package.json(1 hunks)packages/common-helpers/package.json(1 hunks)packages/config/eslint-config/package.json(1 hunks)packages/config/typescript-config/package.json(1 hunks)packages/config/vitest-config/package.json(1 hunks)packages/create-zenstack/package.json(1 hunks)packages/dialects/sql.js/package.json(1 hunks)packages/language/package.json(1 hunks)packages/language/res/stdlib.zmodel(1 hunks)packages/language/src/utils.ts(3 hunks)packages/language/src/validators/attribute-application-validator.ts(4 hunks)packages/language/src/validators/datasource-validator.ts(2 hunks)packages/orm/package.json(2 hunks)packages/orm/src/client/executor/name-mapper.ts(5 hunks)packages/orm/src/client/executor/zenstack-query-executor.ts(1 hunks)packages/plugins/policy/package.json(1 hunks)packages/schema/package.json(1 hunks)packages/schema/src/schema.ts(1 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/ts-schema-generator.ts(4 hunks)packages/server/package.json(1 hunks)packages/testtools/package.json(1 hunks)packages/testtools/src/client.ts(5 hunks)packages/testtools/src/schema.ts(2 hunks)packages/zod/package.json(1 hunks)samples/next.js/package.json(1 hunks)samples/orm/package.json(1 hunks)tests/e2e/orm/client-api/pg-custom-schema.test.ts(1 hunks)tests/e2e/package.json(1 hunks)tests/regression/package.json(1 hunks)tests/regression/test/v2-migrated/issue-2283/.gitignore(1 hunks)tests/regression/test/v2-migrated/issue-2283/regression.test.ts(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:
packages/orm/src/client/executor/zenstack-query-executor.tspackages/orm/src/client/executor/name-mapper.ts
🧬 Code graph analysis (7)
tests/e2e/orm/client-api/pg-custom-schema.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(53-182)
packages/language/src/utils.ts (1)
packages/language/src/generated/ast.ts (14)
TypeDef(725-733)TypeDef(735-735)DataModel(377-387)DataModel(389-389)isTypeDef(737-739)isDataModel(391-393)DataModelAttribute(395-400)DataModelAttribute(402-402)DataFieldAttribute(348-353)DataFieldAttribute(355-355)InternalAttribute(520-525)InternalAttribute(527-527)Expression(115-115)Expression(117-117)
tests/regression/test/v2-migrated/issue-2283/regression.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(192-203)
packages/testtools/src/client.ts (4)
packages/testtools/src/schema.ts (1)
generateTsSchema(40-74)packages/orm/src/client/options.ts (1)
ClientOptions(45-96)packages/testtools/src/utils.ts (1)
loadDocumentWithPlugins(3-6)packages/cli/src/utils/exec-utils.ts (1)
execSync(6-15)
packages/language/src/validators/datasource-validator.ts (3)
packages/language/src/utils.ts (1)
getStringLiteral(75-77)packages/language/src/constants.ts (1)
SUPPORTED_PROVIDERS(4-11)packages/language/src/generated/ast.ts (2)
isConfigArrayExpr(290-292)isLiteralExpr(133-135)
packages/language/src/validators/attribute-application-validator.ts (3)
packages/language/src/utils.ts (2)
getAttributeArg(325-330)getStringLiteral(75-77)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/language/src/generated/ast.ts (4)
isModel(567-569)isDataSource(417-419)isConfigArrayExpr(290-292)isLiteralExpr(133-135)
packages/sdk/src/ts-schema-generator.ts (3)
packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/language/src/generated/ast.ts (4)
isLiteralExpr(133-135)Model(559-563)Model(565-565)isDataSource(417-419)packages/language/src/utils.ts (1)
getAttributeArg(325-330)
⏰ 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: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (24)
tests/regression/test/v2-migrated/issue-2283/regression.test.ts (1)
643-679: Clarify authentication context for policy testing.The query is executed without an explicit authentication context, yet the schema contains extensive
@@allowrules that referenceauth(). While the query filters forpublished: true(which should allow unauthenticated access based on line 93's policy), it would be clearer to explicitly document whether this test is verifying unauthenticated access.Consider either:
- Adding a comment explaining this tests unauthenticated access to published content
- Or setting an auth context if this is meant to test authenticated access:
const db: any = await createPolicyTestClient( `...`, { provider: 'sqlite', dbFile: path.join(__dirname, 'dev.db'), user: { id: 'user-id' }, // Set auth context if needed }, );tests/regression/test/v2-migrated/issue-2283/.gitignore (1)
1-1: The original review comment is based on incorrect assumptions about this test.This is a regression test for issue #2283 that validates complex query behavior with access control policies. Regression tests legitimately require committed, pre-populated databases to reproduce exact conditions where the bug occurred. The schema is complex with many nested models, and the test verifies specific access control outcomes—conditions that cannot be reliably reproduced with programmatic seeding. The 204K database file is appropriately sized for this schema. Whitelisting
.dbfiles for this regression test is correct and should not be refactored.Likely an incorrect or invalid review comment.
packages/language/package.json (1)
4-4: Version bump is consistent and isolated.The version change from 3.0.0-beta.20 to 3.0.0-beta.21 is applied cleanly with no dependency or structural modifications. All workspace references remain unchanged.
package.json (1)
3-3: Root package version bump is consistent with monorepo release.Version updated cleanly to 3.0.0-beta.21 without modifying dependencies or configuration. Aligns with PR objective for coordinated release across packages.
packages/dialects/sql.js/package.json (1)
3-3: Version bump is clean and consistent.The version change from 3.0.0-beta.20 to 3.0.0-beta.21 is properly applied with no collateral changes to dependencies or configuration.
packages/cli/package.json (1)
6-6: Version bump applied cleanly with no dependency modifications.The version change to 3.0.0-beta.21 is isolated and consistent with the monorepo release. All workspace and catalog references remain intact.
packages/common-helpers/package.json (1)
3-3: Version bump is straightforward and consistent.Version updated to 3.0.0-beta.21 with no dependency or configuration changes. Aligns with monorepo release versioning.
packages/plugins/policy/package.json (1)
3-3: Version bump is properly isolated with no dependency changes.The version change to 3.0.0-beta.21 is applied cleanly. Plugin exports and configuration remain unchanged.
packages/clients/tanstack-query/package.json (1)
3-3: Version bump is cleanly applied with no dependency modifications.The version change to 3.0.0-beta.21 is isolated from dependency or export changes. Framework integrations (react, vue, svelte) remain properly configured.
samples/next.js/package.json (1)
3-3: Version bump for sample app is consistent and clean.Version updated to 3.0.0-beta.21 without modifying sample application dependencies or scripts. Workspace references to ZenStack packages remain intact.
packages/testtools/package.json (1)
3-3: Version bump aligned with release 3.0.0-beta.21.Consistent with other packages in the monorepo. All dependencies remain stable.
tests/e2e/package.json (1)
3-3: Version bump aligned with release 3.0.0-beta.21.Consistent with other packages in the monorepo. All dependencies remain stable.
packages/config/typescript-config/package.json (1)
3-3: Version bump aligned with release 3.0.0-beta.21.Consistent with other packages in the monorepo.
packages/config/eslint-config/package.json (1)
3-3: Version bump aligned with release 3.0.0-beta.21.Consistent with other packages in the monorepo.
tests/regression/package.json (1)
3-3: Version bump aligned with release 3.0.0-beta.21.Consistent with other packages in the monorepo. All dependencies remain stable.
packages/schema/package.json (1)
3-3: Version bump aligned with release 3.0.0-beta.21.Consistent with other packages in the monorepo. All dependencies remain stable.
packages/zod/package.json (1)
3-3: Version bump aligned with release 3.0.0-beta.21.Consistent with other packages in the monorepo. All dependencies remain stable.
samples/orm/package.json (1)
3-3: Version bump aligned with release 3.0.0-beta.21.Consistent with other packages in the monorepo. All dependencies remain stable.
packages/language/src/utils.ts (3)
177-177: Good tightening of base resolution logic.The predicate now requires exact name equality (
d.name === base.$refText) rather than just checking types. This prevents false matches and improves correctness when resolving base references.
325-330: Well-designed attribute argument extraction API.This new utility function improves code safety and maintainability by:
- Using name-based lookup instead of positional array indexing
- Handling missing arguments gracefully (returns undefined)
- Supporting all attribute types (DataModelAttribute, DataFieldAttribute, InternalAttribute)
- Reducing brittleness when attribute argument order changes
332-334: LGTM! Consistent type signature expansion.The signature now accepts
InternalAttribute, making it consistent with the newgetAttributeArgfunction and expanding type coverage for attribute argument access.packages/sdk/src/ts-schema-generator.ts (3)
237-255: LGTM! Clean defaultSchema integration.The provider object construction cleanly integrates the optional
defaultSchemafield:
- Retrieves defaultSchema using the new helper method
- Conditionally includes it only when present using spread operator
- Maintains proper TypeScript typing with string literals
631-656: Well-implemented provider and defaultSchema extraction.Both methods follow consistent patterns:
- Proper invariant checks with clear error messages
- Type guards (isLiteralExpr) before accessing expression values
getDataSourceDefaultSchemacorrectly returns undefined when field is missing- Type narrowing ensures type-safe access to string values
872-876: Excellent refactoring for robustness.Switching from direct array indexing to
getAttributeArg(attr, 'fields')significantly improves code safety:
- Name-based parameter lookup is more resilient to argument reordering
- Explicit guard check (
if (!fieldsArg)) handles missing arguments gracefully- Eliminates non-null assertions (
!) that could hide bugs- Aligns with the new attribute argument extraction API
|
Claude finished @ymc9's task —— View job PR Review for v3.0.0-beta.21I've completed a comprehensive review of this release PR. Here are my findings: ✅ Strong PointsPostgreSQL Custom Schema Support - Excellent Implementation
Code Quality
Test Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/language/src/validators/datasource-validator.ts (1)
41-64: Prevent cascading validation errors when provider is invalid.When
providerValueis undefined (because the provider field is missing or not a string literal), the subsequent checks on lines 44 and 60 still execute, producing confusing error messages aboutdefaultSchemaandschemasfields even though the root cause is an invalid provider.Apply this diff to return early when provider validation fails:
} else if (!SUPPORTED_PROVIDERS.includes(providerValue)) { accept( 'error', `Provider "${providerValue}" is not supported. Choose from ${SUPPORTED_PROVIDERS.map( (p) => '"' + p + '"', ).join(' | ')}.`, { node: provider.value }, ); + return; } const defaultSchemaField = ds.fields.find((f) => f.name === 'defaultSchema');Alternatively, guard the subsequent checks:
const defaultSchemaField = ds.fields.find((f) => f.name === 'defaultSchema'); let defaultSchemaValue: string | undefined; if (defaultSchemaField) { - if (providerValue !== 'postgresql') { + if (providerValue && providerValue !== 'postgresql') { accept('error', '"defaultSchema" is only supported for "postgresql" provider', { node: defaultSchemaField, }); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/language/src/validators/datasource-validator.ts(2 hunks)packages/sdk/src/prisma/prisma-schema-generator.ts(7 hunks)tests/e2e/orm/client-api/pg-custom-schema.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/sdk/src/prisma/prisma-schema-generator.ts (3)
packages/sdk/src/prisma/prisma-builder.ts (2)
SimpleField(6-6)Model(115-164)packages/language/src/utils.ts (2)
getAllAttributes(546-570)getStringLiteral(75-77)packages/language/src/generated/ast.ts (3)
Model(559-563)Model(565-565)isDataSource(417-419)
tests/e2e/orm/client-api/pg-custom-schema.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(53-182)
packages/language/src/validators/datasource-validator.ts (3)
packages/language/src/utils.ts (1)
getStringLiteral(75-77)packages/language/src/constants.ts (1)
SUPPORTED_PROVIDERS(4-11)packages/language/src/generated/ast.ts (2)
isConfigArrayExpr(290-292)isLiteralExpr(133-135)
⏰ 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). (3)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (5)
tests/e2e/orm/client-api/pg-custom-schema.test.ts (1)
1-261: LGTM! Comprehensive test coverage for PostgreSQL schema support.The test suite thoroughly validates PostgreSQL multi-schema functionality:
- Default public schema behavior for both ORM and query builder operations
- Custom schema configuration via
@@schemaattribute- Provider-specific validation (PostgreSQL-only)
- Cross-field validation (defaultSchema must be in schemas array)
- Partial schema specification with fallback to defaultSchema
The SQL logging approach effectively verifies that the correct schema-qualified table names are emitted in generated queries.
packages/sdk/src/prisma/prisma-schema-generator.ts (4)
68-69: Good separation of concerns for ZModel-specific fields.Defining
NON_PRISMA_DATASOURCE_FIELDScleanly separates fields that exist only in ZModel from those that should be passed through to Prisma schema, ensuring the generated Prisma schema remains valid.
113-119: Correct filtering of datasource fields for Prisma schema.The filter correctly excludes ZModel-specific fields (like
defaultSchema) from the generated Prisma datasource declaration, as Prisma doesn't recognize these fields.
185-204: Smart default for multi-schema PostgreSQL setups.Automatically adding a
@@schemaattribute when the datasource declaresschemasensures all models are explicitly assigned to a schema (usingdefaultSchemaor 'public' as fallback), preventing ambiguity in multi-schema configurations.
216-228: Clean helper methods for datasource field access.These utility methods provide a clear, reusable way to access datasource configuration, with sensible defaults (
'public'whendefaultSchemais not specified).
…ult schema (#391) * fix: stricter default schema validation and proper transformation to Prisma schema * feat(orm): support enum name mappings * fix db pusher
|
Claude finished @ymc9's task —— View job PR Review for v3.0.0-beta.21I've completed a comprehensive review of this release PR. Here are my findings: ✅ Strong PointsPostgreSQL Custom Schema Support - Excellent Implementation
Code Quality
Test Coverage
|
Summary by CodeRabbit
New Features
Tests
Chores