From 96670fe82a38116ac10592a6ccbd34800f8ad9f8 Mon Sep 17 00:00:00 2001 From: Moritz Bunkus Date: Thu, 18 Aug 2016 10:04:24 +0200 Subject: [PATCH] =?utf8?q?=C2=BBwith=5Ftransaction=C2=AB=20anstelle=20von?= =?utf8?q?=20=C2=BBdo=5Ftransaction=C2=AB=20verwenden?= MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Es sollte so selten wie möglich »do_transaction« verwndet werden, damit man sich immer angewöhnt, »with_transaction« zu nutzen. Hintergründe und Unterschiede zwischen den beiden Funktionen sind in der Dokumentation von SL/DB.pm beschrieben. --- SL/BackgroundJob/CreatePeriodicInvoices.pm | 4 ++- SL/Controller/Buchungsgruppen.pm | 25 ++++++++------ SL/Controller/CustomerVendor.pm | 14 +++++--- SL/Controller/MassInvoiceCreatePrint.pm | 16 ++++----- SL/Controller/Order.pm | 13 ++++--- SL/Controller/RequirementSpec.pm | 7 ++-- SL/Controller/RequirementSpecItem.pm | 2 +- SL/Controller/RequirementSpecPart.pm | 4 +-- SL/Controller/RequirementSpecTextBlock.pm | 4 +-- SL/Controller/Taxzones.pm | 25 ++++++++------ SL/DB/Helper/ActsAsList.pm | 16 ++++----- SL/DB/Invoice.pm | 8 ++--- SL/DB/Object.pm | 40 ++++++++-------------- SL/DB/RequirementSpec.pm | 2 +- t/ar/ar.t | 7 ++-- 15 files changed, 95 insertions(+), 92 deletions(-) diff --git a/SL/BackgroundJob/CreatePeriodicInvoices.pm b/SL/BackgroundJob/CreatePeriodicInvoices.pm index 36dcb1ba7..22f9bf985 100644 --- a/SL/BackgroundJob/CreatePeriodicInvoices.pm +++ b/SL/BackgroundJob/CreatePeriodicInvoices.pm @@ -205,7 +205,7 @@ sub _create_periodic_invoice { my $order = $config->order; my $invoice; - if (!$self->{db_obj}->db->do_transaction(sub { + if (!$self->{db_obj}->db->with_transaction(sub { 1; # make Emacs happy $invoice = SL::DB::Invoice->new_from($order); @@ -254,6 +254,8 @@ sub _create_periodic_invoice { _log_msg("_create_invoice created for period start date $period_start_date id " . $invoice->id . " number " . $invoice->invnumber . " netamount " . $invoice->netamount . " amount " . $invoice->amount); # die $invoice->transaction_description; + + 1; })) { $::lxdebug->message(LXDebug->WARN(), "_create_invoice failed: " . join("\n", (split(/\n/, $self->{db_obj}->db->error))[0..2])); return undef; diff --git a/SL/Controller/Buchungsgruppen.pm b/SL/Controller/Buchungsgruppen.pm index daaf94f5c..4776bd640 100644 --- a/SL/Controller/Buchungsgruppen.pm +++ b/SL/Controller/Buchungsgruppen.pm @@ -85,12 +85,13 @@ sub action_delete { # allow deletion of unused Buchungsgruppen. Will fail, due to database # constraint, if Buchungsgruppe is connected to a part - my $db = $self->{config}->db; - $db->do_transaction(sub { - my $taxzone_charts = SL::DB::Manager::TaxzoneChart->get_all(where => [ buchungsgruppen_id => $self->config->id ]); - foreach my $taxzonechart ( @{$taxzone_charts} ) { $taxzonechart->delete }; - $self->config->delete(); - flash_later('info', $::locale->text('The booking group has been deleted.')); + $self->{config}->db->with_transaction(sub { + my $taxzone_charts = SL::DB::Manager::TaxzoneChart->get_all(where => [ buchungsgruppen_id => $self->config->id ]); + foreach my $taxzonechart ( @{$taxzone_charts} ) { $taxzonechart->delete }; + $self->config->delete(); + flash_later('info', $::locale->text('The booking group has been deleted.')); + + 1; }) || flash_later('error', $::locale->text('The booking group is in use and cannot be deleted.')); $self->redirect_to(action => 'list'); @@ -133,7 +134,7 @@ sub create_or_update { my @errors; my $db = $self->config->db; - $db->do_transaction( sub { + if (!$db->with_transaction(sub { $self->config->assign_attributes(%{ $params }); # assign description and inventory_accno_id @@ -169,9 +170,13 @@ sub create_or_update { $taxzone_chart->save; } } - } ) || die @errors ? join("\n", @errors) . "\n" : $db->error . "\n"; - # die with rollback of taxzone save if saving of any of the taxzone_charts fails - # only show the $db->error if we haven't already identified the likely error ourselves + + 1; + })) { + die @errors ? join("\n", @errors) . "\n" : $db->error . "\n"; + # die with rollback of taxzone save if saving of any of the taxzone_charts fails + # only show the $db->error if we haven't already identified the likely error ourselves + } flash_later('info', $is_new ? t8('The booking group has been created.') : t8('The booking group has been saved.')); $self->redirect_to(action => 'list'); diff --git a/SL/Controller/CustomerVendor.pm b/SL/Controller/CustomerVendor.pm index fdccdc9d7..b2e32e6fa 100644 --- a/SL/Controller/CustomerVendor.pm +++ b/SL/Controller/CustomerVendor.pm @@ -152,7 +152,7 @@ sub _save { my $db = $self->{cv}->db; - $db->do_transaction(sub { + $db->with_transaction(sub { my $cvs_by_nr; if ( $self->is_vendor() ) { if ( $self->{cv}->vendornumber ) { @@ -223,6 +223,8 @@ sub _save { $note->delete(cascade => 'delete'); } } + + 1; }) || die($db->error); } @@ -351,7 +353,7 @@ sub action_delete { $self->action_edit(); } else { - $db->do_transaction(sub { + $db->with_transaction(sub { $self->{cv}->delete(cascade => 1); my $snumbers = $self->is_vendor() ? 'vendornumber_'. $self->{cv}->vendornumber : 'customernumber_'. $self->{cv}->customernumber; @@ -379,7 +381,7 @@ sub action_delete_contact { SL::Helper::Flash::flash('error', $::locale->text('No contact selected to delete')); } else { - $db->do_transaction(sub { + $db->with_transaction(sub { if ( $self->{contact}->used ) { $self->{contact}->detach(); $self->{contact}->save(); @@ -388,6 +390,8 @@ sub action_delete_contact { $self->{contact}->delete(cascade => 1); SL::Helper::Flash::flash('info', $::locale->text('Contact deleted.')); } + + 1; }) || die($db->error); $self->{contact} = $self->_new_contact_object; @@ -405,7 +409,7 @@ sub action_delete_shipto { SL::Helper::Flash::flash('error', $::locale->text('No shipto selected to delete')); } else { - $db->do_transaction(sub { + $db->with_transaction(sub { if ( $self->{shipto}->used ) { $self->{shipto}->detach(); $self->{shipto}->save(cascade => 1); @@ -414,6 +418,8 @@ sub action_delete_shipto { $self->{shipto}->delete(cascade => 1); SL::Helper::Flash::flash('info', $::locale->text('Shipto deleted.')); } + + 1; }) || die($db->error); $self->{shipto} = SL::DB::Shipto->new(); diff --git a/SL/Controller/MassInvoiceCreatePrint.pm b/SL/Controller/MassInvoiceCreatePrint.pm index c65d3b5ec..b7212c780 100644 --- a/SL/Controller/MassInvoiceCreatePrint.pm +++ b/SL/Controller/MassInvoiceCreatePrint.pm @@ -56,9 +56,9 @@ sub action_create_invoices { } my $db = SL::DB::Invoice->new->db; + my @invoices; - if (!$db->do_transaction(sub { - my @invoices; + if (!$db->with_transaction(sub { foreach my $id (@sales_delivery_order_ids) { my $delivery_order = SL::DB::DeliveryOrder->new(id => $id)->load; @@ -66,17 +66,17 @@ sub action_create_invoices { push @invoices, $invoice; } - my $key = sprintf('%d-%d', Time::HiRes::gettimeofday()); - $::auth->set_session_value("MassInvoiceCreatePrint::ids-${key}" => [ map { $_->id } @invoices ]); - - flash_later('info', t8('The invoices have been created. They\'re pre-selected below.')); - $self->redirect_to(action => 'list_invoices', ids => $key); - 1; })) { $::lxdebug->message(LXDebug::WARN(), "Error: " . $db->error); $::form->error($db->error); } + + my $key = sprintf('%d-%d', Time::HiRes::gettimeofday()); + $::auth->set_session_value("MassInvoiceCreatePrint::ids-${key}" => [ map { $_->id } @invoices ]); + + flash_later('info', t8('The invoices have been created. They\'re pre-selected below.')); + $self->redirect_to(action => 'list_invoices', ids => $key); } sub action_list_invoices { diff --git a/SL/Controller/Order.pm b/SL/Controller/Order.pm index 8023aa2b9..480f8915b 100644 --- a/SL/Controller/Order.pm +++ b/SL/Controller/Order.pm @@ -893,9 +893,9 @@ sub _delete { my ($self) = @_; my $errors = []; - my $db = $self->order->db; + my $db = $self->order->db; - $db->do_transaction( + $db->with_transaction( sub { my @spoolfiles = grep { $_ } map { $_->spoolfile } @{ SL::DB::Manager::Status->get_all(where => [ trans_id => $self->order->id ]) }; $self->order->delete; @@ -915,12 +915,11 @@ sub _save { my ($self) = @_; my $errors = []; - my $db = $self->order->db; + my $db = $self->order->db; - $db->do_transaction( - sub { - SL::DB::OrderItem->new(id => $_)->delete for @{$self->item_ids_to_delete}; - $self->order->save(cascade => 1); + $db->with_transaction(sub { + SL::DB::OrderItem->new(id => $_)->delete for @{$self->item_ids_to_delete}; + $self->order->save(cascade => 1); }) || push(@{$errors}, $db->error); return $errors; diff --git a/SL/Controller/RequirementSpec.pm b/SL/Controller/RequirementSpec.pm index 38bb4460b..68969e61b 100644 --- a/SL/Controller/RequirementSpec.pm +++ b/SL/Controller/RequirementSpec.pm @@ -121,7 +121,7 @@ sub action_ajax_edit_time_and_cost_estimate { sub action_ajax_save_time_and_cost_estimate { my ($self) = @_; - $self->requirement_spec->db->do_transaction(sub { + $self->requirement_spec->db->with_transaction(sub { # Make Emacs happy 1; foreach my $attributes (@{ $::form->{requirement_spec_items} || [] }) { @@ -417,12 +417,13 @@ sub create_or_update { } my $db = $self->requirement_spec->db; - if (!$db->do_transaction(sub { + if (!$db->with_transaction(sub { if ($self->copy_source) { $self->requirement_spec($self->copy_source->create_copy(%{ $params })); } else { $self->requirement_spec->save(cascade => 1); } + 1; })) { $::lxdebug->message(LXDebug::WARN(), "Error: " . $db->error); @errors = ($::locale->text('Saving failed. Error message from the database: #1', $db->error)); @@ -635,7 +636,7 @@ sub update_project_link_create { return $self->js->error(@errors)->render if @errors; my $db = $self->requirement_spec->db; - if (!$db->do_transaction(sub { + if (!$db->with_transaction(sub { $project->save; $self->requirement_spec->update_attributes(project_id => $project->id); diff --git a/SL/Controller/RequirementSpecItem.pm b/SL/Controller/RequirementSpecItem.pm index b7512928e..d32d28481 100644 --- a/SL/Controller/RequirementSpecItem.pm +++ b/SL/Controller/RequirementSpecItem.pm @@ -87,7 +87,7 @@ sub action_dragged_and_dropped { my $old_type = $self->item->item_type; my $new_type = !$dropped_item ? 'section' : $position =~ m/before|after/ ? $dropped_item->item_type : $dropped_item->child_type; - $self->item->db->do_transaction(sub { + $self->item->db->with_transaction(sub { $self->item->remove_from_list; $self->item->parent_id($position =~ m/before|after/ ? $dropped_item->parent_id : $dropped_item->id) if $dropped_item; $self->item->item_type($new_type); diff --git a/SL/Controller/RequirementSpecPart.pm b/SL/Controller/RequirementSpecPart.pm index b9df7f545..57cfb49b3 100644 --- a/SL/Controller/RequirementSpecPart.pm +++ b/SL/Controller/RequirementSpecPart.pm @@ -73,7 +73,7 @@ sub action_ajax_save { my ($self) = @_; my $db = $self->requirement_spec->db; - $db->do_transaction(sub { + $db->with_transaction(sub { # Make Emacs happy 1; my $parts = $::form->{additional_parts} || []; @@ -81,8 +81,6 @@ sub action_ajax_save { $_->{position} = $position++ for @{ $parts }; $self->requirement_spec->update_attributes(parts => $parts)->load; - - 1; }) or do { return $self->js->error(t8('Saving failed. Error message from the database: #1', $db->error))->render; }; diff --git a/SL/Controller/RequirementSpecTextBlock.pm b/SL/Controller/RequirementSpecTextBlock.pm index 264b83b5a..055281907 100644 --- a/SL/Controller/RequirementSpecTextBlock.pm +++ b/SL/Controller/RequirementSpecTextBlock.pm @@ -184,7 +184,7 @@ sub action_dragged_and_dropped { my $dropped_type = $position ne 'last' ? undef : $::form->{dropped_type} =~ m/^ text-blocks- (?:front|back) $/x ? $::form->{dropped_type} : die "Unknown 'dropped_type' parameter"; my $old_where = $self->text_block->output_position; - $self->text_block->db->do_transaction(sub { + $self->text_block->db->with_transaction(sub { 1; $self->text_block->remove_from_list; $self->text_block->output_position($position =~ m/before|after/ ? $dropped_text_block->output_position : $::form->{dropped_type} eq 'text-blocks-front' ? 0 : 1); @@ -482,7 +482,7 @@ sub show_list { sub paste_picture { my ($self, $copied) = @_; - if (!$self->text_block->db->do_transaction(sub { + if (!$self->text_block->db->with_transaction(sub { 1; $self->picture($copied->to_object)->save; # Create new picture from copied data and save $self->text_block->add_pictures($self->picture); # Add new picture to text block diff --git a/SL/Controller/Taxzones.pm b/SL/Controller/Taxzones.pm index 56c9f6334..1594b9b37 100644 --- a/SL/Controller/Taxzones.pm +++ b/SL/Controller/Taxzones.pm @@ -72,12 +72,13 @@ sub action_delete { # allow deletion of unused tax zones. Will fail, due to database # constraints, if tax zone is used anywhere - my $db = $self->{config}->db; - $db->do_transaction(sub { - my $taxzone_charts = SL::DB::Manager::TaxzoneChart->get_all(where => [ taxzone_id => $self->config->id ]); - foreach my $taxzonechart ( @{$taxzone_charts} ) { $taxzonechart->delete }; - $self->config->delete(); - flash_later('info', $::locale->text('The tax zone has been deleted.')); + $self->{config}->db->with_transaction(sub { + my $taxzone_charts = SL::DB::Manager::TaxzoneChart->get_all(where => [ taxzone_id => $self->config->id ]); + foreach my $taxzonechart ( @{$taxzone_charts} ) { $taxzonechart->delete }; + $self->config->delete(); + flash_later('info', $::locale->text('The tax zone has been deleted.')); + + 1; }) || flash_later('error', $::locale->text('The tax zone is in use and cannot be deleted.')); $self->redirect_to(action => 'list'); @@ -121,7 +122,7 @@ sub create_or_update { my @errors; my $db = $self->config->db; - $db->do_transaction( sub { + if (!$db->with_transaction(sub { # always allow editing of description and obsolete $self->config->assign_attributes( %{$params} ) ; @@ -160,9 +161,13 @@ sub create_or_update { $taxzone_chart->save; } } - } ) || die @errors ? join("\n", @errors) . "\n" : $db->error . "\n"; - # die with rollback of taxzone save if saving of any of the taxzone_charts fails - # only show the $db->error if we haven't already identified the likely error ourselves + + 1; + })) { + die @errors ? join("\n", @errors) . "\n" : $db->error . "\n"; + # die with rollback of taxzone save if saving of any of the taxzone_charts fails + # only show the $db->error if we haven't already identified the likely error ourselves + } flash_later('info', $is_new ? t8('The taxzone has been created.') : t8('The taxzone has been saved.')); $self->redirect_to(action => 'list'); diff --git a/SL/DB/Helper/ActsAsList.pm b/SL/DB/Helper/ActsAsList.pm index f48e9967a..ee49f4aed 100644 --- a/SL/DB/Helper/ActsAsList.pm +++ b/SL/DB/Helper/ActsAsList.pm @@ -40,7 +40,7 @@ sub move_position_down { sub remove_from_list { my ($self) = @_; - my $worker = sub { + return $self->db->with_transaction(sub { remove_position($self); # Set to -1 manually because $self->update_attributes() would @@ -56,9 +56,7 @@ sub remove_from_list { SQL $self->db->dbh->do($sql, undef, $self->$primary_key_col); $self->$column(undef); - }; - - return $self->db->in_transaction ? $worker->() : $self->db->do_transaction($worker); + }); } sub add_to_list { @@ -109,12 +107,10 @@ sub add_to_list { ${group_by} SQL - my $worker = sub { + return $self->db->with_transaction(sub { $self->db->dbh->do($query, undef, $new_position - 1, @values); $self->update_attributes($column => $new_position); - }; - - return $self->db->in_transaction ? $worker->() : $self->db->do_transaction($worker); + }); } sub get_next_in_list { @@ -144,7 +140,7 @@ sub reorder_list { my $self = ref($class_or_self) ? $class_or_self : $class_or_self->new; my $column = column_name($self); - my $result = $self->db->do_transaction(sub { + my $result = $self->db->with_transaction(sub { my $query = qq|UPDATE | . $self->meta->table . qq| SET ${column} = ? WHERE id = ?|; my $sth = $self->db->dbh->prepare($query) || die $self->db->dbh->errstr; @@ -153,6 +149,8 @@ sub reorder_list { } $sth->finish; + + 1; }); return $result; diff --git a/SL/DB/Invoice.pm b/SL/DB/Invoice.pm index 121f7659c..fab4974f3 100644 --- a/SL/DB/Invoice.pm +++ b/SL/DB/Invoice.pm @@ -252,7 +252,7 @@ sub post { $params{ar_id} = $chart->id; } - my $worker = sub { + if (!$self->db->with_transaction(sub { my %data = $self->calculate_prices_and_taxes; $self->_post_create_assemblyitem_entries($data{assembly_items}); @@ -267,11 +267,9 @@ sub post { $self->_post_update_allocated($data{allocated}); $self->_post_book_rounding($data{rounding}); - }; - if ($self->db->in_transaction) { - $worker->(); - } elsif (!$self->db->do_transaction($worker)) { + 1; + })) { $::lxdebug->message(LXDebug->WARN(), "convert_to_invoice failed: " . join("\n", (split(/\n/, $self->db->error))[0..2])); return undef; } diff --git a/SL/DB/Object.pm b/SL/DB/Object.pm index fa474643a..4c3e18439 100755 --- a/SL/DB/Object.pm +++ b/SL/DB/Object.pm @@ -140,21 +140,15 @@ sub load { sub save { my ($self, @args) = @_; - my ($result, $exception); - my $worker = sub { - $exception = $EVAL_ERROR unless eval { - SL::DB::Object::Hooks::run_hooks($self, 'before_save'); - $result = $self->SUPER::save(@args); - SL::DB::Object::Hooks::run_hooks($self, 'after_save', $result); - 1; - }; + my $result; - return $result; - }; + $self->db->with_transaction(sub { + SL::DB::Object::Hooks::run_hooks($self, 'before_save'); + $result = $self->SUPER::save(@args); + SL::DB::Object::Hooks::run_hooks($self, 'after_save', $result); - $self->db->in_transaction ? $worker->() : $self->db->do_transaction($worker); - - die $exception if $exception; + 1; + }) || die $self->error; return $result; } @@ -162,21 +156,15 @@ sub save { sub delete { my ($self, @args) = @_; - my ($result, $exception); - my $worker = sub { - $exception = $EVAL_ERROR unless eval { - SL::DB::Object::Hooks::run_hooks($self, 'before_delete'); - $result = $self->SUPER::delete(@args); - SL::DB::Object::Hooks::run_hooks($self, 'after_delete', $result); - 1; - }; - - return $result; - }; + my $result; - $self->db->in_transaction ? $worker->() : $self->db->do_transaction($worker); + $self->db->with_transaction(sub { + SL::DB::Object::Hooks::run_hooks($self, 'before_delete'); + $result = $self->SUPER::delete(@args); + SL::DB::Object::Hooks::run_hooks($self, 'after_delete', $result); - die $exception if $exception; + 1; + }) || die $self->error; return $result; } diff --git a/SL/DB/RequirementSpec.pm b/SL/DB/RequirementSpec.pm index c35540c74..024e60d7b 100644 --- a/SL/DB/RequirementSpec.pm +++ b/SL/DB/RequirementSpec.pm @@ -145,7 +145,7 @@ sub create_copy { return $self->_create_copy(%params) if $self->db->in_transaction; my $copy; - if (!$self->db->do_transaction(sub { $copy = $self->_create_copy(%params) })) { + if (!$self->db->with_transaction(sub { $copy = $self->_create_copy(%params) })) { $::lxdebug->message(LXDebug->WARN(), "create_copy failed: " . join("\n", (split(/\n/, $self->db->error))[0..2])); return undef; } diff --git a/t/ar/ar.t b/t/ar/ar.t index 79fcc0a4a..7a97738ec 100644 --- a/t/ar/ar.t +++ b/t/ar/ar.t @@ -72,7 +72,7 @@ sub ar { my $db = $invoice->db; - $db->do_transaction( sub { + $db->with_transaction( sub { my $tax = SL::DB::Manager::Tax->find_by(taxkey => 0, rate => 0); @@ -91,6 +91,8 @@ sub ar { _save_and_pay_and_check(invoice => $invoice, bank => $bank, pay => 1, check => 1); + 1; + }) || die "something went wrong: " . $db->error; return $invoice->invnumber; }; @@ -119,7 +121,7 @@ sub ar_with_tax { my $db = $invoice->db; - $db->do_transaction( sub { + $db->with_transaction( sub { # TODO: check for currency and exchange rate @@ -140,6 +142,7 @@ sub ar_with_tax { $invoice->create_ar_row( chart => $ar_chart ); _save_and_pay_and_check(invoice => $invoice, bank => $bank, pay => 1, check => 1); + 1; }) || die "something went wrong: " . $db->error; return $invoice->invnumber; }; -- 2.20.1