From 51b71239fe9904ef1a5b347797483a24fab08914 Mon Sep 17 00:00:00 2001 From: Karen Etheridge Date: Fri, 4 Oct 2019 15:32:24 -0700 Subject: [PATCH] fix: log the same response that we are returning This discrepancy occurred in places where $c->status(..) or $c->rendered(..) was called more than once in a dispatch cycle. The after_dispatch hook is only called once, for the first call, but what is returned in the response reflects the last call. - refactor the txn_wrapper helper to not actually render a response, but just use the return value to signal the outcome; the inclusion of the first line of the exception in the response has moved to the status helper. Caught exceptions are now included in the dispatch log. - also added a check in the after_render hook to ensure this does not happen again. --- docs/modules/Conch::Plugin::Database.md | 12 ++++++++---- lib/Conch.pm | 5 +++++ lib/Conch/Controller/HardwareProduct.pm | 10 ++++------ lib/Conch/Controller/Rack.pm | 7 +++---- lib/Conch/Plugin/Database.pm | 18 ++++++++++++------ t/database.t | 5 +++-- 6 files changed, 35 insertions(+), 22 deletions(-) diff --git a/docs/modules/Conch::Plugin::Database.md b/docs/modules/Conch::Plugin::Database.md index a44a7656b..1c6b92485 100644 --- a/docs/modules/Conch::Plugin::Database.md +++ b/docs/modules/Conch::Plugin::Database.md @@ -37,16 +37,20 @@ my $result = $c->txn_wrapper(sub ($c) { # many update, delete queries etc... }); -# if the result code was already set, we errored and rolled back the db... -return if $c->res->code; +# if the result is false, we errored and rolled back the db... +return $c->status(400) if not $result; ``` Wraps the provided subref in a database transaction, rolling back in case of an exception. Any provided arguments are passed to the sub, along with the invocant controller. If the exception is not `'rollback'` (which signals an intentional premature bailout), the -exception will be logged, and a response will be set up as an error response with the first -line of the exception. +exception will be logged and stored in the stash, of which the first line will be included in +the response if no other response is prepared (see ["status" in Conch](/modules/Conch#status)). + +You should **not** render a response in the subref itself, as you will have a difficult time +figuring out afterwards whether `$c->rendered` still needs to be called or not. Instead, +use the subref's return value to signal success. # LICENSING diff --git a/lib/Conch.pm b/lib/Conch.pm index 29b1ec7d3..d0c0599e2 100644 --- a/lib/Conch.pm +++ b/lib/Conch.pm @@ -61,11 +61,16 @@ sub startup { ); $self->hook(after_render => sub ($c, @args) { + warn 'called $c->render twice' if $c->stash->{_rendered}++; + $c->tx->res->headers->add('X-Conch-API', $c->version_tag); }); $self->helper( status => sub ($c, $code, $payload = undef) { + $payload //= { error => (split(/\n/, $c->stash('exception'), 2))[0] } + if $code >= 400 and $code < 500 and $c->stash('exception'); + $payload //= { error => 'Unauthorized' } if $code == 401; $payload //= { error => 'Forbidden' } if $code == 403; $payload //= { error => 'Not Found' } if $code == 404; diff --git a/lib/Conch/Controller/HardwareProduct.pm b/lib/Conch/Controller/HardwareProduct.pm index 5a6823a95..bf19995fe 100644 --- a/lib/Conch/Controller/HardwareProduct.pm +++ b/lib/Conch/Controller/HardwareProduct.pm @@ -107,8 +107,7 @@ sub create ($c) { $c->db_hardware_products->create($input); }); - # if the result code was already set, we errored and rolled back the db.. - return if $c->res->code; + return $c->status(400) if not $hardware_product; $c->log->debug('Created hardware product id '.$hardware_product->id. ($input->{hardware_product_profile} @@ -174,10 +173,9 @@ sub update ($c) { $c->log->debug('Updated hardware product '.$hardware_product->id); $c->log->debug('transaction ended successfully'); - }); - - # if the result code was already set, we errored and rolled back the db.. - return if $c->res->code; + return 1; + }) + or return $c->status(400); $c->status(303, '/hardware_product/'.$hardware_product->id); } diff --git a/lib/Conch/Controller/Rack.pm b/lib/Conch/Controller/Rack.pm index 5aa125b93..2a5082a51 100644 --- a/lib/Conch/Controller/Rack.pm +++ b/lib/Conch/Controller/Rack.pm @@ -300,10 +300,9 @@ sub set_assignment ($c) { { key => 'primary' }, # only search for conflicts by device_id ); } - }); - - # if the result code was already set, we errored and rolled back the db... - return if $c->res->code; + return 1; + }) + or return $c->status(400); $c->log->debug('Updated device assignments for rack '.$c->stash('rack_id')); $c->status(303, '/rack/'.$c->stash('rack_id').'/assignment'); diff --git a/lib/Conch/Plugin/Database.pm b/lib/Conch/Plugin/Database.pm index 612816948..23c01af31 100644 --- a/lib/Conch/Plugin/Database.pm +++ b/lib/Conch/Plugin/Database.pm @@ -6,6 +6,7 @@ use Conch::DB (); use Lingua::EN::Inflexion 'noun'; use Try::Tiny; use Conch::DB::Util; +use Safe::Isa; =pod @@ -115,15 +116,19 @@ the C attribute (see L). # many update, delete queries etc... }); - # if the result code was already set, we errored and rolled back the db... - return if $c->res->code; + # if the result is false, we errored and rolled back the db... + return $c->status(400) if not $result; Wraps the provided subref in a database transaction, rolling back in case of an exception. Any provided arguments are passed to the sub, along with the invocant controller. If the exception is not C<'rollback'> (which signals an intentional premature bailout), the -exception will be logged, and a response will be set up as an error response with the first -line of the exception. +exception will be logged and stored in the stash, of which the first line will be included in +the response if no other response is prepared (see L). + +You should B render a response in the subref itself, as you will have a difficult time +figuring out afterwards whether C<< $c->rendered >> still needs to be called or not. Instead, +use the subref's return value to signal success. =cut @@ -138,8 +143,9 @@ line of the exception. $c->log->debug('rolled back transaction'); if ($exception !~ /^rollback/) { $c->log->error($exception); - my ($error) = split(/\n/, $exception, 2); - $c->status($c->res->code // 400, { error => $error }); + $c->stash('exception', + ($exception->$_isa('Mojo::Exception') ? $exception + : Mojo::Exception->new($exception))->inspect); } return; }; diff --git a/t/database.t b/t/database.t index d851c42fd..645567bf2 100644 --- a/t/database.t +++ b/t/database.t @@ -128,15 +128,16 @@ subtest 'transactions' => sub { $r->get( '/test_txn_wrapper2', sub ($c) { - $c->txn_wrapper(sub ($my_c, $id) { + my $user = $c->txn_wrapper(sub ($my_c, $id) { $my_c->db_user_accounts->create({ id => $id, name => 'new user', email => 'foo@bar', password => 'foo', }); - $my_c->status(204); }, $c->req->query_params->param('id')); + + $c->status($user ? 204 : 400); }, );