The Phrasebook Design Pattern (O'Reilly) [LWN.net]

Rani Pinchuk examines Perl/SQL database issues on O'Reilly's Perl.com: "if you look at the code above, you will see two languages: Perl and SQL. This makes the code not that readable. Besides, SQL statements that are only slightly different may appear in several places, and this will make it more difficult to maintain. In addition, suppose you have an SQL expert who should optimize your SQL calls. How do you let him work on the SQL; should he look through your Perl code for it? And what if that guy is so ignorant that he doesn't even know Perl? "

The Phrasebook Design Pattern (O'Reilly)

Posted Oct 31, 2002 4:39 UTC (Thu) by slamb (guest, #1070) [Link] (2 responses)

Looking more closely, their Class::Phrasebook::SQL subclass sucks as-is. I say that because

it embeds the literal values rather than using prepared statements and placeholders (see the quotes around $description? They wouldn't be there otherwise). First, this is a huge security flaw. They are completely open to SQL injection attacks. Second, you can execute the same prepared statement many times. On a database that supports it (Oracle does, PostgreSQL 7.3 will, etc.), this can be a performance gain because it won't need to reparse or recreate the execution plan. They're missing out on that.
it drops lines that contain undefined placeholder values. First, what if you want to insert a null? You can't, though it'd be easy to fix to by changing to dropping lines that don't exist in the parameter hash (they can exist but be undefined). Harder to fix: removing the final one in their example wouldn't work this way, because there'd be a trailing comma and the statement would not parse. They really need some sort of list builder for this to work. (I'll have to think about that; my library doesn't have quite this feature yet, though I am building lists for another purpose.)

Please don't use it until they fix the security flaw. The second point is more minor.

The Phrasebook Design Pattern (O'Reilly)

Posted Oct 31, 2002 4:50 UTC (Thu) by slamb (guest, #1070) [Link]

Actually, looking at the actual code, it isn't quite what I thought. He does escape quotes in variables if they are within quotes in the statement, but:

he doesn't do it in a database-specific way, so it's still vulnerable to the attack on PostgreSQL at least. Escaping ' as '' doesn't work, because if they have "\'" in the phrase, it will be changed to "\''", which is an escaped quote (\ is an alternate way to escape in PostgreSQL!) followed by an unescaped quote
since Perl isn't strongly typed, it would be easy to think something is an integer when it's actually an arbitrary string. And it will still parse with quotes. So unquoted values are dangerous, even if they are ints on the database side.

so really, the quoting as-is is pretty useless.

The Phrasebook Design Pattern (O'Reilly)

Posted Oct 31, 2002 5:07 UTC (Thu) by slamb (guest, #1070) [Link]

The comma thing isn't true, either - the class does a fair amount of parsing of update statements. It filters out the trailing comma if it's present. Though it would choke if there's a /\swhere\s/ anywhere in literal text. Unlikely but possible that you would have a statement with a literal hardcoded as containing that word.