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

Sqlbuilder template regex #280

Closed
madagaga opened this issue Apr 30, 2015 · 3 comments
Closed

Sqlbuilder template regex #280

madagaga opened this issue Apr 30, 2015 · 3 comments

Comments

@madagaga
Copy link

The current regular expression in Sqlbuilder template causes issue when clause is at the begining.
For example "SELECT /** whatever **/ F1, F2 .. From T1 /** where **/"
the result is "SELECT "

Replacing with this regex will fix the issue: \/ \*\*[^\*]*\*\*\/

@jods4
Copy link
Contributor

jods4 commented Oct 5, 2016

After replacing placeholders, Dapper removes unused templates with the regex on L67, which is \/\*\*.+\*\*\/.
The issue here is that Regex is greedy by default, so .+ will match as much as possible (i.e. the whole string) and backtrack from there until it matches the end of the pattern (**/). This is inefficient (backtracking a long string) and matches far too much if your template looks like /** 1 **/ keep this /** 2 **/.

The proposed solution is ok-ish in this case but not perfect. By not matching *, the following block will not match: /** x*y **/.

I suggest using a non-greedy modifier .+?, the full regex becomes \/\*\*.+?\*\*\/.
A non-greedy regex will match as little as possible, which is exactly what we want.
It will perform better and match the comments correctly, even when there's a star inside.

Another option comes to mind: obviously the syntax /** xyz **/ was chosen because it is a SQL comment, hence valid SQL. So why remove the unused templates at all? I guess it would be even more efficient to do nothing, no?

jods4 added a commit to jods4/dapper-dot-net that referenced this issue Oct 5, 2016
jods4 added a commit to jods4/dapper-dot-net that referenced this issue Oct 5, 2016
Fixes DapperLib#436, DapperLib#280
My editor has also normalized a few line endings to LF like 99% of the
file was.
@jods4
Copy link
Contributor

jods4 commented Oct 5, 2016

PR #625 fixes this bug, assuming you want to keep removing the unused comments/templates.
@NickCraver would be nice if you could merge and publish an updated package. This has been open for 1.5 years and can generate at best incorrect SQL, at worse security issues.

@mgravell
Copy link
Member

mgravell commented Oct 6, 2016

Merged PR

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

No branches or pull requests

4 participants