depot/kysely-planetscale

Incorrect type for numDeletedRows being returned from deleteFrom query

Closed this issue ยท 8 comments

Vyast commented

๐Ÿ‘‹

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!

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.

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

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:

const numAffectedRows = results.rowsAffected == null ? undefined : BigInt(results.rowsAffected)

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

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

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:

const numAffectedRows = results.rowsAffected == null ? undefined : BigInt(results.rowsAffected)

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

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.

I've opened the issue planetscale/database-js#88.

Hey @Vyast this should be fixed thanks to planetscale/database-js#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!