depot / kysely-planetscale

A Kysely dialect for PlanetScale Serverless
https://depot.dev/blog/kysely-dialect-planetscale
MIT License
350 stars 15 forks source link

Incorrect type for numDeletedRows being returned from deleteFrom query #14

Closed Vyast closed 1 year ago

Vyast commented 1 year ago

👋

I recently opened an issue in the kysely repo and was redirected here.

const result = await db
    .deleteFrom("Log")
    .where("expiresAt", "<=", date)
    .executeTakeFirst();

const count = result.numDeletedRows ? Number(result.numDeletedRows) : 0;

Running the example above using the planetscale dialect, result is always an object with type DeleteResult and the type of result.numDeletedRows is always bigint according to typescript.

result is never undefined, but the issue I am experiencing is that if 0 rows are deleted, result.numDeletedRows returns undefined even though the type should always be bigint according to typescript, otherwise if 1 or more rows are deleted the correct type is returned.

Looking further into the kysely implementation, it seems as the type for result.numDeletedRows is supposed to be bigint | undefined.

I just wanted to ask and see if this is a bug that could be fixed or if I am simply overlooking something.

{
  "kysely": "^0.23.4",
  "kysely-planetscale": "^1.3.0",
  "kysely-codegen": "^0.9.0",
  "@planetscale/database": "^1.5.0"
}
export const db = new Kysely<DB>({
  dialect: new PlanetScaleDialect({
    host: "aws.connect.psdb.cloud",
    username: env.DATABASE_USERNAME,
    password: env.DATABASE_PASSWORD,
  }),
});

Thanks!

koskimas commented 1 year ago

Looking further into the kysely implementation, it seems as the type for result.numDeletedRows is supposed to be bigint | undefined

This is not correct. All kysely's core dialects are implemented in a way that numDeletedRows is never undefined. bigint is the correct type. Instead of undefined, the value should be 0 when no rows are deleted.

So the correct fix here is to make kysely-planetscale return 0 instead of undefined when no rows get deleted. Same is true for update queries.

koskimas commented 1 year ago

numAffectedRows can be undefined in the internal QueryResult type. That's for the cases where that value doesn't exist, like select queries.

jacobwgillespie commented 1 year ago

This might be something we can fix here, or might be something that should be fixed in @planetscale/database.

Currently we're returning numAffectedRows if the underlying database connection returns that value:

https://github.com/depot/kysely-planetscale/blob/e80b2383af7741d71cee435af96fb64ae7b3c53f/src/index.ts#L139

I believe that implies that the serverless driver is not returning a value for rowsAffected for your query. If you're able to run the query directly with the serverless driver and let me know what it returns, that would be super useful.

If it's not returning the string "0" then it might be worth opening an issue in https://github.com/planetscale/database-js. And if it is, we might have a bug here.

Vyast commented 1 year ago

Looking further into the kysely implementation, it seems as the type for result.numDeletedRows is supposed to be bigint | undefined

This is not correct. All kysely's core dialects are implemented in a way that numDeletedRows is never undefined. bigint is the correct type. Instead of undefined, the value should be 0 when no rows are deleted.

So the correct fix here is to make kysely-planetscale return 0 instead of undefined when no rows get deleted. Same is true for update queries.

Ah, thank you for the correction. I was confusing numDeletedRows with numAffectedRows as you mentioned in the other comment.

Vyast commented 1 year ago

This might be something we can fix here, or might be something that should be fixed in @planetscale/database.

Currently we're returning numAffectedRows if the underlying database connection returns that value:

https://github.com/depot/kysely-planetscale/blob/e80b2383af7741d71cee435af96fb64ae7b3c53f/src/index.ts#L139

I believe that implies that the serverless driver is not returning a value for rowsAffected for your query. If you're able to run the query directly with the serverless driver and let me know what it returns, that would be super useful.

If it's not returning the string "0" then it might be worth opening an issue in https://github.com/planetscale/database-js. And if it is, we might have a bug here.

Looks like it is indeed as you say, @planetscale/database returns null for rowsAffected when 0 rows are deleted.

Running the following code:

import { Client } from "@planetscale/database";

const client = new Client({
  host: "aws.connect.psdb.cloud",
  username: env.DATABASE_USERNAME,
  password: env.DATABASE_PASSWORD,
});

export async function POST() {
  const date = new Date();

  const conn = client.connection();
  const results = await conn.execute("DELETE FROM Log WHERE expiresAt <= ?", [
    date,
  ]);

  console.log(results);

  return new Response("Complete");
}

Returned the following results:

{
  "headers": [],
  "types": {},
  "fields": [],
  "rows": [],
  "rowsAffected": null,
  "insertId": null,
  "size": 0,
  "statement": "DELETE FROM Log WHERE expiresAt <= '2023-03-03T21:54:59.159'",
  "time": 5.799408
}

Its a simple workaround and not a big deal for me, but something to keep in mind I suppose.

Thank you for your time! @jacobwgillespie @koskimas

koskimas commented 1 year ago

I guess kysely could default to zero when a falsy value is given by a dialect. It's easy there since we know the type of the query (select, delete, update, insert). I'll work around this in kysely, but I think it should be fixed in planetscale/database too.

jacobwgillespie commented 1 year ago

I've opened the issue https://github.com/planetscale/database-js/issues/88.

jacobwgillespie commented 1 year ago

Hey @Vyast this should be fixed thanks to https://github.com/planetscale/database-js/pull/89, which was released in v1.6.0 of @planetscale/database - the serverless driver will now return 0 instead of null.

I'll mark this issue closed, but feel free to ping if you run into any issues!