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 too permissive to be accepted by findFirst and findMany.

Interesting. Why do that instead of directly typing it?

Hm, what exactly do you mean by typing it directly?

ok I get it now. You want statsQuery.orderBy.date to be of type desc or asc and using as const is easier than typing the variable like statsQuery: statsQueryInfo = {...}

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 }; });

I had that idea too. These are called interactive transactions I believe.

The thing is, transactions should be very short-lived. Calling an API over the network in a transaction is a dangerous (if the call hangs, it can block the entire database).

Prisma recommends against this as well:

Use interactive transactions with caution. Keeping transactions open for a long time hurts database performance and can even cause deadlocks. Try to avoid performing network requests and executing slow queries inside your transaction functions. We recommend you get in and out as quick as possible!

I think having credits as a number is the problem here, credits should somehow be calculated based on some list of usage e.g. there are 10 entries for successful GPT generations, that -10 credits.

Another thing that could help us here is some sort of a queue of pending generations that "freeze" credits e.g. 2 pending generations temporarily block 2 credits and on success, decrement 2 credits.

Maybe it's too complex for an example app, but maybe not, it's just one extra table + maybe a cleanup job. It maybe makes sense to create an issue for an "ideal system" if we want to go that way and solve it in the future.

We can revisit it, sure. I'll create an issue.

I still think an extra table and cleanup jobs are a bit too much though, but could be convinced otherwise :)

Issue created: #421

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

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.

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

Made an issue that explains it in detail (it's a core Wasp issue more than an Open Saas one): wasp-lang/wasp#2675

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

This is a different problem, but I think it's fine too.

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)

Nice trick, but doesn't help us unfortunately because we still call the API between checking the user's eligibility and decrementing their credits.

I've decided it's no big deal after all and left a comment in the code.

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.

But can GPT even return invalid JSON or is it deterministic?

Also, if it can return invalid json and this does fail, what do we want to do about user credits?

But can GPT even return invalid JSON or is it deterministic?

I think that it can't return invalid JSON because we are using function calling with a certain JSON schema.

Also, if it can return invalid json and this does fail, what do we want to do about user credits?

It feels like the same case as we had here: https://github.com/wasp-lang/open-saas/pull/391/files#diff-a29403ed1508d66f40eb6cf37393b27eef76a0ed649ad79211a29444071664a1R59

Maybe we should parse ASAP and fail sooner?

I think that it can't return invalid JSON because we are using function calling with a certain JSON schema.

That's how it looks to me as well. @vincanger what makes you think the json string can be invalid?

In the meantime, I'm assuming it's always valid and am treating it as such.

yeah that was me being ignorant. keep assuming it's valid.

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?

The best practice is usually to always default to the less damaging/more secure state. This pattern is called "fail-safe defaults" or "implicit deny."

It's the principle behind defining firewall rules (e.g., deny everything by default, specify a list of IPs and protocols you allow) and authorization (unauthorized unless proven otherwise).

The idea is that, if a new thing appears and you forget to update the rules, you want it to fail in a way that's safe. Let's assume you add a new state (allowed or unapproved) and forget to update this check.
It's better to temporarily reject some eligible users (they'll complain and you'll fix it quickly) than to allow non-eligible users (bad actors will drain your resoruces and keep it to themselves).

makes sense. thanks!

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.

Made an issue: #418

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 · 2025-04-15T12:02:49Z

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

+  // TODO: Do I need a try catch now that I'm saving a response and
+  // decrementing credits in a transaction?

-    const gptArgs = completion?.choices[0]?.message?.tool_calls?.[0]?.function.arguments;
+  // 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


@infomiho Please comment on this

We are concerned here with the case call to OpenAI failed:

if we decrement before -> user loses credits

if we decrement after -> use can potentially spam many requests at once and abuse the system

I guess it depends on what the Open Saas user optimises for, for some it might be easier just to give users more credits if something goes wrong. Some maybe want to make sure users don't feel cheated.

What I'm trying to say, I'm not sure which way to go - decrement before or after. I think it maybe makes sense to lay out the pros and cons in a comment so users can maybe adjust it to their needs?

I'd say decrement after. They could spam a bit but wouldn't get very far. I like keeping it simple for the purposes of an example and giving the user the benefit of the doubt.

Great, that's what we did. And the potential improvements are documented here: #421

sodic · 2025-04-15T12:21:55Z

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".

Made an issue that explains it in detail (it's a core Wasp issue more than an Open Saas one): wasp-lang/wasp#2675

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

This is a different problem, but I think it's fine too.

infomiho · 2025-04-18T11:19:56Z

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

+const openAi = getOpenAi();
+function getOpenAi(): OpenAI | null {
+  if (process.env.OPENAI_API_KEY) {
+    return new OpenAI({ apiKey: process.env.OPENAI_API_KEY });
+  } else {
+    return null;
  }
-  return new OpenAI({ apiKey: process.env.OPENAI_API_KEY });
 }


This kind of change goes against what we have been doing in the framework ourselves: throw if env vars are missing immediately. We should migrate all of the env vars validation to use the new Zod env vars validation feature (in a separate PR of course) and when we do it - it will throw an error when the env vars is missing. To keep the behavior consistent - I'd do the same here.

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

infomiho

Left some comments and thoughts :)

vincanger

Noice 👊

sodic · 2025-04-28T20:43:51Z

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


+  const { hours } = ensureArgsSchemaOrThrowHttpError(generateGptResponseInputSchema, rawArgs);


We parse user input only after the user successfully authenticated.

vincanger

i like

vincanger

Thanks for the detailed responses. Looks good.

vincanger · 2025-04-29T09:19:32Z

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

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


yeah that was me being ignorant. keep assuming it's valid.

vincanger · 2025-04-29T09:20:17Z

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;


makes sense. thanks!

vincanger · 2025-04-29T09:27:16Z

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

+  // TODO: Do I need a try catch now that I'm saving a response and
+  // decrementing credits in a transaction?

-    const gptArgs = completion?.choices[0]?.message?.tool_calls?.[0]?.function.arguments;
+  // 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


I'd say decrement after. They could spam a bit but wouldn't get very far. I like keeping it simple for the purposes of an example and giving the user the benefit of the doubt.

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

template/app/src/demo-ai-app/operations.ts Show resolved Hide resolved

infomiho reviewed Feb 28, 2025

View reviewed changes

vincanger reviewed Mar 4, 2025

View reviewed changes

sodic self-assigned this Apr 8, 2025

sodic commented Apr 15, 2025

View reviewed changes

sodic added 2 commits April 15, 2025 14:23

Review changes

994cd1e

Remove another todo

1cb0772

infomiho self-requested a review April 18, 2025 11:16

infomiho reviewed Apr 18, 2025

View reviewed changes

template/app/src/demo-ai-app/operations.ts Outdated Show resolved Hide resolved

infomiho reviewed Apr 18, 2025

View reviewed changes

vincanger approved these changes Apr 22, 2025

View reviewed changes

sodic added 2 commits April 28, 2025 20:43

Merge branch 'main' into filip-transactions

b381af3

Address review comments

e3588fa

sodic commented Apr 28, 2025

View reviewed changes

sodic added 2 commits April 28, 2025 22:45

Change name of open AI setup function

5c39bcd

Convert string to number

36aa5df

infomiho approved these changes Apr 28, 2025

View reviewed changes

sodic mentioned this pull request Apr 28, 2025

Think about making our Demo AI app more robust #421

Open

vincanger approved these changes Apr 29, 2025

View reviewed changes

Explain diff problem in gitignore

6963e38

sodic merged commit 855ddf1 into main Apr 29, 2025
1 check passed

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


		const { hours } = ensureArgsSchemaOrThrowHttpError(generateGptResponseInputSchema, rawArgs);

Uh oh!

Add database transactions where necessary #391

Add database transactions where necessary #391

Uh oh!

Conversation

sodic commented Feb 28, 2025

Uh oh!

sodic Feb 28, 2025 • edited by infomiho Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincanger Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sodic Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sodic Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincanger Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincanger Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sodic Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincanger Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

sodic Feb 28, 2025 •

edited by infomiho

Loading

vincanger Mar 4, 2025 •

edited

Loading

sodic Apr 14, 2025 •

edited

Loading

sodic Apr 28, 2025 •

edited

Loading

vincanger Mar 4, 2025 •

edited

Loading

vincanger Mar 4, 2025 •

edited

Loading

sodic Apr 28, 2025 •

edited

Loading

vincanger Mar 4, 2025 •

edited

Loading

infomiho Apr 18, 2025 •

edited

Loading