Skip to content
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

Fix Bulk Insert: Single Column and MySQL earlier upsert syntax #734

Merged
merged 5 commits into from May 15, 2021

Conversation

QuangTung97
Copy link
Contributor

@QuangTung97 QuangTung97 commented Apr 27, 2021

This change handle the case of using single column in "VALUES(...)".
And allow using VALUES in "INSERT ... ON DUPLICATE KEY UPDATE ..." kind of queries, e.g issue #722
Possible fixes for #701 #695 #694

@coveralls
Copy link

coveralls commented Apr 27, 2021

Pull Request Test Coverage Report for Build 240

  • 25 of 25 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 74.166%

Totals Coverage Status
Change from base Build 224: 0.3%
Covered Lines: 1223
Relevant Lines: 1649

💛 - Coveralls

@QuangTung97 QuangTung97 changed the title Fix Bulk Insert: Single Column and MySQL old upsert syntax Fix Bulk Insert: Single Column and MySQL earlier upsert syntax Apr 28, 2021
@jakeaglass
Copy link

This solves issues I was having with the new batch insert feature, which was generating statements with unbalanced parentheses, eg INSERT (id) VALUES (coalesce(:a,:b)) compiled to INSERT (id) VALUES (coalesce(1,2)

Many thanks @QuangTung97

@w1ck3dg0ph3r
Copy link

Thanks @QuangTung97!
Was about to create similar PR that does away with this brittle regex)

By the way, this should also fix some additional cases:

  • Functions inside values: INSERT INTO foo (a,b,c,d) VALUES (:name, cast(:age as int), :first, :last)
  • Some strange regex indentation failure I was having on multi-line queries (regex failed on some levels of indentation and not the others, go figure).

Could you add those test cases as well, please? Here's the gist of the ones I added: https://gist.github.com/w1ck3dg0ph3r/cfbce1590fdce1ce54a179b6cb74f6a4.

@QuangTung97
Copy link
Contributor Author

Thanks @QuangTung97!
Was about to create similar PR that does away with this brittle regex)

By the way, this should also fix some additional cases:

  • Functions inside values: INSERT INTO foo (a,b,c,d) VALUES (:name, cast(:age as int), :first, :last)
  • Some strange regex indentation failure I was having on multi-line queries (regex failed on some levels of indentation and not the others, go figure).

Could you add those test cases as well, please? Here's the gist of the ones I added: https://gist.github.com/w1ck3dg0ph3r/cfbce1590fdce1ce54a179b6cb74f6a4.

Thanks. I added test for the multiple lines query case. But the case of function calls might be redundant with the test case two level depth function call I already added.

@jmoiron jmoiron merged commit 92bfa36 into jmoiron:master May 15, 2021
@jmoiron
Copy link
Owner

jmoiron commented May 15, 2021

This looks great; this regex has seen a lot of authors and is difficult to get right, but this looks great and more tests are also welcome. Going to cut a new dot release w/ this sometime this weekend, triaging more in the backlog.

@@ -224,29 +224,47 @@ func bindStruct(bindType int, query string, arg interface{}, m *reflectx.Mapper)
return bound, arglist, nil
}

var valueBracketReg = regexp.MustCompile(`(?i)VALUES\s*(\([^(]*.[^(]\))`)
var valuesReg = regexp.MustCompile(`\)\s*(?i)VALUES\s*\(`)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also care for inserts without an explicit column list here, e.g. insert into tblname values (...)? Will some cases break, If we're not looking for closing bracket before "values"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Thanks for pointing out. But I haven't found a good way to both satisfying that case and the case when the name of table containing suffix: "values".
Any idea?
But because it's usually a bad idea to do that, so might be simple as documenting it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think \W+(?i)VALUES\s*\( should handle most cases, including:

INSERT INTO foo_values (a,b,c,d) VALUES (?, ?, ?, ?)
INSERT INTO foo_values VALUES (?, ?, ?, ?)
INSERT INTO `values`VALUES (?, ?, ?, ?)

While generally being a bad idea, I have no doubt that somebody does implement it, given such a wide adoption of sqlx)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants