drizzle-orm icon indicating copy to clipboard operation
drizzle-orm copied to clipboard

Add proper error handling and wrapping

Open dankochetov opened this issue 2 years ago • 26 comments

dankochetov avatar Apr 04 '23 14:04 dankochetov

What is missing?

kibertoad avatar May 21 '23 21:05 kibertoad

Is this for returning errors, so we can prevent bugs from forgetting to handle exceptions and from incorrectly guessing the error type?

probablykasper avatar Jun 17 '23 09:06 probablykasper

This would be super helpful for our code quality. I noticed this when Drizzle threw me an error, when I tested our API with a wrong resource ID to test if the 404 works. Postgres was already mad, because the wrong ID wasn't a valid UUIDv4. Catching this now and throwing an appropriate 404 is kinda smelly.

Error codes would be nice to have!

fermentfan avatar Jul 29 '23 10:07 fermentfan

Another example of how error handling could improve code quality is with errors raised from constraints.

For example, when see a unique containst error, we get a fairly raw looking error object in our try..catch(e).

{
  code: '23505'
  column: undefined
  constraint: 'users_email_index'
  dataType: undefined
  detail: 'Key (email)=([email protected]) already exists.'
  file: 'nbtinsert.c'
  hint: undefined
  internalPosition: undefined
  internalQuery: undefined
  length: 231
  line: '664'
  name: 'error'
  position: undefined
  routine: '_bt_check_unique'
  schema: 'public'
  severity: 'ERROR'
  table: 'users'
  where: undefined
  message: 'duplicate key value violates unique constraint "users_email_index"'
  stack: <stacktrace_goes_here>
}

We then need to check that certain properties exist with specific values to determine what type of error it was.

A few options are:

// Check the constraint name or message
e.constraint === 'users_email_index';
//or
e.message === 'duplicate key value violates unique constraint "users_email_index"';

// Checking the detail field with a regex
const rx = new RegExp(/Key \(email\).*already exists/);
rx.test(e.detail);

// Checking more internal specifics
e.routine === '_bt_check_unique';

It would be really nice to have some higher level type that hides the internals and allows for simpler use within a catch statement.

swelham avatar Nov 02 '23 19:11 swelham

💎 $50 bounty created by @john-griffin 👉 To claim this bounty, submit your pull request on Algora 📝 Before proceeding, please make sure you can receive payouts in your country 💵 Payment arrives in your account 2-5 days after the bounty is rewarded 💯 You keep 100% of the bounty award 🙏 Thank you for contributing to drizzle-team/drizzle-orm!

algora-pbc avatar Nov 11 '23 16:11 algora-pbc

What's the exactly scope of this? @john-griffin

rishi-raj-jain avatar Nov 11 '23 18:11 rishi-raj-jain

Is the issue still open? I am interested in working on this.

kesh-007 avatar Nov 16 '23 13:11 kesh-007

Hey. Yes this is still open.

From this issue we (@john-griffin and myself) are hoping to achieve a higher level error to work with that has better support for typescript. For example, looking at my comment above, all of the examples rely on checking strings for certain values in a way that doesn't allow us to capture typos at compile time.

I don't have a firm solution and maybe the drizzle team can make some recommendations here, but if we could achieve a type safe way to check for expected database errors, that would achieve the scope of the bounty.

Here are some rough ideas that we have discussed, but are by no means prescriptive.

// Generic error class
class QueryError extends Error {
  type: "NonNull" | "UniqueConstraint" | "CheckConstraint" | "ForeignKey" // etc...
  table: string; // Can this be typed to all known tables
  field: string; // Can this by typed to all known fields
  message: string;
}

// or

// Error type specific error classes
class UniqueConstraintError extends Error {
  table: string; // Can this be typed to all known tables
  field: string; // Can this by typed to all known fields
  message: string;
}
class NonNullError extends Error {
  table: string; // Can this be typed to all known tables
  field: string; // Can this by typed to all known fields
  message: string;
}

Something like this would allow checking for errors without dealing directly with the error class from the pg module and could potentially allow better typing.

catch (e) {
  if (e instanceof QueryError) {
    if (e.type === 'unique' and e.field === 'email') { ... }
    if (e.type === 'unknown_type' and e.field === 'email') { ... } // < type error
  }
}

// or

catch (e) {
  if (e instanceof UniqueConstraintError) {
    if (e.field === 'email') { ... }
    if (e.field === 'unknown_field') { ... } // < type error
  }  
}

Again, happy to defer to the drizzle team for API specific guidance.

swelham avatar Nov 17 '23 15:11 swelham

Those working on this one should do the slash command for algora. Screenshot 2023-11-17 100843

Angelelz avatar Nov 17 '23 15:11 Angelelz

add error classes fast bro

objecthuman avatar Jan 16 '24 15:01 objecthuman

any updates on where this is prioritized in roadmap? Seems like someone might be open to working on it, as long they get guidance from drizzle team on what would work best for drizzle's API? @Angelelz

afogel avatar Feb 28 '24 10:02 afogel

Bumping @afogel 's question, good error handling would be great for displaying the errors on the forms.

dinogomez avatar Mar 16 '24 19:03 dinogomez

Yet another unique constraint user here. Very much +1

StepanMynarik avatar Mar 30 '24 22:03 StepanMynarik

+1

bytekai avatar Apr 10 '24 16:04 bytekai

+1

nantokaworks avatar Apr 10 '24 23:04 nantokaworks

+1 It would really help alot with debugging and error reporting if you had a usable stacktrace. Had a time figuring out why that it was my /[anything]/layout.tsx was causing the issue, and not the /layout.tsx or the /(main)/layout.tsx.

 ⨯ PostgresError: invalid input syntax for type uuid: "some-404-page"
    at Socket.emit (node:events:519:28)
digest: "2286078463"
 GET /some-404-page 500 in 97ms

ThomasAunvik avatar Apr 24 '24 15:04 ThomasAunvik

@Angelelz just a gentle bump reminder here -- any additional guidance the team can give here?

afogel avatar Apr 25 '24 07:04 afogel

For the ones having issues with invalid uuids, I have kind of a work around. I am using uuid_v1(), and those strings are always 32 letters/numbers and 4 hyphens, so 36 characters.

I am using Sveltekit and put this in my +page.server.ts:

 if (params.uuid.length !== 36) {
	throw error(404, 'Not found here');
}

const [event] = await db.select().from(eventTable).where(eq(eventTable.uuid, params.uuid));

if (!event) {
	throw error(404, 'Not found here');
}

You can even add some regex to check if there is a combination of 4 hyphens and 32 letters/numbers.

The best solution would still be a way to handle those errors.

creadevv avatar May 15 '24 09:05 creadevv

I'm working with superforms and trpc. For the moment I'm handling errors like so:

// +page.server.ts

export const actions: Actions = {
	default: async ({ request, fetch }) => {
		const form = await superValidate(request, zod(schema));

		if (!form.valid) {
			return fail(400, { form });
		}

		try {
			const trpc = trpcOnServer(fetch);
			const [newClient] = await trpc.clients.save.mutate({ name: form.data.clientName });
			setMessage(form, `The project was created correctly.`);

		} catch (error) {
			if (error instanceof TRPCError || error instanceof TRPCClientError) {
				if (error.message == 'duplicate key value violates unique constraint "client_name_unique"')
					setError(form, 'clientName', 'Client name already exists.');
			} else {
				setMessage(form, 'Could not create the project.', {
					status: 400
				});
			}
		}

		return { form };
	}
};

but would be good to be able to avoid hardcoding it

cellulosa avatar Jun 02 '24 19:06 cellulosa

+1 on this - would be great!

jonnicholson94 avatar Jul 01 '24 14:07 jonnicholson94

+1 would indeed be nice to have

RobertMercieca avatar Jul 08 '24 19:07 RobertMercieca

+1 love to see good error handling from drizzle

cybercoder-naj avatar Aug 03 '24 15:08 cybercoder-naj