feat: convert InsightsBookingService to use Prisma.sql raw queries - #7
feat: convert InsightsBookingService to use Prisma.sql raw queries#7CodingKylo 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>
![re-entry-ai[bot]](https://avatars.githubusercontent.com/in/3466347?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🚨 Risk Score: 91/100 · CRITICAL
| Dimension | Level |
|---|---|
| Likelihood | 🔴 Critical |
| Impact | 🔴 Critical |
| Detectability | 🟠 High |
Summary
Code Review Summary
Risk Level: CRITICAL (Score: 91/100)
Files Changed: 0
High Risk Areas: 2
Key Concerns
- SECURITY (critical): Authorization conditions are rebuilt from structured Prisma where-objects into raw Prisma.sql expressions, which can subtly change semantics and therefore access control behavior.
- CONFIGURATION (critical): The service’s condition types and composition logic are fundamentally changed (where-input objects -> Prisma.sql fragments), which can alter operator precedence/parentheses and null/empty behavior.
- TESTCOVERAGE (medium): The integration test file is updated to assert Prisma.sql fragments, but the diff truncation suggests some test coverage (e.g., caching tests) may have been removed, reducing confidence in behavior beyond condition equality.
Posted by re-entry.ai · Risk governance for autonomous engineering teams
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🟡 Risk Score: 29/100 · MEDIUM
| Dimension | Level |
|---|---|
| Likelihood | 🟡 Medium |
| Impact | 🟡 Medium |
| Detectability | 🟢 Low |
Intent
Convert InsightsBookingService authorization/filtering logic from Prisma WhereInput objects to Prisma.sql raw query fragments.
Summary
The PR replaces structured Prisma where conditions with Prisma.sql fragments for authorization and filtering, and updates integration tests to assert SQL output. The highest risk is a correctness/security crash path in SQL composition when the authorization disjunction set is empty (leading to an exception before returning a safe “no rows” condition). There are also semantic regressions from collapsing “no auth restriction” vs “always-false” into a single truthy SQL object, plus a numeric truthiness bug that can broaden results when eventTypeId is 0. Verify that empty authorization inputs deterministically produce 1=0, that all numeric filters use explicit null/undefined checks, and that all call sites/tests align with the new public options contract.
🎯 Review Focus
The authorization SQL composition in getAuthorizationConditions()—specifically ensure empty principal sets deterministically return the deny-all sentinel without throwing, and that subsequent condition-building does not rely on truthiness of Prisma.Sql fragments.
✅ Action Checklist
- [ ] SUGGESTION — Centralize the “deny all” sentinel to avoid drift between service and tests. Right now both files define their own
NOTHING_CONDITION(packages/lib/server/service/insightsBooking.tsdefinesconst NOTHING_CONDITION = Prisma.sql t1=0andpackages/lib/server/service/__tests__/insightsBooking.integration-test.tsdefinesconst NOTHING_CONDITION = Prisma.sql t1=0). Fix: export it from the service module (e.g.,export const NOTHING_CONDITION = Prisma.sql t1=0 ;) and import it in the test. - [ ] SUGGESTION — Decouple tests from Prisma’s exact SQL rendering/parenthesization. The test now asserts exact SQL strings like
expect(conditions).toEqual(Prisma.sql t("userId" = ${...}) AND ("teamId" IS NULL) );(packages/lib/server/service/__tests__/insightsBooking.integration-test.ts). Fix: assert semantic equivalence by normalizingtoString()(strip whitespace/parentheses) or assert stable fragments (e.g., contains"userId" =and"teamId" IS NULL) rather than full equality, to reduce brittleness across Prisma versions. - [ ] SUGGESTION — Verify all call sites compile and behave with the new constructor options contract. The service constructor now expects
InsightsBookingServicePublicOptions(packages/lib/server/service/insightsBooking.ts), but only this test file appears updated. Fix: search for allnew InsightsBookingService({ options: ... })instantiations and update them; add a type-level test or compile-time assertion to prevent old shapes from slipping in.
Suggestions
- Centralize the “deny all” sentinel to avoid drift between service and tests. Right now both files define their own
NOTHING_CONDITION(packages/lib/server/service/insightsBooking.tsdefinesconst NOTHING_CONDITION = Prisma.sql t1=0andpackages/lib/server/service/__tests__/insightsBooking.integration-test.tsdefinesconst NOTHING_CONDITION = Prisma.sql t1=0). Fix: export it from the service module (e.g.,export const NOTHING_CONDITION = Prisma.sql t1=0 ;) and import it in the test. - Decouple tests from Prisma’s exact SQL rendering/parenthesization. The test now asserts exact SQL strings like
expect(conditions).toEqual(Prisma.sql t("userId" = ${...}) AND ("teamId" IS NULL) );(packages/lib/server/service/__tests__/insightsBooking.integration-test.ts). Fix: assert semantic equivalence by normalizingtoString()(strip whitespace/parentheses) or assert stable fragments (e.g., contains"userId" =and"teamId" IS NULL) rather than full equality, to reduce brittleness across Prisma versions. - Verify all call sites compile and behave with the new constructor options contract. The service constructor now expects
InsightsBookingServicePublicOptions(packages/lib/server/service/insightsBooking.ts), but only this test file appears updated. Fix: search for allnew InsightsBookingService({ options: ... })instantiations and update them; add a type-level test or compile-time assertion to prevent old shapes from slipping in.
📝 This review includes 3 inline comments (1 critical, 2 warnings)
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| ): Promise<Prisma.BookingTimeStatusDenormalizedWhereInput> { | ||
| ): Promise<Prisma.Sql> { | ||
| const teamRepo = new TeamRepository(this.prisma); | ||
| const childTeamOfOrg = await teamRepo.findByIdAndParentId({ |
There was a problem hiding this comment.
🚨 CRITICAL
This is a real crash path: conditions.reduce(...) has no initial value, so when both teamIds and userIdsFromOrg are empty the authorization builder throws before returning 1=0. The root cause is that the new raw-SQL composition assumes at least one disjunct, but the empty-set case is not handled defensively.
| @@ -60,89 +65,86 @@ export class InsightsBookingService { | |||
| this.filters = filters; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
authConditions is always truthy here because getAuthorizationConditions() now returns a Prisma.Sql object, and getBaseConditions() no longer has a way to represent a missing auth clause. That means the else if branches are effectively dead code, and the method can no longer distinguish between "no auth restriction" and "always-false". The underlying issue is that the previous nullable/structured condition model was collapsed into a single truthy SQL object, which changes the control flow semantics.
| return conditions.length > 0 ? { AND: conditions } : null; | ||
| if (conditions.length === 0) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This guard still treats 0 as absent. If eventTypeId is a valid identifier in the data model, the filter will silently skip it and return broader results than intended. The bug comes from using truthiness checks on numeric IDs instead of explicit null/undefined checks.
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🚨 Risk Score: 91/100 · CRITICAL
| Dimension | Level |
|---|---|
| Likelihood | 🔴 Critical |
| Impact | 🔴 Critical |
| Detectability | 🟠 High |
Intent
Refactor InsightsBookingService to build authorization/filter constraints using Prisma.sql fragments instead of typed Prisma where-input objects.
Summary
The service now represents “no results” and authorization/filter predicates as Prisma.sql fragments (e.g., 1=0 and SQL expressions) and updates integration tests to assert SQL fragments rather than where-input objects. This is a contract-breaking change: any caller expecting Prisma where-inputs (or any code path that still passes these conditions into a Prisma where: clause) will fail or behave differently at runtime. The highest risk is authorization semantics drifting due to manual SQL composition and empty/NULL handling differences, plus correctness issues from truthiness checks (e.g., skipping eventTypeId = 0). The reviewer must verify all call sites and ensure the service’s new return types are only used in contexts that execute SQL fragments correctly, and that the SQL predicates are semantically equivalent to the previous where-input logic.
🎯 Review Focus
Authorization predicate construction and contract correctness: confirm that every place using the new Prisma.Sql conditions executes them in the right Prisma API context (not as where: objects) and that the SQL predicates are semantically equivalent to the previous where-input logic for all combinations of auth scope, NULL teamId, and empty/no-filter cases.
✅ Action Checklist
- [ ] SUGGESTION — Verify and update all consumers of
getAuthorizationConditions()/getFilterConditions()to match the new return type (Prisma.Sql). Add TypeScript compile-time enforcement by changing method signatures and removing any implicitany/structural typing that could allow passingPrisma.Sqlinto a Prismawhere:clause. [packages/lib/server/service/insightsBooking.ts:L60-L90] - [ ] SUGGESTION — Stop asserting exact SQL string templates in integration tests; assert semantic equivalence by executing the query and checking returned rows. Current tests are coupled to rendering details (parameter quoting, parentheses, identifier casing), which can cause false failures and also miss real semantic drift. Example: replace
expect(conditions).toEqual(Prisma.sql"...")with a test that runsservice.findMany(...)and asserts the resulting bookings. [packages/lib/server/service/tests/insightsBooking.integration-test.ts:L267-L310] - [ ] SUGGESTION — Check for instance lifecycle/caching correctness: cached auth/filter conditions are now
Prisma.Sql. If the service instance can be reused across requests with differentoptions/filters, cached SQL could be stale. Fix by either making the service immutable per request (no reuse) or keying the cache by(scope, userId, orgId, teamId, filters.eventTypeId, filters.memberUserId)or removing caching. [packages/lib/server/service/insightsBooking.ts:L60-L75] - [ ] SUGGESTION — If you keep the
NOTHING_CONDITION = Prisma.sql1=0`` approach, centralize it in a shared helper/constant and ensure all services use the same sentinel semantics. This avoids inconsistent “no results” behavior across the codebase. [packages/lib/server/service/insightsBooking.ts:L60] and [packages/lib/server/service/tests/insightsBooking.integration-test.ts:L11-L15]
Suggestions
- Verify and update all consumers of
getAuthorizationConditions()/getFilterConditions()to match the new return type (Prisma.Sql). Add TypeScript compile-time enforcement by changing method signatures and removing any implicitany/structural typing that could allow passingPrisma.Sqlinto a Prismawhere:clause. [packages/lib/server/service/insightsBooking.ts:L60-L90] - Stop asserting exact SQL string templates in integration tests; assert semantic equivalence by executing the query and checking returned rows. Current tests are coupled to rendering details (parameter quoting, parentheses, identifier casing), which can cause false failures and also miss real semantic drift. Example: replace
expect(conditions).toEqual(Prisma.sql"...")with a test that runsservice.findMany(...)and asserts the resulting bookings. [packages/lib/server/service/tests/insightsBooking.integration-test.ts:L267-L310] - Check for instance lifecycle/caching correctness: cached auth/filter conditions are now
Prisma.Sql. If the service instance can be reused across requests with differentoptions/filters, cached SQL could be stale. Fix by either making the service immutable per request (no reuse) or keying the cache by(scope, userId, orgId, teamId, filters.eventTypeId, filters.memberUserId)or removing caching. [packages/lib/server/service/insightsBooking.ts:L60-L75] - If you keep the
NOTHING_CONDITION = Prisma.sql1=0`` approach, centralize it in a shared helper/constant and ensure all services use the same sentinel semantics. This avoids inconsistent “no results” behavior across the codebase. [packages/lib/server/service/insightsBooking.ts:L60] and [packages/lib/server/service/tests/insightsBooking.integration-test.ts:L11-L15]
📝 This review includes 4 inline comments (4 warnings)
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| scope: "user" | "org" | "team"; | ||
| userId: number; | ||
| orgId: number; | ||
| teamId?: number; |
There was a problem hiding this comment.
Valid concern: the refactor replaces the old typed sentinel { id: -1 } with a raw SQL fragment 1=0. That is still a correct “match nothing” predicate, but it changes the contract from a Prisma where-input to a SQL fragment, so any caller or test that expects a structured filter object will break. The root cause is the API boundary shift from Prisma query objects to raw SQL without a compatibility layer.
| }); | ||
| if (authConditions && filterConditions) { | ||
| return Prisma.sql`(${authConditions}) AND (${filterConditions})`; | ||
| } else if (authConditions) { |
There was a problem hiding this comment.
This is a real behavioral change: the old implementation always merged auth and filter constraints into a Prisma AND array, which naturally tolerated null entries and preserved object semantics. The new raw-SQL branch logic changes how empty conditions are represented and combined, so downstream code that relied on the previous shape/merge behavior can no longer treat these as interchangeable. The underlying issue is that the service now returns SQL fragments instead of a composable query AST.
| return null; | ||
| } | ||
|
|
||
| if (this.filters.eventTypeId) { |
There was a problem hiding this comment.
Valid bug: eventTypeId is checked with a truthiness test, so a legitimate 0 value would be skipped and the filter silently omitted. This is a correctness issue introduced by the refactor because the code now manually reconstructs the predicate logic instead of relying on Prisma's typed equality handling. Use an explicit !== undefined/!== null check if 0 is a possible identifier.
| return await this.buildOrgAuthorizationCondition(this.options); | ||
| } else if (this.options.scope === "team") { | ||
| conditions.push(await this.buildTeamAuthorizationCondition(this.options)); | ||
| return await this.buildTeamAuthorizationCondition(this.options); |
There was a problem hiding this comment.
The concern is real, but the specific risk is not the absence of an id column. 1=0 is a valid always-false predicate regardless of schema. The actual regression is that getAuthorizationConditions() no longer returns a Prisma where-input, so any code path that still expects to pass this into Prisma's where option will fail or require a separate SQL execution path. This is a contract break caused by the raw-query migration.
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🚨 Risk Score: 86/100 · CRITICAL
| Dimension | Level |
|---|---|
| Likelihood | 🔴 Critical |
| Impact | 🔴 Critical |
| Detectability | 🟡 Medium |
Intent
Refactor InsightsBookingService to build authorization and filter predicates using Prisma.sql fragments instead of Prisma where-input objects.
Summary
The service now composes query conditions as Prisma.sql fragments (including a “nothing” sentinel 1=0) and updates integration tests to assert SQL fragments rather than structured where-inputs. The highest risk is authorization/filter semantics changing due to SQL composition details (parentheses, empty-array handling, and operator precedence) and the contract change of return types from WhereInput to Prisma.Sql. You must verify that all consumers of getAuthorizationConditions/getFilterConditions/getBaseConditions are updated for the new return types and that the resulting SQL is semantically equivalent to the previous Prisma where-input logic, especially for edge cases like empty teamIds and null teamId handling.
🎯 Review Focus
Verify authorization predicate equivalence: ensure the composed Prisma.sql conditions (including empty teamIds and null teamId cases) produce the same effective access control behavior as the prior Prisma where-input logic, and confirm all callers/tests are updated for the Prisma.Sql return-type contract change.
✅ Action Checklist
- [ ] SUGGESTION — Contract audit: search for all usages of
getAuthorizationConditions(),getFilterConditions(), andgetBaseConditions()and update their expected return types fromPrisma.BookingTimeStatusDenormalizedWhereInputtoPrisma.Sql(or add a compatibility wrapper). This is required because the refactor changes the method return types and can break callers at compile-time or runtime. [packages/lib/server/service/insightsBooking.ts:L60-L86] - [ ] SUGGESTION — Test strategy: stop asserting exact SQL string equality for authorization correctness; instead, execute the query end-to-end and assert returned rows/IDs for representative cases (owner/admin, non-owner, null teamId, empty teamIds). String-based tests can pass while semantics differ due to parentheses/precedence changes. [packages/lib/server/service/tests/insightsBooking.integration-test.ts:L267-L310]
- [ ] SUGGESTION — Sentinel consistency: the test defines its own
const NOTHING_CONDITION = Prisma.sql1=0`` while the service definesNOTHING_CONDITIONinternally; export the sentinel from the service (or a shared test helper) so both sides stay consistent if the sentinel strategy changes. [packages/lib/server/service/insightsBooking.ts:L60] and [packages/lib/server/service/tests/insightsBooking.integration-test.ts:L11-L14]
Suggestions
- Contract audit: search for all usages of
getAuthorizationConditions(),getFilterConditions(), andgetBaseConditions()and update their expected return types fromPrisma.BookingTimeStatusDenormalizedWhereInputtoPrisma.Sql(or add a compatibility wrapper). This is required because the refactor changes the method return types and can break callers at compile-time or runtime. [packages/lib/server/service/insightsBooking.ts:L60-L86] - Test strategy: stop asserting exact SQL string equality for authorization correctness; instead, execute the query end-to-end and assert returned rows/IDs for representative cases (owner/admin, non-owner, null teamId, empty teamIds). String-based tests can pass while semantics differ due to parentheses/precedence changes. [packages/lib/server/service/tests/insightsBooking.integration-test.ts:L267-L310]
- Sentinel consistency: the test defines its own
const NOTHING_CONDITION = Prisma.sql1=0`` while the service definesNOTHING_CONDITIONinternally; export the sentinel from the service (or a shared test helper) so both sides stay consistent if the sentinel strategy changes. [packages/lib/server/service/insightsBooking.ts:L60] and [packages/lib/server/service/tests/insightsBooking.integration-test.ts:L11-L14]
📝 This review includes 2 inline comments (2 warnings)
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| @@ -1,4 +1,4 @@ | |||
| import type { Prisma } from "@prisma/client"; | |||
| import { Prisma } from "@prisma/client"; | |||
There was a problem hiding this comment.
Quote: import { Prisma } from "@prisma/client";
Issue: This file previously imported type { Prisma } (type-only). The diff changes it to a runtime import. If @prisma/client is not intended to be loaded in this context (e.g., serverless edge, test environment, or where prisma client generation differs), this can cause runtime failures or increase bundle size.
Fix: If only Prisma.sql is needed, prefer a type-only import plus a value import from the correct module, or keep it as type-only if your tooling supports Prisma.sql via the existing prisma instance. For example:
import type { Prisma } from "@prisma/client";- and ensure
Prisma.sqlis available from the correct namespace (oftenimport { Prisma } from "@prisma/client"is required, but then ensure this file is executed only in environments where the generated client exists).
| return await this.buildTeamAuthorizationCondition(this.options); | ||
| } else { | ||
| return NOTHING; | ||
| return NOTHING_CONDITION; |
There was a problem hiding this comment.
Quote: const conditions: Prisma.Sql[] = [Prisma.sql("teamId" = ANY(${teamIds})) AND ("isTeamBooking" = true)];
Issue: teamIds is derived from teamsFromOrg.map((t) => t.id) and could be an empty array. In PostgreSQL, = ANY('{}') is valid but always false; however, the surrounding OR logic and the intended authorization semantics might differ from the previous structured Prisma in: teamIds behavior (which also becomes false, but the overall OR composition could differ when combined with other branches).
Fix: Add explicit handling for empty arrays to preserve intended semantics. For example:
- If
teamIds.length === 0, omit the team booking condition entirely and rely on the user-based condition. - Similarly, ensure the final predicate matches the previous behavior exactly.
2 participants
Martian Code Review Benchmark PR (mirrored from source #5)