Feature/bts 616 (arangodb#14906)

Co-authored-by: Jan <jsteemann@users.noreply.github.com>
leonardoadame · Oct 25, 2021 · b055fce · b055fce
1 parent 144307e
commit b055fce
Show file tree

Hide file tree

Showing 16 changed files with 352 additions and 162 deletions.
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,6 +1,10 @@
 devel
 -----
 
+* BTS-616: Added support for client tools arangoimport, arangodump,
+  arangorestore, arangobench and arangoexport to handle HTTP responses when 
+  they are not in JSON format (e.g. text/html).   
+
 * Updated ArangoDB Starter to 0.15.3-preview-1.
 
 * Remove dysfunctional CMake variable `USE_OPTIMIZE_FOR_ARCHITECTURE`.
@@ -34,6 +38,7 @@ devel
 
 * Web UI - Added missing HTML escaping inside the file upload plugin used in the
   section of deploying a new Foxx application when uploading a zip file.
+
 * Now, arangoimport supports merging of attributes. When importing data from a
   file into a collection, a document attribute can be comprised of merging
   attributes from the file into it, with separators and other literal strings.

diff --git a/arangosh/Benchmark/BenchFeature.cpp b/arangosh/Benchmark/BenchFeature.cpp
@@ -56,6 +56,7 @@
 #include "ProgramOptions/ProgramOptions.h"
 #include "ProgramOptions/Section.h"
 #include "Shell/ClientFeature.h"
+#include "SimpleHttpClient/HttpResponseChecker.h"
 #include "SimpleHttpClient/SimpleHttpClient.h"
 #include "SimpleHttpClient/SimpleHttpResult.h"
 
@@ -275,23 +276,17 @@ void BenchFeature::start() {
     std::unordered_map<std::string, std::string> headers;
     headers.emplace(StaticStrings::ContentTypeHeader, StaticStrings::MimeTypeVPack);
 
-    auto result = createDbClient->request(rest::RequestType::POST,
+    std::unique_ptr<SimpleHttpResult> result(createDbClient->request(RequestType::POST,
                                           "/_api/database", b.slice().startAs<char>(),
-                                          b.slice().byteSize(), headers);
-
-    if (!result || result->wasHttpError()) {
-      std::string msg;
-      if (result) {
-        msg = result->getHttpReturnMessage();
-        delete result;
-      }
+                                          b.slice().byteSize(), headers));
 
+    auto check = arangodb::HttpResponseChecker::check(createDbClient->getErrorMessage(), result.get());
+    if (check.fail()) {
       LOG_TOPIC("5cda8", FATAL, arangodb::Logger::BENCH)
-          << "failed to create the specified database: " << msg;
+          << "failed to create the specified database: " << check.errorMessage();
       FATAL_ERROR_EXIT();
     }
-
-    delete result;
+
     client.setDatabaseName(connectDB);
   }
   int ret = EXIT_SUCCESS;

diff --git a/arangosh/Benchmark/BenchmarkThread.h b/arangosh/Benchmark/BenchmarkThread.h
@@ -47,6 +47,7 @@
 #include "Rest/HttpRequest.h"
 #include "Shell/ClientFeature.h"
 #include "SimpleHttpClient/GeneralClientConnection.h"
+#include "SimpleHttpClient/HttpResponseChecker.h"
 #include "SimpleHttpClient/SimpleHttpClient.h"
 #include "SimpleHttpClient/SimpleHttpResult.h"
 
@@ -160,10 +161,10 @@ class BenchmarkThread : public arangodb::Thread {
     // test the connection
     std::unique_ptr<httpclient::SimpleHttpResult> result(
         _httpClient->request(rest::RequestType::GET, "/_api/version", nullptr, 0, _headers));
-
-    if (!result || !result->isComplete()) {
+    auto check = arangodb::HttpResponseChecker::check(_httpClient->getErrorMessage(), result.get());
+    if (check.fail()) {
       LOG_TOPIC("5cda7", FATAL, arangodb::Logger::BENCH)
-          << "could not connect to server";
+          << check.errorMessage();
       FATAL_ERROR_EXIT();
     }
 
@@ -296,6 +297,7 @@ class BenchmarkThread : public arangodb::Thread {
     std::unique_ptr<httpclient::SimpleHttpResult> result(
         _httpClient->request(rest::RequestType::POST, "/_api/batch",
                              _payloadBuffer.data(), _payloadBuffer.size(), _headers));
+
     double delta = TRI_microtime() - start;
     trackTime(delta);
 
@@ -351,7 +353,8 @@ class BenchmarkThread : public arangodb::Thread {
     char const* type = (batch ? "batch" : "single");
     TRI_ASSERT(numOperations > 0);
 
-    if (result != nullptr && result->isComplete() && !result->wasHttpError()) {
+    auto check = arangodb::HttpResponseChecker::check(_httpClient->getErrorMessage(), result);
+    if (check.ok()) {
       if (batch) {
         // for batch requests we have to check the error header in addition
         auto const& headers = result->getHeaderFields();
@@ -375,14 +378,10 @@ class BenchmarkThread : public arangodb::Thread {
       _operationsCounter->incIncompleteFailures(numOperations);
     }
     if (++_warningCount < maxWarnings) {
-      if (result != nullptr && result->wasHttpError()) {
+      if (check.fail()) {
         LOG_TOPIC("fb835", WARN, arangodb::Logger::BENCH)
             << type << " request for URL '" << _requestData.url
-            << "' failed with HTTP code " << result->getHttpReturnCode() << ": "
-            << std::string(result->getBody().c_str(), result->getBody().length());
-      } else {
-        LOG_TOPIC("f5982", WARN, arangodb::Logger::BENCH)
-            << type << " operation failed because server did not reply";
+            << check.errorMessage();
       }
     } else if (_warningCount == maxWarnings) {
       LOG_TOPIC("6daf1", WARN, arangodb::Logger::BENCH)

diff --git a/arangosh/Dump/DumpFeature.cpp b/arangosh/Dump/DumpFeature.cpp
@@ -24,11 +24,8 @@
 
 #include "DumpFeature.h"
 
-#include <algorithm>
 #include <chrono>
-#include <thread>
 
-#include <velocypack/Builder.h>
 #include <velocypack/Collection.h>
 #include <velocypack/Iterator.h>
 #include <velocypack/velocypack-aliases.h>
@@ -38,24 +35,25 @@
 #include "Basics/FileUtils.h"
 #include "Basics/MutexLocker.h"
 #include "Basics/NumberOfCores.h"
-#include "Basics/files.h"
 #include "Basics/Result.h"
 #include "Basics/ScopeGuard.h"
 #include "Basics/StaticStrings.h"
 #include "Basics/StringUtils.h"
 #include "Basics/VelocyPackHelper.h"
 #include "Basics/application-exit.h"
+#include "Basics/files.h"
 #include "Basics/system-functions.h"
 #include "FeaturePhases/BasicFeaturePhaseClient.h"
 #include "Maskings/Maskings.h"
 #include "ProgramOptions/ProgramOptions.h"
 #include "Random/RandomGenerator.h"
 #include "Shell/ClientFeature.h"
+#include "SimpleHttpClient/HttpResponseChecker.h"
 #include "SimpleHttpClient/SimpleHttpClient.h"
 #include "SimpleHttpClient/SimpleHttpResult.h"
 #include "Ssl/SslInterface.h"
-#include "Utils/ManagedDirectory.h"
 #include "Utilities/NameValidator.h"
+#include "Utils/ManagedDirectory.h"
 
 namespace {
 
@@ -78,31 +76,6 @@ constexpr uint64_t MaxChunkSize = 1024 * 1024 * 96;
 const arangodb::Result ErrorMalformedJsonResponse = {
     TRI_ERROR_INTERNAL, "got malformed JSON response from server"};
 
-/// @brief check whether HTTP response is valid, complete, and not an error
-arangodb::Result checkHttpResponse(arangodb::httpclient::SimpleHttpClient& client,
-                                   std::unique_ptr<arangodb::httpclient::SimpleHttpResult> const& response) {
-  using arangodb::basics::StringUtils::concatT;
-  using arangodb::basics::StringUtils::itoa;
-  if (response == nullptr || !response->isComplete()) {
-    return {TRI_ERROR_INTERNAL,
-            "got invalid response from server: " + client.getErrorMessage()};
-  }
-  if (response->wasHttpError()) {
-    auto errorNum = static_cast<int>(TRI_ERROR_INTERNAL);
-    std::string errorMsg = response->getHttpReturnMessage();
-    std::shared_ptr<arangodb::velocypack::Builder> bodyBuilder(response->getBodyVelocyPack());
-    arangodb::velocypack::Slice error = bodyBuilder->slice();
-    if (!error.isNone() && error.hasKey(arangodb::StaticStrings::ErrorMessage)) {
-      errorNum = error.get(arangodb::StaticStrings::ErrorNum).getNumericValue<int>();
-      errorMsg = error.get(arangodb::StaticStrings::ErrorMessage).copyString();
-    }
-    auto err = ErrorCode{errorNum};
-    return {err, concatT("got invalid response from server: HTTP ",
-                         itoa(response->getHttpReturnCode()), ": ", errorMsg)};
-  }
-  return {};
-}
-
 /// @brief checks that a file pointer is valid and file status is ok
 bool fileOk(arangodb::ManagedDirectory::File* file) {
   return (file && file->status().ok());
@@ -128,7 +101,8 @@ std::pair<arangodb::Result, std::vector<std::string>> getDatabases(arangodb::htt
 
   std::unique_ptr<arangodb::httpclient::SimpleHttpResult> response(
       client.request(arangodb::rest::RequestType::GET, url, "", 0));
-  auto check = ::checkHttpResponse(client, response);
+  auto check = arangodb::HttpResponseChecker::check(client.getErrorMessage(), response.get());
+
   if (check.fail()) {
     LOG_TOPIC("47882", ERR, arangodb::Logger::DUMP)
         << "An error occurred while trying to determine list of databases: " << check.errorMessage();
@@ -184,7 +158,7 @@ std::pair<arangodb::Result, uint64_t> startBatch(arangodb::httpclient::SimpleHtt
 
   std::unique_ptr<arangodb::httpclient::SimpleHttpResult> response(
       client.request(arangodb::rest::RequestType::POST, url, body.c_str(), body.size()));
-  auto check = ::checkHttpResponse(client, response);
+  auto check = ::arangodb::HttpResponseChecker::check(client.getErrorMessage(), response.get());
   if (check.fail()) {
     LOG_TOPIC("34dbf", ERR, arangodb::Logger::DUMP)
         << "An error occurred while creating dump context: " << check.errorMessage();
@@ -327,7 +301,7 @@ arangodb::Result dumpCollection(arangodb::httpclient::SimpleHttpClient& client,
     // make the actual request for data
     std::unique_ptr<arangodb::httpclient::SimpleHttpResult> response(
         client.request(arangodb::rest::RequestType::GET, url, nullptr, 0, headers));
-    auto check = ::checkHttpResponse(client, response);
+    auto check = ::arangodb::HttpResponseChecker::check(client.getErrorMessage(), response.get());
     if (check.fail()) {
       LOG_TOPIC("ac972", ERR, arangodb::Logger::DUMP)
           << "An error occurred while dumping collection '" << name
@@ -812,7 +786,7 @@ Result DumpFeature::runDump(httpclient::SimpleHttpClient& client,
                             uint64_t batchId) {
   std::unique_ptr<httpclient::SimpleHttpResult> response(
       client.request(rest::RequestType::GET, baseUrl, nullptr, 0));
-  auto check = ::checkHttpResponse(client, response);
+  auto check = ::arangodb::HttpResponseChecker::check(client.getErrorMessage(), response.get());
   if (check.fail()) {
     LOG_TOPIC("eb7f4", ERR, arangodb::Logger::DUMP)
         << "An error occurred while fetching inventory: " << check.errorMessage();

diff --git a/arangosh/Export/ExportFeature.cpp b/arangosh/Export/ExportFeature.cpp
@@ -25,16 +25,17 @@
 
 #include "ApplicationFeatures/ApplicationServer.h"
 #include "Basics/FileUtils.h"
-#include "Basics/files.h"
 #include "Basics/ScopeGuard.h"
 #include "Basics/StaticStrings.h"
 #include "Basics/StringUtils.h"
 #include "Basics/application-exit.h"
+#include "Basics/files.h"
 #include "FeaturePhases/BasicFeaturePhaseClient.h"
 #include "Logger/Logger.h"
 #include "ProgramOptions/ProgramOptions.h"
 #include "Shell/ClientFeature.h"
 #include "SimpleHttpClient/GeneralClientConnection.h"
+#include "SimpleHttpClient/HttpResponseChecker.h"
 #include "SimpleHttpClient/SimpleHttpClient.h"
 #include "SimpleHttpClient/SimpleHttpResult.h"
 
@@ -572,25 +573,13 @@ std::shared_ptr<VPackBuilder> ExportFeature::httpCall(SimpleHttpClient* httpClie
       httpClient->request(requestType, url, postBody.c_str(), postBody.size()));
   _httpRequestsDone++;
 
-  if (response == nullptr || !response->isComplete()) {
-    LOG_TOPIC("c590f", FATAL, Logger::CONFIG) << "got invalid response from server: " + httpClient->getErrorMessage();
+  auto check = arangodb::HttpResponseChecker::check(httpClient->getErrorMessage(), response.get());
+  if (check.fail()) {
+    LOG_TOPIC("c590f", FATAL, Logger::CONFIG) << check.errorMessage();
     FATAL_ERROR_EXIT();
   }
-
-  std::shared_ptr<VPackBuilder> parsedBody;
 
-  if (response->wasHttpError()) {
-    parsedBody = response->getBodyVelocyPack();
-    arangodb::velocypack::Slice error = parsedBody->slice();
-    std::string errorMsg;
-    if (!error.isNone() && error.hasKey(arangodb::StaticStrings::ErrorMessage)) {
-      errorMsg = " - " + error.get(arangodb::StaticStrings::ErrorMessage).copyString();
-    }
-    //std::cout << parsedBody->toJson() << std::endl;
-    LOG_TOPIC("dbf58", FATAL, Logger::CONFIG) << "got invalid response from server: HTTP " +
-               StringUtils::itoa(response->getHttpReturnCode()) + ": " << response->getHttpReturnMessage() << errorMsg;
-    FATAL_ERROR_EXIT();
-  }
+  std::shared_ptr<VPackBuilder> parsedBody;
 
   try {
     parsedBody = response->getBodyVelocyPack();

diff --git a/arangosh/Import/ImportHelper.cpp b/arangosh/Import/ImportHelper.cpp
@@ -35,6 +35,7 @@
 #include "Logger/Logger.h"
 #include "Rest/GeneralResponse.h"
 #include "Shell/ClientFeature.h"
+#include "SimpleHttpClient/HttpResponseChecker.h"
 #include "SimpleHttpClient/SimpleHttpClient.h"
 #include "SimpleHttpClient/SimpleHttpResult.h"
 #include "Utils/ManagedDirectory.h"
@@ -1027,21 +1028,12 @@ bool ImportHelper::collectionExists() {
     return true;
   }
 
-  std::shared_ptr<velocypack::Builder> bodyBuilder(result->getBodyVelocyPack());
-  velocypack::Slice error = bodyBuilder->slice();
-  if (!error.isNone()) {
-    auto errorNum = error.get(StaticStrings::ErrorNum).getUInt();
-    auto errorMsg = error.get(StaticStrings::ErrorMessage).copyString();
+  auto check = arangodb::HttpResponseChecker::check(_httpClient->getErrorMessage(), result.get());
+
+  if (check.fail()) {
     LOG_TOPIC("f2c4a", ERR, arangodb::Logger::FIXME)
         << "unable to access collection '" << _collectionName
-        << "', server returned status code: " << static_cast<int>(code)
-        << "; error [" << errorNum << "] " << errorMsg;
-  } else {
-    LOG_TOPIC("57d57", ERR, arangodb::Logger::FIXME)
-        << "unable to accesss collection '" << _collectionName
-        << "', server returned status code: " << static_cast<int>(code)
-        << "; server returned message: "
-        << Logger::CHARS(result->getBody().c_str(), result->getBody().length());
+        << "', " << check.errorMessage();
   }
   return false;
 }
@@ -1081,21 +1073,11 @@ bool ImportHelper::checkCreateCollection() {
     return true;
   }
 
-  std::shared_ptr<velocypack::Builder> bodyBuilder(result->getBodyVelocyPack());
-  velocypack::Slice error = bodyBuilder->slice();
-  if (!error.isNone()) {
-    auto errorNum = error.get(StaticStrings::ErrorNum).getUInt();
-    auto errorMsg = error.get(StaticStrings::ErrorMessage).copyString();
+  auto check = arangodb::HttpResponseChecker::check(_httpClient->getErrorMessage(), result.get());
+  if (check.fail()) {
     LOG_TOPIC("09478", ERR, arangodb::Logger::FIXME)
         << "unable to create collection '" << _collectionName
-        << "', server returned status code: " << static_cast<int>(code)
-        << "; error [" << errorNum << "] " << errorMsg;
-  } else {
-    LOG_TOPIC("2211f", ERR, arangodb::Logger::FIXME)
-        << "unable to create collection '" << _collectionName
-        << "', server returned status code: " << static_cast<int>(code)
-        << "; server returned message: "
-        << Logger::CHARS(result->getBody().c_str(), result->getBody().length());
+        << "', " << check.errorMessage();
   }
   _hasError = true;
   return false;
@@ -1122,9 +1104,12 @@ bool ImportHelper::truncateCollection() {
     return true;
   }
 
-  LOG_TOPIC("f8ae4", ERR, arangodb::Logger::FIXME)
-      << "unable to truncate collection '" << _collectionName
-      << "', server returned status code: " << static_cast<int>(code);
+  auto check = arangodb::HttpResponseChecker::check(_httpClient->getErrorMessage(), result.get());
+  if (check.fail()) {
+    LOG_TOPIC("f8ae4", ERR, arangodb::Logger::FIXME)
+        << "unable to truncate collection '" << _collectionName << "', "
+        << check.errorMessage();
+  }
   _hasError = true;
   _errorMessages.push_back("Unable to overwrite collection");
   return false;

diff --git a/arangosh/Import/SenderThread.cpp b/arangosh/Import/SenderThread.cpp
@@ -166,6 +166,9 @@ void SenderThread::handleResult(httpclient::SimpleHttpResult* result) {
     haveBody = true;
   } catch (...) {
     // no body, likely error situation
+    _errorMessage = result->getHttpReturnMessage();
+    // will trigger the waiting ImportHelper thread to cancel the import
+    _hasError = true;
   }
 
   if (haveBody) {