feat: convert InsightsBookingService to use Prisma.sql raw queries

[akshayutture-augment]

Test 5

---

Replicated from ai-code-review-evaluation/cal.com-greptile#5

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

Code Review Complete ✅

I've reviewed this PR that converts InsightsBookingService to use Prisma.sql raw queries. Here's my detailed feedback:

---

🎯 Overview

This PR successfully converts the service from Prisma object notation to Prisma.sql template literals, reducing code by ~117 lines while improving query performance. The conversion follows the same pattern as InsightsRoutingService.

---

✅ Strengths

1. SQL Injection Protection: Excellent use of Prisma.sql template literals throughout, which properly parameterizes all user inputs and prevents SQL injection vulnerabilities.

2. Code Simplification: The conversion has simplified the codebase significantly:

   • Removed complex Prisma object condition builders
   • More direct SQL generation with clearer intent
   • Better readability for developers familiar with SQL

3. Comprehensive Test Coverage: The test file demonstrates thorough coverage:

   • Tests for all authorization scopes (user, org, team)
   • Filter condition tests (eventTypeId, memberUserId, combined)
   • Integration test with actual database query execution
   • Proper cleanup to avoid test data pollution

4. Proper Authorization Checks: The isOrgOwnerOrAdmin method correctly validates that users have appropriate permissions before allowing access.

---

🔍 Issues Found

🔴 Critical: SQL Injection Vulnerability Risk

Location: packages/lib/server/service/insightsBooking.ts:164, 168

The use of ANY(${array}) with array parameters needs verification:

Prisma.sql`("teamId" = ANY(${teamIds})) AND ("isTeamBooking" = true)`
Prisma.sql`("userId" = ANY(${uniqueUserIds})) AND ("isTeamBooking" = false)`

Concern: While Prisma.sql generally handles parameterization, the ANY() operator with arrays requires special handling. Verify that:

• Array values are properly escaped/parameterized by Prisma
• Empty arrays don't cause SQL syntax errors
• Large arrays don't cause performance issues

Recommendation: Test edge cases with empty arrays and add defensive checks:

if (teamIds.length === 0) {
  return NOTHING_CONDITION;
}

🟡 Medium: Potential Performance Issue

Location: packages/lib/server/service/insightsBooking.ts:148-174

The buildOrgAuthorizationCondition method performs multiple database queries:

1. teamRepo.findAllByParentId - fetches all teams in org
2. MembershipRepository.findAllByTeamIds - fetches all memberships

Issues:

• No limit on the number of teams/users retrieved
• Could be expensive for large organizations
• The ANY(${userIds}) clause could become very large

Recommendation: Consider pagination or batching for large organizations, or add query limits with documentation about the constraint.

🟡 Medium: Array Deduplication Logic

Location: packages/lib/server/service/insightsBooking.ts:167

const uniqueUserIds = Array.from(new Set(userIdsFromOrg));

Issue: This deduplication is only done for org scope, not for team scope (line 194). This inconsistency could lead to:

• Duplicate user IDs in SQL queries for team scope
• Potential performance differences between scopes

Recommendation: Apply consistent deduplication in both methods or document why it's different.

🟡 Medium: Error Handling Missing

Location: Throughout the service

Issue: No error handling for:

• Database query failures
• Repository method failures
• Validation failures beyond the initial schema check

Recommendation: Add try-catch blocks or let errors bubble with proper context:

async getAuthorizationConditions(): Promise<Prisma.Sql> {
  try {
    if (this.cachedAuthConditions === undefined) {
      this.cachedAuthConditions = await this.buildAuthorizationConditions();
    }
    return this.cachedAuthConditions;
  } catch (error) {
    // Log error with context
    return NOTHING_CONDITION; // Fail closed for security
  }
}

---

🟢 Minor Issues

1. Type Safety Concern

Location: packages/lib/server/service/insightsBooking.ts:47

private options: InsightsBookingServiceOptions | null;

The options can be null if validation fails, but this is silently handled. Consider throwing an error in the constructor if validation fails, or document this behavior clearly.

2. Test Data Unique Constraint Risk

Location: packages/lib/server/service/__tests__/insightsBooking.integration-test.ts:22-33

The test uses Date.now() + random suffix for uniqueness, but Date.now() could theoretically collide if tests run in parallel within the same millisecond. Consider using a more robust unique ID generator (e.g., uuid).

3. Inconsistent Condition Wrapping

Location: Throughout service file

Some conditions wrap with extra parentheses, others don't:

• Line 106: ("eventTypeId" = ${...}) OR ("eventParentId" = ${...})
• Line 135: ("userId" = ${...}) AND ("teamId" IS NULL)

This inconsistency doesn't affect functionality but could be standardized for maintainability.

4. Magic Constant

Location: packages/lib/server/service/insightsBooking.ts:43

const NOTHING_CONDITION = Prisma.sql`1=0`;

Consider adding a comment explaining why 1=0 is used (always false condition to return no results).

---

🧪 Test Quality Assessment

Excellent coverage overall, but consider adding:

1. Edge case tests:

   • Empty organization (no teams, no users)
   • Very large organizations (performance testing)
   • Rejected memberships (only accepted memberships should count)

2. Error case tests:

   • Invalid user IDs
   • Non-existent team IDs
   • Database connection failures

3. Integration test enhancement (getBaseConditions test):

   • Currently only tests the happy path
   • Should test with no matching bookings
   • Should test with multiple matching bookings

---

📋 Recommendations Summary

Before Merging:

1. ✅ Verify ANY() array parameterization is safe in Prisma
2. ✅ Add defensive checks for empty arrays
3. ✅ Add consistent deduplication logic
4. ⚠️ Consider error handling strategy

Future Improvements:

• Add performance monitoring for large organizations
• Consider query optimization for orgs with 1000+ teams/users
• Add more comprehensive edge case testing

---

🎓 Overall Assessment

Verdict: ✅ Approve with minor concerns

The conversion to Prisma.sql is well-executed and improves code clarity. The SQL injection protection is properly implemented. My main concerns are around edge cases (empty arrays) and performance at scale, but these are manageable risks that can be addressed with defensive checks and monitoring.

The code quality is good, tests are comprehensive, and the approach is sound. Nice work on the refactoring!

---

[github-actions]

This PR is being marked as stale due to inactivity.