Add database transactions where necessary #391

sodic · 2025-02-28T09:31:24Z

No description provided.

sodic · 2025-02-28T09:34:10Z

template/app/src/analytics/operations.ts

    orderBy: {
      date: 'desc',
    },
    include: {
      sources: true,
    },
-  });
+  } as const;


FYI: Necessary to prevent type widening.

For example, fi I didn't use as const, TS would infer the type of statsQuery.orderBy.date as string, which is to permissive to be accepted by findFirst and findMany.

Interesting. Why do that instead of directly typing it?

sodic · 2025-02-28T09:37:27Z

template/app/src/demo-ai-app/operations.ts

@@ -35,11 +35,15 @@ export const generateGptResponse: GenerateGptResponse<GenerateGptResponseInput,
  context
 ) => {


This operation went through many changes.

It's not 100% foolproof (i.e., doesn't cover all the edge cases).

It can be done, but I can't do it in a way that's simple enough to make sense for an OpenSaas example app. I believe I'd need an extra table with pending responses and a job that periodically deletes unfinished responses and refunds the user.

In any case, I'm not merging this before a call with Vinny since I don't fully understand the old code. I left a bunch of TODOs and NOTEs with my questions.

What about packing everyting into the transaction itself (by doing this do we lose Wasp's automatic cache invalidation) ?

const eligiblityCheckPrismaTx = await prisma.$transaction(async (tx) => { const freshUserData = await tx.user.findUnique({ where: { id: context.user!.id }, }); if (!freshUserData) { throw new HttpError(404, 'User not found'); } if (!isEligibleForResponse(freshUserData)) { throw new HttpError(402, 'User has not paid or is out of credits'); } if (!userHasSubscriptionPlanEffect(freshUserData)) { const updatedUser = await tx.user.update({ where: { id: context.user!.id }, data: { credits: { decrement: 1 } }, }); } const gptResponse = await tx.gptResponse.create({ data: { user: { connect: { id: context.user!.id } }, content: dailyPlanJson, }, }); return { updatedUser, gptResponse }; });

infomiho · 2025-02-28T09:44:35Z

template/app/src/demo-ai-app/operations.ts

-    description,
-    time,
-  }));
+  console.log('Calling open AI api');


Leftover log

Actually, I included this one on purpose 😄

We already had "Decrementing credits," so it seemed fitting, especially since the "decrementing credits" message now appears very late (after we get the response from GPT).

infomiho · 2025-02-28T09:48:28Z

template/app/src/demo-ai-app/operations.ts

@@ -296,3 +211,98 @@ export const getAllTasksByUser: GetAllTasksByUser<void, Task[]> = async (_args,
  });
 };
 //#endregion
+
+// TODO: Why is hours a string?


It can be easily be changed, update the operations validation, parse the input as number and change it here as well. I guess it was easier to just send the string that was received from input directly.

infomiho · 2025-02-28T09:50:04Z

template/app/src/demo-ai-app/operations.ts

+
+// TODO: Why is hours a string?
+async function getDailyPlanFromGpt(tasks: Task[], hours: string): Promise<string | null> {
+  // TODO: Why was this a singleton


It's like creating an instance of axios and using it everywhere. We init the OpenAI client and use it in multiple places. In some projects we would even put it in openai.ts and import it as a service we use. I'd keep it that way, since most users are used to that kind of usage.

vincanger · 2025-03-04T16:32:11Z

template/app/src/demo-ai-app/operations.ts

+  // NOTE: I changed this up, first I do the request, and then I decrement
+  // credits. Is that dangerous? Protecting from it could be too complicated
+  // TODO: Potential issue here, user can lose credits between the point we
+  // inject it into the context and here


I don't understand what you mean here by "user can lose credits between the point we inject it into context and here".

But, yes, I see the problem. When using prisma transactions we have to run them at the end after the openai request, and I think that's fine.

vincanger · 2025-03-04T16:33:42Z

template/app/src/demo-ai-app/operations.ts

+  // credits. Is that dangerous? Protecting from it could be too complicated
+  // TODO: Potential issue here, user can lose credits between the point we
+  // inject it into the context and here
+  // Seems to me that there should be users in the database with negative credits


where: { id: user.id, credits: { gte: 1 }, },

This could solve that issue, but I've also suggested another approach which is putting the eligibility check within the transaction itself (see my other comment)

vincanger · 2025-03-04T16:35:04Z

template/app/src/demo-ai-app/operations.ts

-    console.log('gpt function call arguments: ', gptArgs);
+  // NOTE: Since these two are now brought together, I put them in a single transaction - so no rollback necessary
+  // TODO: But what if the server crashes after the transaction and before
+  // the response? I guess no big deal, since the response is in the db.


vincanger · 2025-03-04T16:38:02Z

template/app/src/demo-ai-app/operations.ts

-    throw new HttpError(statusCode, errorMessage);
-  }
+  // TODO: Can this ever fail?
+  return JSON.parse(dailyPlanJson);


If it's an invalid json string then yes it can. We could check it first to make sure it is before parsing perhaps.

vincanger · 2025-03-04T16:41:08Z

template/app/src/demo-ai-app/operations.ts

+  // TODO: Why not check for allowed states?
+  const isUserSubscribed =
+    user.subscriptionStatus !== SubscriptionStatus.Deleted &&
+    user.subscriptionStatus !== SubscriptionStatus.PastDue;


I assumed that it's easier to check for two unapproved states rather than more allowed states (the app could add more allowed states like trialing, for example). What's best practice?

vincanger · 2025-03-04T16:46:51Z

template/app/src/demo-ai-app/operations.ts

+  // shouldn't take priority over credits since it makes no sece spending
+  // credits when a subscription is active?
+  // If they aren't subscribed, then it would make sense to spend credits.
+  // The old code always subtracted credits so I kept that, is this a bug?


Yeah, it is. We could add a function like returnPaymentPlanEffect and if it's subscription then we don't pass the credit decrement user update.

vincanger · 2025-03-04T16:55:06Z

template/app/src/analytics/operations.ts

    orderBy: {
      date: 'desc',
    },
    include: {
      sources: true,
    },
-  });
+  } as const;


Interesting. Why do that instead of directly typing it?

vincanger · 2025-03-04T18:51:37Z

template/app/src/demo-ai-app/operations.ts

@@ -35,11 +35,15 @@ export const generateGptResponse: GenerateGptResponse<GenerateGptResponseInput,
  context
 ) => {


What about packing everyting into the transaction itself (by doing this do we lose Wasp's automatic cache invalidation) ?

const eligiblityCheckPrismaTx = await prisma.$transaction(async (tx) => { const freshUserData = await tx.user.findUnique({ where: { id: context.user!.id }, }); if (!freshUserData) { throw new HttpError(404, 'User not found'); } if (!isEligibleForResponse(freshUserData)) { throw new HttpError(402, 'User has not paid or is out of credits'); } if (!userHasSubscriptionPlanEffect(freshUserData)) { const updatedUser = await tx.user.update({ where: { id: context.user!.id }, data: { credits: { decrement: 1 } }, }); } const gptResponse = await tx.gptResponse.create({ data: { user: { connect: { id: context.user!.id } }, content: dailyPlanJson, }, }); return { updatedUser, gptResponse }; });

sodic added 3 commits February 27, 2025 14:42

Add transactions to getDailyStats

8055995

Refactor generateGptResponse to include a transaction

ed6add7

Merge branch 'main' into filip-transactions

6c191d2

sodic commented Feb 28, 2025

View reviewed changes

infomiho reviewed Feb 28, 2025

View reviewed changes

vincanger reviewed Mar 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add database transactions where necessary #391

Add database transactions where necessary #391

sodic commented Feb 28, 2025

sodic Feb 28, 2025

vincanger Mar 4, 2025

sodic Feb 28, 2025

vincanger Mar 4, 2025 •

edited

Loading

infomiho Feb 28, 2025

sodic Feb 28, 2025

infomiho Feb 28, 2025

infomiho Feb 28, 2025

vincanger Mar 4, 2025

vincanger Mar 4, 2025 •

edited

Loading

vincanger Mar 4, 2025

vincanger Mar 4, 2025 •

edited

Loading

vincanger Mar 4, 2025

vincanger Mar 4, 2025

vincanger Mar 4, 2025

vincanger Mar 4, 2025 •

edited

Loading

		@@ -35,11 +35,15 @@ export const generateGptResponse: GenerateGptResponse<GenerateGptResponseInput,
		context
		) => {

Add database transactions where necessary #391

Are you sure you want to change the base?

Add database transactions where necessary #391

Conversation

sodic commented Feb 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincanger Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincanger Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincanger Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincanger Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

vincanger Mar 4, 2025 •

edited

Loading

vincanger Mar 4, 2025 •

edited

Loading

vincanger Mar 4, 2025 •

edited

Loading

vincanger Mar 4, 2025 •

edited

Loading