-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
NamedExec Bulk Insert Fix #718
Conversation
This change-set is how I addressed the issues of bulk inserts not being handled cleanly when they're not at the end of the query. It works by first adding VALUES to the expression (not sure if this is universally applicable to all supported SQL dialects) then adding a submatch to the regular expression for the fields we want to repeat. Using the submatch, we're able to repeat the fields that we're concerned with to expand the insertion parameters.
f7bb766
to
6258c9a
Compare
Pull Request Test Coverage Report for Build 217
💛 - Coveralls |
// defensive guard. loc should be len 4 representing the starting and | ||
// ending index for the whole regex match and the starting + ending | ||
// index for the single inside group | ||
if len(loc[0]) != 4 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If loc[0] != 4
, that indicates that the regex was changed and either the submatcher was removed or another submatcher was added.
Just tried this patch myself and it works fine with |
@@ -214,7 +218,7 @@ func TestNamedQueries(t *testing.T) { | |||
} | |||
|
|||
_, err = db.NamedExec(`INSERT INTO person (first_name, last_name, email) | |||
VALUES (:first_name, :last_name, :email) `, slsMap) | |||
VALUES (:first_name, :last_name, :email) ;--`, slsMap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this extra comment necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment here is exercising the regex in the sense that anything after the closing paren is ignored.
I could update it to something more clear like ON CONFLICT
perhaps? I imagine that most people are using that behavior moreso than random junk at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy with the comment, just did not want to add some extra load bearing requirement here. I think it's clear from the tests later on that these are testing some boundary conditions.
Tests looked good, tagged as v1.3.2 |
This change-set is how I addressed the issues of bulk inserts not being handled cleanly when they're not at the end of the query.
It works by first adding
VALUES
to the expression (not sure if this is universally applicable to all supported SQL dialects) then adding a submatch to the regular expression for the fields we want to repeat. Using the submatch, we're able to repeat the fields that we're concerned with to expand the insertion parameters.Likely? Fixes #657, #694, #705, #709, #712, #713 ?