fix: prisma serialisation errors#24452
Conversation
WalkthroughThe PermissionRepository permission-check query was rewritten to use a JSONB-based approach: permission pairs are serialized (JSON.stringify) and expanded via jsonb_array_elements, then validated against RolePermission rows using EXISTS per required permission. The previous multi-CASE, WITH, and UNION ALL SQL logic and intermediate arrays were removed. External behavior is unchanged: returns true only if all requested permissions are satisfied; empty-permissions validation remains. An integration test suite was added covering exact matches, wildcards, universal wildcard, empty inputs, non-existent roles, and complex multi-rule scenarios. No exported/public API signatures changed. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (1 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 |
| SELECT 1 as match FROM "RolePermission" | ||
| WHERE "roleId" = ${roleId} AND ("resource", "action") = ANY(${resourceActions}) | ||
| // Check if each requested permission is satisfied by at least one role permission | ||
| const matchingPermissions = await this.client.$queryRaw<[{ count: bigint }]>` |
There was a problem hiding this comment.
packages/features/pbac/infrastructure/repositories/PermissionRepository.ts L247
We also do this here
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/features/pbac/infrastructure/repositories/PermissionRepository.ts (2)
24-33: Useselectinstead ofincludein Prisma queries.Per project guidelines, avoid
include; select only required fields from related models.- const memberships = await this.client.membership.findMany({ - where: { userId }, - include: { - customRole: { - include: { - permissions: true, - }, - }, - }, - }); + const memberships = await this.client.membership.findMany({ + where: { userId }, + select: { + teamId: true, + customRole: { + select: { + id: true, + permissions: true, + }, + }, + }, + });As per coding guidelines
140-156: Alias JSONB element column for clarity: useAS required_perm(perm)and update references toperm->>'resource'andperm->>'action'; the existing@@unique([roleId,resource,action])already provides the needed composite index.packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration.test.ts (2)
18-45: Use a single unique suffix per test run to reduce collision risk.Date.now() called multiple times can repeat within the same ms. Generate one suffix and reuse it for email/username/slug.
- // Create test user - const testUser = await prisma.user.create({ + const suffix = `${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + // Create test user + const testUser = await prisma.user.create({ data: { - email: `test-${Date.now()}@example.com`, - username: `testuser-${Date.now()}`, + email: `test-${suffix}@example.com`, + username: `testuser-${suffix}`, }, }); @@ - const testTeam = await prisma.team.create({ + const testTeam = await prisma.team.create({ data: { - name: `Test Team ${Date.now()}`, - slug: `test-team-${Date.now()}`, + name: `Test Team ${suffix}`, + slug: `test-team-${suffix}`, }, });
66-68: Double‑check global Prisma client lifecycle in tests.Disconnecting prisma in afterAll can interfere with other test suites sharing the same client. Prefer centralized teardown or a locally instantiated client per suite.
Would you like me to generate a Vitest globalSetup/globalTeardown example to manage Prisma once?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/pbac/infrastructure/repositories/PermissionRepository.ts(1 hunks)packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration.test.tspackages/features/pbac/infrastructure/repositories/PermissionRepository.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration.test.tspackages/features/pbac/infrastructure/repositories/PermissionRepository.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration.test.tspackages/features/pbac/infrastructure/repositories/PermissionRepository.ts
**/*Repository.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Repository files must include
Repositorysuffix, prefix with technology if applicable (e.g.,PrismaAppRepository.ts), and use PascalCase matching the exported class
Files:
packages/features/pbac/infrastructure/repositories/PermissionRepository.ts
🧬 Code graph analysis (1)
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration.test.ts (2)
packages/features/pbac/infrastructure/repositories/PermissionRepository.ts (1)
PermissionRepository(15-286)packages/features/pbac/domain/types/permission-registry.ts (1)
PermissionString(67-67)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
packages/features/pbac/infrastructure/repositories/PermissionRepository.ts (1)
136-138: Good move: serialize permission pairs to JSONB.Passing a JSON string as a bound parameter and casting to jsonb avoids serialization errors and keeps the raw SQL parameterized (no injection risk).
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration.test.ts (1)
70-174: Solid integration coverage for the JSONB‑based permission checks.Scenarios comprehensively cover exact matches, wildcards, universal wildcard, partial matches, empty input, and bug reproduction. LGTM.
Also applies to: 175-206, 208-219, 221-237, 239-255, 257-273, 275-303
There was a problem hiding this comment.
Added an intergration test to ensure we correctly test the checkPermission logic sql is working as expected. It s a bit complex and mocking it as a unit missed a serialisation error.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts (3)
18-45: Use a unique suffix and select only the needed fields on createReduce collision risk and align with “select only what you need” for Prisma responses. Return only ids.
As per coding guidelines
beforeEach(async () => { - // Create test user - const testUser = await prisma.user.create({ - data: { - email: `test-${Date.now()}@example.com`, - username: `testuser-${Date.now()}`, - }, - }); + const unique = `${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 8)}`; + // Create test user + const testUser = await prisma.user.create({ + data: { + email: `test-${unique}@example.com`, + username: `testuser-${unique}`, + }, + select: { id: true }, + }); testUserId = testUser.id; // Create test team const testTeam = await prisma.team.create({ data: { - name: `Test Team ${Date.now()}`, - slug: `test-team-${Date.now()}`, + name: `Test Team ${unique}`, + slug: `test-team-${unique}`, }, - }); + select: { id: true }, + }); testTeamId = testTeam.id; // Create test role const testRole = await prisma.role.create({ data: { - name: `Test Role ${Date.now()}`, + name: `Test Role ${unique}`, teamId: testTeamId, }, - }); + select: { id: true }, + }); testRoleId = testRole.id; });
47-64: Wrap cleanup in a single transaction for speed and atomicityConsolidates multiple round-trips and ensures consistent teardown.
afterEach(async () => { - // Clean up test data - await prisma.rolePermission.deleteMany({ - where: { roleId: testRoleId }, - }); - await prisma.membership.deleteMany({ - where: { userId: testUserId }, - }); - await prisma.role.deleteMany({ - where: { id: testRoleId }, - }); - await prisma.team.deleteMany({ - where: { id: testTeamId }, - }); - await prisma.user.deleteMany({ - where: { id: testUserId }, - }); + // Clean up test data + await prisma.$transaction([ + prisma.rolePermission.deleteMany({ where: { roleId: testRoleId } }), + prisma.membership.deleteMany({ where: { userId: testUserId } }), + prisma.role.deleteMany({ where: { id: testRoleId } }), + prisma.team.deleteMany({ where: { id: testTeamId } }), + prisma.user.deleteMany({ where: { id: testUserId } }), + ]); });
71-86: Deduplicate overlapping tests to reduce runtimeThese two tests validate the same single-permission path. Consider keeping only the bug reproduction case and removing/merging the earlier one.
Also applies to: 287-303
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts
🧬 Code graph analysis (1)
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts (2)
packages/features/pbac/infrastructure/repositories/PermissionRepository.ts (1)
PermissionRepository(15-286)packages/features/pbac/domain/types/permission-registry.ts (1)
PermissionString(67-67)
🔇 Additional comments (1)
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts (1)
70-174: Strong coverage across exact, wildcard, universal, negative, and single-permission pathsThe suite exercises the core matching logic well and guards against the prior serialization issue. LGTM.
Also applies to: 175-286, 306-326
...features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts
Outdated
Show resolved
Hide resolved
…ermissionRepository.integration-test.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts
🧬 Code graph analysis (1)
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts (2)
packages/features/pbac/infrastructure/repositories/PermissionRepository.ts (1)
PermissionRepository(15-286)packages/features/pbac/domain/types/permission-registry.ts (1)
PermissionString(67-67)
...features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts
Show resolved
Hide resolved
E2E results are ready! |
3 participants
What does this PR do?
Fixes an issue found by @supalarry during PBAC api v2 implementation