test: migrate more migration cases, a few minor fixes #293
Conversation
|
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. WalkthroughCommented out public Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used📓 Path-based instructions (1){packages,samples,tests}/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)packages/language/src/zmodel-scope.ts (1)
⏰ 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)
🔇 Additional comments (2)
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 encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates additional test cases from the v1 test suite to the v2 migrated tests and includes some minor fixes to the codebase.
- Migrates 20+ regression test cases from various issues to the v2 test structure
- Fixes a bug with
@updatedAtfields being updated even when only relational data changes - Adds support for optional
whereclause in to-one relation upserts - Comments out field-level policy attributes in the standard library
Reviewed Changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/regression/test/v2-migrated/*.test.ts | Migrated regression test cases for various GitHub issues |
| tests/regression/package.json | Added decimal.js dependency and reordered devDependencies |
| tests/e2e/orm/client-api/update.test.ts | Added test case for @updatedat field behavior |
| packages/runtime/src/client/crud/validator/index.ts | Made upsert where clause optional for to-one relations |
| packages/runtime/src/client/crud/operations/base.ts | Fixed @updatedat logic and improved entity ID resolution |
| packages/language/res/stdlib.zmodel | Commented out field-level policy attributes |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude finished @ymc9's task —— View job Pull Request ReviewReviewing PR #293: "test: migrate more migration cases, a few minor fixes" Review Checklist
Analysis SummaryI've analyzed the core runtime changes in this PR. Here are the key findings: 🔧 Bug Fixes Identified:
📝 Test Migration Quality: ✅ Copilot Issue Verified: The identified issue on line 21 of ✅ Test Coverage: 20+ regression test cases migrated covering:
✅ Test Structure: Migrated tests properly use:
🔒 Security Assessment: ✅ Access Control Improvements: Commenting out field-level policy attributes prevents potential security holes from incomplete implementations ✅ Input Validation: No security regressions identified in validation logic changes ⚡ Performance Considerations: ✅ Minimal Impact: Changes are focused on correctness rather than performance
🏗️ Code Quality: ✅ Well-Structured Changes:
Recommendation: ✅ APPROVEThis PR successfully migrates important regression test cases and includes valuable bug fixes that improve the correctness of @updatedat behavior and relation handling. The changes are conservative, well-tested, and maintain backward compatibility. |
![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 (15)
tests/regression/test/v2-migrated/issue-735.test.ts (1)
4-19: Consider adding a comment explaining what issue 735 addressed.The test follows the common regression test pattern of verifying that schema loading doesn't throw an error. However, the test name doesn't explain what specific aspect of issue 735 is being verified. The schema notably uses potentially problematic identifiers (
importas a reserved keyword,viewas both a field name and model name), which suggests this is testing that these cases are now handled correctly.Consider adding a brief comment above the test to document what was fixed:
+// Issue 735: Verify that reserved keywords (import) and field names +// matching model names (view) are handled correctly in schema definitions it('verifies issue 735', async () => {Alternatively, if you'd like to make the test intent more explicit, you could verify that the schema loads successfully:
it('verifies issue 735', async () => { - await loadSchema( + const schema = await loadSchema( ` model MyModel { id String @id @default(cuid()) view String import Int } model view { id String @id @default(cuid()) name String } `, ); + // Verify schema loaded successfully without throwing + expect(schema).toBeDefined(); });tests/regression/test/v2-migrated/issue-971.test.ts (1)
4-22: Consider adding explicit assertions or test documentation.The test implicitly verifies that the schema loads successfully (no exception thrown), but the intent isn't immediately clear. The inline comments suggest different behaviors across inheritance levels ("works" vs "doesn't work"), yet nothing explicitly verifies these claims.
Consider one of the following improvements:
- Add a comment explaining what issue 971 was and what this test specifically verifies (e.g., "Regression test for issue 971: validates that @@Validate rules work across 3+ levels of abstract model inheritance")
- Add explicit runtime validation checks to verify the rules actually work (e.g., create an instance of
Fooand confirm validation triggers)- If the issue was purely about schema compilation, add an assertion like
expect(schema).toBeDefined()to make the success criterion explicitExample with documentation:
it('verifies issue 971', async () => { + // Regression test for issue 971: @@validate rules should work across multiple inheritance levels await loadSchema(Or with runtime verification:
it('verifies issue 971', async () => { const { enhance } = await loadSchema( ` abstract model Level1 { id String @id @default(cuid()) URL String? @@validate(URL != null, "URL must be provided") } abstract model Level2 extends Level1 { @@validate(URL != null, "URL must be provided") } abstract model Level3 extends Level2 { @@validate(URL != null, "URL must be provided") } model Foo extends Level3 { } `, ); const db = enhance(); // Verify validation at Level3 works (should reject null URL) await expect(db.foo.create({ data: { id: 'test' } })).rejects.toThrow('URL must be provided'); });tests/regression/test/v2-migrated/issue-765.test.ts (1)
17-20: Consider renaming back-relation field for consistency.The back-relation field
Userin the Post model uses a capitalized name, which is unconventional for Prisma schemas. Typically, field names should be lowercase (or camelCase), with capitalization reserved for model names and types.Consider applying this diff:
model Post { id Int @id @default(autoincrement()) title String - User User[] + users User[] @@allow('all', true) }tests/regression/test/v2-migrated/issue-965.test.ts (2)
5-28: Consider more descriptive test names.The test name
regression1doesn't convey what aspect of issue 965 is being tested. Consider renaming to something likeshould load schema with models inheriting ID from composed Base typeto improve clarity for future maintainers.
30-52: Consider more descriptive test names.Similar to the first test,
regression2doesn't convey the intent. Consider renaming to something likeshould fail when models lack unique criteria from composed typesto improve readability.tests/regression/test/v2-migrated/issue-864.test.ts (3)
5-63: Add assertion or clarify test intent.The test executes nested update operations but lacks any assertions to verify the result. For regression tests, it's good practice to either:
- Add assertions verifying the expected outcome (e.g., that C was created successfully with correct values), or
- Add a comment explaining that the test simply verifies the operation completes without throwing an error
Additionally, consider documenting what issue 864 was and what "safe create" means in this context to improve test maintainability.
65-124: Add assertion and consider reducing schema duplication.Similar to the first test, this lacks assertions to verify behavior. The test name "unsafe create nested in to-many" suggests this might test an edge case or potential issue (possibly related to explicit
idassignment on line 113), but without assertions or documentation, the expected behavior is unclear.Consider:
- Adding assertions or a comment explaining the test's purpose
- Refactoring the duplicated schema definition into a shared constant or helper function to reduce maintenance burden
126-182: Add assertion and clarify the to-one variant test.This test exercises a to-one relation update (note
B?and@uniqueonaId), which is an important variant to test separately. However, like the other tests, it lacks assertions to verify the expected outcome.The use of
update: { data: {...} }(line 167) instead of an array syntax is appropriate for to-one relations, but documenting why this variant is considered "unsafe" would help future maintainers understand the test's purpose.tests/regression/test/v2-migrated/issue-764.test.ts (1)
4-46: Consider adding assertions to verify the results.The test performs operations but doesn't include assertions to verify the outcome. While regression tests often pass simply by not throwing errors, adding explicit assertions would improve test quality and make the expected behavior clearer.
Consider adding assertions after the update operation to verify:
- The post was created/updated successfully
- The user's
postIdis set correctly- The post's title matches the expected value
const updatedUser = await db.user.findUnique({ where: { id: user.id }, include: { post: true }, }); expect(updatedUser?.post).toBeDefined(); expect(updatedUser?.post?.title).toBe('Hello World');tests/regression/test/v2-migrated/issue-925.test.ts (1)
38-68: Test logic is correct.The test properly validates successful schema loading when using
thisto disambiguate the field reference. The schema duplication between tests keeps each test self-contained, which aids readability for regression tests.If you prefer to reduce duplication, consider extracting a helper function that generates the schema with a parameterized access control rule:
function createSchema(accessRule: string) { return ` model User { id Int @id @default(autoincrement()) company Company[] test Int @@allow('read', ${accessRule}) } model Company { id Int @id @default(autoincrement()) user User @relation(fields: [userId], references: [id]) userId Int staff Staff[] @@allow('read', true) } model Staff { id Int @id @default(autoincrement()) company Company @relation(fields: [companyId], references: [id]) companyId Int @@allow('read', true) } `; }Then use
createSchema("auth().company?[staff?[companyId == test]]")andcreateSchema("auth().company?[staff?[companyId == this.test]]")in the respective tests.tests/regression/test/v2-migrated/issue-674.test.ts (1)
4-14: Consistent enum formatting and add test summary.
- Reformat the enum into block style (one value per line) to match other regression tests.
- Prepend a brief comment explaining “Issue 674: ensure unused enums with @@Map don’t crash schema parsing.”
tests/regression/test/v2-migrated/issue-689.test.ts (1)
43-50: Consider clarifying the intent of theconnectoperation.On line 47, you're connecting UserRole id=1 (which already has
userId=1from lines 34-36) to the newly created user with id=2. In Prisma, this will update the foreign key, effectively moving the Admin role from user 1 to user 2.Final state after both creates:
- User id=1: only has UserRole id=2 (Student)
- User id=2: only has UserRole id=1 (Admin)
If this is intentional to test a specific edge case in issue 689, consider adding a comment explaining the role reassignment. If not intended, you might want to create a new UserRole instead.
tests/regression/test/v2-migrated/issue-756.test.ts (1)
18-18: Consider usingStringfor the email field for better realism.The email field is typed as
Int, which is unusual. While this doesn't affect the test's correctness (the test focuses on the policy error on line 26), usingStringwould make the schema more realistic and less confusing for future readers.Apply this diff for improved clarity:
- email Int + email Stringtests/regression/test/v2-migrated/issue-825.test.ts (1)
4-23: Consider a more descriptive test name.The test name 'verifies issue 825' doesn't convey what authorization scenario is being tested. A more descriptive name would improve test readability and maintenance.
Example:
-it('verifies issue 825', async () => { +it('allows admin to update customer role to staff', async () => {Or include the issue reference if preferred:
-it('verifies issue 825', async () => { +it('issue-825: admin can update customer role without triggering deny rules', async () => {tests/regression/test/v2-migrated/issue-714.test.ts (1)
87-145: Avoid hard-coding auto-increment IDsThe seeding relies on
id: 1for both the user and property. That’s brittle—any future fixture change (extra records, different seed order, non-reset sequences) will break the test for reasons unrelated to issue 714. Grab the IDs returned by the create calls and reuse them instead.- await db.user.create({ + const { id: userId } = await db.user.create({ data: { username: '[email protected]', }, }); @@ - user: { - connect: { - id: 1, - }, - }, + user: { + connect: { + id: userId, + }, + }, }, }, propertyUsers: { connect: { - id: 1, + id: userId, }, }, @@ - await db.property.update({ + const { id: propertyId } = await db.property.findFirstOrThrow({ + select: { id: true }, + }); + + await db.property.update({ @@ - connect: { - id: 1, - }, + connect: { + id: userId, + }, }, }, }, }, where: { - id: 1, + id: propertyId, }, });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
packages/language/res/stdlib.zmodel(2 hunks)packages/runtime/src/client/crud/operations/base.ts(2 hunks)packages/runtime/src/client/crud/validator/index.ts(1 hunks)tests/e2e/orm/client-api/update.test.ts(1 hunks)tests/regression/package.json(1 hunks)tests/regression/test/v2-migrated/issue-657.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-665.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-674.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-689.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-703.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-714.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-735.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-756.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-764.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-765.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-804.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-811.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-814.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-825.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-864.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-866.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-925.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-947.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-961.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-965.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-971.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-992.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/src/client/crud/validator/index.tstests/regression/test/v2-migrated/issue-925.test.tstests/regression/test/v2-migrated/issue-825.test.tstests/regression/test/v2-migrated/issue-735.test.tstests/regression/test/v2-migrated/issue-764.test.tstests/regression/test/v2-migrated/issue-866.test.tstests/regression/test/v2-migrated/issue-804.test.tstests/regression/test/v2-migrated/issue-674.test.tstests/regression/test/v2-migrated/issue-657.test.tstests/regression/test/v2-migrated/issue-689.test.tstests/regression/test/v2-migrated/issue-714.test.tstests/e2e/orm/client-api/update.test.tstests/regression/test/v2-migrated/issue-992.test.tstests/regression/test/v2-migrated/issue-864.test.tspackages/language/res/stdlib.zmodeltests/regression/test/v2-migrated/issue-814.test.tstests/regression/test/v2-migrated/issue-765.test.tstests/regression/test/v2-migrated/issue-961.test.tstests/regression/test/v2-migrated/issue-703.test.tstests/regression/package.jsontests/regression/test/v2-migrated/issue-665.test.tstests/regression/test/v2-migrated/issue-947.test.tstests/regression/test/v2-migrated/issue-756.test.tstests/regression/test/v2-migrated/issue-971.test.tstests/regression/test/v2-migrated/issue-811.test.tstests/regression/test/v2-migrated/issue-965.test.tspackages/runtime/src/client/crud/operations/base.ts
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
End-to-end tests must live under
tests/e2e/
Files:
tests/e2e/orm/client-api/update.test.ts
🧬 Code graph analysis (16)
tests/regression/test/v2-migrated/issue-825.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-764.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-866.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-167)
tests/regression/test/v2-migrated/issue-657.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-167)
tests/regression/test/v2-migrated/issue-689.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-714.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/e2e/orm/client-api/update.test.ts (1)
tests/e2e/orm/client-api/utils.ts (1)
createUser(6-21)
tests/regression/test/v2-migrated/issue-992.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-864.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-814.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-765.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-961.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-703.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-665.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-811.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
packages/runtime/src/client/crud/operations/base.ts (2)
packages/runtime/src/client/query-builder.ts (1)
ToKysely(19-19)packages/sdk/src/schema/schema.ts (1)
GetModels(109-109)
🔇 Additional comments (31)
tests/regression/test/v2-migrated/issue-735.test.ts (1)
1-2: LGTM!The imports are appropriate for a ZenStack regression test using Vitest.
tests/regression/test/v2-migrated/issue-866.test.ts (2)
1-2: LGTM!Standard test utilities imported appropriately.
4-22: Verify that the expected API shape and behavior match the planned implementation.The test is well-structured as a placeholder for future Zod schema support. However, please confirm:
API shape: The test expects
createTestClientto return an object with azodSchemasproperty, but the current implementation returns aZenStackClientinstance. Is this the planned API when Zod support is added?Default value behavior: The test expects Zod schema parsing of
{ id: 1 }to automatically apply Prisma/database defaults (a=100, b='', c=now()). Should Zod schemas generated by ZenStack include logic to apply these defaults during parsing, or should they only validate that provided values match the schema?These questions help ensure the test accurately reflects the intended feature implementation when the TODO is addressed.
tests/regression/test/v2-migrated/issue-992.test.ts (4)
1-2: LGTM!The imports are correct and use the appropriate testing utilities.
4-24: Test appropriately skipped pending feature implementation.The test is correctly skipped with a clear TODO comment indicating that global omit support is required. The schema definition is valid, with appropriate policy rules and the
@omitattribute on thedeletedfield.
26-39: LGTM!The use of
$unuseAll()to bypass policies during test data setup is appropriate, ensuring the category and products are created regardless of policy rules. The nested creation syntax is correct.
41-43: LGTM!The assertions correctly verify that the
deletedfield is omitted from the query results, which is the expected behavior once global omit support is implemented.tests/regression/test/v2-migrated/issue-765.test.ts (1)
4-35: Verify this test correctly reproduces issue 765.The test logic appears sound: it creates a user with an explicitly undefined relation field and asserts that the field remains undefined in the result. However, please confirm that:
- This test fails without the fix for issue 765
- The test passes with the fix applied
- The test accurately captures the root cause described in issue 765
You can verify the test's validity by temporarily reverting the fix for issue 765 and confirming the test fails as expected.
tests/regression/test/v2-migrated/issue-965.test.ts (1)
1-4: LGTM!The imports and test suite declaration are correct and follow standard Vitest patterns.
tests/regression/test/v2-migrated/issue-665.test.ts (4)
1-2: LGTM!The imports are correct and appropriate for a vitest-based policy test.
8-12: Verify field-level policy syntax when implementing support.The field-level policy
@allow("all", auth() == this)compares the entire auth context object to the entire User model instance. This comparison will typically fail because it compares object references rather than specific fields.When field-level policy support is implemented (per the TODO on Line 4), verify whether the intended syntax should be:
@allow("all", auth().id == this.id)to compare the authenticated user's ID with the User record's ID.
Additionally, confirm that the
@passwordattribute on Line 12 is properly defined in the stdlib or schema, as this appears to be a custom attribute.
21-21: LGTM!Correct use of
$unuseAllto bypass policy checks during test setup.
23-37: LGTM!The test scenarios appropriately cover the four authorization contexts (admin, owner, anonymous, non-owner) and validate the expected field-level visibility behavior. The assertions correctly expect the
usernamefield to be visible for admin and owner contexts, and undefined for anonymous and non-owner contexts.tests/regression/test/v2-migrated/issue-864.test.ts (1)
1-2: LGTM!The imports are correct and appropriately used throughout the test file.
tests/regression/test/v2-migrated/issue-764.test.ts (4)
1-2: LGTM!The imports are appropriate for a policy-based regression test.
7-23: Verify the relation pattern matches issue 764's intent.The schema defines an unusual relation pattern where:
Userhas an optional to-one relation toPost(viapostId)Posthas a one-to-many back-relation toUserThis allows multiple Users to reference the same Post. While technically valid, ensure this pattern matches the original issue 764 scenario. Typically, relations are either one-to-many from Post to User or one-to-many from User to Post.
27-29: Verify if$unuseAll()is necessary.The user creation bypasses policies with
$unuseAll(), but the schema already allows all operations via@@allow('all', true). This call appears unnecessary unless it's required to reproduce the original issue 764 scenario or test a specific code path.
31-45: LGTM!The nested upsert on the to-one
postrelation without awhereclause correctly exercises the validator adjustment mentioned in the PR summary. This is the core behavior that issue 764 is testing.tests/regression/test/v2-migrated/issue-947.test.ts (2)
1-2: LGTM!Imports are correct and necessary for the test implementation.
4-23: Confirm test scope for issue 947. This test only ensures the schema loads without errors; verify whether the issue also required specific runtime behavior assertions and add them if needed.tests/regression/test/v2-migrated/issue-925.test.ts (3)
1-2: LGTM!The imports are appropriate for a regression test using vitest and ZenStack testtools.
4-69: Well-structured regression test.The test suite properly covers both the error case (missing
thisreference) and the success case (correctthisusage), following standard vitest patterns.
5-36: Correct validation of reference resolution error.The test accurately verifies that omitting
thiswhen referencing a field in a nested access control filter produces the expected resolution error.tests/regression/test/v2-migrated/issue-689.test.ts (3)
1-2: LGTM!Standard imports for a policy-driven regression test.
4-27: Verify if an auth context is needed.The schema includes auth()-based policy rules (lines 22-24), but no auth context is provided to
createPolicyTestClient. Without an auth context, these rules won't apply, and only the@@allow('create,read', true)rule (line 21) will permit reads.This might be intentional if testing unauthenticated access, but please confirm that the test accurately reproduces issue 689.
52-71: LGTM!The count queries correctly verify that policy-filtered results match raw database results, ensuring that authorization rules don't incorrectly affect count operations.
packages/language/res/stdlib.zmodel (2)
695-695: Field-level @deny attribute is unused. Search across all .zmodel files found no single-@denyoccurrences—only model-level@@deny. Safe to remove/comment out.
679-679: Field-level@allowis unused; removal is non-breaking. Removing the commented-out single-@allowattribute does not affect any existing ZModel files, as no schemas use field-level@allow.Likely an incorrect or invalid review comment.
tests/regression/test/v2-migrated/issue-756.test.ts (1)
4-31: LGTM! Test correctly verifies the error condition.The test properly uses
loadSchemaWithErrorto verify that the invalid policy expression on line 26 (auth().posts.authorId == authorId) produces the expected error message. The expression is correctly identified as invalid becausepostsis a collection, and accessingauthorIddirectly on it is not supported.tests/regression/test/v2-migrated/issue-825.test.ts (2)
1-2: LGTM!The imports are correct and appropriate for a policy-based regression test.
25-39: LGTM!The test execution correctly validates the authorization scenario:
- Uses
$unuseAll()to bypass policies during test setup- Sets admin context with
$setAuth(admin)- Verifies that an admin can update a customer's role to 'staff' without triggering deny rules
- The assertion confirms the expected outcome
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tests/regression/test/v2-migrated/issue-814.test.ts (1)
27-40: Consider adding positive test case and null checks for completeness.The test only verifies the blocking behavior when
private: true. Consider adding:
- A positive case where
private: falseto verify the field IS accessible when permitted- Null checks before accessing
r.nameandr1.profile.nameto ensure the objects exist and only the field is filteredThese additions would make the test more comprehensive and reduce false positives if the implementation blocks the entire profile rather than just filtering the field.
Example additions:
expect(r1.profile.name).toBeUndefined(); + + // Positive case: field should be accessible when private is false + const user2 = await db.$unuseAll().user.create({ + data: { profile: { create: { name: 'Bar', private: false } } }, + include: { profile: true }, + }); + + const r2 = await db.profile.findFirst({ where: { id: user2.profile.id } }); + expect(r2).toBeDefined(); + expect(r2.name).toBe('Bar'); + + const r3 = await db.user.findFirst({ + where: { id: user2.id }, + include: { profile: true }, + }); + expect(r3.profile).toBeDefined(); + expect(r3.profile.name).toBe('Bar'); });tests/regression/test/v2-migrated/issue-657.test.ts (1)
7-29: Consider expanding test coverage when enabling this test.The validation assertions are logically correct, but edge case coverage could be improved:
- Missing lower boundary test:
decimalNumber = 0.1should fail since the constraint is@gt(0.1)(strictly greater than), not@gte(0.1).- Missing mid-range values: Consider testing valid mid-range values like
0.2,1,5to ensure the constraint works correctly throughout the valid range.- Precision edge cases: Testing values like
0.10000000001and9.99999999999could reveal precision handling issues.When enabling this test, consider adding these assertions:
// Lower boundary - should fail (@gt, not @gte) expect(schema.safeParse({ decimalNumber: 0.1 }).success).toBeFalsy(); expect(schema.safeParse({ decimalNumber: '0.1' }).success).toBeFalsy(); expect(schema.safeParse({ decimalNumber: new Decimal('0.1') }).success).toBeFalsy(); // Mid-range valid values expect(schema.safeParse({ decimalNumber: 0.2 }).success).toBeTruthy(); expect(schema.safeParse({ decimalNumber: 5 }).success).toBeTruthy(); expect(schema.safeParse({ decimalNumber: 9.9 }).success).toBeTruthy();tests/regression/test/v2-migrated/issue-765.test.ts (2)
20-20: Consider renaming for Prisma naming conventions.The field
User User[]uses a capitalized name that matches the model name, which is unconventional. Prisma typically uses lowercase field names for relations.Apply this diff to follow Prisma naming conventions:
model Post { id Int @id @default(autoincrement()) title String - User User[] + users User[] @@allow('all', true) }
27-34: Consider explicitly including the relation to strengthen the test.Without
include: { post: true }, Prisma doesn't include the relation in the result by default, sor.postwill always beundefinedregardless of whether the relation was set in the database. Explicitly including the relation would make the assertion more robust.Apply this diff to explicitly include the relation:
const r = await db.user.create({ data: { name: 'Me', post: undefined, }, + include: { + post: true, + }, }); expect(r.name).toBe('Me'); -expect(r.post).toBeUndefined(); +expect(r.post).toBeNull();Note: The expected value should be
nullrather thanundefinedwhen the relation is explicitly included but not set.tests/regression/test/v2-migrated/issue-811.test.ts (1)
4-70: Test logic is sound; schema modeling is unconventional but likely intentional.The test correctly exercises nested updates with policy enforcement. A few observations:
Schema modeling: The relationship between User and Profile is one-to-one (via
@uniqueonprofileId), but Profile uses plural namingusers User[]for the back-relation. While unconventional, this is valid in Prisma and may be intentional for testing edge cases in issue 811.Policy quirk: Line 34's policy
users?[id == auth().userId]treatsusersas a collection despite the one-to-one relation. This works but is unusual given the constraint.Test coverage: The assertions verify the Membership role update and nested Profile firstName update. Optionally, you could add an assertion on the User entity to confirm it wasn't corrupted during the nested update, but this is a minor point.
Optional enhancement:
Consider adding an assertion to verify the User entity itself:
expect(r1.role).toBe('VIP'); +expect(r1.user.id).toBe(userId); expect(r1.user.profile.firstName).toBe('Jerry');tests/regression/test/v2-migrated/issue-864.test.ts (1)
1-183: Tests validate nested create operations correctly.The three test cases effectively cover different scenarios for nested creates with policy enforcement. While the tests lack explicit assertions, this is acceptable for regression tests that primarily validate operation success without throwing errors.
Consider adding brief inline comments to clarify the distinction between "safe" and "unsafe" scenarios, particularly regarding the presence or absence of explicit IDs in nested creates.
tests/regression/test/v2-migrated/issue-689.test.ts (1)
4-4: Consider adding context about what issue 689 tests.The test description "verifies issue 689" lacks context. Consider adding a brief comment or more descriptive test name explaining what specific behavior or bug this test verifies.
For example:
-it('verifies issue 689', async () => { +it('verifies issue 689 - policy count matches raw count with nested relation filters', async () => {Or add a comment above the test:
// Issue 689: Policy-enabled count queries with nested relation filters should // return the same results as raw queries when policies allow all reads it('verifies issue 689', async () => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
packages/language/res/stdlib.zmodel(2 hunks)packages/runtime/src/client/crud/operations/base.ts(2 hunks)packages/runtime/src/client/crud/validator/index.ts(1 hunks)tests/e2e/orm/client-api/update.test.ts(1 hunks)tests/regression/package.json(1 hunks)tests/regression/test/v2-migrated/issue-657.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-665.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-674.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-689.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-703.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-714.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-735.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-756.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-764.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-765.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-804.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-811.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-814.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-825.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-864.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-866.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-925.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-947.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-961.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-965.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-971.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-992.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
tests/regression/test/v2-migrated/issue-689.test.tspackages/runtime/src/client/crud/validator/index.tstests/regression/test/v2-migrated/issue-971.test.tstests/regression/test/v2-migrated/issue-714.test.tstests/regression/test/v2-migrated/issue-804.test.tstests/regression/test/v2-migrated/issue-657.test.tstests/regression/test/v2-migrated/issue-811.test.tstests/regression/test/v2-migrated/issue-765.test.tstests/e2e/orm/client-api/update.test.tstests/regression/test/v2-migrated/issue-825.test.tstests/regression/test/v2-migrated/issue-665.test.tspackages/language/res/stdlib.zmodeltests/regression/test/v2-migrated/issue-764.test.tstests/regression/test/v2-migrated/issue-703.test.tstests/regression/test/v2-migrated/issue-947.test.tstests/regression/test/v2-migrated/issue-735.test.tstests/regression/test/v2-migrated/issue-992.test.tstests/regression/test/v2-migrated/issue-961.test.tstests/regression/test/v2-migrated/issue-925.test.tstests/regression/test/v2-migrated/issue-965.test.tstests/regression/test/v2-migrated/issue-864.test.tstests/regression/test/v2-migrated/issue-814.test.tstests/regression/test/v2-migrated/issue-756.test.tspackages/runtime/src/client/crud/operations/base.tstests/regression/test/v2-migrated/issue-866.test.tstests/regression/package.jsontests/regression/test/v2-migrated/issue-674.test.ts
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
End-to-end tests must live under
tests/e2e/
Files:
tests/e2e/orm/client-api/update.test.ts
🧬 Code graph analysis (16)
tests/regression/test/v2-migrated/issue-689.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-714.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-657.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-167)
tests/regression/test/v2-migrated/issue-811.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-765.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/e2e/orm/client-api/update.test.ts (1)
tests/e2e/orm/client-api/utils.ts (1)
createUser(6-21)
tests/regression/test/v2-migrated/issue-825.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-665.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-764.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-703.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-992.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-961.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-864.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-814.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
packages/runtime/src/client/crud/operations/base.ts (2)
packages/runtime/src/client/query-builder.ts (1)
ToKysely(19-19)packages/sdk/src/schema/schema.ts (1)
GetModels(109-109)
tests/regression/test/v2-migrated/issue-866.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-167)
⏰ 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 (21)
tests/regression/test/v2-migrated/issue-814.test.ts (1)
4-5: LGTM! Appropriate skip pattern for unimplemented feature.The TODO comment clearly documents why the test is skipped, and skipping is the correct approach for a regression test covering functionality that isn't implemented yet.
tests/regression/test/v2-migrated/issue-971.test.ts (1)
4-22: Clarify test intent and add explicit assertions.
- The test only calls
loadSchema()with no assertions, so URL validation (especially on Level3) isn’t actually verified.- The “doesn’t work” comment on Level3 is misleading because there’s no runtime check.
- Rename the test to describe its goal (e.g. “should validate URL across multi-level inheritance”).
- If you’re verifying validation behavior, instantiate
Foowith/without a URL and assert success or expected errors.- If you only intend to test schema loading, update the comments and test name to reflect that.
tests/regression/test/v2-migrated/issue-657.test.ts (2)
1-3: LGTM!Imports are appropriate for the test's requirements.
5-6: Skip is appropriate pending Zod schema support. NozodSchemasdefinitions exist; keep theit.skipand TODO untilcreateTestClientis extended to returnzodSchemas.models.FooUpdateSchema, then remove the skip.tests/regression/test/v2-migrated/issue-765.test.ts (1)
1-35: LGTM! Regression test structure is solid.The test correctly verifies that creating a user with an explicit
post: undefinedrelation field works as expected. The use ofcreatePolicyTestClientwith an inline schema is appropriate for this regression test.tests/regression/test/v2-migrated/issue-811.test.ts (1)
1-2: LGTM!Standard imports for a policy-enabled regression test using vitest and testtools.
tests/regression/test/v2-migrated/issue-804.test.ts (1)
4-32: Solid regression guardThis pins the operand-type error we hit before; thanks for adding the safeguard.
tests/regression/test/v2-migrated/issue-947.test.ts (1)
4-22: Good schema sanity checkCovers the enum default case nicely and keeps the migration suite comprehensive.
tests/regression/test/v2-migrated/issue-992.test.ts (1)
26-42: Confirm$unuseAllchaining behavior
await db.$unuseAll().category.create(...)presumes$unuseAll()synchronously returns the client instance. If it instead yields a promise/void,.categorywill be undefined at runtime when this test is eventually unskipped. Can you double-check the helper’s signature or refactor toawait db.$unuseAll(); await db.category.create(...)to be safe?tests/regression/test/v2-migrated/issue-756.test.ts (1)
4-30: Thanks for pinning the missing member referenceLocks in the error message we expect for the mis-specified policy path—appreciated.
tests/regression/test/v2-migrated/issue-925.test.ts (1)
1-69: LGTM! Well-structured regression test.The test effectively covers both the error case (member reference without
this) and the success case (member reference withthis). The expected error message is specific and actionable.tests/regression/test/v2-migrated/issue-961.test.ts (1)
1-211: LGTM! Comprehensive test coverage.The test suite effectively validates nested
deleteManyandupdateManyoperations with policy enforcement, including edge cases like non-existing filters and policy-denied operations. The assertions clearly verify expected behavior.tests/regression/test/v2-migrated/issue-674.test.ts (1)
1-14: LGTM! Simple and effective regression test.The test validates that schemas with unused enums (including the
@@mapattribute) load successfully without errors.tests/regression/test/v2-migrated/issue-735.test.ts (1)
1-19: LGTM! Tests handling of reserved words as identifiers.The test validates that the schema parser correctly handles potentially reserved words (
importas a field name on line 10 andviewas a model name on line 13) as valid identifiers.tests/regression/test/v2-migrated/issue-703.test.ts (1)
1-26: Appropriately skipped test aligns with stdlib changes.The test is correctly marked as skipped with a clear TODO comment indicating field-level policy support is not yet available. This aligns with the changes in
packages/language/res/stdlib.zmodelwhere field-level@allowand@denyattributes were commented out (lines 679, 695).packages/language/res/stdlib.zmodel (2)
679-679: Confirm deliberate deferral of field-level policies.Commented-out field-level
@allow/@denyattributes instdlib.zmodeland the skipped test inissue-703.test.tsindicate field-level policy support is intentionally disabled—a breaking change for users relying on this feature.Please confirm:
- Is this deferral temporary or a permanent removal?
- Are there migration notes or deprecation warnings for existing users?
- If the feature isn’t returning, should the commented code be removed entirely?
695-695: Confirm intentional removal of field-level@denyand document it. The commented-out single-@denyattribute drops support for field-level denial policies—this is a breaking change that must be noted in the release notes.tests/regression/test/v2-migrated/issue-689.test.ts (4)
43-50: Verify intended data setup: connecting existing UserRole to new User.The
connect: { id: 1 }operation at line 47 links UserRole id=1 (originally created under User 1 with role='Admin') to User 2. In Prisma's one-to-many relations, this updates UserRole id=1'suserIdfrom 1 to 2.Result after seeding:
- User 1 has UserRole id=2 (Student)
- User 2 has UserRole id=1 (Admin)
Impact on the test:
The query filters foruserRole.some { role: 'Student' }, which matches only User 1.If this is intentional for testing issue 689's specific scenario, consider adding a comment explaining the setup. Otherwise, verify whether this reflects the intended test data structure.
29-29: LGTM: Correctly obtains policy-disabled client.Using
$unuseAll()to get a raw database client for comparison is the correct approach for testing policy behavior.
52-70: LGTM: Query logic and assertion are sound.The test correctly:
- Executes identical count queries on both raw and policy-enabled clients
- Filters for users with
userRole.some { role: 'Student' }andNOT { deleted: true }- Asserts equality to verify policy enforcement doesn't incorrectly filter results
Given the
@@allow('create,read', true)rule at line 21, all reads are permitted, so the counts should match regardless of the missing auth context.
22-24: No changes needed: direct object/relation comparisons are supported
Existing tests (e.g. issue-811.test.ts, issue-703.test.ts) use@@allow('read', auth() == this)and@@allow(..., user == auth())successfully.
|
Claude finished @ymc9's task —— View job Pull Request ReviewReviewing PR #293: "test: migrate more migration cases, a few minor fixes" Review Checklist
Analysis SummaryI've analyzed the core runtime changes in this PR. Here are the key findings: 🔧 Bug Fixes Identified:
📝 Test Migration Quality: ✅ Copilot Issue Verified: The identified issue on line 21 of ✅ Test Coverage: 20+ regression test cases migrated covering:
✅ Test Structure: Migrated tests properly use:
🔒 Security Assessment: ✅ Access Control Improvements: Commenting out field-level policy attributes prevents potential security holes from incomplete implementations ✅ Input Validation: No security regressions identified in validation logic changes ⚡ Performance Considerations: ✅ Minimal Impact: Changes are focused on correctness rather than performance
🏗️ Code Quality: ✅ Well-Structured Changes:
Recommendation: ✅ APPROVEThis PR successfully migrates important regression test cases and includes valuable bug fixes that improve the correctness of @updatedat behavior and relation handling. The changes are conservative, well-tested, and maintain backward compatibility. |
|
Claude finished @ymc9's task —— View job Pull Request ReviewReviewing PR #293: "test: migrate more migration cases, a few minor fixes" Review Checklist
Analysis SummaryI've analyzed the core runtime changes in this PR. Here are the key findings: 🔧 Bug Fixes Identified:
📝 Test Migration Quality: ✅ Copilot Issue Verified: The identified issue on line 21 of ✅ Test Coverage: 20+ regression test cases migrated covering:
✅ Test Structure: Migrated tests properly use:
🔒 Security Assessment: ✅ Access Control Improvements: Commenting out field-level policy attributes prevents potential security holes from incomplete implementations ✅ Input Validation: No security regressions identified in validation logic changes ⚡ Performance Considerations: ✅ Minimal Impact: Changes are focused on correctness rather than performance
🏗️ Code Quality: ✅ Well-Structured Changes:
Recommendation: ✅ APPROVEThis PR successfully migrates important regression test cases and includes valuable bug fixes that improve the correctness of @updatedat behavior and relation handling. The changes are conservative, well-tested, and maintain backward compatibility. |
|
Claude finished @ymc9's task —— View job Pull Request ReviewReviewing PR #293: "test: migrate more migration cases, a few minor fixes" Review Checklist
Analysis SummaryI've analyzed the core runtime changes in this PR. Here are the key findings: 🔧 Bug Fixes Identified:
📝 Test Migration Quality: ✅ Copilot Issue Verified: The identified issue on line 21 of ✅ Test Coverage: 20+ regression test cases migrated covering:
✅ Test Structure: Migrated tests properly use:
🔒 Security Assessment: ✅ Access Control Improvements: Commenting out field-level policy attributes prevents potential security holes from incomplete implementations ✅ Input Validation: No security regressions identified in validation logic changes ⚡ Performance Considerations: ✅ Minimal Impact: Changes are focused on correctness rather than performance
🏗️ Code Quality: ✅ Well-Structured Changes:
Recommendation: ✅ APPROVEThis PR successfully migrates important regression test cases and includes valuable bug fixes that improve the correctness of @updatedat behavior and relation handling. The changes are conservative, well-tested, and maintain backward compatibility. |
|
Claude finished @ymc9's task —— View job Pull Request ReviewReviewing PR #293: "test: migrate more migration cases, a few minor fixes" Review Checklist
Analysis SummaryI've analyzed the core runtime changes in this PR. Here are the key findings: 🔧 Bug Fixes Identified:
📝 Test Migration Quality: ✅ Copilot Issue Verified: The identified issue on line 21 of ✅ Test Coverage: 20+ regression test cases migrated covering:
✅ Test Structure: Migrated tests properly use:
🔒 Security Assessment: ✅ Access Control Improvements: Commenting out field-level policy attributes prevents potential security holes from incomplete implementations ✅ Input Validation: No security regressions identified in validation logic changes ⚡ Performance Considerations: ✅ Minimal Impact: Changes are focused on correctness rather than performance
🏗️ Code Quality: ✅ Well-Structured Changes:
Recommendation: ✅ APPROVEThis PR successfully migrates important regression test cases and includes valuable bug fixes that improve the correctness of @updatedat behavior and relation handling. The changes are conservative, well-tested, and maintain backward compatibility. |
|
Claude finished @ymc9's task —— View job Pull Request ReviewReviewing PR #293: "test: migrate more migration cases, a few minor fixes" Review Checklist
Analysis SummaryI've analyzed the core runtime changes in this PR. Here are the key findings: 🔧 Bug Fixes Identified:
📝 Test Migration Quality: ✅ Copilot Issue Verified: The identified issue on line 21 of ✅ Test Coverage: 20+ regression test cases migrated covering:
✅ Test Structure: Migrated tests properly use:
🔒 Security Assessment: ✅ Access Control Improvements: Commenting out field-level policy attributes prevents potential security holes from incomplete implementations ✅ Input Validation: No security regressions identified in validation logic changes ⚡ Performance Considerations: ✅ Minimal Impact: Changes are focused on correctness rather than performance
🏗️ Code Quality: ✅ Well-Structured Changes:
Recommendation: ✅ APPROVEThis PR successfully migrates important regression test cases and includes valuable bug fixes that improve the correctness of @updatedat behavior and relation handling. The changes are conservative, well-tested, and maintain backward compatibility. |
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Revert