-
Notifications
You must be signed in to change notification settings - Fork 12
Optimize cache key overflow management with performance improvements #1
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
Changes from all commits
cb78692
4e1048b
0cbc8a3
48bd12b
97bde08
42a4a38
dbaa072
fa47456
7813d37
ee9699e
16a7bb7
eb4eebe
098b520
c88ed8c
7269aca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,9 @@ import React from "react"; | |
| import { getServerSession } from "@calcom/features/auth/lib/getServerSession"; | ||
| import type { TeamFeatures } from "@calcom/features/flags/config"; | ||
| import { FeaturesRepository } from "@calcom/features/flags/features.repository"; | ||
| import { PermissionMapper } from "@calcom/features/pbac/domain/mappers/PermissionMapper"; | ||
| import { Resource, CrudAction } from "@calcom/features/pbac/domain/types/permission-registry"; | ||
| import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service"; | ||
|
|
||
| import { buildLegacyRequest } from "@lib/buildLegacyCtx"; | ||
|
|
||
|
|
@@ -23,6 +26,15 @@ const getTeamFeatures = unstable_cache( | |
| } | ||
| ); | ||
|
|
||
| const getCachedResourcePermissions = unstable_cache( | ||
| async (userId: number, teamId: number, resource: Resource) => { | ||
| const permissionService = new PermissionCheckService(); | ||
| return permissionService.getResourcePermissions({ userId, teamId, resource }); | ||
| }, | ||
| ["resource-permissions"], | ||
| { revalidate: 120 } | ||
| ); | ||
|
|
||
| export default async function SettingsLayoutAppDir(props: SettingsLayoutProps) { | ||
| const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) }); | ||
| const userId = session?.user?.id; | ||
|
|
@@ -31,20 +43,30 @@ export default async function SettingsLayoutAppDir(props: SettingsLayoutProps) { | |
| } | ||
|
|
||
| let teamFeatures: Record<number, TeamFeatures> | null = null; | ||
| let canViewRoles = false; | ||
| const orgId = session?.user?.profile?.organizationId ?? session?.user.org?.id; | ||
|
|
||
| // For now we only grab organization features but it would be nice to fetch these on the server side for specific team feature flags | ||
| if (orgId) { | ||
| const features = await getTeamFeatures(orgId); | ||
| const [features, rolePermissions] = await Promise.all([ | ||
| getTeamFeatures(orgId), | ||
| getCachedResourcePermissions(userId, orgId, Resource.Role), | ||
| ]); | ||
|
|
||
| if (features) { | ||
| teamFeatures = { | ||
| [orgId]: features, | ||
| }; | ||
|
|
||
| // Check if user has permission to read roles | ||
| const roleActions = PermissionMapper.toActionMap(rolePermissions, Resource.Role); | ||
| canViewRoles = roleActions[CrudAction.Read] ?? false; | ||
|
Comment on lines
+62
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: The permission check logic is nested inside the |
||
| } | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| <SettingsLayoutAppDirClient {...props} teamFeatures={teamFeatures ?? {}} /> | ||
| <SettingsLayoutAppDirClient {...props} teamFeatures={teamFeatures ?? {}} canViewRoles={canViewRoles} /> | ||
| </> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -54,6 +54,12 @@ export function AdvancedPermissionGroup({ | |||||||||||||||||||||
| } | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const handleCheckedChange = (checked: boolean | string) => { | ||||||||||||||||||||||
| if (!disabled) { | ||||||||||||||||||||||
| onChange(toggleResourcePermissionLevel(resource, checked ? "all" : "none", selectedPermissions)); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Helper function to check if read permission is auto-enabled | ||||||||||||||||||||||
| const isReadAutoEnabled = (action: string) => { | ||||||||||||||||||||||
| if (action === CrudAction.Read) return false; | ||||||||||||||||||||||
|
|
@@ -68,25 +74,38 @@ export function AdvancedPermissionGroup({ | |||||||||||||||||||||
| <button | ||||||||||||||||||||||
| type="button" | ||||||||||||||||||||||
| className="flex cursor-pointer items-center justify-between gap-1.5 p-4" | ||||||||||||||||||||||
| onClick={() => setIsExpanded(!isExpanded)}> | ||||||||||||||||||||||
| <Icon | ||||||||||||||||||||||
| name={isAllResources ? "chevron-right" : "chevron-down"} | ||||||||||||||||||||||
| className={classNames( | ||||||||||||||||||||||
| "h-4 w-4 transition-transform", | ||||||||||||||||||||||
| isExpanded && !isAllResources ? "rotate-180" : "" | ||||||||||||||||||||||
| )} | ||||||||||||||||||||||
| /> | ||||||||||||||||||||||
| onClick={(e) => { | ||||||||||||||||||||||
| // Only toggle expansion if clicking on the button itself, not child elements | ||||||||||||||||||||||
| if (e.target === e.currentTarget) { | ||||||||||||||||||||||
| setIsExpanded(!isExpanded); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+78
to
+81
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: The condition |
||||||||||||||||||||||
| }}> | ||||||||||||||||||||||
| <div className="flex items-center gap-1.5" onClick={() => setIsExpanded(!isExpanded)}> | ||||||||||||||||||||||
| <Icon | ||||||||||||||||||||||
| name="chevron-right" | ||||||||||||||||||||||
| className={classNames( | ||||||||||||||||||||||
| "h-4 w-4 transition-transform", | ||||||||||||||||||||||
| isExpanded && !isAllResources ? "rotate-90" : "" | ||||||||||||||||||||||
| )} | ||||||||||||||||||||||
| /> | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
| <div className="flex items-center gap-2"> | ||||||||||||||||||||||
| <Checkbox | ||||||||||||||||||||||
| checked={isAllSelected} | ||||||||||||||||||||||
| onCheckedChange={() => handleToggleAll} | ||||||||||||||||||||||
| onCheckedChange={handleCheckedChange} | ||||||||||||||||||||||
| onClick={handleToggleAll} | ||||||||||||||||||||||
| disabled={disabled} | ||||||||||||||||||||||
|
Comment on lines
93
to
97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Checkbox has both
Suggested change
|
||||||||||||||||||||||
| /> | ||||||||||||||||||||||
| <span className="text-default text-sm font-medium leading-none"> | ||||||||||||||||||||||
| <span | ||||||||||||||||||||||
| className="text-default cursor-pointer text-sm font-medium leading-none" | ||||||||||||||||||||||
| onClick={() => setIsExpanded(!isExpanded)}> | ||||||||||||||||||||||
| {t(resourceConfig._resource?.i18nKey || "")} | ||||||||||||||||||||||
| </span> | ||||||||||||||||||||||
| <span className="text-muted text-sm font-medium leading-none">{t("all_permissions")}</span> | ||||||||||||||||||||||
| <span | ||||||||||||||||||||||
| className="text-muted cursor-pointer text-sm font-medium leading-none" | ||||||||||||||||||||||
| onClick={() => setIsExpanded(!isExpanded)}> | ||||||||||||||||||||||
| {t("all_permissions")} | ||||||||||||||||||||||
| </span> | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
| </button> | ||||||||||||||||||||||
| {isExpanded && !isAllResources && ( | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { describe, it, expect } from "vitest"; | ||
|
|
||
| import { usePermissions } from "../usePermissions"; | ||
|
|
||
| describe("usePermissions", () => { | ||
| const { getResourcePermissionLevel } = usePermissions(); | ||
|
|
||
| describe("getResourcePermissionLevel", () => { | ||
| it("should return 'all' for any resource when *.* permission is present", () => { | ||
| const permissions = ["*.*", "eventType.create", "eventType.read"]; | ||
|
|
||
| expect(getResourcePermissionLevel("eventType", permissions)).toBe("all"); | ||
| expect(getResourcePermissionLevel("booking", permissions)).toBe("all"); | ||
| expect(getResourcePermissionLevel("team", permissions)).toBe("all"); | ||
| }); | ||
|
|
||
| it("should return 'all' for resource with all individual permissions", () => { | ||
| const permissions = ["eventType.create", "eventType.read", "eventType.update", "eventType.delete"]; | ||
|
|
||
| expect(getResourcePermissionLevel("eventType", permissions)).toBe("all"); | ||
| }); | ||
| it("should return 'read' for resource with only read permission", () => { | ||
| const permissions = ["eventType.read"]; | ||
|
|
||
| expect(getResourcePermissionLevel("eventType", permissions)).toBe("read"); | ||
| }); | ||
|
|
||
| it("should return 'none' for resource with no permissions", () => { | ||
| const permissions = ["booking.create"]; | ||
|
|
||
| expect(getResourcePermissionLevel("eventType", permissions)).toBe("none"); | ||
| }); | ||
|
|
||
| it("should handle * resource correctly", () => { | ||
| const permissionsWithAll = ["*.*"]; | ||
| const permissionsWithoutAll = ["eventType.read"]; | ||
|
|
||
| expect(getResourcePermissionLevel("*", permissionsWithAll)).toBe("all"); | ||
| expect(getResourcePermissionLevel("*", permissionsWithoutAll)).toBe("none"); | ||
| }); | ||
|
|
||
| it("should prioritize *.* over individual permissions", () => { | ||
| const permissions = ["*.*", "eventType.read"]; // Has global all but only read for eventType individually | ||
|
|
||
| expect(getResourcePermissionLevel("eventType", permissions)).toBe("all"); | ||
| }); | ||
| }); | ||
| }); |
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.
logic: The cache key
["resource-permissions"]is too generic and could lead to cache collisions. Consider including userId, teamId, and resource in the cache key for better cache isolation.