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

closes bos#21 and bos #12 #23

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

mgmeier
Copy link

@mgmeier mgmeier commented Sep 29, 2015

Take actual parsing or regex matching out of the equation for extracting substitution groups from query templates. Makes substitution groups more permissive wrt. to MySQL functions and removes pcre-light dependency.

@bos
Copy link
Collaborator

bos commented Sep 30, 2015

Are you sure this is correct? There are no tests, your code is quite complex, and I see several uses of B.map toLower that raise yellow flags for me. Because the code is difficult to follow, I worry that parts of the query are being lowercased, which was not the pre-existing behaviour.

@mgmeier
Copy link
Author

mgmeier commented Oct 1, 2015

Thanks for looking into it. B.map toLower just lowercases the template to case-insensitively search for the "values" substring (as did the regex). The original template is never affected by that; you can see that the lowercased version doesn't even get bound to a variable.
Anyway I'm sorry if it seems quite complex, maybe I should have commented more.

The approach is:

  • Find index of "values" substring in template, break template after that index (cf. (6+) <$>)
  • Go through the substitution group, counting how deep I'm inside (possibly nested) brackets -- unfortunately we have to enter the domain of context-free expressions here :(
  • Break the template again after the index of a closing bracket at depth 0.
  • Since all this is a Maybe monad, failure at any point leads to "no valid substitution group found"

INSERT INTO test VALUES (?, ?, SUBSTRING(?, 2, 2))
becomes
Just ("INSERT INTO test VALUES", "(?, ?, SUBSTRING(?, 2, 2))", "")

I'm sorry I didn't write any test; since I didn't know if you wanted to change the substitution group logic at all, my implementation was meant more as a proposal on how I'd thought to do it, so you could see what I meant.

If the code is less expressive than I hoped it to be (aka yellow flags), please don't hesitate to ask again.

@mgmeier
Copy link
Author

mgmeier commented Oct 1, 2015

... also I haven't had much time... leaving for a 3 week vacation today :)

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.

2 participants