do not swallow error messages when transactions fail (#3836)

UnitTests/HttpInterface/api-transactions-noncluster-spec.rb

@@ -362,7 +362,7 @@ describe ArangoDB do
         doc.headers['content-type'].should eq("application/json; charset=utf-8")
         doc.parsed_response['error'].should eq(true)
         doc.parsed_response['code'].should eq(500)
-        doc.parsed_response['errorNum'].should eq(1650) #internal transactin error
+        doc.parsed_response['errorNum'].should eq(1650) #internal transaction error
       end
     end
 

arangod/Transaction/Methods.cpp

@@ -724,7 +724,7 @@ Result transaction::Methods::commit() {
 
   if (_state == nullptr || _state->status() != transaction::Status::RUNNING) {
     // transaction not created or not running
-    return Result(TRI_ERROR_TRANSACTION_INTERNAL);
+    return Result(TRI_ERROR_TRANSACTION_INTERNAL, "transaction not running on commit");
   }
   
   ExecContext const* exe = ExecContext::CURRENT;
@@ -751,7 +751,7 @@ Result transaction::Methods::commit() {
 Result transaction::Methods::abort() {
   if (_state == nullptr || _state->status() != transaction::Status::RUNNING) {
     // transaction not created or not running
-    return TRI_ERROR_TRANSACTION_INTERNAL;
+    return Result(TRI_ERROR_TRANSACTION_INTERNAL, "transaction not running on abort");
   }
 
   CallbackInvoker invoker(this);
@@ -769,16 +769,7 @@ Result transaction::Methods::abort() {
 
 /// @brief finish a transaction (commit or abort), based on the previous state
 Result transaction::Methods::finish(int errorNum) {
-  if (errorNum == TRI_ERROR_NO_ERROR) {
+  return finish(Result(errorNum));
-    // there was no previous error, so we'll commit
-    return this->commit();
-  }
-
-  // there was a previous error, so we'll abort
-  this->abort();
-
-  // return original error number
-  return errorNum;
 }
 
 /// @brief finish a transaction (commit or abort), based on the previous state
@@ -2844,7 +2835,7 @@ bool transaction::Methods::isLocked(LogicalCollection* document,
 Result transaction::Methods::lock(TRI_voc_cid_t cid,
                                   AccessMode::Type type) {
   if (_state == nullptr || _state->status() != transaction::Status::RUNNING) {
-    return TRI_ERROR_TRANSACTION_INTERNAL;
+    return Result(TRI_ERROR_TRANSACTION_INTERNAL, "transaction not running on lock");
   }
   TransactionCollection* trxColl = trxCollection(cid, type);
   TRI_ASSERT(trxColl != nullptr);
@@ -2855,7 +2846,7 @@ Result transaction::Methods::lock(TRI_voc_cid_t cid,
 Result transaction::Methods::unlock(TRI_voc_cid_t cid,
                                     AccessMode::Type type) {
   if (_state == nullptr || _state->status() != transaction::Status::RUNNING) {
-    return TRI_ERROR_TRANSACTION_INTERNAL;
+    return Result(TRI_ERROR_TRANSACTION_INTERNAL, "transaction not running on unlock");
   }
   TransactionCollection* trxColl = trxCollection(cid, type);
   TRI_ASSERT(trxColl != nullptr);

arangod/VocBase/Methods/Transactions.cpp

@@ -21,7 +21,6 @@
 
 namespace arangodb {
 
-
 Result executeTransaction(
     v8::Isolate* isolate,
     basics::ReadWriteLock& lock,
@@ -314,6 +313,7 @@ Result executeTransactionJS(
                                             trxOptions));
     
   rv = trx->begin();
+  
   if (rv.fail()) {
     return rv;
   }
@@ -323,9 +323,12 @@ Result executeTransactionJS(
     result = action->Call(current, 1, &arguments);
     if (tryCatch.HasCaught()) {
       trx->abort();
-      std::tuple<bool,bool,Result> rvTuple = extractArangoError(isolate, tryCatch);
+      std::tuple<bool, bool, Result> rvTuple = extractArangoError(isolate, tryCatch, TRI_ERROR_TRANSACTION_INTERNAL);
       if (std::get<1>(rvTuple)) {
         rv = std::get<2>(rvTuple);
+      } else {
+        // some general error we don't know about
+        rv = Result(TRI_ERROR_TRANSACTION_INTERNAL, "an unknown error occured while executing the transaction");  
       }
     }
   } catch (arangodb::basics::Exception const& ex) {
@@ -342,8 +345,7 @@ Result executeTransactionJS(
     return rv;
   }
 
-  rv = trx->commit();
+  return trx->commit();
-  return rv;
 }
 
 } // arangodb

arangosh/Benchmark/test-cases.h

@@ -1172,7 +1172,7 @@ struct TransactionCountTest : public BenchmarkOperation {
     TRI_AppendStringStringBuffer(buffer,
                                  "\\\"]; var startcount = c.count(); for (var "
                                  "i = 0; i < 50; ++i) { if (startcount + i !== "
-                                 "c.count()) { throw \\\"error\\\"; } c.save({ "
+                                 "c.count()) { throw \\\"error, counters deviate!\\\"; } c.save({ "
                                  "}); } }\" }");
 
     *length = TRI_LengthStringBuffer(buffer);
@@ -1318,7 +1318,7 @@ struct TransactionMultiTest : public BenchmarkOperation {
                                    "var r1 = 0; c1.toArray().forEach(function "
                                    "(d) { r1 += d.count }); var r2 = "
                                    "c2.document(\\\"sum\\\").count; if (r1 !== "
-                                   "r2) { throw \\\"error\\\"; }");
+                                   "r2) { throw \\\"error, counters deviate!\\\"; }");
     }
 
     TRI_AppendStringStringBuffer(buffer, " }\" }");

js/server/tests/aql/aql-query-cache-noncluster.js

@@ -1,5 +1,5 @@
 /*jshint globalstrict:false, strict:false, maxlen: 500 */
-/*global fail, assertEqual, assertTrue, assertFalse, AQL_EXECUTE, 
+/*global fail, assertEqual, assertTrue, assertFalse, assertMatch, AQL_EXECUTE, 
   AQL_QUERY_CACHE_PROPERTIES, AQL_QUERY_CACHE_INVALIDATE */
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -951,7 +951,7 @@ function ahuacatlQueryCacheTestSuite () {
         });
         fail();
       } catch (err) {
-        assertEqual("peng!", String(err));
+        assertMatch(/peng!/, String(err));
       }
             
       var result = AQL_EXECUTE(query, { "@collection": c1.name() });

lib/V8/v8-helper.h

@@ -24,9 +24,12 @@
 #ifndef ARANGODB_V8_V8__HELPER_H
 #define ARANGODB_V8_V8__HELPER_H 1
 
-#include <v8.h>
 #include "v8-globals.h"
 #include "V8/v8-conv.h"
+#include "V8/v8-utils.h"
+
+#include <v8.h>
+
 namespace arangodb {
 
 inline std::string stringify(v8::Isolate* isolate, v8::Handle<v8::Value> value) {
@@ -87,7 +90,7 @@ inline bool isContextCanceled(v8::Isolate* isolate){
   return v8g->_canceled;
 }
 
-inline std::tuple<bool,bool,Result> extractArangoError(v8::Isolate* isolate, v8::TryCatch& tryCatch){
+inline std::tuple<bool, bool, Result> extractArangoError(v8::Isolate* isolate, v8::TryCatch& tryCatch, int errorCode) {
   // function tries to receive arango error form tryCatch Object
   // return tuple:
   //   bool - can continue
@@ -100,23 +103,42 @@ inline std::tuple<bool,bool,Result> extractArangoError(v8::Isolate* isolate, v8:
 
   if (!tryCatch.CanContinue()) {
     std::get<0>(rv) = false;
+    std::get<1>(rv) = true;
+    std::get<2>(rv).reset(TRI_ERROR_REQUEST_CANCELED);
     TRI_GET_GLOBALS();
     v8g->_canceled = true;
+    return rv;
   }
 
   v8::Handle<v8::Value> exception = tryCatch.Exception();
+  if (exception->IsString()) {
+    // the error is a plain string
+    std::string errorMessage = *v8::String::Utf8Value(exception->ToString());
+    std::get<1>(rv) = true;
+    std::get<2>(rv).reset(errorCode, errorMessage);
+    tryCatch.Reset();
+    return rv;
+  }
+
   if (!exception->IsObject()) {
+    // we have no idea what this error is about
+    std::get<1>(rv) = true;
+    TRI_Utf8ValueNFC exception(tryCatch.Exception());
+    char const* exceptionString = *exception;
+    if (exceptionString == nullptr) {
+      std::get<2>(rv).reset(errorCode, "JavaScript exception");
+    } else {
+      std::get<2>(rv).reset(errorCode, exceptionString);
+    }
     return rv;
   }
 
   v8::Handle<v8::Object> object = v8::Handle<v8::Object>::Cast(exception);
 
   try {
-
     if (object->Has(TRI_V8_ASCII_STRING(isolate, "errorNum")) &&
         object->Has(TRI_V8_ASCII_STRING(isolate, "errorMessage"))
-      )
+       ) {
-    {
       int errorNum = static_cast<int>(TRI_ObjectToInt64(object->Get(TRI_V8_ASCII_STRING(isolate, "errorNum"))));
       std::string errorMessage = *v8::String::Utf8Value(object->Get(TRI_V8_ASCII_STRING(isolate, "errorMessage")));
       std::get<1>(rv) = true;
@@ -127,8 +149,7 @@ inline std::tuple<bool,bool,Result> extractArangoError(v8::Isolate* isolate, v8:
 
     if (object->Has(TRI_V8_ASCII_STRING(isolate, "name")) &&
         object->Has(TRI_V8_ASCII_STRING(isolate, "message"))
-      )
+       ) {
-    {
       std::string  name = *v8::String::Utf8Value(object->Get(TRI_V8_ASCII_STRING(isolate, "name")));
       std::string  message = *v8::String::Utf8Value(object->Get(TRI_V8_ASCII_STRING(isolate, "message")));
       if(name == "TypeError"){