aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authormkanat%kerio.com <>2005-03-05 08:18:47 +0000
committermkanat%kerio.com <>2005-03-05 08:18:47 +0000
commitec610fd673feb6d6e18d121b5e67aa3f87e7f4ea (patch)
treeb1d6fe9b10b89a30e2b1932d050d5678362f638a
parente4b8b770b0229a0761a3ef5116c7fcad4e5352d5 (diff)
downloadbugs-ec610fd673feb6d6e18d121b5e67aa3f87e7f4ea.tar
bugs-ec610fd673feb6d6e18d121b5e67aa3f87e7f4ea.tar.gz
bugs-ec610fd673feb6d6e18d121b5e67aa3f87e7f4ea.tar.bz2
bugs-ec610fd673feb6d6e18d121b5e67aa3f87e7f4ea.tar.xz
bugs-ec610fd673feb6d6e18d121b5e67aa3f87e7f4ea.zip
Bug 277782: _throw_error should unlock tables when tables are locked, automatically
Patch By Tomas Kopal <Tomas.Kopal@altap.cz> r=travis, r=LpSolit, a=justdave
-rw-r--r--Bugzilla/Auth/Login/WWW/CGI.pm2
-rwxr-xr-xBugzilla/Bug.pm9
-rw-r--r--Bugzilla/Error.pm24
-rw-r--r--CGI.pl4
-rwxr-xr-xeditclassifications.cgi2
-rwxr-xr-xeditcomponents.cgi7
-rwxr-xr-xeditmilestones.cgi11
-rwxr-xr-xeditusers.cgi18
-rwxr-xr-xeditversions.cgi3
-rw-r--r--globals.pl9
-rwxr-xr-xpost_bug.cgi7
-rwxr-xr-xprocess_bug.cgi20
12 files changed, 46 insertions, 70 deletions
diff --git a/Bugzilla/Auth/Login/WWW/CGI.pm b/Bugzilla/Auth/Login/WWW/CGI.pm
index 42e454f86..47c2b92b7 100644
--- a/Bugzilla/Auth/Login/WWW/CGI.pm
+++ b/Bugzilla/Auth/Login/WWW/CGI.pm
@@ -184,7 +184,7 @@ sub login {
# If we get here, then we've run out of options, which shouldn't happen
ThrowCodeError("authres_unhandled", { authres => $authres,
- type => $type, });
+ type => $type });
}
# This auth style allows the user to log out.
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index a6758d36f..b2261e1ee 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -511,21 +511,18 @@ sub ValidateTime {
# (allow negatives, though, so people can back out errors in time reporting)
if ($time !~ /^-?(?:\d+(?:\.\d*)?|\.\d+)$/) {
ThrowUserError("number_not_numeric",
- {field => "$field", num => "$time"},
- "abort");
+ {field => "$field", num => "$time"});
}
# Only the "work_time" field is allowed to contain a negative value.
if ( ($time < 0) && ($field ne "work_time") ) {
ThrowUserError("number_too_small",
- {field => "$field", num => "$time", min_num => "0"},
- "abort");
+ {field => "$field", num => "$time", min_num => "0"});
}
if ($time > 99999.99) {
ThrowUserError("number_too_large",
- {field => "$field", num => "$time", max_num => "99999.99"},
- "abort");
+ {field => "$field", num => "$time", max_num => "99999.99"});
}
}
diff --git a/Bugzilla/Error.pm b/Bugzilla/Error.pm
index 4c6288a28..6eac5c94b 100644
--- a/Bugzilla/Error.pm
+++ b/Bugzilla/Error.pm
@@ -32,13 +32,15 @@ use Bugzilla::Util;
use Date::Format;
sub _throw_error {
- my ($name, $error, $vars, $unlock_tables) = @_;
+ my ($name, $error, $vars) = @_;
$vars ||= {};
$vars->{error} = $error;
- Bugzilla->dbh->bz_unlock_tables(UNLOCK_ABORT) if $unlock_tables;
+ # Make sure any locked tables are unlocked
+ # and the transaction is rolled back (if supported)
+ Bugzilla->dbh->bz_unlock_tables(UNLOCK_ABORT);
# If a writable data/errorlog exists, log error details there.
if (-w "data/errorlog") {
@@ -95,6 +97,10 @@ sub ThrowCodeError {
sub ThrowTemplateError {
my ($template_err) = @_;
+ # Make sure any locked tables are unlocked
+ # and the transaction is rolled back (if supported)
+ Bugzilla->dbh->bz_unlock_tables(UNLOCK_ABORT);
+
my $vars = {};
if (Bugzilla->batch) {
die("error: template error: $template_err");
@@ -149,16 +155,16 @@ Bugzilla::Error - Error handling utilities for Bugzilla
ThrowUserError("error_tag",
{ foo => 'bar' });
- # supplying "abort" to ensure tables are unlocked
- ThrowUserError("another_error_tag",
- { foo => 'bar' }, 'abort');
-
=head1 DESCRIPTION
Various places throughout the Bugzilla codebase need to report errors to the
user. The C<Throw*Error> family of functions allow this to be done in a
generic and localisable manner.
+These functions automatically unlock the database tables, if there were any
+locked. They will also roll back the transaction, if it is supported by
+the underlying DB.
+
=head1 FUNCTIONS
=over 4
@@ -170,12 +176,6 @@ of variables as a second argument. These are used by the
I<global/user-error.html.tmpl> template to format the error, using the passed
in variables as required.
-An optional third argument may be supplied. If present, the error
-handling code will unlock the database tables: it is a Bugzilla standard
-to provide the string "abort" as the argument value. In the long term,
-this argument will go away, to be replaced by transactional C<rollback>
-calls. There is no timeframe for doing so, however.
-
=item C<ThrowCodeError>
This function is used when an internal check detects an error of some sort.
diff --git a/CGI.pl b/CGI.pl
index 69ec8f64f..d650ea08e 100644
--- a/CGI.pl
+++ b/CGI.pl
@@ -112,7 +112,7 @@ sub CheckFormField (\%$;\@) {
$field = $fieldname;
}
- ThrowCodeError("illegal_field", { field => $field }, "abort");
+ ThrowCodeError("illegal_field", { field => $field });
}
}
@@ -213,7 +213,7 @@ sub CheckEmailSyntax {
my ($addr) = (@_);
my $match = Param('emailregexp');
if ($addr !~ /$match/ || $addr =~ /[\\\(\)<>&,;:"\[\] \t\r\n]/) {
- ThrowUserError("illegal_email_address", { addr => $addr }, 'abort');
+ ThrowUserError("illegal_email_address", { addr => $addr });
}
}
diff --git a/editclassifications.cgi b/editclassifications.cgi
index a390ed4c6..deeffdce7 100755
--- a/editclassifications.cgi
+++ b/editclassifications.cgi
@@ -300,12 +300,10 @@ if ($action eq 'update') {
if ($classification ne $classificationold) {
unless ($classification) {
- $dbh->bz_unlock_tables(UNLOCK_ABORT);
ThrowUserError("classification_not_specified")
}
if (TestClassification($classification)) {
- $dbh->bz_unlock_tables(UNLOCK_ABORT);
ThrowUserError("classification_already_exists", { name => $classification });
}
$sth = $dbh->prepare("UPDATE classifications
diff --git a/editcomponents.cgi b/editcomponents.cgi
index 12a25905d..d21518fce 100755
--- a/editcomponents.cgi
+++ b/editcomponents.cgi
@@ -66,7 +66,7 @@ sub CheckProduct ($)
# do we have a product?
unless ($prod) {
- ThrowUserError('product_not_specified');
+ ThrowUserError('product_not_specified');
exit;
}
@@ -585,7 +585,6 @@ if ($action eq 'update') {
if ($description ne $descriptionold) {
unless ($description) {
- $dbh->bz_unlock_tables(UNLOCK_ABORT);
ThrowUserError('component_blank_description',
{'name' => $componentold});
exit;
@@ -603,7 +602,6 @@ if ($action eq 'update') {
my $initialownerid = login_to_id($initialowner);
unless ($initialownerid) {
- $dbh->bz_unlock_tables(UNLOCK_ABORT);
ThrowUserError('component_need_valid_initialowner',
{'name' => $componentold});
exit;
@@ -621,7 +619,6 @@ if ($action eq 'update') {
if (Param('useqacontact') && $initialqacontact ne $initialqacontactold) {
my $initialqacontactid = login_to_id($initialqacontact);
if (!$initialqacontactid && $initialqacontact ne '') {
- $dbh->bz_unlock_tables(UNLOCK_ABORT);
ThrowUserError('component_need_valid_initialqacontact',
{'name' => $componentold});
exit;
@@ -638,13 +635,11 @@ if ($action eq 'update') {
if ($component ne $componentold) {
unless ($component) {
- $dbh->bz_unlock_tables(UNLOCK_ABORT);
ThrowUserError('component_must_have_a_name',
{'name' => $componentold});
exit;
}
if (TestComponent($product, $component)) {
- $dbh->bz_unlock_tables(UNLOCK_ABORT);
ThrowUserError('component_already_exists',
{'name' => $component});
exit;
diff --git a/editmilestones.cgi b/editmilestones.cgi
index 7364d4d06..7317e7220 100755
--- a/editmilestones.cgi
+++ b/editmilestones.cgi
@@ -59,14 +59,14 @@ sub CheckProduct ($)
# do we have a product?
unless ($product) {
- &::ThrowUserError('product_not_specified');
+ ThrowUserError('product_not_specified');
exit;
}
# Does it exist in the DB?
unless (TestProduct $product) {
- &::ThrowUserError('product_doesnt_exist',
- {'product' => $product});
+ ThrowUserError('product_doesnt_exist',
+ {'product' => $product});
exit;
}
}
@@ -506,12 +506,9 @@ if ($action eq 'update') {
my $stored_sortkey = $sortkey;
if ($sortkey != $sortkeyold) {
if (!detaint_natural($sortkey)) {
-
- $dbh->bz_unlock_tables(UNLOCK_ABORT);
ThrowUserError('milestone_sortkey_invalid',
{'name' => $milestone,
'sortkey' => $stored_sortkey});
-
exit;
}
@@ -532,12 +529,10 @@ if ($action eq 'update') {
if ($milestone ne $milestoneold) {
unless ($milestone) {
- $dbh->bz_unlock_tables(UNLOCK_ABORT);
ThrowUserError('milestone_blank_name');
exit;
}
if (TestMilestone($product, $milestone)) {
- $dbh->bz_unlock_tables(UNLOCK_ABORT);
ThrowUserError('milestone_already_exists',
{'name' => $milestone,
'product' => $product});
diff --git a/editusers.cgi b/editusers.cgi
index 87a8b69bd..8169d0b93 100755
--- a/editusers.cgi
+++ b/editusers.cgi
@@ -212,8 +212,7 @@ if ($action eq 'search') {
canSeeUser($otherUserID)
|| ThrowUserError('auth_failure', {reason => "not_visible",
action => "modify",
- object => "user"},
- 'abort');
+ object => "user"});
# Cleanups
my $login = trim($cgi->param('login') || '');
@@ -231,11 +230,10 @@ if ($action eq 'search') {
if ($login ne $loginold) {
# Validate, then trick_taint.
- $login || ThrowUserError('user_login_required', undef, 'abort');
+ $login || ThrowUserError('user_login_required');
CheckEmailSyntax($login);
is_available_username($login) || ThrowUserError('account_exists',
- {'email' => $login},
- 'abort');
+ {'email' => $login});
trick_taint($login);
push(@changedFields, 'login_name');
push(@values, $login);
@@ -493,19 +491,17 @@ if ($action eq 'search') {
'whine_events WRITE');
Param('allowuserdeletion')
- || ThrowUserError('users_deletion_disabled', undef, 'abort');
+ || ThrowUserError('users_deletion_disabled');
$editusers || ThrowUserError('auth_failure',
{group => "editusers",
action => "delete",
- object => "users"},
- 'abort');
+ object => "users"});
canSeeUser($otherUserID) || ThrowUserError('auth_failure',
{reason => "not_visible",
action => "delete",
- object => "user"},
- 'abort');
+ object => "user"});
productResponsibilities($otherUserID)
- && ThrowUserError('user_has_responsibility', undef, 'abort');
+ && ThrowUserError('user_has_responsibility');
Bugzilla->logout_user_by_id($otherUserID);
diff --git a/editversions.cgi b/editversions.cgi
index ee4a83d77..60f60057e 100755
--- a/editversions.cgi
+++ b/editversions.cgi
@@ -406,16 +406,13 @@ if ($action eq 'update') {
if ($version ne $versionold) {
unless ($version) {
- $dbh->bz_unlock_tables(UNLOCK_ABORT);
ThrowUserError('version_blank_name');
exit;
}
if (TestVersion($product,$version)) {
- $dbh->bz_unlock_tables(UNLOCK_ABORT);
ThrowUserError('version_already_exists',
{'name' => $version,
'product' => $product});
-
exit;
}
SendSQL("UPDATE bugs
diff --git a/globals.pl b/globals.pl
index e71493f6b..44bf7dc3e 100644
--- a/globals.pl
+++ b/globals.pl
@@ -613,11 +613,11 @@ sub ValidatePassword {
my ($password, $matchpassword) = @_;
if (length($password) < 3) {
- ThrowUserError("password_too_short", undef, 'abort');
+ ThrowUserError("password_too_short");
} elsif (length($password) > 16) {
- ThrowUserError("password_too_long", undef, 'abort');
+ ThrowUserError("password_too_long");
} elsif ((defined $matchpassword) && ($password ne $matchpassword)) {
- ThrowUserError("passwords_dont_match", undef, 'abort');
+ ThrowUserError("passwords_dont_match");
}
}
@@ -649,8 +649,7 @@ sub DBNameToIdAndCheck {
return $result;
}
- ThrowUserError("invalid_username",
- { name => $name }, "abort");
+ ThrowUserError("invalid_username", { name => $name });
}
sub get_classification_id {
diff --git a/post_bug.cgi b/post_bug.cgi
index 6b52b447d..d701a9172 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -318,8 +318,7 @@ if (UserInGroup("editbugs") && defined($::FORM{'dependson'})) {
}
ThrowUserError("dependency_loop_multi",
- { both => $both },
- "abort");
+ { both => $both });
}
}
my $tmp = $me;
@@ -378,14 +377,14 @@ foreach my $b (grep(/^bit-\d*$/, keys %::FORM)) {
if ($::FORM{$b}) {
my $v = substr($b, 4);
$v =~ /^(\d+)$/
- || ThrowCodeError("group_id_invalid", undef, "abort");
+ || ThrowCodeError("group_id_invalid");
if (!GroupIsActive($v)) {
# Prevent the user from adding the bug to an inactive group.
# Should only happen if there is a bug in Bugzilla or the user
# hacked the "enter bug" form since otherwise the UI
# for adding the bug to the group won't appear on that form.
$vars->{'bit'} = $v;
- ThrowCodeError("inactive_group", undef, "abort");
+ ThrowCodeError("inactive_group");
}
SendSQL("SELECT user_id FROM user_group_map
WHERE user_id = $::userid
diff --git a/process_bug.cgi b/process_bug.cgi
index ea2180c3c..1fb54d2a4 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -109,7 +109,7 @@ foreach my $field ("estimated_time", "work_time", "remaining_time") {
if (UserInGroup(Param('timetrackinggroup'))) {
my $wk_time = $::FORM{'work_time'};
if ($::FORM{'comment'} =~ /^\s*$/ && $wk_time && $wk_time != 0) {
- ThrowUserError('comment_required', undef, "abort");
+ ThrowUserError('comment_required');
}
}
@@ -241,7 +241,7 @@ if ((($::FORM{'id'} && $::FORM{'product'} ne $::oldproduct)
$vars->{'newvalue'} = $::FORM{'product'};
$vars->{'field'} = 'product';
$vars->{'privs'} = $PrivilegesRequired;
- ThrowUserError("illegal_change", $vars, "abort");
+ ThrowUserError("illegal_change", $vars);
}
CheckFormField(\%::FORM, 'product', \@::legal_product);
@@ -1232,7 +1232,7 @@ foreach my $id (@idlist) {
$vars->{'field'} = $col;
}
$vars->{'privs'} = $PrivilegesRequired;
- ThrowUserError("illegal_change", $vars, "abort");
+ ThrowUserError("illegal_change", $vars);
}
}
@@ -1251,13 +1251,13 @@ foreach my $id (@idlist) {
$vars->{'newvalue'} = "no keywords";
$vars->{'field'} = "keywords";
$vars->{'privs'} = $PrivilegesRequired;
- ThrowUserError("illegal_change", $vars, "abort");
+ ThrowUserError("illegal_change", $vars);
}
$oldhash{'product'} = get_product_name($oldhash{'product_id'});
if (!CanEditProductId($oldhash{'product_id'})) {
ThrowUserError("product_edit_denied",
- { product => $oldhash{'product'} }, "abort");
+ { product => $oldhash{'product'} });
}
if (defined $::FORM{'product'}
@@ -1265,7 +1265,7 @@ foreach my $id (@idlist) {
&& $::FORM{'product'} ne $oldhash{'product'}
&& !CanEnterProduct($::FORM{'product'})) {
ThrowUserError("entry_access_denied",
- { product => $::FORM{'product'} }, "abort");
+ { product => $::FORM{'product'} });
}
if ($requiremilestone) {
# musthavemilestoneonaccept applies only if at least two
@@ -1283,7 +1283,7 @@ foreach my $id (@idlist) {
# if musthavemilestoneonaccept == 1, then the target
# milestone must be different from the default one.
if ($value eq $defaultmilestone) {
- ThrowUserError("milestone_required", { bug_id => $id }, "abort");
+ ThrowUserError("milestone_required", { bug_id => $id });
}
}
}
@@ -1319,7 +1319,7 @@ foreach my $id (@idlist) {
next if $i eq "";
if ($id eq $i) {
- ThrowUserError("dependency_loop_single", undef, "abort");
+ ThrowUserError("dependency_loop_single");
}
if (!exists $seen{$i}) {
push(@{$deptree{$target}}, $i);
@@ -1363,8 +1363,7 @@ foreach my $id (@idlist) {
}
ThrowUserError("dependency_loop_multi",
- { both => $both },
- "abort");
+ { both => $both });
}
}
my $tmp = $me;
@@ -1573,6 +1572,7 @@ foreach my $id (@idlist) {
shift @oldlist;
} else {
if ($oldlist[0] != $newlist[0]) {
+ $dbh->bz_unlock_tables(UNLOCK_ABORT);
die "Error in list comparing code";
}
shift @oldlist;