#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