Skip to content
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

feat: allows forcing/skipping calendar cache serving #18224

Merged
merged 7 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 42 additions & 28 deletions packages/app-store/googlecalendar/lib/CalendarService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { MeetLocationType } from "@calcom/app-store/locations";
import dayjs from "@calcom/dayjs";
import { CalendarCache } from "@calcom/features/calendar-cache/calendar-cache";
import type { FreeBusyArgs } from "@calcom/features/calendar-cache/calendar-cache.repository.interface";
import { getTimeMax, getTimeMin } from "@calcom/features/calendar-cache/lib/datesForCache";
import { getLocation, getRichDescription } from "@calcom/lib/CalEventParser";
import type CalendarService from "@calcom/lib/CalendarService";
import {
Expand Down Expand Up @@ -510,16 +511,22 @@ export default class GoogleCalendarService implements Calendar {
return apiResponse.json;
}

async getCacheOrFetchAvailability(args: FreeBusyArgs): Promise<EventBusyDate[] | null> {
const { timeMin, timeMax, items } = args;
let freeBusyResult: calendar_v3.Schema$FreeBusyResponse = {};
async getFreeBusyResult(
args: FreeBusyArgs,
shouldServeCache?: boolean
): Promise<calendar_v3.Schema$FreeBusyResponse> {
Comment on lines +514 to +517
Copy link
Member Author

Choose a reason for hiding this comment

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

Extracted to take advantage of early returns

if (shouldServeCache === false) return await this.fetchAvailability(args);
Copy link
Member Author

Choose a reason for hiding this comment

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

If the parameter is explicitly set to false we skip cache altogether

const calendarCache = await CalendarCache.init(null);
const cached = await calendarCache.getCachedAvailability(this.credential.id, args);
if (cached) {
freeBusyResult = cached.value as unknown as calendar_v3.Schema$FreeBusyResponse;
} else {
freeBusyResult = await this.fetchAvailability({ timeMin, timeMax, items });
}
if (cached) return cached.value as unknown as calendar_v3.Schema$FreeBusyResponse;
return await this.fetchAvailability(args);
}

async getCacheOrFetchAvailability(
args: FreeBusyArgs,
shouldServeCache?: boolean
): Promise<EventBusyDate[] | null> {
const freeBusyResult = await this.getFreeBusyResult(args, shouldServeCache);
if (!freeBusyResult.calendars) return null;

const result = Object.values(freeBusyResult.calendars).reduce((c, i) => {
Expand All @@ -537,7 +544,8 @@ export default class GoogleCalendarService implements Calendar {
async getAvailability(
dateFrom: string,
dateTo: string,
selectedCalendars: IntegrationCalendar[]
selectedCalendars: IntegrationCalendar[],
shouldServeCache?: boolean
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a fan to drilling but I couldn't find a cleaner way to pass this setting around.

): Promise<EventBusyDate[]> {
this.log.debug("Getting availability", safeStringify({ dateFrom, dateTo, selectedCalendars }));
const calendar = await this.authedCalendar();
Expand All @@ -563,11 +571,14 @@ export default class GoogleCalendarService implements Calendar {

// /freebusy from google api only allows a date range of 90 days
if (diff <= 90) {
const freeBusyData = await this.getCacheOrFetchAvailability({
timeMin: dateFrom,
timeMax: dateTo,
items: calsIds.map((id) => ({ id })),
});
const freeBusyData = await this.getCacheOrFetchAvailability(
{
timeMin: dateFrom,
timeMax: dateTo,
items: calsIds.map((id) => ({ id })),
},
shouldServeCache
);
if (!freeBusyData) throw new Error("No response from google calendar");

return freeBusyData;
Expand All @@ -583,11 +594,14 @@ export default class GoogleCalendarService implements Calendar {
if (endDate.isAfter(originalEndDate)) endDate = originalEndDate;

busyData.push(
...((await this.getCacheOrFetchAvailability({
timeMin: startDate.format(),
timeMax: endDate.format(),
items: calsIds.map((id) => ({ id })),
})) || [])
...((await this.getCacheOrFetchAvailability(
{
timeMin: startDate.format(),
timeMax: endDate.format(),
items: calsIds.map((id) => ({ id })),
},
shouldServeCache
)) || [])
);

startDate = endDate.add(1, "minutes");
Expand Down Expand Up @@ -672,12 +686,7 @@ export default class GoogleCalendarService implements Calendar {
}
async unwatchCalendar({ calendarId }: { calendarId: string }) {
const credentialId = this.credential.id;
const sc = await prisma.selectedCalendar.findFirst({
where: {
credentialId,
externalId: calendarId,
},
});
const sc = await SelectedCalendarRepository.findByExternalId(credentialId, calendarId);
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to repo

// Delete the calendar cache to force a fresh cache
await prisma.calendarCache.deleteMany({ where: { credentialId } });
const calendar = await this.authedCalendar();
Expand All @@ -699,6 +708,12 @@ export default class GoogleCalendarService implements Calendar {
googleChannelResourceUri: null,
googleChannelExpiration: null,
});
// Populate the cache back for the remaining calendars, if any
const remainingCalendars =
sc?.credential?.selectedCalendars.filter((sc) => sc.externalId !== calendarId) || [];
if (remainingCalendars.length > 0) {
await this.fetchAvailabilityAndSetCache(remainingCalendars);
}
Comment on lines +711 to +716
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a concern that @hariombalhara exposed

}

async setAvailabilityInCache(args: FreeBusyArgs, data: calendar_v3.Schema$FreeBusyResponse): Promise<void> {
Expand All @@ -707,12 +722,11 @@ export default class GoogleCalendarService implements Calendar {
}

async fetchAvailabilityAndSetCache(selectedCalendars: IntegrationCalendar[]) {
const date = new Date();
const parsedArgs = {
/** Expand the start date to the start of the month */
timeMin: new Date(date.getFullYear(), date.getMonth(), 1, 0, 0, 0, 0).toISOString(),
timeMin: getTimeMax(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to util to reuse when saving and fetching

/** Expand the end date to the end of the month */
timeMax: new Date(date.getFullYear(), date.getMonth() + 1, 0, 0, 0, 0, 0).toISOString(),
timeMax: getTimeMin(),
items: selectedCalendars.map((sc) => ({ id: sc.externalId })),
};
const data = await this.fetchAvailability(parsedArgs);
Expand Down
57 changes: 9 additions & 48 deletions packages/core/CalendarManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { getUid } from "@calcom/lib/CalEventParser";
import logger from "@calcom/lib/logger";
import { getPiiFreeCalendarEvent, getPiiFreeCredential } from "@calcom/lib/piiFreeData";
import { safeStringify } from "@calcom/lib/safeStringify";
import { performance } from "@calcom/lib/server/perfObserver";
import type {
CalendarEvent,
EventBusyDate,
Expand Down Expand Up @@ -136,51 +135,6 @@ const cleanIntegrationKeys = (
return rest;
};

// here I will fetch the page json file.
export const getCachedResults = async (
Copy link
Member Author

Choose a reason for hiding this comment

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

Dead code

withCredentials: CredentialPayload[],
dateFrom: string,
dateTo: string,
selectedCalendars: SelectedCalendar[]
): Promise<EventBusyDate[][]> => {
const calendarCredentials = withCredentials.filter((credential) => credential.type.endsWith("_calendar"));
const calendars = await Promise.all(calendarCredentials.map((credential) => getCalendar(credential)));
performance.mark("getBusyCalendarTimesStart");
const results = calendars.map(async (c, i) => {
/** Filter out nulls */
if (!c) return [];
/** We rely on the index so we can match credentials with calendars */
const { type, appId } = calendarCredentials[i];
/** We just pass the calendars that matched the credential type,
* TODO: Migrate credential type or appId
*/
const passedSelectedCalendars = selectedCalendars.filter((sc) => sc.integration === type);
if (!passedSelectedCalendars.length) return [];
/** We extract external Ids so we don't cache too much */
const selectedCalendarIds = passedSelectedCalendars.map((sc) => sc.externalId);
/** If we don't then we actually fetch external calendars (which can be very slow) */
performance.mark("eventBusyDatesStart");
const eventBusyDates = await c.getAvailability(dateFrom, dateTo, passedSelectedCalendars);
performance.mark("eventBusyDatesEnd");
performance.measure(
`[getAvailability for ${selectedCalendarIds.join(", ")}][$1]'`,
"eventBusyDatesStart",
"eventBusyDatesEnd"
);

return eventBusyDates.map((a: object) => ({ ...a, source: `${appId}` }));
});
const awaitedResults = await Promise.all(results);
performance.mark("getBusyCalendarTimesEnd");
performance.measure(
`getBusyCalendarTimes took $1 for creds ${calendarCredentials.map((cred) => cred.id)}`,
"getBusyCalendarTimesStart",
"getBusyCalendarTimesEnd"
);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return awaitedResults as any;
};

/**
* Get months between given dates
* @returns ["2023-04", "2024-05"]
Expand All @@ -203,7 +157,8 @@ export const getBusyCalendarTimes = async (
withCredentials: CredentialPayload[],
dateFrom: string,
dateTo: string,
selectedCalendars: SelectedCalendar[]
selectedCalendars: SelectedCalendar[],
shouldServeCache?: boolean
) => {
let results: EventBusyDate[][] = [];
// const months = getMonths(dateFrom, dateTo);
Expand All @@ -212,7 +167,13 @@ export const getBusyCalendarTimes = async (
const startDate = dayjs(dateFrom).subtract(11, "hours").format();
// Add 14 hours from the start date to avoid problems in UTC+ time zones.
const endDate = dayjs(dateTo).endOf("month").add(14, "hours").format();
results = await getCalendarsEvents(withCredentials, startDate, endDate, selectedCalendars);
results = await getCalendarsEvents(
withCredentials,
startDate,
endDate,
selectedCalendars,
shouldServeCache
);
} catch (e) {
log.warn(safeStringify(e));
}
Expand Down
5 changes: 4 additions & 1 deletion packages/core/getBusyTimes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export async function getBusyTimes(params: {
})[]
| null;
bypassBusyCalendarTimes: boolean;
shouldServeCache?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this nullable? Seems like we should either just pass true or false.

}) {
const {
credentials,
Expand All @@ -60,6 +61,7 @@ export async function getBusyTimes(params: {
rescheduleUid,
duration,
bypassBusyCalendarTimes = false,
shouldServeCache,
} = params;

logger.silly(
Expand Down Expand Up @@ -243,7 +245,8 @@ export async function getBusyTimes(params: {
credentials,
startTime,
endTime,
selectedCalendars
selectedCalendars,
shouldServeCache
);
const endConnectedCalendarsGet = performance.now();
logger.debug(
Expand Down
25 changes: 18 additions & 7 deletions packages/core/getCalendarsEvents.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,12 @@ describe("getCalendarsEvents", () => {
[selectedCalendar]
);

expect(getAvailabilitySpy).toHaveBeenCalledWith("2010-12-01", "2010-12-04", [selectedCalendar]);
expect(getAvailabilitySpy).toHaveBeenCalledWith(
"2010-12-01",
"2010-12-04",
[selectedCalendar],
undefined
);
expect(result).toEqual([
availability.map((av) => ({
...av,
Expand Down Expand Up @@ -190,12 +195,18 @@ describe("getCalendarsEvents", () => {
[selectedGoogleCalendar, selectedOfficeCalendar]
);

expect(getGoogleAvailabilitySpy).toHaveBeenCalledWith("2010-12-01", "2010-12-04", [
selectedGoogleCalendar,
]);
expect(getOfficeAvailabilitySpy).toHaveBeenCalledWith("2010-12-01", "2010-12-04", [
selectedOfficeCalendar,
]);
expect(getGoogleAvailabilitySpy).toHaveBeenCalledWith(
"2010-12-01",
"2010-12-04",
[selectedGoogleCalendar],
undefined
);
expect(getOfficeAvailabilitySpy).toHaveBeenCalledWith(
"2010-12-01",
"2010-12-04",
[selectedOfficeCalendar],
undefined
);
expect(result).toEqual([
googleAvailability.map((av) => ({
...av,
Expand Down
10 changes: 8 additions & 2 deletions packages/core/getCalendarsEvents.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ const getCalendarsEvents = async (
withCredentials: CredentialPayload[],
dateFrom: string,
dateTo: string,
selectedCalendars: SelectedCalendar[]
selectedCalendars: SelectedCalendar[],
shouldServeCache?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this nullable? Seems like we should either just pass true or false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Undefined will fallback to the DB default. If we force a boolean value the fallback will never execute.

): Promise<EventBusyDate[][]> => {
const calendarCredentials = withCredentials
.filter((credential) => credential.type.endsWith("_calendar"))
Expand Down Expand Up @@ -45,7 +46,12 @@ const getCalendarsEvents = async (
selectedCalendars: passedSelectedCalendars.map(getPiiFreeSelectedCalendar),
})
);
const eventBusyDates = await c.getAvailability(dateFrom, dateTo, passedSelectedCalendars);
const eventBusyDates = await c.getAvailability(
dateFrom,
dateTo,
passedSelectedCalendars,
shouldServeCache
);
performance.mark("eventBusyDatesEnd");
performance.measure(
`[getAvailability for ${selectedCalendarIds.join(", ")}][$1]'`,
Expand Down
4 changes: 4 additions & 0 deletions packages/core/getUserAvailability.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const availabilitySchema = z
withSource: z.boolean().optional(),
returnDateOverrides: z.boolean(),
bypassBusyCalendarTimes: z.boolean().optional(),
shouldServeCache: z.boolean().optional(),
})
.refine((data) => !!data.username || !!data.userId, "Either username or userId should be filled in.");

Expand Down Expand Up @@ -245,6 +246,7 @@ const _getUserAvailability = async function getUsersWorkingHoursLifeTheUniverseA
duration?: number;
returnDateOverrides: boolean;
bypassBusyCalendarTimes: boolean;
shouldServeCache?: boolean;
},
initialData?: {
user?: GetUser;
Expand Down Expand Up @@ -279,6 +281,7 @@ const _getUserAvailability = async function getUsersWorkingHoursLifeTheUniverseA
duration,
returnDateOverrides,
bypassBusyCalendarTimes = false,
shouldServeCache,
} = availabilitySchema.parse(query);

if (!dateFrom.isValid() || !dateTo.isValid()) {
Expand Down Expand Up @@ -393,6 +396,7 @@ const _getUserAvailability = async function getUsersWorkingHoursLifeTheUniverseA
duration,
currentBookings: initialData?.currentBookings,
bypassBusyCalendarTimes,
shouldServeCache,
});

const detailedBusyTimes: EventBusyDetails[] = [
Expand Down
13 changes: 1 addition & 12 deletions packages/features/calendar-cache/calendar-cache.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type { Calendar } from "@calcom/types/Calendar";

import type { ICalendarCacheRepository } from "./calendar-cache.repository.interface";
import { watchCalendarSchema } from "./calendar-cache.repository.schema";
import { getTimeMax, getTimeMin } from "./lib/datesForCache";

const log = logger.getSubLogger({ prefix: ["CalendarCacheRepository"] });

Expand All @@ -15,18 +16,6 @@ const MS_PER_DAY = 24 * 60 * 60 * 1000;
const ONE_MONTH_IN_MS = 30 * MS_PER_DAY;
const CACHING_TIME = ONE_MONTH_IN_MS;

/** Expand the start date to the start of the month */
function getTimeMin(timeMin: string) {
const dateMin = new Date(timeMin);
return new Date(dateMin.getFullYear(), dateMin.getMonth(), 1, 0, 0, 0, 0).toISOString();
}

/** Expand the end date to the end of the month */
function getTimeMax(timeMax: string) {
const dateMax = new Date(timeMax);
return new Date(dateMax.getFullYear(), dateMax.getMonth() + 1, 0, 0, 0, 0, 0).toISOString();
}

export function parseKeyForCache(args: FreeBusyArgs): string {
const { timeMin: _timeMin, timeMax: _timeMax, items } = args;
const { timeMin, timeMax } = handleMinMax(_timeMin, _timeMax);
Expand Down
16 changes: 16 additions & 0 deletions packages/features/calendar-cache/lib/datesForCache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/** Expand the start date to the beginning of the current month */
export const getTimeMin = (timeMin?: string) => {
const date = timeMin ? new Date(timeMin) : new Date();
date.setDate(1);
date.setHours(0, 0, 0, 0);
return date.toISOString();
};

/** Expand the end date to the end of the next month */
export function getTimeMax(timeMax?: string) {
const date = timeMax ? new Date(timeMax) : new Date();
date.setMonth(date.getMonth() + 2);
date.setDate(0);
date.setHours(0, 0, 0, 0);
return date.toISOString();
}
1 change: 1 addition & 0 deletions packages/features/flags/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
**/
export type AppFlags = {
"calendar-cache": boolean;
"calendar-cache-serve": boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

Whether to serve the cache by default or not. Override-able via URL params. @keithwillcode

emails: boolean;
insights: boolean;
teams: boolean;
Expand Down
1 change: 1 addition & 0 deletions packages/features/flags/features.repository.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ import type { AppFlags } from "./config";
export interface IFeaturesRepository {
checkIfFeatureIsEnabledGlobally(slug: keyof AppFlags): Promise<boolean>;
checkIfUserHasFeature(userId: number, slug: string): Promise<boolean>;
checkIfTeamHasFeature(teamId: number, slug: keyof AppFlags): Promise<boolean>;
}
Loading
Loading