kysely-org / kysely

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

A few ideas and thank you #5

Closed michael-land closed 2 years ago

michael-land commented 3 years ago

Hi there, I have been trying this lib this weekend. By far, this is the most powerful and lightweight type-safe sql query builder IMO. (compares to knex, zapatos and slonik)

I'm using prisma2 in production but would love to migrate some of the queries to kysely once it become stable.

I love the amazing work you have done and I think this work could be improved if it could support stricter type.

Assuming i have the following DDL

CREATE TABLE person
(
  id                  INTEGER GENERATED BY DEFAULT AS IDENTITY
    CONSTRAINT person_pk
      PRIMARY KEY,
  created_at          TIMESTAMP WITH TIME ZONE DEFAULT NOW()            NOT NULL
  name                text                                              NOT NULL
)

I would expect the following code throw because name non-nullable

interface Person {
  id: number
  created_at: string
  name: string
}

interface Database {
  person: Person
}

const db = new Kysely<Database>

await db.insertInto('person').values({})

The current behavior typescript does not raise exception because the method value shape is Partial<Model>.

export type MutationObject<DB, TB extends keyof DB> = {
    [C in keyof DB[TB]]?: MutationValueExpression<DB[TB][C]>;
};

So, Instead of passing single database model definition to new Kysely<Models>, it would be nice to allow user pass both selectable definition and insertable definition in the future. Then we could get stricter type for both insert and update mutation.

interface PersonSelectable {
  id: number
  created_at: string
  name: string
}

interface PersonInsertable {
  id?: number
  created_at?: string
  name: string
}

interface DatabaseSelectable {
  person: PersonSelectable
}

interface DatabaseInsertable {
  person: PersonInsertable
}

const db = new Kysely<DatabaseSelectable, DatabaseInsertable>

// it will throw error because name is required in PersonInsertable
await db.insertInto('person').values({})

Also, do you have any plan to build/integrate database typing generator into this library? I'm using patched version of sql-ts for now, it works great but it has some limitations. I could help on that if you like

Keep up the good work 👍

koskimas commented 3 years ago

That's an excellend idea and I've considered it a lot. The problem is values generated in the database, like id in this example (also all created_at and updated_at columns that are generated in the db).

If we required all required fields in the insert, in this example we'd also need to require the id which we definitely don't want to provide from the code.

The options:

Separate interfaces for insert and select (and update?)

This is the same as your solution. This would cause the types to be at least twice as complex everywhere. We would need to pass around at least two Database interface everywhere. This adds too much complexity for a smallish impact.

Decorators

Decorators are a runtime feature and since Kysely uses interfaces for which there is no runtime code, we cannot use decorators like this:

interface Person {
  @generated
  id: number
}

Wrapper types

interface Person {
  id: Generated<number>
}

This could maybe be possible, but it would probably cause problems where { id: number } is not assignable to { id: Generated<number> } and vice versa. I may need to look into this some more.

Based on small tests, something like this seems to work:

type Generated<T> = T & { __generated__?: true }

const x: Generated<number> = 1
const y: number = x

but you can access the fake property __generated__ through x. I'm also afraid this would pollute the types in other ways and make things way more complex than they need to.

Generated specifier for inserts

In this option, the fields of the insert object are not optional. Instead, you need to either pass a value or a db.generated (or something similar) placeholder to tell Kysely to not insert the values.

await db.insertInto('person').values({
  id: db.generated,
  created_at: db.generated,
  name:  'Testerson',
})

Any thoughts on this option?

Also, do you have any plan to build/integrate database typing generator into this library? I'm using patched version of sql-ts for now, it works great but it has some limitations. I could help on that if you like

Yes, I've considered adding a cli command for generating the database interfaces based on the database.l It would be a cool feature for people migrating an existing project to Kysely. Help on this would be awesome! If you decide to work on this, let's have a small discussion about it somewhere. I should probably add some kind of realtime chat for Kysely. Slack maybe? Do you know any good free alternatives for open source projects? I've used gitter before, but it sucks

tonivj5 commented 3 years ago

I really like the db.generated placeholder, looks explicit 👍🏻

michael-land commented 3 years ago

db.generated` is better and easier to implement :-)

I should probably add some kind of realtime chat for Kysely. Slack maybe? Do you know any good free alternatives for open source projects? I've used gitter before, but it sucks

I think either slack or discord will do the job

tonivj5 commented 3 years ago

I think discord is more popular for OSS projects these days 😉

koskimas commented 3 years ago

All fields are now required for inserts in the latest 0.3.5 version and the db.generated placeholder has been added. I wasn't able to make nullable fields optional, so any fields that have a type T | null and you want to leave to null, you need to explicitly set to null

await db.insertInto('person').values({
  id: db.generated,
  created_at: db.generated,
  name: 'Testerson',
  some_nullable_column: null,
})

I'll revisit the optionality later.

koskimas commented 3 years ago

@xiaoyu-tamu @tonivj5 There's now a discord badge in the readme. I don't know if I set it up correctly. Could you try it out?

michael-land commented 3 years ago

image

Looks like there is not channel available yet

koskimas commented 3 years ago

Weird. For me it opens a channel (there are a few). Maybe I need use the invite link in the badge? (I've never used discord). Here's the invite link https://discord.gg/C9aJ49mCra. Does that go to the right place?

michael-land commented 3 years ago

The invite link works.

jirutka commented 3 years ago

I have one idea how to solve this issue on the type level. I’ve developed it some time ago, but haven’t had time to integrate it into our internal project yet. I’ll dig it out once I have a while.

The main idea is to use generic wrapper types to encode metadata (such as PK), but in a way that doesn’t require any type casting when used, i.e. it behaves exactly like the wrapped type for the users. These wrappers are used in helper types that transform the model type into derived types (insertable, selectable etc.) and even allow things like “expanding” references.

It’s sorta black-magic-like typing, but it worked wonderfully. However, it need testing in real scenarios. I have a bit an unpleasant experience with TypeScript in the way that any complicated types fall apart in various corner cases, especially when you try to combine them with any other non-trivial types.

koskimas commented 3 years ago

@jirutka I'd love to see your solution!

michael-land commented 3 years ago

I created a typings and shortcuts generator that using kysely for kysely over this weekend.

This library is still work in progress and docs and tests are still missing.

But the database introspection is ready to use. npx ormless@latest -c ormless.config.json

https://github.com/xiaoyu-tamu/ormless/tree/main/example https://github.com/xiaoyu-tamu/ormless

import { CustomerRepository, Database, DatabaseSchema } from './database';

  const db = new Kysely<DatabaseSchema>({
    database: process.env.DATABASE_NAME!,
    host: process.env.DATABASE_HOST!,
    user: process.env.DATABASE_USER!,
    password: process.env.DATABASE_PASSWORD!,
    dialect: 'postgres',
    plugins: [new CamelCasePlugin()],
  });

  // select unique customer by primary key
  const fetchedCustomer = await customerRepo.selectOne({
    db,
    select,
    where: { customerPkey: { customerId: -1 } },
  });

  // update unique customer by unique key
  const updatedCustomer = await customerRepo.updateOne({
    db,
    select,
    where: { customerEmailUk: { email: 'lookup@email.com' } },
    data: { activebool: true },
  });     
elderapo commented 2 years ago

How about something like this:

import { table, t, Table, Selectable, Insertable, Updatable, Default } from "kysely";

export const Person = table({
  id: t.number("BIGINT").primary(), // table could ensure that table schema object contains exactly 1 primary column
  name: t.varchar(123).nullable(), // default length could be specified like this
  age: t.number("INT").default(18), // `number` is a little vague for a db so internal number type could be specifiable like this (in typesafe manner ofc)
});
export type Person = Table<typeof Person>; // this type would contain "metadata" (ex. `__generated__`, `__default__` ) that would be stripped out by below types

///

export type PersonSelectable = Selectable<Person>; // { id: readonly number; name: string | null; age: number; }
export type PersonInsertable = Insertable<Person>; // { name: string | null; age: number | Default }
export type PersonUpdatable = Updatable<Person>; // { name?: string | null; age?: number }

This is a long shot but a system like this would improve type-safety and would allow for automatic (at least semi-automatic) migrations generation type-orm style in the future.

koskimas commented 2 years ago

@elderapo Your suggestion is a different project completely. Kysely aims to be just a query builder. It builds and executes queries, nothing more. I don't see Kysely ever having that kind of model/automigration system.

In my experience, as soon as you start building abstraction layers on top of SQL, things get order of magnitude harder. You need to either drop a vast majority of SQL features or build weird complex abstractions that support all or at least most of them. You either get a shitty product (most node.js ORMs) or a massive one (Hibernate).

elderapo commented 2 years ago

@koskimas I think you misunderstood what I meant :(

The part about auto migrations was solely a side effect (in fact cool/useful in my opinion but that's besides my point) of having a strongly defined schema. The main purpose of this strongly defined schema (ex. id a numeric value but auto-generated by the database) was to instruct kysely what it should expect from the user based on certain conditions ex.:

Essentially the Selectable<T>, Insertable<T>, Updatable<T> type helpers would be internally used by kysely to warn users at compile time about writing incorrect SQL queries :)

koskimas commented 2 years ago

Yes, the hard part is implementing the Selectable, Insertable etc. types. For them to work, we'd need to make the Person in your example a complex type with all kinds of metadata added into it. It wouldn't represent a Person row anymore. All types would need to use Selectable<T> etc. everywhere instead of just T.

For example, see the types in table-parser.ts file. Now consider the table types under the DB parameter were some kind of complex types where you'd need to use Selectable<DB[T]> to get the columns out. I think the complexity would get out of hand.

Sure, the Person type in your example could simply be something like

{
  __insertable__: { foo: string }
  __selectable__: { foo: string, id: string }
}

and then Selectable would be

type Selectable<T extends { __selectable__: any }> = T['__selectable__']

that's easy. But using these internally wouldn't be.

elderapo commented 2 years ago

I totally agree that such a change would increase the complexity of the internal/library code but would also drastically improve the type-safety of the userland code which is the main purpose of this library, no?

I've quickly created a simple proof of concept of the Selectable, Insertable, Updatable types. You can check it out here. Let me know what you think :)

martinblostein commented 2 years ago

I totally agree that such a change would increase the complexity of the internal/library code but would also drastically improve the type-safety of the userland code which is the main purpose of this library, no?

I don't believe it would improve type safety beyond the current solution (which requires all nullable columns with type T | null to be specified when using insertInto). What am I missing?

It seems it would only make things a bit more convenient because you could omit generated or nullable columns in those calls. (In #27 I note that you can avoid specifying nullable columns if you make them optional fields instead of a null-union, but I'm not clear on all the ramifications of that choice.)

elderapo commented 2 years ago

@martinblostein If you make:

Link to playground.

martinblostein commented 2 years ago

@martinblostein If you make:

  • id nullable then when you select id from the db kysely types it as number | null which is inaccurate.
  • id optional then the same thing applies with the exception that type would essentially be number | undefined which is inaccurate as well.

I don't understand, if you type id as nullable, and then upon select kysely types it as number | null , that's accurate.

If your issue is that id is a non-nullable generated column, make it non-nullable in the type defintion and use db.generated as its value when inserting.

elderapo commented 2 years ago

id (primary keys) are usually not nullable on the schema (SQL level). However, you as a user don't need to specify them when inserting new records because they get automatically generated by the database engine. You can think of it as:

db.generated is very similar to any in typescript. You can use it by mistake on any column - even on ones that are neither automatically generated (by db engine) or have a default value (on db schema level) - which allows for unnecessary runtime errors because kysely doesn't know where it should be allowed so it allows it everywhere, just like any in ts.

martinblostein commented 2 years ago

I understand how generated columns work. What I'm saying is that there is a type-safe way of handling them in kysely currently. You mark the column as non-nullable in the type definition that represents the table, e.g.:

interface Person {
  id: number
  name: string
}

interface Database {
  people: Person
}

Then when you insert into that table, you use db.generated to avoid having to specify a value:

db.insertInto("people").values([{ id: db.generated, string: "Jim" }])
elderapo commented 2 years ago

@martinblostein please read my message again. I've initially misread your message and then edited it.

martinblostein commented 2 years ago

Ah, yes I see. You have a good point about how db.generated can be supplied to a non-generated column. That's not good.

I'm hopefully that could be fixed by introducing using a Generated<T> type (as was suggested above.) Then db.generated could only be passed to those columns. The only remaining case is not allowing values to be passed to an ALWAYS GENERATED, which will throw an error if passed a value. I wonder if this could be achieved as well either via an AlwaysGenerated<T> type or another type parameter on Generated.

The reason I'm hopeful that a Generated generic type can solve this is that it wouldn't require many changes to the library. Your design is another good approach, but I agree with @koskimas that it's different enough that it probably deserves a separate project. Or at least a fork :)

koskimas commented 2 years ago

@elderapo Your suggestion is cool, but we have to consider the tradeoff here. Your suggestion would achieve slightly better type-safety since currently you can pass db.generated to a non-generated column. But on the other hand it would make all complex type magic in table-parser.ts and select-parser.ts (and many other places) much more complex.

You can use db.raw anywhere anyway. If you really want, you can mess up all type-safety using db.raw. So removing db.generated is not even that big of an improvement, and it's definitely not big enough to justify making all type magic unmaintainable.

Currently the types, while complex, are still manageable. DB is always just a record of interfaces that represent the selected rows.

The goal of Kysely is not to be 100% type-safe at all cost, but remain usable and maintainble while being "type-safe enough".

koskimas commented 2 years ago

I think this kind of general discussion should be continued in discord. I'll close this issue now.