DBI Improvements

lib/Plack/Session/Store/DBI.pm

@@ -12,7 +12,7 @@ use Storable ();
 
 use parent 'Plack::Session::Store';
 
-use Plack::Util::Accessor qw[ dbh get_dbh table_name serializer deserializer ];
+use Plack::Util::Accessor qw[ dbh get_dbh table_name serializer deserializer id_column data_column];
 
 sub new {
     my ($class, %params) = @_;
@@ -22,6 +22,8 @@ sub new {
     }
 
     $params{table_name}   ||= 'sessions';
+    $params{data_column}  ||= 'session_data';
+    $params{id_column}    ||= 'id';
     $params{serializer}   ||= 
         sub { MIME::Base64::encode_base64( Storable::nfreeze( $_[0] ) ) };
     $params{deserializer} ||= 
@@ -39,8 +41,17 @@ sub _dbh {
 sub fetch {
     my ($self, $session_id) = @_;
     my $table_name = $self->{table_name};
+    my $data_column = $self->{data_column};
+    my $id_column = $self->{id_column};
     my $dbh = $self->_dbh;
-    my $sth = $dbh->prepare_cached("SELECT session_data FROM $table_name WHERE id = ?");
+    my $sth = $dbh->prepare_cached(
+        sprintf(
+            "SELECT %s FROM %s WHERE %s = ?",
+            $dbh->quote_identifier($data_column),
+            $self->_quote_table_name($table_name),
+            $dbh->quote_identifier($id_column),
+        )
+    );
     $sth->execute( $session_id );
     my ($data) = $sth->fetchrow_array();
     $sth->finish;
@@ -50,38 +61,84 @@ sub fetch {
 sub store {
     my ($self, $session_id, $session) = @_;
     my $table_name = $self->{table_name};
+    my $data_column = $self->{data_column};
+    my $id_column = $self->{id_column};
+
+    my $dbh = $self->_dbh;
 
     # XXX To be honest, I feel like there should be a transaction 
     # call here.... but Catalyst didn't have it, so I'm not so sure
 
-    my $sth = $self->_dbh->prepare_cached("SELECT 1 FROM $table_name WHERE id = ?");
+    my $sth = $dbh->prepare_cached(
+        sprintf(
+            "SELECT 1 FROM %s WHERE %s = ?",
+            $self->_quote_table_name($table_name),
+            $dbh->quote_identifier($id_column),
+        )
+    );
     $sth->execute($session_id);
 
     # need to fetch. on some DBD's execute()'s return status and
     # rows() is not reliable
     my ($exists) = $sth->fetchrow_array(); 
 
     $sth->finish;
+
+    my %column_data = (
+        $self->additional_column_data($session),
+
+        $data_column => $self->serializer->($session),
+        $id_column => $session_id,
+    );
 
+    my @columns = sort keys %column_data;
     if ($exists) {
-        my $sth = $self->_dbh->prepare_cached("UPDATE $table_name SET session_data = ? WHERE id = ?");
-        $sth->execute( $self->serializer->($session), $session_id );
+        my $sth = $dbh->prepare_cached(
+            sprintf(
+                "UPDATE %s SET %s WHERE %s = ?",
+                $self->_quote_table_name($table_name),
+                join(',', map { $dbh->quote_identifier($_).' = ?' } @columns),
+                $dbh->quote_identifier($id_column),
+            )
+        );
+        $sth->execute( @column_data{@columns}, $session_id );
     }
     else {
-        my $sth = $self->_dbh->prepare_cached("INSERT INTO $table_name (id, session_data) VALUES (?, ?)");
-        $sth->execute( $session_id , $self->serializer->($session) );
+        my $sth = $dbh->prepare_cached(
+            sprintf(
+                "INSERT INTO %s (%s) VALUES (%s)",
+                $self->_quote_table_name($table_name),
+                join(',', map { $dbh->quote_identifier($_) } @columns),
+                join(',', map { '?' } @columns),
+            )
+         );
+        $sth->execute( @column_data{@columns});
     }
-
 }
 
+sub additional_column_data { }
+
 sub remove {
     my ($self, $session_id) = @_;
     my $table_name = $self->{table_name};
-    my $sth = $self->_dbh->prepare_cached("DELETE FROM $table_name WHERE id = ?");
+    my $id_column = $self->{id_column};
+    my $sth = $self->_dbh->prepare_cached(
+        sprintf(
+            "DELETE FROM %s WHERE %s = ?",
+            $self->_quote_table_name($table_name),
+            $self->_dbh->quote_identifier($id_column),
+        )
+    );
     $sth->execute( $session_id );
     $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__
@@ -132,13 +189,15 @@ Plack::Session::Store::DBI - DBI-based session store
   };
 
 
-  # use custom session table name
+  # use custom session table name, session ID or data columns
 
   builder {
       enable 'Session',
           store => Plack::Session::Store::DBI->new(
-              dbh        => DBI->connect( @connect_args ),
-              table_name => 'my_session_table',
+              dbh         => DBI->connect( @connect_args ),
+              table_name  => 'my_session_table',
+              id_column   => 'session_id',
+              data_column => 'data',
           );
       $app;
   };
@@ -165,6 +224,22 @@ Note that MySQL TEXT fields only store 64KB, so if your session data
 will exceed that size you'll want to move to MEDIUMTEXT, MEDIUMBLOB,
 or larger.
 
+You can opt to specify alternative table names (using table_name), as well as
+alternative columns to use for session ID (id_column) and session data storage
+(data_column), especially useful if you're converting from an existing session
+mechanism.
+
+=head1 EXTENDING
+
+Plack::Session::Store::DBI has built in functionality to allow for inheriting
+modules to set additional columns on each session row.
+
+By overriding the additional_column_data function, you can return a hash of
+columns and values to set.  The session data hashref will be passed to the
+overridden additional_column_data function as its only argument, so that you
+can use session data values as appropriate for any additional column data you
+would like to set.
+
 =head1 AUTHORS
 
 Many aspects of this module were partially based upon L<Catalyst::Plugin::Session::Store::DBI>

t/006_basic_w_dbi_store.t

@@ -26,6 +26,16 @@ CREATE TABLE sessions (
 );
 EOSQL
 
+# Building the table with these weird names will simultaneously prove that we
+# accept custom table and column names while also demonstrating that we do
+# quoting correctly, which the previous code did not.
+$dbh->do(<<EOSQL);
+CREATE TABLE 'insert' (
+    'where' CHAR(72) PRIMARY KEY,
+    'set' TEXT
+);
+EOSQL
+
 TestSession::run_all_tests(
     store  => Plack::Session::Store::DBI->new( dbh => $dbh ),
     state  => Plack::Session::State->new,
@@ -43,6 +53,28 @@ TestSession::run_all_tests(
     },
 );
 
+TestSession::run_all_tests(
+    store  => Plack::Session::Store::DBI->new(
+        dbh         => $dbh,
+        table_name  => 'insert',
+        id_column   => 'where',
+        data_column => 'set',
+    ),
+    state  => Plack::Session::State->new,
+    env_cb => sub {
+        open my $in, '<', \do { my $d };
+        my $env = {
+            'psgi.version'    => [ 1, 0 ],
+            'psgi.input'      => $in,
+            'psgi.errors'     => *STDERR,
+            'psgi.url_scheme' => 'http',
+            SERVER_PORT       => 80,
+            REQUEST_METHOD    => 'GET',
+            QUERY_STRING      => join "&" => map { $_ . "=" . $_[0]->{ $_ } } keys %{$_[0] || +{}},
+        };
+    },
+);
+
 TestSession::run_all_tests(
     store  => Plack::Session::Store::DBI->new( get_dbh => sub { $dbh }  ),
     state  => Plack::Session::State->new,
@@ -63,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;