#96 ✓resolved
Matthew King

ambition: hard-coded MySQL syntax in WhereProcessor methods

Reported by Matthew King | August 30th, 2007 @ 08:01 AM

The SQL generated by the WhereProcessor has MySQL syntax baked in. So far, I've noticed backquotes to escape column names and the REGEX operator.

The methods in ActiveRecord::ConnectionAdapters::Quoting should take care of the quoting variations, but there's nothing native in ActiveRecord to handle db-specific regex operators.

Comments and changes to this ticket

  • Matthew King

    Matthew King August 30th, 2007 @ 08:19 AM

    • Title changed from “Hard-coded MySQL syntax in WhereProcessor methods” to “ambition: hard-coded MySQL syntax in WhereProcessor methods”
  • Chris Wanstrath

    Chris Wanstrath August 30th, 2007 @ 10:56 AM

    • State changed from “new” to “open”

    I thought RLIKE was MySQL-specific and REGEXP was SQL standard. Am I getting it backwards?

    Yes, Ambition was written for MySQL and ActiveRecord. But I don't want it to stay that way. Any help on general compatibility would be great.

    So, backticks are a no-go? I don't like them that much, anyway.

  • Matthew King

    Matthew King August 30th, 2007 @ 10:59 AM

    Here's a patch that adds a quote_column_name facade for the ActiveRecord connection to Processor, which is then used in WhereProcessor#translate.

    The patch has no tests, because I can't figure out a way to test without using a live ActiveRecord connnection.

    I will gladly add some tests, if someone can give me a clue on how to mock or stub or prestidigitate an AR connection, so that I can access the adapter-specific quoting methods.

  • Matthew King

    Matthew King August 30th, 2007 @ 11:27 AM

    Regarding REGEXP and SQL standards, I dunno. I haven't been using postgres long, and I have to look everything up anyway. Speaking of which, the PostgreSQL manual has a table showing regex operators:

    http://www.postgresql.org/docs/8...

    It should be easy to add a clumsy helper method that picks a regex operator based on the AR::Base.connection adapter, but that seems ugly.

  • Chris Wanstrath

    Chris Wanstrath August 30th, 2007 @ 12:29 PM

    I think having database-specific translators maybe the only way. Since that requires some architecture re-thinking, I may take that on later tonight sometime. Suck.

    I added the column_name thing. I need to improve to_sql generation -- right now it's not matching the actual SQL called. Note to self.

    http://projects.require.errthebl...

  • Matthew King

    Matthew King August 30th, 2007 @ 06:59 PM

    Re: database-specific translators for regexp

    How bad of an idea is this?

    Monkeypatch each ActiveRecord adapter with regexp-producing methods, then call them in WhereProcessor#process_match3 using:

    ActiveRecord::Base.connection.regexp(value)
    

    or a suitable helper method.

  • Matthew King

    Matthew King August 31st, 2007 @ 10:06 AM

    I'm uploading a meaty patch (with tests!) that implements the idea in my last comment. I.e., it monkeypatches the Abstract, MySQL, and PostgreSQL adapters with a method named ambition_regexp. It also adds a method named adapter_regexp to Processor and slightly smartens the rescue clauses in the quoting methods. The results are not as ugly as I had feared.

    In where_test.rb, I added contexts for MySQL, PostgreSQL, and an adapter that doesn't get monkeypatched (so I can test the Abstract adapter). It turns out that you can fake a new PostgreSQL connection by passing it strings for args, and you can fake a MySQL connection by overriding its connect method and passing strings for args. This appears to be sufficient to test the helper methods in these classes, namely the quoting.

  • Chris Wanstrath

    Chris Wanstrath September 1st, 2007 @ 03:17 PM

    Do you have AIM or hang out on IRC? I'm doing some refactoring today and want to talk to you about this patch / concept.

  • Matthew King

    Matthew King September 1st, 2007 @ 05:12 PM

    I usually don't do the chatty things, but I can on weekdays. On weekends, my real boss requests that I keep computer work to a minimum.

  • Chris Wanstrath

    Chris Wanstrath September 2nd, 2007 @ 12:38 PM

    • State changed from “open” to “resolved”

    I took a slightly different approach, but the idea is the same. Should be fixed now:

    http://projects.require.errthebl...

    The statement method opens the door for future DB-specific stuffs, too (one hopes).

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Shared Ticket Bins

People watching this ticket

Tags

Pages