DBI Improvements

[defc0n]

These changes fix/improve the following with the Plack::Session::Store::DBI module:

• Fix SQL quoting issues with table / column names
• Add support for data_column & id_column attributes to customize
• Add support for additional_column_data() customization

[haarg]

This generally looks good, but could be a problem for some uses. Without this, the table name could be specified with a schema, like other_database.my_table_name.

[defc0n]

What about doing something like:

diff --git a/lib/Plack/Session/Store/DBI.pm b/lib/Plack/Session/Store/DBI.pm
index e8a1176..6822f87 100644
--- a/lib/Plack/Session/Store/DBI.pm
+++ b/lib/Plack/Session/Store/DBI.pm
@@ -48,7 +48,7 @@ sub fetch {
         sprintf(
             "SELECT %s FROM %s WHERE %s = ?",
             $dbh->quote_identifier($data_column),
-            $dbh->quote_identifier($table_name),
+            $self->_quote_table_name($table_name),
             $dbh->quote_identifier($id_column),
         )
     );
@@ -72,7 +72,7 @@ sub store {
     my $sth = $dbh->prepare_cached(
         sprintf(
             "SELECT 1 FROM %s WHERE %s = ?",
-            $dbh->quote_identifier($table_name),
+            $self->_quote_table_name($table_name),
             $dbh->quote_identifier($id_column),
         )
     );
@@ -96,7 +96,7 @@ sub store {
         my $sth = $dbh->prepare_cached(
             sprintf(
                 "UPDATE %s SET %s WHERE %s = ?",
-                $dbh->quote_identifier($table_name),
+                $self->_quote_table_name($table_name),
                 join(',', map { $dbh->quote_identifier($_).' = ?' } @columns),
                 $dbh->quote_identifier($id_column),
             )
@@ -107,7 +107,7 @@ sub store {
         my $sth = $dbh->prepare_cached(
             sprintf(
                 "INSERT INTO %s (%s) VALUES (%s)",
-                $dbh->quote_identifier($table_name),
+                $self->_quote_table_name($table_name),
                 join(',', map { $dbh->quote_identifier($_) } @columns),
                 join(',', map { '?' } @columns),
             )
@@ -125,7 +125,7 @@ sub remove {
     my $sth = $self->_dbh->prepare_cached(
         sprintf(
             "DELETE FROM %s WHERE %s = ?",
-            $self->_dbh->quote_identifier($table_name),
+            $self->_quote_table_name($table_name),
             $self->_dbh->quote_identifier($id_column),
         )
     );
@@ -133,6 +133,12 @@ sub remove {
     $sth->finish;
 }

+sub _quote_table_name {
+    my ($self, $table_name) = @_;
+    my @parts = split /\./, $table_name;
+    return join '.', map { $self->_dbh->quote_identifier( $_ ) } @parts;
+}
+
 1;

 __END__
diff --git a/t/006_basic_w_dbi_store.t b/t/006_basic_w_dbi_store.t
index e223792..662d180 100644
--- a/t/006_basic_w_dbi_store.t
+++ b/t/006_basic_w_dbi_store.t
@@ -95,4 +95,8 @@ TestSession::run_all_tests(

 $dbh->disconnect;

+my $store = Plack::Session::Store::DBI->new( dbh => $dbh );
+is $store->_quote_table_name('mysession'), '"mysession"';
+is $store->_quote_table_name('otherdb.mysession'), '"otherdb"."mysession"';
+
 done_testing;

[haarg]

That seems like a reasonable solution to me.

[defc0n]

That seems like a reasonable solution to me.

@haarg any chance this could be included in a release soon?