ExpressionBuilder allows invalid fields in update/set, causing runtime failures

[johnpyp]

Most set/update update styles in Kysely work as expected, disallowing extraneous properties not present in the schema, as otherwise the queries will fail when executed.

Unfortunately, there's one case that is quite natural to use, which dangerously doesn't check for this: using an ExpressionBuilder function which returns an object -

await db
  .insertInto("person")
  .values({ 
    first_name: "first name", 
    last_name: "last name",
    gender: "male"
  })
  .onConflict((oc) =>
    oc.column("first_name").doUpdateSet((eb) => ({
      first_name: "asdf",
      asdf: "asdf"
    }))
  )
  .execute();

Full example: https://kyse.link/klyC9

In the above example in the onConflict clause, the first_name property is required, and as expected, if no valid fields are present, then the query fails.

However, once at least one other valid property is in the object, you can add any other extraneous properties and they won't fail typechecking. This is incorrect and will lead to runtime query failures (my team uses this pattern extensively, and were just bitten by it 😢 ).

The example above is the exact case that my team ran into, but it looks like it also applies to cases like set((eb) => ({ ... }).

---

To mitigate this issue, you can pass the object directly to doUpdateSet instead, but you'll have to use a (eb) => closure in every field instead, which makes it the much less appealing option (for example when doing mass eb.ref("excluded.field").

[igalklebanov]

Hey 👋

This is part of a long standing TypeScript issue.

edit: it is actually microsoft/TypeScript#241.

[johnpyp]

Ah interesting. So to generalize, this specific issue is because it's not possible to make return type checking as strict as you currently can in parameter checking?

Would it be possible to have some kind of simple "forwarding" function that converts this from effectively a return-type check to a parameter check? I'm not familiar enough with the type wizardry to know if this could actually work like I'm imagining.

Like change:

 qry.onConflict((oc) =>
    oc.column("first_name").doUpdateSet((eb) => ({
      first_name: "asdf",
      asdf: "asdf"
    }))
  )

to:

import { strictParams as sp } from 'kysely'
// ...
qry.onConflict((oc) =>
    oc.column("first_name").doUpdateSet((eb) => sp({
      first_name: "asdf",
      asdf: "asdf"
    }))
  )

[igalklebanov]

I'm afraid this won't work. sp is not "bound", it doesn't know what it should expect.

There are probably some hacks out there, but they would hurt compile-time performance for everyone, they don't cover all cases probably, and they're not future-proof and could break with any new TypeScript version.

There is hope in catching these with typescript-eslint if this proposal gets through.