[Dbix-class] RFC: InflateColumn::File proposed changes

[Marc Mims]

Last week, I attempted to use InflateColumn::File to discover that the
latest version on CPAN is broken.

I have created a branch to resurrect it, since it appeared to require
more than a simple bug fix.
http://dev.catalyst.perl.org/repos/bast/DBIx-Class/0.08/branches/file_column

I have committed changes that I believe restore the functionality to
match the original spec.  There are, however, some design considerations
and I would like some feedback before merging to trunk.

Below, I outline some problems I believe exist in the original design,
propose some design changes to address them, and discuss why changing
the API *may* be acceptable and not break existing user code.

There may be flaws in my analysis and implementation, so I welcome
review and feedback.  I especially welcome any feedback for the
original author to ensure his design goals are met.



The original design inflates to a hashref:

    { handle => $fh, filename => $filename }

And deflates to $filename.

I find the file file handle troublesome for a couple of reasons:

    - An open file handle for each row of a result set seems like a
      potential drain on resources that should be avoided.

    - On insert, the hashref presumably contains a file handle, ready to
      read.  On return from insert, the file handle will be positioned
      to EOF, but still open, making it fairly useless for the remaining
      life of the object.

In the original design, on deflation, the file is saved to the file
system as (pseudo code, here):

    $file_column_path/$pk/$filename

This has the advantage of keeping $filename visible in the file system.
It has the disadvantage of potentially leaving orphaned files behind in
the file system, and worse, stomping on files that belong to other
columns.

For example, if $pk is 1 and we have a "file column" named fc1, storing
a file named 'foo', we have the following in the file system:

    $file_column_path/1/foo

Consider the update:

    $row->fc1({ handle => $fh, filename => 'bar' });
    # later...
    $row->update;

Without some extra code to keep track the prior filename, 'foo', so that
we can delete it on update, we now have the following in the file
system:

    $file_column_path/1/bar
    $file_column_path/1/foo

'foo' is orphaned.

In a more troubling case, consider a table with multiple file columns,
say 'fc1' and 'fc2'.  If both columns are assigned files with the same
name, one will overwrite the other.


So, I propose the following:

    - By default, store to: $file_column_path/$pk/$column (from the
      prior examples, $file_column_path/1/fc1 instead of .../1/foo).

    - Inflate to a Path::Class object.  This encapsulates the file name
      and allows: $fh = $row->fc1->open('r') as many times as necessary
      during the life of the object.

    - Provide a method encapsulating the file storage naming algorithm
      making it easy to override.  The original design uses a separate
      path for each "file column", but it may be advantageous to use a
      different file structure for some applications.

We might consider some other enhancements, as well.  For instance,
rather than inflating to a Path::Class, perhaps a class that also
includes an optional MIME type.  The class would serialize the filename
and MIME type on deflation.  I welcome some input, here.


InflateColumn::File apparently worked at some point, but unless I'm
mistaken, the working version was lost somewhere between the working
development copy and release to CPAN.  The problems that prevented it
from working for me with the current CPAN release seem to exist in the
version 0.07006 where it first appears.  I didn't find any early version
in the repository.  If someone has it, it would be a useful reference.

If no working version was ever released to CPAN, then presumably there
is no existing user code that relies on it and we can make some design
changes to address the problems I've pointed out without breaking
existing code.

If that isn't the case, the after some feedback and testing, we should
merge this version (with any necessary corrections) to the current trunk
to restore the documented functionality.


	-Marc