fix: resolve edge case with varying shapes in multiple item inserts causing undefined in parameters.

[naorpeled]

closes #1310

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
kysely	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 18, 2025 9:30pm

[pkg-pr-new]

Open in Stackblitz • kysely_koa_example

npm i https://pkg.pr.new/kysely-org/kysely@1311

commit: 0c2df8b

[koskimas]

Looking good otheriwse but: when inserting a single row, the value should actually be missing and not DEFAULT. The change now caused DEFAULT being used even when you insert a single row with an undefined value (see all the failing tests).

Most databases support DEFAULT but not all. For example on sqlite we need to use null instead. Mapping undefined to null is not a good idea when it can be avoided.

[naorpeled]

Looking good otheriwse but: when inserting a single row, the value should actually be missing and not DEFAULT. The change now caused DEFAULT being used even when you insert a single row with an undefined value (see all the failing tests).

Most databases support DEFAULT but not all. For example on sqlite we need to use null instead. Mapping undefined to null is not a good idea when it can be avoided.

Makes sense,
how would you separate those two cases in the cleanest way?

[koskimas]

how would you separate those two cases in the cleanest way?

Don't know. Maybe just a special case when rows.length === 1? Add a comment why the special case is there in case you end up with this solution.

[naorpeled]

@koskimas let me know what you think 🙏

[koskimas]

LGTM 👍

[igalklebanov]

Hey @naorpeled @koskimas 👋

The approved changes resolved the issue but were producing more sql than needed.

Here's an added test case that reproduces #1310 output with those changes:

There's an extra column ("children") that's always undefined in the items array - so we should not add it to the query.

I have reverted those changes and found the issue in:

  const hasMissingColumns = rowColumns.length < columns.size

This check was false because columns.size was equal to rowColumns.length, but the row had a missing column actually, replaced by an unindexed column with an undefined value.

When it is false and there are no complex expressions, it just creates a primitive list - which included undefined values!

We need to count how many indexed columns the current row has and compare that to the indexed columns count.