kysely-org / kysely

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

Branded types broken in version 0.23 #261

Closed steida closed 1 year ago

steida commented 1 year ago

This code worked with 0.22

let q = db
  .selectFrom("node")
  .select(["id", "title"])
  .orderBy("createdAt")
  .where("isDeleted", "is not", model.cast(true));

ids.forEach((adjacentId) => {
  // @ts-expect-error Kysely 0.23 bug, it was working with 0.22.
  q = q.where("id", "in", (qb) =>
    qb
      .selectFrom("edge")
      .where("isDeleted", "is not", model.cast(true))
      .where("b", "=", adjacentId)
      .select("a as id")
      .union(
        qb
          .selectFrom("edge")
          .where("isDeleted", "is not", model.cast(true))
          .where("a", "=", adjacentId)
          .select("b as id")
      )
  );
});

Argument of type '(qb: ExpressionBuilder<From<DbSchemaToType<{ node: { id: ZodBranded<ZodEffects<ZodString, string, string>, "NodeId">; title: ZodBranded<ZodString, "NonEmptyString1000">; }; edge: { ...; }; }, { ...; }>, "node">, "node">) => SelectQueryBuilder<...>' is not assignable to parameter of type 'OperandValueExpressionOrList<From<DbSchemaToType<{ node: { id: ZodBranded<ZodEffects<ZodString, string, string>, "NodeId">; title: ZodBranded<ZodString, "NonEmptyString1000">; }; edge: { ...; }; }, { ...; }>, "node">, "node", "id">'.
  Type '(qb: ExpressionBuilder<From<DbSchemaToType<{ node: { id: ZodBranded<ZodEffects<ZodString, string, string>, "NodeId">; title: ZodBranded<ZodString, "NonEmptyString1000">; }; edge: { ...; }; }, { ...; }>, "node">, "node">) => SelectQueryBuilder<...>' is not assignable to type '(eb: ExpressionBuilder<From<DbSchemaToType<{ node: { id: ZodBranded<ZodEffects<ZodString, string, string>, "NodeId">; title: ZodBranded<ZodString, "NonEmptyString1000">; }; edge: { ...; }; }, { ...; }>, "node">, "node">) => SelectQueryBuilder<...>'.
    Call signature return types 'SelectQueryBuilder<From<From<DbSchemaToType<{ node: { id: ZodBranded<ZodEffects<ZodString, string, string>, "NodeId">; title: ZodBranded<ZodString, "NonEmptyString1000">; }; edge: { ...; }; }, { ...; }>, "node">, "edge">, FromTables<...>, Selection<...>>' and 'SelectQueryBuilder<any, any, Record<string, string & BRAND<"NodeId">>>' are incompatible.
      The types of 'expressionType' are incompatible between these types.
        Type 'Selection<From<From<DbSchemaToType<{ node: { id: ZodBranded<ZodEffects<ZodString, string, string>, "NodeId">; title: ZodBranded<ZodString, "NonEmptyString1000">; }; edge: { ...; }; }, { ...; }>, "node">, "edge">, FromTables<...>, "a as id"> | undefined' is not assignable to type 'Record<string, string & BRAND<"NodeId">> | undefined'.
          Type 'Selection<From<From<DbSchemaToType<{ node: { id: ZodBranded<ZodEffects<ZodString, string, string>, "NodeId">; title: ZodBranded<ZodString, "NonEmptyString1000">; }; edge: { ...; }; }, { ...; }>, "node">, "edge">, FromTables<...>, "a as id">' is not assignable to type 'Record<string, string & BRAND<"NodeId">>'.```
igalklebanov commented 1 year ago

Hey 👋

Union is not broken. That's a misleading title.

Using same query with a simpler interface works.

interface DB {
  node: { id: string; title: string; createdAt: string; isDeleted: boolean }
  edge: { isDeleted: boolean; b: string; a: string }
}

You can't expect us to test every feature with 3rd party library types, especially ones as rare as zod branded types.

Can you provide full reproduction code?

steida commented 1 year ago

Sorry for the misleading title; I will update it. The full repo is here https://github.com/evoluhq/evolu.me/blob/main/components/NodeList.tsx#L90

Branded types are very useful for modeling DB types. They worked well until 0.23.

igalklebanov commented 1 year ago

Branded types are very useful for modeling DB types

My comment was not against branded types.


If you turn (qb) => qb (.where rhs) into (qb) => { return qb the squiggly error line now points to the return type of the callback with same error message - nothing to do with the .union call.

For some reason there's a mismatch between subquery's O (a Selection<...>) & expected rhs O (Record<string, string & BRAND<'NodeId'>>).

It has something to do with recently introduced Expression<T> interface and perhaps DbSchemaToType (3rd party type).

In the meantime, you can bypass this by simply appending .castTo<{ id: string & BRAND<"NodeId"> }>() to the subquery.

koskimas commented 1 year ago

I'm trying to reproduce this without zod and failing to do so. @steida could you modify this (without installing zod) so that the error appears? I'm trying to fix this + add a regression test.

import { Generated, Kysely } from '../..'
import { expectType, expectError } from 'tsd'

// Type taken from zod:
// https://github.com/colinhacks/zod/blob/827cb5d47af510a6704d7bfb111c19a6c71a6b81/src/types.ts#L4189
export const BRAND: unique symbol = Symbol('zod_brand')
export type BRAND<T extends string | number | symbol> = {
  [BRAND]: { [k in T]: true }
}

type NodeId = string & BRAND<'NodeId'>

interface Database {
  node: {
    id: Generated<NodeId>
    title: string
    createdAt: Date
    isDeleted: boolean
  }
  edge: {
    a: NodeId
    b: NodeId
    isDeleted: boolean
  }
}

function brandedTypes(db: Kysely<Database>) {
  const ids: NodeId[] = ['foo', 'bar'] as unknown as any

  let q = db
    .selectFrom('node')
    .select(['id', 'title'])
    .orderBy('createdAt')
    .where('isDeleted', 'is not', true)

  ids.forEach((adjacentId) => {
    q = q.where('id', 'in', (qb) =>
      qb
        .selectFrom('edge')
        .where('isDeleted', 'is not', true)
        .where('b', '=', adjacentId)
        .select('a as id')
        .union(
          qb
            .selectFrom('edge')
            .where('isDeleted', 'is not', true)
            .where('a', '=', adjacentId)
            .select('b as id')
        )
    )
  })
}
koskimas commented 1 year ago

It seems like the type of edge.a is not assignable to node.id. Are you sure those types are assignable to each other?

If I modify my example by giving a different branded type for edge.a, I get the same error you have.

0.22.x was less strict in these cases and didn't require the types to be assignable to each other.

koskimas commented 1 year ago

Yep, confirmed with a bunch of tests. For some reason, your types are not assignable to each other. Not a bug, just a result of stricter types.

koskimas commented 1 year ago

Nope, I'm wrong 😄

This actually has nothing to do with zod or branded types. edge.a is nullable, while node.id isn't. You should be able to say WHERE a IN b where b is nullable and a isn't.

So this is a bug, but not related to branded types.

koskimas commented 1 year ago

Should be fixed now in 0.23.3

steida commented 1 year ago

@koskimas It is. Thank you!

nikeee commented 1 year ago

Got a similar issue in 0.26.0: https://kyse.link/?p=s&i=R1cC81UCg06fZQQMeCPd

import { Generated, GeneratedAlways } from "kysely"

declare global {
  interface DB {
    user: UserTable
  }
  type Brand<TBrand> = {
    _type: TBrand
  }

  type Nominal<T, TBrand> = T & Brand<TBrand>
  type CountryCode = Nominal<string, "CountryCode">

  interface UserTable {
    id: GeneratedAlways<number>
    cc: Generated<CountryCode>
  }
}
await db
  .selectFrom("user")
  .where("cc", "<>", "foo")
  .selectAll()
  .executeTakeFirstOrThrow()

"foo" is marked as an error. The generated SQL seems correct to me. Works:

igalklebanov commented 1 year ago

@nikeee You expect "foo" to not error here? it's a plain string, not of type CountryCode (the branded type).

nikeee commented 1 year ago

Oh you're right. That was kinda embarrassing. Thanks for pointing this out! :)