[Fwd: Re: [scmbug-users] Error on changing status of bugs with Bugzilla 3.4.x]

[Yavor Nikolov]

Hi,

Kristis will explain best what he means - here are my comments below inline
(since I'm the one intending to wrap bugzilla calls in eval block). Excuse
me for my ignorance since I'm not a Perl guru (I just got involved to fix my
problems as end user).

On Fri, Apr 16, 2010 at 02:24, Max Kanat-Alexander <mkanat at bugzilla.org>wrote:

> On 04/15/2010 10:04 AM, Kristis Makris wrote:
> > How does bugzilla 3.2/3.4/3.6 handle errors with Bug::check() ? Does it
> > throw exceptions with die() ? Is this necessary ?
>
>         It calls ThrowUserError. By default this calls exit(), but you can
> switch to Bugzilla->error_mode(ERROR_MODE_DIE) before calling it (and
> then switch back to the normal error mode) if you'd like.
>
We're calling this from separate perl application - not as CGI. So I believe
error_mode is ERROR_MODE_DIE by default in our case.


>
>        However, generally you shouldn't be calling things inside of an
> eval--there's always ways to check things for yourself. What
> specifically are you trying to do?
>
Good to know if there are better alternatives. We have a daemon (perl
application) which is integrating version control systems (e.g. Subversion)
with Bug-tracking system (Bugzilla). So for example - on commit to version
control we want to perform following with Bugzilla on behalf of the user
performing scm commit (we have the mapping to the corresponding bugzilla
username):
 - add comment to the bug (extracted from version control system commit
message)
 - update bugzilla bug status and resolution; or reassign bug
 - send e-mail notifications to interested parties (that's my custom
extension not yet in official scmbug release)
 - Some other checks and verifications (e.g. possible workflow transitions
are extracted to signal some errors even before version control commit).
* And on error version control user is supposed to get appropriate error
message (e.g. the given duplicate bug id is wrong; the new asaignee user is
invalid; Lack of permissions to modify bug).


For example - consider following fragment of code:
        my $dbh = Bugzilla->dbh;
        my @ret_list = eval {
        my $userid = Bugzilla::User::login_to_id( $username );
        if ( $userid > 0 ) {
            $dbh->bz_start_transaction();
            my $user = new Bugzilla::User($userid);
            Bugzilla->set_user($user);
            my $bug = Bugzilla::Bug->check($bugid);
            $bug->add_comment($comment, {isprivate => 0, work_time => 0,
type => Bugzilla::Constants->CMT_NORMAL, extra_data => ""} );
            $bug->update();
            $dbh->bz_commit_transaction();
            return 0;
        } else {
            # This should never happen. Each user should have a
            # corresponding userid in the database schema
            return 1, "Login '$username' could not be converted to an id in
Bugzilla. Is username mapping setup correctly in daemon.conf ?\n" ;
        }
        }; # eval
        if ($@) {
            my $err = $@;
            log_daemon_warn( undef, $err );
            $dbh->bz_rollback_transaction() if $dbh->bz_in_transaction();
            return 1, "Error while adding bug comment: $err";
        }
        else {
            return @ret_list;
        }

We didn't have eval block here before - which resulted in aborting
processing when check() fails and notifying scm user with quite general
error message.

The case with change of status resolution is more interesting - you can see
last section of additions (within eval block) in the attached patch.

I added that patch to simplify a bit the communication between scmbug and
Bugzilla and reduce compatibility issues with current and future releases.
What we had before was code initially developed against older versions of
Bugzilla with multiple direct database calls, explicit validations.

So from my point of view - some kind of exception handling seems better in
order to avoid bloating code and checking return codes on each step. However
I see some drawbacks with the current die/eval/if($@) approach that I'm
trying to hook in scmbug with that patch:
 - die/eval has some limitations - it's not easy /if possible at all/ to
track original call stack (in order to see where did the error came from).
 - Nor type of error could be easily distinguished (it's just plain text).
 + Recent versions of Perl support object oriented exception handling
mechanism which sounds better to me.


>
> > I'm sure you folks must have a more user-friendly API than this. We must
> > be missing something.
>
>         Yeah, the WebService is the API.
>
I can't find the link now but I've found some discussion where Kristis
explained that WebService API was not powerful enough (maybe since the
scmbug daemon should be able to impersonate as any user without requesting
version control user to provide a password).
As far as I see - Bugzilla cgi modules are using Perl API instead of
WebService too.


>
>        -Max
> --
> http://www.everythingsolved.com/
> Competent, Friendly Bugzilla and Perl Services. Everything Else, too.
> -
> To view or change your list settings, click here:
> <http://bugzilla.org/cgi-bin/mj_wwwusr?user=nikolov.javor@gmail.com>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bugzilla.org/pipermail/developers/attachments/20100416/75cb658c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Bugzilla.pm-resolution-and-comment-latest_version.patch
Type: application/octet-stream
Size: 6434 bytes
Desc: not available
URL: <http://lists.bugzilla.org/pipermail/developers/attachments/20100416/75cb658c/attachment.obj>