feat: use bind params in bulkInsertQuery #17131
Conversation
Americas
force-pushed
the
main
branch
7 times, most recently
from
a98e83c
to
75c9cd0
Compare
Americas
force-pushed
the
main
branch
7 times, most recently
from
3771ada
to
3c5194c
Compare
Americas
force-pushed
the
main
branch
5 times, most recently
from
a2dc36d
to
5c09897
Compare
| @@ -40,6 +40,7 @@ type InsertOptions = ParameterOptions & | |||
|
|
|||
| type BulkInsertOptions = ParameterOptions & { | |||
| hasTrigger?: boolean; | |||
| bindParam?: false | ((value: unknown) => string); | |||
There was a problem hiding this comment.
bindParam is a legacy option. Could you add support for parameterStyle instead?
enum ParameterStyle {
bind = 'bind',
replacement = 'replacement',
}There was a problem hiding this comment.
Judging from the comment on my previous PR, I hope I did it right, but please explain if I did not get it right.
| @@ -93,13 +94,13 @@ export class AbstractQueryGenerator extends AbstractQueryGeneratorTypeScript { | |||
| valueHash: object, | |||
| columnDefinitions?: { [columnName: string]: NormalizedAttributeOptions }, | |||
| options?: InsertOptions, | |||
| ): { query: string; bind?: unknown[] }; | |||
| ): { query: string; bind?: Record<string, unknown> }; | |||
There was a problem hiding this comment.
could you move { query, bind } to its own reusable type?
Americas
force-pushed
the
main
branch
6 times, most recently
from
7693d58
to
c951e6e
Compare
Americas
force-pushed
the
main
branch
5 times, most recently
from
b580d53
to
1b7781d
Compare
|
I have a question on this, all the dialects have varying numbers of allowed parameters with MSSQL having the least at 2100. The only reason for asking is that I initially was going to include this change in PR #16988 however I opted not to due to this potential issue. |
|
While I do understand this problem has a higher chance of happening in a bulk insert, the same is true in any other query that uses bind params. It certainly seems important to deal with this, or at least make sure the error is understandable. To note, this PR does not add bind params to mssql, both mysql and postgres seem to handle around 65k, not sure how likely it is to hit that. This PR also still allows to use replacement instead of bind. If this is truly a problem, I can make the default behavior be replacement instead of bind. |
|
Note that there will eventually be a global option to configure the defaults, but the exact API is still TBD, so even if we pick replacements by default, you could configure your Sequelize instance to use bind for this query |
sequelize-bot
bot
added
the
conflicted
Description of Changes
When using bulk create, the generated SQL does not use bindings. This creates an issue, since the data is logged along with the running query.
With this PR, the default generated SQL for a bulk insert will use bindings by default, unless
bindParamsis false.This is a followup on #17128, I still hope we can get that one on the v6 branch, since it will still be a while for v7 to be usable. @ephys
List of Breaking Changes
QueryGenerator.bulkInsertQuery()now returns an object with 2 properties:query(always returned) andbind(only if bind params are used). This follows the same pattern that is used in other methods from this class, likeInsertQuery.