Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A few ideas and thank you #5

Closed
michael-land opened this issue Sep 20, 2021 · 26 comments
Closed

A few ideas and thank you #5

michael-land opened this issue Sep 20, 2021 · 26 comments
Labels
enhancement New feature or request typescript Related to Typescript

Comments

@michael-land
Copy link

michael-land commented Sep 20, 2021

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
Copy link
Member

koskimas commented Sep 20, 2021

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
Copy link
Contributor

tonivj5 commented Sep 20, 2021

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

@michael-land
Copy link
Author

michael-land commented Sep 20, 2021

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
Copy link
Contributor

tonivj5 commented Sep 20, 2021

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

@koskimas
Copy link
Member

koskimas commented Sep 21, 2021

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
Copy link
Member

@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
Copy link
Author

image

Looks like there is not channel available yet

@koskimas
Copy link
Member

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
Copy link
Author

The invite link works.

@jirutka
Copy link

jirutka commented Sep 22, 2021

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
Copy link
Member

@jirutka I'd love to see your solution!

@michael-land
Copy link
Author

michael-land commented Oct 3, 2021

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
Copy link
Contributor

elderapo commented Dec 1, 2021

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
Copy link
Member

koskimas commented Dec 3, 2021

@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
Copy link
Contributor

elderapo commented Dec 3, 2021

@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.:

  • when building an insert query kysely shouldn't expect the developer to specify id because its generation is going to be internally handled by the database engine but when some non-nullable/default column is omitted/skipped it should yell at compile time to not allow unnecessary runtime errors.
  • when building an update query kysely shouldn't allow the developer to update of id column of the user record because it's a primary key and it might mess up the relations of other records pointing to it (not 100% sure primary key updates are always bad and should be forbidden but it's just an example).

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
Copy link
Member

koskimas commented Dec 3, 2021

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
Copy link
Contributor

elderapo commented Dec 4, 2021

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
Copy link

martinblostein commented Dec 4, 2021

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
Copy link
Contributor

elderapo commented Dec 4, 2021

@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.

Link to playground.

@martinblostein
Copy link

martinblostein commented Dec 4, 2021

@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
Copy link
Contributor

elderapo commented Dec 4, 2021

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:

  • when inserting id is number | null.
  • when selecting id is always number.

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
Copy link

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
Copy link
Contributor

elderapo commented Dec 4, 2021

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

@martinblostein
Copy link

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
Copy link
Member

koskimas commented Dec 4, 2021

@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
Copy link
Member

koskimas commented Dec 4, 2021

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

@koskimas koskimas closed this as completed Dec 4, 2021
@igalklebanov igalklebanov added enhancement New feature or request typescript Related to Typescript labels Oct 12, 2022
shiyuhang0 added a commit to shiyuhang0/kysely that referenced this issue Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request typescript Related to Typescript
Projects
None yet
Development

No branches or pull requests

7 participants