On Fri, Sep 09, 2011 at 03:50:53PM +0100, Andrew Ford wrote:
> Currently DBD::Gofer::dr:connect() contains code to set up the transport:
>
> my $transport_class = delete $go_attr{go_transport}
> or return $drh->set_err($DBI::stderr, "No transport argument
> in '$orig_dsn'");
> $transport_class = "DBD::Gofer::Transport::$transport_class"
> unless $transport_class =~ /::/;
> _load_class($transport_class)
> or return $drh->set_err($DBI::stderr, "Can't load
> $transport_class: $@");
> my $go_transport = eval { $transport_class->new(\%go_attr) }
> or return $drh->set_err($DBI::stderr, "Can't instanciate
> $transport_class: $@");
>
> I would imagine that we could have a "middleware" keyword that was a
> comma-separated list of names that were assumed to be in the
> DBDx::GoferMiddleware namespace - these would be installed in
> sequence with a similar block of code that follows on from the
> snippet above, looking something like:
>
> my $middleware_classes = delete $go_attr{go_middleware};
> foreach my $middleware_class (split /,/, $middleware_classes) {
> $middleware_class = "DBDx::GoferMiddleware::$middleware_class"
> unless $middleware_class =~ /::/;
> _load_class($middleware_class)
> or return $drh->set_err($DBI::stderr, "Can't load
> $middleware_class: $@");
> $go_transport->middleware( sub { return
> $middleware_class->HANDLER(shift, $go_transport->middleware) } );
> }
>
> Each middleware handler would take a request object and a reference
> to the next handler in the chain.
Calling $go_transport->middleware within the sub isn't right if we go
with a chain of closures, which I'd prefer. I'd expect that last line
to be something like:
$go_transport->middleware( $middleware_class->new_handler(
$go_transport->middleware ) );
where new_handler would look something like:
sub new_handler {
my ($class, $existing_handler) = @_;
my $new_handler = sub {
my ($self, $request) = @_;
...do something with $request...
my $response = $existing_handler->($self, $request);
...do something with $response...
return $response;
} );
return $new_handler;
}
So at setup-time a chain of closures is created.
> The DSNs would then look like:
>
>
> dbi:Gofer:transport=null;middleware=adaptor;transform=MyMapper;dsn=dbi:XXX...
I don't see a need for both middleware= and transform=. Once the
middleware concept is in Gofer then middleware=MyMapper would suffice.
(Unless I'm missing something.)
> There are a few holes in this that I can think of:
>
> * We might want to instantiate a middleware object for each
> component and use that in installing the middleware handler
I don't see a need. Got a use case?
> * I am not sure how we could specify a handler name other than the
> default for individual middleware components
I don't see a need. Got a use case?
> * Also not sure how attributes for each middleware component could
> be specified such that the component for which the attribute is
> intended could be specified - the example DSN above assumes that
> the attribute (transform in the example DSN) is just dumped into
> the general "go_" attribute hash)
Some smarter parsing should make this style possible:
dbi:Gofer:transport=null;middleware=Foo(a=1,b=2),Bar(a=2,b=1);dsn=dbi:XXX...
Anything more fancy could be done by writing a new module with whatever
logic is needed.
> * The go_middleware attribute could be set in the DBI connect
> attribute hash to a list of subroutine references (code would need
> to differentiate between a comma-separated list value and a list
> reference value)
Umm, firstly having a go_middleware attribute that's different to the
$go_transport->middleware attribute is going to get confusing.
Let's keep 'middleware' as the name for the concept but rename the
go_middleware attribute to go_via. That's shorter and more descriptive:
dbi:Gofer:transport=null;via=MyMapper;dsn=dbi:XXX...
And rename $go_transport->middleware to $go_transport->sender.
That's also shorter and actually more descriptive since it is just a
sender (the fact that the sender code ref may reference layers of
middleware is a separate issue).
Let's ignore the semantics of go_via being a reference for now.
Doesn't seem like a priority.
> * Should the middleware components be listed innermost first (as
> implemented in the pseudo-code above) or outermost first?
Innermost first would match the implementation, i.e., when read left to
right each is wrapping the previous (where 'previous' for the first one
is the transport itself).
> * We might want to add some convenience methods to the request class
> to build response objects (as suggested in my
> DBD::Gofer::Transport::adaptor code)
Maybe ensure the relevant server classes can be used safely on the client side.
Adding some docs to http://search.cpan.org/perldoc?DBI::Gofer::Response
would be a good start.
Some light refactoring might be needed. Specifically new_response_with_err
in DBI::Gofer::Execute should probably move into DBI::Gofer::Response.
Other server-specific stuff in DBI::Gofer::Execute might need to move out
(perhaps to a DBI::Gofer::ServerExecute subclass) so it can be used as a
base class for middlewares that want to appear to 'execute' some requests.
Certainly middlewares ought to inherit from a base class supplied by the
DBI to enable safer evolution.
> I'll have a go at getting this coded over the next few days, but it
> looks as if the changes required to implement middleware are
> relatively trivial.
Yeah! Many thanks for doing this Andrew. It's a great addition to Gofer.
> >Your module could then use this mechanism. If you wanted to release it
> >to CPAN then a name like DBDx::GoferMiddleware::FOO would be good.
> >(Note the DBDx not DBIx since this is client-side. We may well end up
> >with server-side gofer middlewares as well.)
>
> I think I would take the name DBDx::GoferMiddleware::adaptor for my
> transformation middleware, unless anyone has a better idea
Umm, adaptor seems too generic. Has almost no meaning out of context.
You'd have to make the logic *much* more generic for the name to fit.
A name like RewriteSPcalls would be more descriptive of the current logic.
> (are we keeping the final class name component for the middleware
> class in lower case, as it is with the transport classes?).
I think I'd prefer a capitalized name for these.
> If we have a DBD::Gofer middleware feature then it would be helpful to
> add a new passthrough (or "passthru") transport. This would be based
> on the null transport, but without the serialization/deserialization,
> i.e. something as simple as:
For efficiency reasons the DBI::Gofer::Execute class assumes it can
modify elements of the request object in-place. The serialization is
needed to avoid subtle bugs that that would otherwise cause. I looked
into 'fixing' that some years ago but there wasn't a good use case then
- the null transport was only ever used for testing. It's probably not
worth trying to fix though. The overhead of serialization/deserialization
is probably (I'm guessing) a relatively small part of the overall
overhead of using Gofer.
Feel free to try, but I wouldn't make it a priority :)
Tim.
