fix some tests, and remove legacy API response

arangod/Aql/AqlItemBlock.cpp

@@ -580,8 +580,6 @@ void AqlItemBlock::toVelocyPack(transaction::Methods* trx, VPackBuilder& result)
   result.add("nrItems", VPackValue(_nrItems));
   result.add("nrRegs", VPackValue(_nrRegs));
   result.add("error", VPackValue(false));
-  // Backwards compatbility 3.3
-  result.add("exhausted", VPackValue(false));
 
   enum State {
     Empty,       // saw an empty value

arangod/Aql/DistributeConsumerNode.h

@@ -80,12 +80,12 @@ class DistributeConsumerNode : public ExecutionNode {
     // This node is not allowed to be cloned.
     // Clone specialization!
     TRI_ASSERT(false);
-    THROW_ARANGO_EXCEPTION(TRI_ERROR_INTERNAL);
+    THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "DistributeConsumerNode cannot be cloned");
   }
 
   CostEstimate estimateCost() const override {
     TRI_ASSERT(false);
-    THROW_ARANGO_EXCEPTION(TRI_ERROR_INTERNAL);
+    THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "DistributeConsumerNode cannot be estimated");
   }
 
   void cloneRegisterPlan(ScatterNode* dependency);

arangod/Aql/EngineInfoContainerCoordinator.cpp

@@ -88,19 +88,22 @@ Result EngineInfoContainerCoordinator::EngineInfo::buildEngine(
   // For _id == 0 this thread will always maintain the handle to
   // the engine and will clean up. We do not keep track of it seperately
   if (_id != 0) {
+    coordinatorQueryIds.emplace_back(_id);
+
     double ttl = query.queryOptions().ttl;
     TRI_ASSERT(ttl > 0);
     try {
       queryRegistry->insert(_id, &query, ttl, true, false);
     } catch (basics::Exception const& e) {
+      coordinatorQueryIds.pop_back();
       return {e.code(), e.message()};
     } catch (std::exception const& e) {
+      coordinatorQueryIds.pop_back();
       return {TRI_ERROR_INTERNAL, e.what()};
     } catch (...) {
-      return {TRI_ERROR_INTERNAL};
+      coordinatorQueryIds.pop_back();
+      return {TRI_ERROR_INTERNAL, "unable to store query in registry"};
     }
-
-    coordinatorQueryIds.emplace_back(_id);
   }
 
   return {TRI_ERROR_NO_ERROR};

arangod/Aql/ExecutionEngine.cpp

@@ -589,12 +589,11 @@ std::pair<ExecutionState, size_t> ExecutionEngine::skipSome(size_t atMost) {
 }
 
 Result ExecutionEngine::shutdownSync(int errorCode) noexcept {
-  Result res{TRI_ERROR_INTERNAL};
+  Result res{TRI_ERROR_INTERNAL, "unable to shutdown query"};
   ExecutionState state = ExecutionState::WAITING;
   try {
     std::shared_ptr<SharedQueryState> sharedState = _query.sharedState();
     if (sharedState != nullptr) {
-
       while (state == ExecutionState::WAITING) {
         std::tie(state, res) = shutdown(errorCode);
         if (state == ExecutionState::WAITING) {
@@ -602,6 +601,8 @@ Result ExecutionEngine::shutdownSync(int errorCode) noexcept {
         }
       }
     }
+  } catch (std::exception const& ex) {
+    res.reset(TRI_ERROR_INTERNAL, ex.what());
   } catch (...) {
     res.reset(TRI_ERROR_INTERNAL);
   }

arangod/Aql/InputAqlItemRow.cpp

@@ -134,8 +134,6 @@ void InputAqlItemRow::toVelocyPack(transaction::Methods* trx, VPackBuilder& resu
   result.add("nrItems", VPackValue(1));
   result.add("nrRegs", VPackValue(getNrRegisters()));
   result.add("error", VPackValue(false));
-  // Backwards compatbility 3.3
-  result.add("exhausted", VPackValue(false));
 
   enum State {
     Empty,       // saw an empty value

arangod/Aql/RemoteExecutor.cpp

@@ -104,7 +104,7 @@ std::pair<ExecutionState, SharedAqlItemBlockPtr> ExecutionBlockImpl<RemoteExecut
   // For every call we simply forward via HTTP
   if (_lastError.fail()) {
     TRI_ASSERT(_lastResponse == nullptr);
-    Result res = _lastError;
+    Result res = std::move(_lastError);
     _lastError.reset();
     // we were called with an error need to throw it.
     THROW_ARANGO_EXCEPTION(res);
@@ -236,7 +236,7 @@ std::pair<ExecutionState, Result> ExecutionBlockImpl<RemoteExecutor>::initialize
     InputAqlItemRow const& input) {
   // For every call we simply forward via HTTP
   if (_requestInFlight.load(std::memory_order_acquire)) {
-    return {ExecutionState::WAITING, 0};
+    return {ExecutionState::WAITING, TRI_ERROR_NO_ERROR};
   }
 
   if (!_isResponsibleForInitializeCursor) {
@@ -259,11 +259,22 @@ std::pair<ExecutionState, Result> ExecutionBlockImpl<RemoteExecutor>::initialize
     auto response = std::move(_lastResponse);
 
     // Result is the response which is an object containing the ErrorCode
+    int errorNumber = TRI_ERROR_INTERNAL; // default error code
     VPackSlice slice = response->slice();
-    if (slice.hasKey("code")) {
-      return {ExecutionState::DONE, slice.get("code").getNumericValue<int>()};
+    VPackSlice errorSlice = slice.get(StaticStrings::ErrorNum);
+    if (!errorSlice.isNumber()) {
+      errorSlice = slice.get(StaticStrings::Code);
     }
-    return {ExecutionState::DONE, TRI_ERROR_INTERNAL};
+    if (errorSlice.isNumber()) {
+      errorNumber = errorSlice.getNumericValue<int>();
+    }
+
+    std::string errorMessage;
+    errorSlice = slice.get(StaticStrings::ErrorMessage);
+    if (errorSlice.isString()) {
+      return {ExecutionState::DONE, {errorNumber, errorSlice.copyString()}};
+    }
+    return {ExecutionState::DONE, errorNumber};
   }
 
   VPackOptions options(VPackOptions::Defaults);
@@ -274,13 +285,10 @@ std::pair<ExecutionState, Result> ExecutionBlockImpl<RemoteExecutor>::initialize
   VPackBuilder builder(buffer, &options);
   builder.openObject(/*unindexed*/ true);
 
-  // Backwards Compatibility 3.3
-  // NOTE: Removing this breaks tests in current devel - is this really for
-  // bc only?
-  builder.add("exhausted", VPackValue(false));
   // Used in 3.4.0 onwards
   builder.add("done", VPackValue(false));
-  builder.add("error", VPackValue(false));
+  builder.add(StaticStrings::Code, VPackValue(TRI_ERROR_NO_ERROR));
+  builder.add(StaticStrings::Error, VPackValue(false));
   // NOTE API change. Before all items have been send.
   // Now only the one output row is send.
   builder.add("pos", VPackValue(0));
@@ -338,7 +346,7 @@ std::pair<ExecutionState, Result> ExecutionBlockImpl<RemoteExecutor>::shutdown(i
     _didReceiveShutdownRequest = true;
     
     TRI_ASSERT(_lastResponse == nullptr);
-    Result res = _lastError;
+    Result res = std::move(_lastError);
     _lastError.reset();
 
     if (res.is(TRI_ERROR_QUERY_NOT_FOUND)) {

arangod/Aql/RestAqlHandler.cpp

@@ -674,6 +674,7 @@ RestStatus RestAqlHandler::handleUseQuery(std::string const& operation, Query* q
         }
         // Used in 3.4.0 onwards.
         answerBuilder.add("done", VPackValue(state == ExecutionState::DONE));
+        answerBuilder.add(StaticStrings::Code, VPackValue(TRI_ERROR_NO_ERROR));
         if (items.get() == nullptr) {
           // Backwards Compatibility
           answerBuilder.add(StaticStrings::Error, VPackValue(false));
@@ -734,7 +735,7 @@ RestStatus RestAqlHandler::handleUseQuery(std::string const& operation, Query* q
         answerBuilder.add(StaticStrings::Code, VPackValue(res.errorNumber()));
       } else if (operation == "shutdown") {
         int errorCode =
-            VelocyPackHelper::getNumericValue<int>(querySlice, "code", TRI_ERROR_INTERNAL);
+            VelocyPackHelper::readNumericValue<int>(querySlice, StaticStrings::Code, TRI_ERROR_INTERNAL);
 
         ExecutionState state;
         Result res;

arangod/Aql/RestAqlHandler.h

@@ -71,8 +71,8 @@ class RestAqlHandler : public RestVocbaseBaseHandler {
   //   "number": must be a positive integer, the cursor skips as many items,
   //             possibly exhausting the cursor.
   //             The result is a JSON with the attributes "error" (boolean),
-  //             "errorMessage" (if applicable) and "exhausted" (boolean) [3.3
-  //             and earlier] "done" (boolean) [3.4.0 and later] to indicate
+  //             "errorMessage" (if applicable) and
+  //             "done" (boolean) [3.4.0 and later] to indicate
   //             whether or not the cursor is exhausted.
   RestStatus useQuery(std::string const& operation, std::string const& idString);