feat: convert InsightsBookingService to use Prisma.sql raw queries#5
feat: convert InsightsBookingService to use Prisma.sql raw queries#5everettbu wants to merge 1 commit into
Conversation
…22345) * fix: use raw query at InsightsBookingService * feat: convert InsightsBookingService to use Prisma.sql raw queries - Convert auth conditions from Prisma object notation to Prisma.sql - Convert filter conditions from Prisma object notation to Prisma.sql - Update return types from Prisma.BookingTimeStatusDenormalizedWhereInput to Prisma.Sql - Fix type error in isOrgOwnerOrAdmin method - Follow same pattern as InsightsRoutingService conversion Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * feat: convert InsightsBookingService to use Prisma.sql raw queries - Convert auth conditions from Prisma object notation to Prisma.sql - Convert filter conditions from Prisma object notation to Prisma.sql - Update return types from Prisma.BookingTimeStatusDenormalizedWhereInput to Prisma.Sql - Fix type error in isOrgOwnerOrAdmin method - Follow same pattern as InsightsRoutingService conversion Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * fix: update InsightsBookingService integration tests for Prisma.sql format - Replace Prisma object notation expectations with Prisma.sql template literals - Add NOTHING_CONDITION constant for consistency with InsightsRoutingService - Update all test cases to use direct Prisma.sql comparisons - Use $queryRaw for actual database integration testing - Follow same testing patterns as InsightsRoutingService Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * fix: exclude intentionally skipped jobs from required CI check failure - Remove 'skipped' from failure condition in pr.yml and all-checks.yml - Allow E2E jobs to be skipped without failing the required check - Only actual failures and cancelled jobs will cause required check to fail Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * fix tests * Revert "fix: exclude intentionally skipped jobs from required CI check failure" This reverts commit 6ff44fc9a8f14ad657f7bba7c2e454e192b66c8f. * clean up tests * address feedback --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant InsightsBookingService
participant Database
Test->>InsightsBookingService: new InsightsBookingService(options)
Test->>InsightsBookingService: getBaseConditions()
InsightsBookingService->>InsightsBookingService: getAuthorizationConditions()
InsightsBookingService->>InsightsBookingService: getFilterConditions()
InsightsBookingService->>InsightsBookingService: Combine SQL fragments (AND/NOTHING_CONDITION)
InsightsBookingService-->>Test: return Prisma.Sql condition
Test->>Database: Execute raw SQL with returned condition
Database-->>Test: Return result rows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (2)
packages/lib/server/service/insightsBooking.ts (2)
29-35: Consider removing redundant type definition.The
InsightsBookingServicePublicOptionstype appears to be a less strict version ofInsightsBookingServiceOptions. Since the constructor immediately validates the options using the schema, this intermediate type may be unnecessary.Consider using the schema's input type directly:
-export type InsightsBookingServicePublicOptions = { - scope: "user" | "org" | "team"; - userId: number; - orgId: number; - teamId?: number; -}; - export type InsightsBookingServiceOptions = z.infer<typeof insightsBookingServiceOptionsSchema>; +export type InsightsBookingServicePublicOptions = z.input<typeof insightsBookingServiceOptionsSchema>;
145-175: Consider performance impact with large organizations.The implementation is logically correct, but using
ANYoperator with potentially large arrays might impact query performance.For organizations with many teams/users, consider:
- Adding indexes on
teamIdanduserIdcolumns if not already present- Monitoring query performance with large datasets
- Potentially implementing pagination or limiting scope if performance degrades
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/lib/server/service/__tests__/insightsBooking.integration-test.ts(10 hunks)packages/lib/server/service/insightsBooking.ts(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/lib/server/service/insightsBooking.ts (3)
packages/prisma/index.ts (1)
readonlyPrisma(71-75)packages/lib/server/repository/team.ts (1)
TeamRepository(169-336)packages/platform/libraries/index.ts (1)
MembershipRole(98-98)
🔇 Additional comments (9)
packages/lib/server/service/__tests__/insightsBooking.integration-test.ts (4)
2-2: LGTM! Appropriate imports and constant definition.The addition of the
Prismaimport and theNOTHING_CONDITIONconstant are consistent with the service's refactoring to use raw SQL conditions.Also applies to: 10-11
210-210: Authorization test assertions correctly updated for SQL conditions.The test expectations accurately reflect the SQL authorization logic:
NOTHING_CONDITIONfor invalid/unauthorized cases- Proper SQL fragments with parameterized values for authorized scopes
- Correct use of
ANYoperator for array comparisons in team/org scopesAlso applies to: 245-245, 273-273, 295-299, 332-343
384-386: Filter condition tests properly validate SQL generation.The assertions correctly verify:
- Event type filtering with OR logic for eventTypeId/eventParentId
- Simple equality for memberUserId filter
- Proper AND combination of multiple filter conditions
Also applies to: 407-407, 429-431
437-437: Test suite correctly refactored for getBaseConditions.The test properly validates the new SQL-based approach:
- Directly executes the generated SQL conditions
- Verifies results match expected bookings
- Good test case using user-scoped data with filters
Also applies to: 480-491
packages/lib/server/service/insightsBooking.ts (5)
49-50: Cache property types correctly updated.The cache properties now properly store
Prisma.Sqltypes, matching the new method return types.Also applies to: 58-58
97-123: Filter condition building implemented correctly.The method properly:
- Handles each filter type with appropriate SQL conditions
- Safely parameterizes values using Prisma.sql
- Combines multiple conditions with AND operator
- Returns null when no filters are present
125-143: Authorization condition routing implemented correctly.The method properly:
- Validates user permissions before building conditions
- Routes to appropriate scope-specific builders
- Has proper fallbacks for invalid cases
177-220: Team authorization and permission check implemented correctly.Both methods properly implement their logic:
- Security validation that team belongs to organization
- Consistent SQL generation pattern
- Simplified and clear role checking
68-81: Critical: Potential authorization bypass when auth returns NOTHING_CONDITION.When
authConditionsreturnsNOTHING_CONDITION(1=0) butfilterConditionsexists, the current logic returns only the filter conditions, potentially allowing unauthorized access.Apply this fix to ensure NOTHING_CONDITION always blocks access:
async getBaseConditions(): Promise<Prisma.Sql> { const authConditions = await this.getAuthorizationConditions(); const filterConditions = await this.getFilterConditions(); + // If authorization explicitly denies access, always return NOTHING + if (authConditions === NOTHING_CONDITION) { + return NOTHING_CONDITION; + } + if (authConditions && filterConditions) { return Prisma.sql`(${authConditions}) AND (${filterConditions})`; } else if (authConditions) { return authConditions; } else if (filterConditions) { return filterConditions; } else { return NOTHING_CONDITION; } }Likely an incorrect or invalid review comment.
|
This PR is being marked as stale due to inactivity. |
2 participants
Test 5
Summary by CodeRabbit
Refactor
New Features