Skip to content

Conversation

@everettbu
Copy link

Test 5

…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: [email protected] <[email protected]>

* 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: [email protected] <[email protected]>

* 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: [email protected] <[email protected]>

* 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: [email protected] <[email protected]>

* 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>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR refactors the InsightsBookingService from using Prisma's ORM query builder to raw SQL queries via Prisma.sql. The changes convert the service from returning type-safe Prisma WHERE conditions to returning raw SQL fragments that can be composed into larger queries.

Key architectural changes include:

  • Service Interface Change: The constructor now accepts InsightsBookingServicePublicOptions instead of validated options, suggesting a move toward more flexible parameter handling
  • Method Restructuring: The findMany method is removed and replaced with getBaseConditions() that returns Prisma.Sql fragments instead of executing queries directly
  • Authorization Logic: Complex authorization conditions for user, team, and organization scopes are now built using SQL string interpolation with Prisma.sql templates
  • Filter Implementation: Event type and member user filters are converted to raw SQL conditions using parameterized queries

The integration tests have been comprehensively updated to expect Prisma.Sql fragments instead of Prisma query objects, with a new NOTHING_CONDITION constant (Prisma.sql\1=0``) for handling invalid cases. The final integration test demonstrates the new usage pattern where the service provides SQL conditions for composition into raw queries.

This refactoring appears to be part of a performance optimization initiative, as raw SQL queries can offer better performance for complex analytics workloads typical in insights/reporting systems. The change maintains Prisma's parameterization benefits while giving developers direct control over SQL generation.

Confidence score: 2/5

  • This PR introduces significant security and maintainability risks that make it unsafe to merge in its current state
  • The conversion to raw SQL introduces potential SQL injection vulnerabilities, loss of type safety, and complex string construction logic that could lead to runtime errors
  • Files requiring immediate attention: packages/lib/server/service/insightsBooking.ts for security review and packages/lib/server/service/__tests__/insightsBooking.integration-test.ts for test coverage validation

2 files reviewed, 1 comment

Edit Code Review Bot Settings | Greptile

Comment on lines +72 to +73
if (authConditions && filterConditions) {
return Prisma.sql`(${authConditions}) AND (${filterConditions})`;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: SQL condition composition logic doesn't handle the case where authConditions could be NOTHING_CONDITION - this may result in invalid SQL like (1=0) AND (other_condition)

@github-actions
Copy link
Contributor

This PR is being marked as stale due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants