feat: convert InsightsBookingService to use Prisma.sql raw queries

[ShashankFC]

Test 5nn---nReplicated from ai-code-review-evaluation/cal.com-greptile#5

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 1m 59s —— View job

---

Code Review Complete

Tasks

• Read repository guidelines (CLAUDE.md)
• Review the service file changes (insightsBooking.ts)
• Review the test file changes (insightsBooking.integration-test.ts)
• Analyze SQL query conversion approach
• Check for security issues and best practices
• Provide comprehensive feedback

---

Summary

This PR successfully converts InsightsBookingService from using Prisma's ORM-style query building to raw SQL queries using Prisma.sql template literals. The conversion is well-executed with proper SQL injection protection, improved code clarity, and comprehensive test updates.

Strengths

1. Security: SQL Injection Protection ✅
The conversion properly uses Prisma.sql template literals throughout, which provides automatic escaping and parameterization. All user inputs are safely interpolated:

Prisma.sql`("userId" = ${this.options.userId}) AND ("teamId" IS NULL)`
Prisma.sql`("teamId" = ANY(${teamIds})) AND ("isTeamBooking" = true)`

2. Code Simplification ✅
The new approach is significantly cleaner (net -117 lines). The conversion eliminates nested Prisma object structures in favor of more readable SQL:

Before:

return {
  OR: [
    { teamId: { in: teamIds }, isTeamBooking: true },
    { userId: { in: userIds }, isTeamBooking: false }
  ]
};

After:

return Prisma.sql`(("teamId" = ANY(${teamIds})) AND ("isTeamBooking" = true)) OR (("userId" = ANY(${userIds})) AND ("isTeamBooking" = false))`;

3. Consistent Architecture ✅
The PR follows the same pattern as InsightsRoutingService, promoting consistency across the codebase.

4. Comprehensive Test Updates ✅
All tests have been updated to match the new Prisma.Sql format, and the integration test at line 481-491 actually executes $queryRaw to verify the SQL works correctly against the database.

5. Proper Caching ✅
The caching mechanism for authorization and filter conditions remains intact, preventing redundant database queries.

Issues and Recommendations

🔴 Critical: Missing Empty Array Handling

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

The code doesn't handle the case where teamIds or userIdsFromTeam arrays might be empty. PostgreSQL's ANY() operator will fail with an empty array.

Problem in buildTeamAuthorizationCondition (line 196-202):

const conditions: Prisma.Sql[] = [
  Prisma.sql`("teamId" = ${options.teamId}) AND ("isTeamBooking" = true)`,
];

if (userIdsFromTeam.length > 0) {
  conditions.push(Prisma.sql`("userId" = ANY(${userIdsFromTeam})) AND ("isTeamBooking" = false)`);
}

If userIdsFromTeam is empty (when a team has no members), the function still adds the first condition. However, this is logically questionable - should team bookings be accessible if the team has no members?

Problem in buildOrgAuthorizationCondition (line 164):

const conditions: Prisma.Sql[] = [Prisma.sql`("teamId" = ANY(${teamIds})) AND ("isTeamBooking" = true)`];

If teamsFromOrg is empty, teamIds will be [options.orgId], so this is safe. However, there's an edge case: if the org has no teams and no users, only the first condition is added, which might return unexpected results.

Recommendation: Add explicit checks:

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

Fix this →

🟡 Medium: Inconsistent Condition Structure

Location: packages/lib/server/service/insightsBooking.ts:196-207

In buildTeamAuthorizationCondition, the code always creates a conditions array and reduces it, even when there's only one condition. This differs from buildOrgAuthorizationCondition which uses the same pattern but has different edge case handling.

Current code:

const conditions: Prisma.Sql[] = [
  Prisma.sql`("teamId" = ${options.teamId}) AND ("isTeamBooking" = true)`,
];

if (userIdsFromTeam.length > 0) {
  conditions.push(Prisma.sql`("userId" = ANY(${userIdsFromTeam})) AND ("isTeamBooking" = false)`);
}

return conditions.reduce((acc, condition, index) => {
  if (index === 0) return condition;
  return Prisma.sql`(${acc}) OR (${condition})`;
});

Recommendation: For clarity and consistency, consider an early return when there are no users:

const conditions: Prisma.Sql[] = [
  Prisma.sql`("teamId" = ${options.teamId}) AND ("isTeamBooking" = true)`,
];

if (userIdsFromTeam.length === 0) {
  return conditions[0];
}

conditions.push(Prisma.sql`("userId" = ANY(${userIdsFromTeam})) AND ("isTeamBooking" = false)`);
return Prisma.sql`(${conditions[0]}) OR (${conditions[1]})`;

🟡 Medium: Test Coverage Gap

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

The tests don't cover several important edge cases:

1. Empty team (team with no members) - what should happen?
2. Organization with no child teams
3. Empty arrays being passed to ANY() operator
4. Invalid teamId that doesn't belong to the specified orgId

Recommendation: Add test cases for these edge cases to ensure the service handles them gracefully.

Fix this →

🟢 Minor: Missing Type Export

Location: packages/lib/server/service/insightsBooking.ts:29-34

The new InsightsBookingServicePublicOptions type is introduced but it's unclear if this is used elsewhere. The type allows optional teamId while the schema enforces it for team scope.

Current code:

export type InsightsBookingServicePublicOptions = {
  scope: "user" | "org" | "team";
  userId: number;
  orgId: number;
  teamId?: number; // Optional here
};

But the schema at line 21-26 requires teamId for team scope:

z.object({
  scope: z.literal("team"),
  userId: z.number(),
  orgId: z.number(),
  teamId: z.number(), // Required in schema
}),

Recommendation: This is actually fine - the public type is more lenient for the constructor, and validation is handled internally. However, consider adding a JSDoc comment explaining this design choice.

🟢 Minor: Missing Documentation

Location: packages/lib/server/service/insightsBooking.ts:68-81

The new getBaseConditions() method is a public API but lacks JSDoc documentation explaining its purpose and return value.

Recommendation: Add documentation:

/**
 * Combines authorization and filter conditions into a single SQL condition.
 * Used for building WHERE clauses in raw SQL queries.
 * @returns A Prisma.Sql fragment representing the complete query conditions
 */
async getBaseConditions(): Promise<Prisma.Sql> {

🟢 Minor: Removed Method

Location: The findMany method was removed

The original implementation had a findMany method that wrapped the Prisma query. This is now replaced by getBaseConditions(), requiring consumers to call $queryRaw themselves.

Impact: This is a breaking API change. Ensure all call sites have been updated accordingly.

Performance Considerations

Positive:

• Raw SQL queries generally perform better than ORM-generated queries
• The ANY() operator is efficient for array comparisons in PostgreSQL
• Caching of conditions prevents redundant computation

Neutral:

• The conversion maintains the same number of database calls for authorization checks
• No new N+1 query issues introduced

Test Quality

The test suite is comprehensive:

• Tests invalid options handling
• Tests authorization for different roles (owner/admin vs member)
• Tests all three scopes (user, org, team)
• Tests filter conditions independently and combined
• Excellent: Includes actual database integration test using $queryRaw at line 481-491

Final Verdict

Approval Status: ✅ Approve with minor fixes

This is a solid refactoring that improves code clarity and maintains security. The critical issue with empty array handling should be addressed before merging, but it's a straightforward fix. The test coverage is good, though edge cases could be better tested.

Priority Fixes:

1. 🔴 Add empty array handling in authorization condition builders
2. 🟡 Add edge case tests for empty teams/orgs
3. 🟢 Add JSDoc documentation for public methods

---

[github-actions]

This PR is being marked as stale due to inactivity.