kysely-org / kysely

A type-safe typescript SQL query builder
https://kysely.dev
MIT License
10.8k stars 275 forks source link

.executeTakeFirstOrThrow() returns null instead of throwing #205

Closed Devvypaws closed 2 years ago

Devvypaws commented 2 years ago

Description

When executing a SELECT query with .executeTakeFirstOrThrow(), it returns null if the result set is empty instead of throwing an exception.

Environment

Name Version
macOS 13.0
Node 16.17.0
npm 8.15.0
Kysely 0.22.0
Kysely Codegen 0.7.4
MySQL 5.7.38
mysql2 2.3.3

Additional Details

igalklebanov commented 2 years ago

Interesting. Are you using any plugins? 3rd party dialects?

Edit: This is tested.

Can you provide a repository reproducing this?

koskimas commented 2 years ago

I'm guessing it's a third party dialect or a plugin. This is the implementation

  async executeTakeFirst(): Promise<SingleResultType<O>> {
    const [result] = await this.execute()
    return result as SingleResultType<O>
  }

  async executeTakeFirstOrThrow(
    errorConstructor: NoResultErrorConstructor = NoResultError
  ): Promise<O> {
    const result = await this.executeTakeFirst()

    if (result === undefined) {
      throw new errorConstructor(this.toOperationNode())
    }

    return result as O
  }

We should probably test for null too though.

koskimas commented 2 years ago

Actually when I look at the code, the description of this issue simply cannot be correct. If execute returns an empty array, there is no way that code fails, unless some of those methods are overridden.

Devvypaws commented 2 years ago

In Kysely? No, I'm only using it with the built-in MySQL adapter; not even using the CamelCasePlugin yet. (Just realized I should probably include the mysql2 version, too!)

The query — intentionally crafted to return no results — is

db
  .selectFrom("redacted_table_name")
  .select(db.fn.max("redacted_table_name.id").as("maxId"))
  .where("redacted_table_name.id", "<", 0)
  .executeTakeFirstOrThrow()
  .then(
    (value) => Promise.resolve({ kind: "Ok", value }),
    (error) => Promise.resolve({ kind: "Err", error }),
  );

where redacted_table_name.id is of type number as reported by TypeScript, which matches what the interface says it is. The column is also never less than zero, so the query forces an empty result set. Could it possibly be an issue with the use of an aggregate function? I'll try to put together an SSCCE.

Update: This issue only occurs when using the min and max aggregate functions; possibly others.

igalklebanov commented 2 years ago

avg, max, min & sum are nullable when they don't have rows to work with.

In a future release you'll be able to opt-in to hinting they are nullable, and/or wrap them with coalesce.

koskimas commented 2 years ago

Yep, that query always returns one row, even if the set from which max is taken is empty. That's how SQL works. As mentioned, we can improve by making the aggregated values nullable, but it won't change this behavior.

Devvypaws commented 2 years ago

Cool, thanks for looking into it and for the context around the issue!