Merge pull request #907 from joyent/ether/v2-txn_wrapper

fix: log the same response that we are returning

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
 

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;

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);
 }

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');

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<alias> attribute (see L<DBIx::Class::ResultSet/alias>).
         # 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<Conch/status>).
+
+You should B<not> 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;
         };

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);
         },
     );