Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

API is somewhat confusing #3

Closed
mlawren opened this Issue · 12 comments

2 participants

@mlawren

Given the lack of discussion forum I hope writing this as an issue is
ok. It is a bit of a shame that gtihub doesn't let you open an issue by
email. At least not that I could find.

I have a couple of issues with the DBIx::Connector API.

The first is the use of the name "do()" in the context of a DBIx
module. For coders writing DBI-related scripts a do() method has always
meant "run this SQL against the database". The DBIx::Connector do()
method means something different - "run this CODE, and run it again if
database not connected". This is quite confusing. I've spent a good
hour looking at integrating DBIx::Connecter into just one section of my
code, and this similarity of method names for different meanings keeps
tripping me up. Two times do() in three lines...

$conn->do(sub {
    my $dbh = shift;
    $dbh->do('INSERT INTO table1 VALUES (1)');

The second issue I have is that the API doesn't help us move away from
eval / if ($@) constructs (which are apparently a difficult thing to
get right). I generally want to know or take an action if operations
have succeeded or not, and this fast gets noisy and nested:

eval {
    $conn->txn_do(sub {
        my $dbh = shift;
        $dbh->do('INSERT INTO table1 VALUES (1)');
        eval {
            $conn->svp_do(sub {
                shift->do('INSERT INTO table1 VALUES (2)');
                die 'OMGWTF?';
            });
        };
        if ( $@ ) {
            warn "Savepoint failed\n";
            # Handle this
        }
        $dbh->do('INSERT INTO table1 VALUES (3)');
    });
};

if ( $@ ) {
    # Handle this
}

Taking a page from the Try::Tiny module (and maybe using
Return::Value?) wouldn't something like the following be cleaner? By
the way, prototyping txn_do() with coderef would also be nice to avoide
the "sub":

$conn->try({
    my $dbh = shift;
    $dbh->do('INSERT INTO table1 VALUES (1)');
})->catch({
    # Handle the error
});

I'm thinking that in the event that the try() sub dies, the rollback
happens and returns an object whos catch() method is run. If
the try does not die the (other) returned object's catch() method
does nothing.

Here's my idea of the long version:

$conn->txn({
    my $dbh = shift;
    $dbh->do('INSERT INTO table1 VALUES (1)');

    $conn->svp({
        shift->do('INSERT INTO table1 VALUES (2)');
        die 'OMGWTF?';
    })->catch({
        warn "Savepoint failed\n";
        # Handle this
    });

    $dbh->do('INSERT INTO table1 VALUES (3)');
})->catch({
    # Handle this
});

The use of txn(), svp() would mean backwards compatability could be
kept with txn_do...

Thoughts?

Cheers,
Mark.

@mlawren

Or to avoid mucking about with objects how about this alternative:

$conn->try({
    my $dbh = shift;
    $dbh->do('INSERT INTO table1 VALUES (1)');
}, catch {
    # Handle the error
});
@mlawren

Even better - The "catch" subroutine could be optional, and
doesn't even have to come from DBIx::Connector. It could
be whatever TryCatch, Try::Tiny, or my_custom_catcher()
subroutine the caller wants to use.

@mlawren

Ok, so "catch" is generally a no-op... Please ignore the previous.

@theory
Owner

I like the idea of catch(). I'll have to think about implementation, as I don't want to require that the user use it.

Prototyping doesn't work with method calls, alas. But you can use PerlX::MethodCallWithBlock.

—Theory

@theory
Owner

Based on the discussion in this DBIx::Class ticket, I think I'm going to deprecate the do methods and add these methods, instead:

  • run
  • runup
  • txn_run
  • txn_runup
  • svp_run

All will get the connection object as the argument, rather than the database handle. The runup methods will offer the optimistic connection stuff, while the run methods will ping.

—Theory

@theory
Owner

Still pondering better names. Check out my blog entry on the topic.

—Theory

@theory
Owner

Okay, 0.20 is on its way to the CPAN now with run, txn, and svp methods, which should eliminate the confusion with do. I'll do some thinking about exception catching next.

—Theory

@theory
Owner

Mark,

What if I passed errors on to the DBI's HandleError attribute, it it's available?

—Theory

@theory
Owner

Ignore that last comment, it was irrelevant.

I could simply make a rule that, if the argument passed after the block is also a block, that it will handle any exceptions. This would allow you to do something like this:

use Try::Tiny;

$conn->run( sub {
    my $dbh = shift;
    # ....
}, catch {
    warn "caught error: $_";
});

That is, catch is just syntactic sugar for sub, which you could also use. Indeed, any Try/Catch module that basically makes its catch return a code block (as Try::Tiny's does) could then be used there.

The only downside to this plan that I can think of is that, since other arguments to the run(), txn(), and svp() methods are passed through to the execution block, if you wanted to pass a code ref, you couldn't unless you also specified an exception handler. Maybe that's not such a bad thing, though.

Thoughts?

—Theory

@theory
Owner

Okay, check out this commit, which adds exception handling with just a second code ref passed to an execution method. Read the POD I added to describe it (search for "head3 Exception Handling") for how it works and how to take advantage of Try::Tiny at the same time.

I may or may not add our own exported catch function; I have not yet decided.

—Theory

@theory
Owner

Maybe exception handlers should be specified by a keyword?

$conn->run(sub {
    die 'WTF!';
}, catch => sub {
    warn "Died: $_";
});

Just fiddling with ideas…

—Theory

@theory
Owner

I've now released DBIx::Connector to CPAN. You can do this:

$conn->run(sub {
    die 'WTF!';
}, catch => sub {
    warn "Died: $_";
});

Or use it with Try::Tiny, like so:

use Try::Tiny;
$conn->run(sub {
    die 'WTF!';
}, catch {
    warn "Died: $_";
});

I'm pretty happy with this.

—Theory

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.