-
Notifications
You must be signed in to change notification settings - Fork 11.8k
refactor: split flag repositories into Prisma and Cached layers #27186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Rename FeatureRepository to PrismaFeatureRepository (raw DB access) - Rename TeamFeatureRepository to PrismaTeamFeatureRepository (raw DB access) - Rename UserFeatureRepository to PrismaUserFeatureRepository (raw DB access) - Create CachedFeatureRepository with @memoize wrapping PrismaFeatureRepository - Create CachedTeamFeatureRepository with @Memoize/@Unmemoize wrapping PrismaTeamFeatureRepository - Create CachedUserFeatureRepository with @Memoize/@Unmemoize wrapping PrismaUserFeatureRepository - Update DI tokens, modules, and containers for all 6 repositories - Update imports in FeatureOptInService and related modules - Update tests to use new repository structure Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
…o Prisma - Use direct function references for @memoize key (e.g., KEY.all instead of () => KEY.all()) - Simplify batch methods in Cached repositories to delegate to Prisma repository - Update tests to reflect the new delegation pattern Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 20 files
E2E results are ready! |
…c results Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
volnei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment
I can't... see any comment! Did you submit the review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 22 files
…methods (#27195) * refactor: cleanup features repository and add findBySlug, update methods - Remove unused methods from FeaturesRepository (keep getTeamsWithFeatureEnabled) - Add findAll(), findBySlug(), update() to IFeatureRepository interface - Add findAll() with caching to CachedFeatureRepository - Add findBySlug() with caching to CachedFeatureRepository - Add update() with Unmemoize to CachedFeatureRepository - Add checkIfFeatureIsEnabledGlobally() to CachedFeatureRepository - Update toggleFeatureFlag.handler.ts to use repository instead of raw Prisma - Add comprehensive unit tests for all new methods Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * fix: update updatedAt timestamp in feature update method Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * refactor: move feature check methods to specialized repositories - Replace getUserFeaturesStatus with two checkIfUserHasFeature calls in bookings page - Move checkIfTeamHasFeature to PrismaTeamFeatureRepository with pass-through in CachedTeamFeatureRepository - Move checkIfUserHasFeature and checkIfUserHasFeatureNonHierarchical to PrismaUserFeatureRepository with pass-throughs in CachedUserFeatureRepository - Add getEnabledFeatures to PrismaTeamFeatureRepository with caching in CachedTeamFeatureRepository - Keep FeaturesRepository methods as pass-throughs for backward compatibility - Update test to expect updatedAt in feature update Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * refactor: remove getUserFeaturesStatus and unused methods from FeaturesRepository Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * restore comment * fix: invalidate all-features cache on update and enabledFeatures cache on upsert/delete - CachedFeatureRepository: Add KEY.all() to @Unmemoize keys in update() to prevent stale findAll() results - CachedTeamFeatureRepository: Add KEY.enabledFeatures(teamId) to @Unmemoize keys in upsert() and delete() to prevent stale getEnabledFeatures() results Co-Authored-By: unknown <> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Add comprehensive tests for CachedUserFeatureRepository covering: - findByUserIdAndFeatureId (cache hit, cache miss, not found) - findByUserIdAndFeatureIds (empty input, multiple features) - upsert (with cache invalidation) - delete (with cache invalidation) - findAutoOptInByUserId (cache hit, cache miss, not found) - setAutoOptIn (with cache invalidation) Co-Authored-By: unknown <>
…t-manager.devin.ai/proxy/github.com/calcom/cal.com into devin/1769164663-split-flag-repositories
|
@cubic-dev-ai review this PR |
@eunjae-lee I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 29 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/flags/repositories/PrismaFeatureRepository.ts">
<violation number="1" location="packages/features/flags/repositories/PrismaFeatureRepository.ts:20">
P2: Limit Prisma reads to the fields you return by adding a `select` clause here to avoid fetching unused columns.</violation>
<violation number="2" location="packages/features/flags/repositories/PrismaFeatureRepository.ts:37">
P2: Add a `select` clause to fetch only the fields used in the returned DTO.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Devin AI is addressing Cubic AI's review feedbackA Devin session has been created to address the issues identified by Cubic AI. |
| async findByUserIdAndFeatureIds( | ||
| userId: number, | ||
| featureIds: FeatureId[] | ||
| ): Promise<Partial<Record<FeatureId, UserFeaturesDto>>> { | ||
| const results = await Promise.all( | ||
| featureIds.map(async (featureId) => { | ||
| const userFeature = await this.findByUserIdAndFeatureId(userId, featureId); | ||
| return { featureId, userFeature }; | ||
| }) | ||
| ); | ||
|
|
||
| const result: Partial<Record<FeatureId, UserFeaturesDto>> = {}; | ||
| for (const { featureId, userFeature } of results) { | ||
| if (userFeature !== null) { | ||
| result[featureId] = userFeature; | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should 100% use a find Many here this is a N+1 query.
const userFeatures = await this.prisma.userFeatures.findMany({
where: { userId, featureId: { in: featureIds } }
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed: 2bec5d2
| async findByTeamIdsAndFeatureIds( | ||
| teamIds: number[], | ||
| featureIds: FeatureId[] | ||
| ): Promise<Partial<Record<FeatureId, Record<number, TeamFeaturesDto>>>> { | ||
| return this.prismaTeamFeatureRepository.findByTeamIdsAndFeatureIds(teamIds, featureIds); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to bypass cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a combination of teamId x featureId. So I think it's quite tricky to invalidate this. Unless we scan the existing keys, it's hard to know which keys to invalidate when something happens to a certain teamId or a certain featureId. And once it happens, we need to find all the keys of teamId x featureId. It seemed too complex. But I'm happy to iterate on this if you have a good idea.
Remove integration tests for methods that were intentionally removed: - getUserFeatureStates - getTeamsFeatureStates - getUserAutoOptIn - getTeamsAutoOptIn - setUserAutoOptIn - setTeamAutoOptIn Co-Authored-By: unknown <>
…:calcom/cal.com into devin/1769164663-split-flag-repositories
- Add explicit select clauses to findAll, findBySlug, and update methods - Only fetch fields needed for FeatureDto (slug, enabled, description, type, stale, lastUsedAt, createdAt, updatedAt, updatedBy) - Update tests to expect select clauses - Fix UserFeatureRepository test to use findMany mock Co-Authored-By: unknown <>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 30 files
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 30 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx
Outdated
Show resolved
Hide resolved
sean-brydon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested works very well - love this approach.
What does this PR do?
Splits the three flag repositories into raw Prisma repositories and cached repositories that wrap them:
FeatureRepository→PrismaFeatureRepository+CachedFeatureRepositoryTeamFeatureRepository→PrismaTeamFeatureRepository+CachedTeamFeatureRepositoryUserFeatureRepository→PrismaUserFeatureRepository+CachedUserFeatureRepositoryThe Prisma repositories contain only raw database access logic. The Cached repositories wrap the Prisma ones and add
@Memoize/@Unmemoizedecorators for caching behavior.Architecture:
New repository methods added:
CachedFeatureRepository:findBySlug(),update(),checkIfFeatureIsEnabledGlobally()CachedTeamFeatureRepository:checkIfTeamHasFeature(),getEnabledFeatures()CachedUserFeatureRepository:checkIfUserHasFeature(),checkIfUserHasFeatureNonHierarchical()Updates since last revision
selectclauses toPrismaFeatureRepositoryqueries (findAll,findBySlug,update) to only fetch fields needed for FeatureDtoPrismaUserFeatureRepository.findByUserIdAndFeatureIdsto use singlefindManyquery instead of multiplefindUniquecallsFeaturesRepository:getUserFeatureStates,getTeamsFeatureStates,getUserAutoOptIn,getTeamsAutoOptIn,setUserAutoOptIn,setTeamAutoOptInCachedUserFeatureRepositoryupdate()now invalidates bothKEY.bySlugandKEY.all()cachesupsert()anddelete()now invalidateKEY.enabledFeaturescacheInsightsBookingService.integration-test.tsby addingorderBy: { id: "asc" }toTeamRepository.findAllByParentIdMandatory Tasks (DO NOT REMOVE)
How should this be tested?
TZ=UTC yarn vitest run packages/features/flags/repositories/__tests__/yarn type-check:ci --forceHuman Review Checklist
selectclauses inPrismaFeatureRepositoryinclude all fields needed forFeatureDto(slug, enabled, description, type, stale, lastUsedAt, createdAt, updatedAt, updatedBy)@Memoizedecorator accepts direct function references (e.g.,KEY.allvs() => KEY.all())update()invalidates bothKEY.bySlugandKEY.all()to prevent stalefindAll()resultsupsert()anddelete()invalidateKEY.enabledFeaturesto prevent stalegetEnabledFeatures()resultsFeaturesRepositoryChecklist
Link to Devin run: https://app.devin.ai/sessions/e0bf0feaf39846299dd43299f34da7b6
Requested by: @eunjae-lee