From ee422b982f10413884fcef7a39d6a07039aae938 Mon Sep 17 00:00:00 2001
From: Sergey Yakubov <sergey.yakubov@desy.de>
Date: Tue, 30 Jan 2018 11:32:27 +0100
Subject: [PATCH] refactoring
---
CMakeModules/testing_go.cmake | 2 +-
broker/src/hidra2_broker/server/get_next.go | 6 ++--
.../src/hidra2_broker/server/get_next_test.go | 14 ++++-----
broker/src/hidra2_broker/server/listroutes.go | 2 +-
common/cpp/include/common/data_structs.h | 1 +
common/cpp/src/data_structs/data_structs.cpp | 7 +++++
tests/broker/get_next/check_linux.sh | 6 ++--
tests/broker/get_next/check_windows.bat | 6 ++--
worker/api/cpp/CMakeLists.txt | 2 +-
.../src/{io_map.cpp => broker_helpers.cpp} | 2 +-
.../cpp/src/{io_map.h => broker_helpers.h} | 0
worker/api/cpp/src/folder_data_broker.cpp | 13 +++-----
worker/api/cpp/src/server_data_broker.cpp | 31 +++++++++----------
worker/api/cpp/src/server_data_broker.h | 1 +
.../api/cpp/unittests/test_folder_broker.cpp | 10 ------
.../api/cpp/unittests/test_server_broker.cpp | 17 +++++-----
16 files changed, 57 insertions(+), 63 deletions(-)
rename worker/api/cpp/src/{io_map.cpp => broker_helpers.cpp} (96%)
rename worker/api/cpp/src/{io_map.h => broker_helpers.h} (100%)
diff --git a/CMakeModules/testing_go.cmake b/CMakeModules/testing_go.cmake
index 715aab8fa..3fcdb6e50 100644
--- a/CMakeModules/testing_go.cmake
+++ b/CMakeModules/testing_go.cmake
@@ -3,7 +3,7 @@ if (BUILD_TESTS)
endif ()
if (BUILD_TESTS)
- set(HIDRA2_MINIMUM_COVERAGE 90)
+ set(HIDRA2_MINIMUM_COVERAGE 80)
find_program(MEMORYCHECK_COMMAND valgrind)
set(MEMORYCHECK_COMMAND_OPTIONS
"--trace-children=yes --leak-check=full --error-exitcode=1 --suppressions=${CMAKE_SOURCE_DIR}/tests/valgrind.suppressions")
diff --git a/broker/src/hidra2_broker/server/get_next.go b/broker/src/hidra2_broker/server/get_next.go
index af1017798..efdcf4074 100644
--- a/broker/src/hidra2_broker/server/get_next.go
+++ b/broker/src/hidra2_broker/server/get_next.go
@@ -1,14 +1,16 @@
package server
import (
+ "github.com/gorilla/mux"
"hidra2_broker/database"
"hidra2_broker/utils"
"net/http"
)
func extractRequestParameters(r *http.Request) (string, bool) {
- db_name := r.URL.Query().Get("database")
- return db_name, db_name != ""
+ vars := mux.Vars(r)
+ db_name, ok := vars["dbname"]
+ return db_name, ok
}
func routeGetNext(w http.ResponseWriter, r *http.Request) {
diff --git a/broker/src/hidra2_broker/server/get_next_test.go b/broker/src/hidra2_broker/server/get_next_test.go
index 1f706218c..ba177fb4c 100644
--- a/broker/src/hidra2_broker/server/get_next_test.go
+++ b/broker/src/hidra2_broker/server/get_next_test.go
@@ -26,8 +26,8 @@ func doRequest(path string) *httptest.ResponseRecorder {
}
func TestGetNextWithoutDatabaseName(t *testing.T) {
- w := doRequest("/next")
- assert.Equal(t, http.StatusBadRequest, w.Code, "no database name")
+ w := doRequest("/database/next")
+ assert.Equal(t, http.StatusNotFound, w.Code, "no database name")
}
func TestGetNextWithWrongDatabaseName(t *testing.T) {
@@ -37,8 +37,8 @@ func TestGetNextWithWrongDatabaseName(t *testing.T) {
mock_db.On("GetNextRecord", "foo").Return([]byte(""),
&database.DBError{utils.StatusWrongInput, ""})
- w := doRequest("/next?database=foo")
- assert.Equal(t, http.StatusBadRequest, w.Code, "no database name")
+ w := doRequest("/database/foo/next")
+ assert.Equal(t, http.StatusBadRequest, w.Code, "wrong database name")
assertExpectations(t, mock_db)
}
@@ -48,7 +48,7 @@ func TestGetNextWithInternalDBError(t *testing.T) {
defer func() { db = nil }()
mock_db.On("GetNextRecord", "foo").Return([]byte(""), errors.New(""))
- w := doRequest("/next?database=foo")
+ w := doRequest("/database/foo/next")
assert.Equal(t, http.StatusInternalServerError, w.Code, "internal error")
assertExpectations(t, mock_db)
}
@@ -57,9 +57,9 @@ func TestGetNextWithGoodDatabaseName(t *testing.T) {
mock_db := new(database.MockedDatabase)
db = mock_db
defer func() { db = nil }()
- mock_db.On("GetNextRecord", "database").Return([]byte("Hello"), nil)
+ mock_db.On("GetNextRecord", "dbname").Return([]byte("Hello"), nil)
- w := doRequest("/next?database=database")
+ w := doRequest("/database/dbname/next")
assert.Equal(t, http.StatusOK, w.Code, "GetNext OK")
assert.Equal(t, "Hello", string(w.Body.Bytes()), "GetNext sends data")
assertExpectations(t, mock_db)
diff --git a/broker/src/hidra2_broker/server/listroutes.go b/broker/src/hidra2_broker/server/listroutes.go
index 86baeb470..b9b48ee7e 100644
--- a/broker/src/hidra2_broker/server/listroutes.go
+++ b/broker/src/hidra2_broker/server/listroutes.go
@@ -8,7 +8,7 @@ var listRoutes = utils.Routes{
utils.Route{
"GetNext",
"Get",
- "/next",
+ "/database/{dbname}/next",
routeGetNext,
},
}
diff --git a/common/cpp/include/common/data_structs.h b/common/cpp/include/common/data_structs.h
index 93fbdb40b..afa13ae0e 100644
--- a/common/cpp/include/common/data_structs.h
+++ b/common/cpp/include/common/data_structs.h
@@ -18,6 +18,7 @@ class FileInfo {
uint64_t id{0};
std::string Json() const;
bool SetFromJson(const std::string& json_string);
+ std::string FullName(const std::string& base_path);
};
inline bool operator==(const FileInfo& lhs, const FileInfo& rhs) {
diff --git a/common/cpp/src/data_structs/data_structs.cpp b/common/cpp/src/data_structs/data_structs.cpp
index 9cde1a4a5..0467bea29 100644
--- a/common/cpp/src/data_structs/data_structs.cpp
+++ b/common/cpp/src/data_structs/data_structs.cpp
@@ -75,4 +75,11 @@ bool FileInfo::SetFromJson(const std::string& json_string) {
return true;
}
+std::string FileInfo::FullName(const std::string& base_path) {
+ std::string full_name;
+ full_name = base_path.empty() ? "" : base_path + "/";
+ full_name += relative_path.empty() ? "" : relative_path + "/";
+ return full_name + base_name;
+}
+
}
diff --git a/tests/broker/get_next/check_linux.sh b/tests/broker/get_next/check_linux.sh
index 32d6ed6e0..d21ae53e8 100644
--- a/tests/broker/get_next/check_linux.sh
+++ b/tests/broker/get_next/check_linux.sh
@@ -22,7 +22,7 @@ $@ &
sleep 0.3
brokerid=`echo $!`
-curl -v --silent 127.0.0.1:5005/next?database=data --stderr - | grep '"_id":1'
-curl -v --silent 127.0.0.1:5005/next?database=data --stderr - | grep '"_id":2'
+curl -v --silent 127.0.0.1:5005/database/data/next --stderr - | grep '"_id":1'
+curl -v --silent 127.0.0.1:5005/database/data/next --stderr - | grep '"_id":2'
-curl -v --silent 127.0.0.1:5005/next?database=data --stderr - | grep "No Content"
+curl -v --silent 127.0.0.1:5005/database/data/next --stderr - | grep "No Content"
diff --git a/tests/broker/get_next/check_windows.bat b/tests/broker/get_next/check_windows.bat
index 351a03036..009f92c4d 100644
--- a/tests/broker/get_next/check_windows.bat
+++ b/tests/broker/get_next/check_windows.bat
@@ -11,9 +11,9 @@ start /B "" "%full_name%"
ping 1.0.0.0 -n 1 -w 100 > nul
-C:\Curl\curl.exe -v --silent 127.0.0.1:5005/next?database=data --stderr - | findstr \"_id\":1 || goto :error
-C:\Curl\curl.exe -v --silent 127.0.0.1:5005/next?database=data --stderr - | findstr \"_id\":2 || goto :error
-C:\Curl\curl.exe -v --silent 127.0.0.1:5005/next?database=data --stderr - | findstr "No Content" || goto :error
+C:\Curl\curl.exe -v --silent 127.0.0.1:5005/database/data/next --stderr - | findstr \"_id\":1 || goto :error
+C:\Curl\curl.exe -v --silent 127.0.0.1:5005/database/data/next --stderr - | findstr \"_id\":2 || goto :error
+C:\Curl\curl.exe -v --silent 127.0.0.1:5005/database/data/next --stderr - | findstr "No Content" || goto :error
goto :clean
diff --git a/worker/api/cpp/CMakeLists.txt b/worker/api/cpp/CMakeLists.txt
index cc301d778..30f9c63cc 100644
--- a/worker/api/cpp/CMakeLists.txt
+++ b/worker/api/cpp/CMakeLists.txt
@@ -4,7 +4,7 @@ set(SOURCE_FILES
src/data_broker.cpp
src/folder_data_broker.cpp
src/server_data_broker.cpp
- src/io_map.cpp
+ src/broker_helpers.cpp
src/curl_http_client.cpp)
diff --git a/worker/api/cpp/src/io_map.cpp b/worker/api/cpp/src/broker_helpers.cpp
similarity index 96%
rename from worker/api/cpp/src/io_map.cpp
rename to worker/api/cpp/src/broker_helpers.cpp
index c6855d6fa..c9e8dc8e2 100644
--- a/worker/api/cpp/src/io_map.cpp
+++ b/worker/api/cpp/src/broker_helpers.cpp
@@ -1,4 +1,4 @@
-#include "io_map.h"
+#include "broker_helpers.h"
namespace hidra2 {
diff --git a/worker/api/cpp/src/io_map.h b/worker/api/cpp/src/broker_helpers.h
similarity index 100%
rename from worker/api/cpp/src/io_map.h
rename to worker/api/cpp/src/broker_helpers.h
diff --git a/worker/api/cpp/src/folder_data_broker.cpp b/worker/api/cpp/src/folder_data_broker.cpp
index f081fc9a2..bc0801dfe 100644
--- a/worker/api/cpp/src/folder_data_broker.cpp
+++ b/worker/api/cpp/src/folder_data_broker.cpp
@@ -1,7 +1,7 @@
#include "folder_data_broker.h"
#include "system_wrappers/system_io.h"
-#include "io_map.h"
+#include "broker_helpers.h"
namespace hidra2 {
@@ -31,7 +31,7 @@ WorkerErrorCode FolderDataBroker::CanGetData(FileInfo* info, FileData* data, int
return WorkerErrorCode::kSourceNotConnected;
}
- if (info == nullptr && data == nullptr) {
+ if (info == nullptr) {
return WorkerErrorCode::kWrongInput;
}
@@ -53,19 +53,14 @@ WorkerErrorCode FolderDataBroker::GetNext(FileInfo* info, FileData* data) {
return err;
}
- FileInfo file_info = filelist_[nfile_to_get];
- if (info != nullptr) {
- *info = file_info;
- }
+ *info = filelist_[nfile_to_get];
if (data == nullptr) {
return WorkerErrorCode::kOK;
}
IOErrors ioerr;
- *data = io__->GetDataFromFile(base_path_ + "/" + file_info.relative_path +
- (file_info.relative_path.empty() ? "" : "/") +
- file_info.base_name, file_info.size, &ioerr);
+ *data = io__->GetDataFromFile(info->FullName(base_path_), info->size, &ioerr);
return MapIOError(ioerr);
}
diff --git a/worker/api/cpp/src/server_data_broker.cpp b/worker/api/cpp/src/server_data_broker.cpp
index ee712f8e0..26e3b2fe2 100644
--- a/worker/api/cpp/src/server_data_broker.cpp
+++ b/worker/api/cpp/src/server_data_broker.cpp
@@ -1,7 +1,7 @@
#include "server_data_broker.h"
#include "system_wrappers/system_io.h"
#include "curl_http_client.h"
-#include "io_map.h"
+#include "broker_helpers.h"
namespace hidra2 {
@@ -15,25 +15,27 @@ WorkerErrorCode ServerDataBroker::Connect() {
return WorkerErrorCode::kOK;
}
-WorkerErrorCode ServerDataBroker::GetNext(FileInfo* info, FileData* data) {
- if (info == nullptr && data == nullptr) {
- return WorkerErrorCode::kWrongInput;
- }
-
- std::string full_uri = server_uri_ + "/next?database=" + source_name_;
+WorkerErrorCode ServerDataBroker::GetFileInfoFromServer(FileInfo* info, const std::string& operation) {
+ std::string full_uri = server_uri_ + "/database/" + source_name_ + "/" + operation;
WorkerErrorCode err;
auto responce = httpclient__->Get(full_uri, &err);
if (err != WorkerErrorCode::kOK) {
return err;
}
-
- FileInfo file_info;
- if (!file_info.SetFromJson(responce)) {
+ if (!info->SetFromJson(responce)) {
return WorkerErrorCode::kErrorReadingSource;
}
+ return WorkerErrorCode::kOK;
+}
+
+WorkerErrorCode ServerDataBroker::GetNext(FileInfo* info, FileData* data) {
+ if (info == nullptr) {
+ return WorkerErrorCode::kWrongInput;
+ }
- if (info != nullptr) {
- *info = file_info;
+ auto err = GetFileInfoFromServer(info, "next");
+ if (err != WorkerErrorCode::kOK) {
+ return err;
}
if (data == nullptr) {
@@ -41,10 +43,7 @@ WorkerErrorCode ServerDataBroker::GetNext(FileInfo* info, FileData* data) {
}
IOErrors ioerr;
- *data = io__->GetDataFromFile(file_info.relative_path +
- (file_info.relative_path.empty() ? "" : "/") +
- file_info.base_name, file_info.size, &ioerr);
-
+ *data = io__->GetDataFromFile(info->FullName(""), info->size, &ioerr);
return hidra2::MapIOError(ioerr);
}
diff --git a/worker/api/cpp/src/server_data_broker.h b/worker/api/cpp/src/server_data_broker.h
index 6cea3a104..ad3bc604e 100644
--- a/worker/api/cpp/src/server_data_broker.h
+++ b/worker/api/cpp/src/server_data_broker.h
@@ -15,6 +15,7 @@ class ServerDataBroker final : public hidra2::DataBroker {
std::unique_ptr<hidra2::IO> io__; // modified in testings to mock system calls,otherwise do not touch
std::unique_ptr<hidra2::HttpClient> httpclient__;
private:
+ WorkerErrorCode GetFileInfoFromServer(FileInfo* info, const std::string& operation);
std::string server_uri_;
std::string source_name_;
};
diff --git a/worker/api/cpp/unittests/test_folder_broker.cpp b/worker/api/cpp/unittests/test_folder_broker.cpp
index b7131c45e..e736d061e 100644
--- a/worker/api/cpp/unittests/test_folder_broker.cpp
+++ b/worker/api/cpp/unittests/test_folder_broker.cpp
@@ -248,16 +248,6 @@ TEST_F(GetDataFromFileTests, GetNextReturnsDataAndInfo) {
}
-TEST_F(GetDataFromFileTests, GetNextReturnsOnlyData) {
- EXPECT_CALL(mock, GetDataFromFileProxy(_, _, _)).
- WillOnce(DoAll(testing::SetArgPointee<2>(IOErrors::kNoError), testing::Return(new uint8_t[1] {'1'})));
-
- data_broker->GetNext(nullptr, &data);
-
- ASSERT_THAT(data[0], Eq('1'));
-}
-
-
TEST_F(GetDataFromFileTests, GetNextReturnsErrorWhenCannotReadData) {
EXPECT_CALL(mock, GetDataFromFileProxy(_, _, _)).
WillOnce(DoAll(testing::SetArgPointee<2>(IOErrors::kReadError), testing::Return(nullptr)));
diff --git a/worker/api/cpp/unittests/test_server_broker.cpp b/worker/api/cpp/unittests/test_server_broker.cpp
index 6e083a32d..afe61a0dc 100644
--- a/worker/api/cpp/unittests/test_server_broker.cpp
+++ b/worker/api/cpp/unittests/test_server_broker.cpp
@@ -33,13 +33,14 @@ using ::testing::SetArgPointee;
namespace {
-TEST(FolderDataBroker, SetCorrectMembers) {
- auto data_broker = std::unique_ptr<ServerDataBroker> {new ServerDataBroker("test", "database")};
+TEST(FolderDataBroker, SetCorrectIo) {
+ auto data_broker = std::unique_ptr<ServerDataBroker> {new ServerDataBroker("test", "dbname")};
ASSERT_THAT(dynamic_cast<hidra2::SystemIO*>(data_broker->io__.get()), Ne(nullptr));
}
TEST(FolderDataBroker, SetCorrectHttpClient) {
- auto data_broker = std::unique_ptr<ServerDataBroker> {new ServerDataBroker("test", "database")};
+ auto data_broker = std::unique_ptr<ServerDataBroker> {new ServerDataBroker("test", "dbname")};
+ ASSERT_THAT(dynamic_cast<hidra2::CurlHttpClient*>(data_broker->httpclient__.get()), Ne(nullptr));
}
@@ -51,7 +52,7 @@ class ServerDataBrokerTests : public Test {
FileInfo info;
void SetUp() override {
- data_broker = std::unique_ptr<ServerDataBroker> {new ServerDataBroker("test", "database")};
+ data_broker = std::unique_ptr<ServerDataBroker> {new ServerDataBroker("test", "dbname")};
data_broker->io__ = std::unique_ptr<IO> {&mock_io};
data_broker->httpclient__ = std::unique_ptr<hidra2::HttpClient> {&mock_http_client};
}
@@ -72,7 +73,7 @@ TEST_F(ServerDataBrokerTests, GetNextReturnsErrorOnWrongInput) {
}
TEST_F(ServerDataBrokerTests, GetNextUsesCorrectUri) {
- EXPECT_CALL(mock_http_client, Get("test/next?database=database", _)).WillOnce(DoAll(
+ EXPECT_CALL(mock_http_client, Get("test/database/dbname/next", _)).WillOnce(DoAll(
SetArgPointee<1>(WorkerErrorCode::kOK),
Return("")));
data_broker->GetNext(&info, nullptr);
@@ -104,8 +105,7 @@ TEST_F(ServerDataBrokerTests, GetNextReturnsFileInfo) {
auto json = to_send.Json();
EXPECT_CALL(mock_http_client, Get(_, _)).WillOnce(DoAll(
SetArgPointee<1>(WorkerErrorCode::kOK),
- Return(json)
- ));
+ Return(json)));
auto err = data_broker->GetNext(&info, nullptr);
@@ -134,8 +134,7 @@ TEST_F(ServerDataBrokerTests, GetNextReturnsParseError) {
TEST_F(ServerDataBrokerTests, GetNextReturnsIfNoDtataNeeded) {
EXPECT_CALL(mock_http_client, Get(_, _)).WillOnce(DoAll(
SetArgPointee<1>(WorkerErrorCode::kOK),
- Return("blabla")
- ));
+ Return("blabla")));
EXPECT_CALL( mock_io, GetDataFromFile_t(_, _, _)).Times(0);
data_broker->GetNext(&info, nullptr);
--
GitLab