Throw error if the array passed to insert is empty

[nickrum]

Not sure if it would be better to throw an error, but at least the behavior should be consistent.

[kibertoad]

@nickrum What happened previously in this case?

[elhigu]

how do one create an empty query with knex? missing also tests.

[nickrum]

@kibertoad The empty query would be forwarded to the respective database driver. The problem is that each driver handles this differently, e.g. PostgreSQL silently ignores the query, while SQLite throws a SQLITE_MISUSE error.

@elhigu An empty query will be created when insert() is called with an empty array. There might be other cases though, that I am not aware of. Would it be sufficient to add a test for this case, as calling insert() with an empty array is the higher level issue I am trying to fix with this PR?

[kibertoad]

@nickrum Since this feels like a very undesired situation, my personal preference would be to make it consistent by throwing an error always, especially since doing so later would be a breaking change.

[kibertoad]

@nickrum I would say that if the use of empty arrays allows to consistently reproduce the issue, that should be perfectly sufficient.

[nickrum]

@kibertoad My reasoning for not throwing an error was that MySQL, MSSQL (at least before #2857) and PostgreSQL didn't throw an error (which seem to be the major vendors in use) and users might rely on this behavior. At least Directus does so in one case. But I would be fine either way.

[kibertoad]

@nickrum Since we are doing a semver major release now, breaking things should be acceptable, question is what we consider to be more helpful to the end user; from my perspective this is wrong input and user should be informed about that.
@elhigu What do you think?

[elhigu]

I think it should throw an error. Resolving query without actually sending anything to db would be a pretty strange exception how knex behave.

[nickrum]

I changed it to throw an error instead. Maybe it would be better to throw inside insert() directly instead of returning an empty query and throwing later.

[kibertoad]

@nickrum No other operations can potentially lead to the same outcome?

[nickrum]

@kibertoad There are most likely others that lead to the same outcome. I thought it would be better if each operation throws it's own error, but this doesn't seem to be as straight forward as I initially thought.

[kibertoad]

@nickrum This looks good, let's adjust it later if needed. Can you mention the breaking change in UPGRADING.md?

[kibertoad]

Thank you!

[maxkoryukov]

You can not imagine, how many bugs appeared after this patch in the software-based exclusively on Postgres (where it worked just fine).

I don't insist, but maybe it is possible to make this behavior configurable?

[kibertoad]

@elhigu What do you think?

[kibertoad]

@maxkoryukov The way I currently see it, we generally avoid knex-scoped configuration, so even if there was a config option, it would be scoped to a specific insert operation. And if you would be doing it per operation anyway, I suspect that doing the right thing and early return in case of empty array is going to be about as much effort as disabling the empty array check everywhere.

[elhigu]

I don't think it should be configurable. Making every piece and bit configurable just to maintain backwards compatibility will accumulate horrible mess to knex.

If you want override the behaviour in your project, you should be able to extend query builder with some method, that does not throw an error. I'm not sure, but maybe it is even possible to override the original insert with patched version.

[elhigu]

If extending/overriding query builder methods is not working yet, that would be among other things a good unopinionated way to provide way to people to add their own backwards compatibility layers.

[digitalmaster]

Would be great if this error did some more to help us track down exactly which offending insert calls:

[kibertoad]

@digitalmaster Would you consider sending PR for this?

[digitalmaster]

Yup... will give it a shot 👍

[digitalmaster]

Apologies for delay but finally got some time on my hands to ramp up and dig into this one 🤗

Would we be open to reconsidering throwing this error in the query builders' insert() function instead?

... Maybe it would be better to throw inside insert() directly instead of returning an empty query and throwing later.

The reason being that that's where we seem to have access to things like this toString() method which returns a query string that includes the table name (ie. 'select * from "account"').

As @nickrum already mentioned, the downside to throwing right at the query builder is that the condition would need to directly check if the insert value is empty (the only case we know of today that results in an empty query). Meaning if there are other types of inputs that also result in empty queries we'd miss those and have to iterate. The check would look something like this:

diff --git a/lib/query/querybuilder.js b/lib/query/querybuilder.js
index 43b56c54..cdcc3d76 100644
--- a/lib/query/querybuilder.js
+++ b/lib/query/querybuilder.js
@@ -1223,6 +1223,11 @@ class Builder extends EventEmitter {

   // Sets the values for an `insert` query.
   insert(values, returning, options) {
+    if (isEmpty(values)) {
+      throw new Error(
+        `Woops! Attempted to insert ${values}. Query: ${this.toString()}`
+      );
+    }
     this._method = 'insert';
     if (!isEmpty(returning)) this.returning(returning, options);
     this._single.insert = values;

I'm a complete n0ob to this project so would love any advice on the following:

• Is there a known way to get access to the table name from the vendor clients' _steam / _query functions where we throw now? If so, then we could keep it where it's at for now and just update the error message.
• Is there a method to get just the table name without the select * from "account"? Then error could read something like "Attempted to insert an empty array into the foo table"

Lmk if this doesn't make any sense. Appreciate the help.

[digitalmaster]

I was about to ask if there was a way to get a stack trace that showed the exact location of the offending insert command and then realized that when the error is thrown from the query builder it does show the exact location of the insert command:

(My guess is that this is because the async pattern we used to execute queries)

But ya, tracking down the offending insert commands is what makes this debug time-consuming. Otherwise, it's pretty trivial.

LMK if this makes sense and I'll throw up a PR. 🙏🏽