Skip to content

Commit 784e2b2

Browse files
borzunborzun
authored
Refactor the ApiClientLookup::LookupApiClient to use synchronous API (#605)
1. Adds the synchronous version of the ResourcesApi and PlatformApi. 2. Refactor the ApiClientLookup::LookupApiClient to use sync ResourceApi and PlatformApi. This will ease supporting of the API lookup code and will eventually get rid of static network in the tests. 3. Add assertion on leaked network and task scheduler in DataserviceWriteStreamLayerClientTest. Relates-to: OLPEDGE-1331, OLPEDGE-1424 Signed-off-by: Bohdan Kurylovych <[email protected]>
1 parent e1d2c20 commit 784e2b2

File tree

8 files changed

+172
-92
lines changed

8 files changed

+172
-92
lines changed

‎olp-cpp-sdk-dataservice-write/src/ApiClientLookup.cpp‎

Lines changed: 55 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,11 @@
1919

2020
#include "ApiClientLookup.h"
2121

22+
#include <olp/core/cache/KeyValueCache.h>
2223
#include <olp/core/client/ApiError.h>
2324
#include <olp/core/client/Condition.h>
2425
#include <olp/core/client/HRN.h>
2526
#include <olp/core/client/OlpClient.h>
26-
#include <olp/core/cache/KeyValueCache.h>
2727
#include <olp/core/logging/Log.h>
2828
#include "generated/PlatformApi.h"
2929
#include "generated/ResourcesApi.h"
@@ -157,86 +157,82 @@ ApiClientLookup::ApiClientResponse ApiClientLookup::LookupApiClient(
157157
}
158158
}
159159

160-
client::Condition condition;
161-
// when the network operation took too much time we cancel it and exit
162-
// execution, to make sure that network callback will not access dangling
163-
// references we protect them with atomic bool flag.
164-
auto flag = std::make_shared<std::atomic_bool>(true);
165-
166-
ApiClientResponse api_response;
167-
auto api_callback = [&, flag](ApiClientResponse response) {
168-
if (flag->exchange(false)) {
169-
api_response = std::move(response);
170-
condition.Notify();
171-
}
172-
};
173-
174-
cancellation_context.ExecuteOrCancelled(
175-
[&, flag]() {
176-
auto client_ptr = std::make_shared<olp::client::OlpClient>();
177-
client_ptr->SetSettings(settings);
178-
179-
auto token = ApiClientLookup::LookupApiClient(
180-
client_ptr, service, service_version, catalog, api_callback);
181-
return client::CancellationToken([&, token, flag]() {
182-
if (flag->exchange(false)) {
183-
token.Cancel();
184-
condition.Notify();
185-
}
186-
});
187-
},
188-
[&]() {
189-
// if context was cancelled before the execution setup, unblock the
190-
// upcoming wait routine.
191-
condition.Notify();
192-
});
193-
if (!condition.Wait(std::chrono::seconds(settings.retry_settings.timeout))) {
194-
cancellation_context.CancelOperation();
195-
OLP_SDK_LOG_INFO_F(kLogTag, "LookupApi(%s/%s): %s - timeout",
196-
service.c_str(), service_version.c_str(),
197-
catalog.partition.c_str());
198-
return client::ApiError(client::ErrorCode::RequestTimeout,
199-
"Network request timed out.");
160+
OLP_SDK_LOG_TRACE_F(kLogTag, "LookupApiClient(%s/%s): %s", service.c_str(),
161+
service_version.c_str(), catalog.partition.c_str());
162+
163+
// compare the hrn
164+
const auto base_url = GetDatastoreServerUrl(catalog.partition);
165+
if (base_url.empty()) {
166+
OLP_SDK_LOG_INFO_F(
167+
kLogTag, "LookupApiClient(%s/%s): %s Lookup URL not found",
168+
service.c_str(), service_version.c_str(), catalog.partition.c_str());
169+
return client::ApiError(client::ErrorCode::NotFound,
170+
"Invalid or broken HRN");
200171
}
201172

202-
flag->store(false);
173+
ApisResponse api_response;
174+
client::OlpClient input_client;
175+
input_client.SetBaseUrl(base_url);
176+
input_client.SetSettings(settings);
203177

204-
if (cancellation_context.IsCancelled()) {
205-
// We can't use api response here because it could potentially be
206-
// uninitialized.
207-
OLP_SDK_LOG_INFO_F(kLogTag, "LookupApi(%s/%s): %s - cancelled",
178+
if (service == "config") {
179+
OLP_SDK_LOG_INFO_F(kLogTag, "LookupApiClient(%s/%s): %s - config service",
180+
service.c_str(), service_version.c_str(),
181+
catalog.partition.c_str());
182+
183+
// scan apis at platform apis
184+
api_response = PlatformApi::GetApis(input_client, service, service_version,
185+
cancellation_context);
186+
} else {
187+
OLP_SDK_LOG_INFO_F(kLogTag, "LookupApiClient(%s/%s): %s - resource service",
208188
service.c_str(), service_version.c_str(),
209189
catalog.partition.c_str());
210-
return client::ApiError(client::ErrorCode::Cancelled,
211-
"Operation cancelled.");
190+
191+
// scan apis at resource endpoint
192+
api_response =
193+
ResourcesApi::GetApis(input_client, catalog.ToCatalogHRNString(),
194+
service, service_version, cancellation_context);
212195
}
213196

214197
if (!api_response.IsSuccessful()) {
198+
OLP_SDK_LOG_INFO_F(kLogTag, "LookupApiClient(%s/%s): %s - unsuccessful: %s",
199+
service.c_str(), service_version.c_str(),
200+
catalog.partition.c_str(),
201+
api_response.GetError().GetMessage().c_str());
215202
return api_response.GetError();
203+
} else if (api_response.GetResult().empty()) {
204+
OLP_SDK_LOG_INFO_F(
205+
kLogTag, "LookupApiClient(%s/%s): %s - service not available",
206+
service.c_str(), service_version.c_str(), catalog.partition.c_str());
207+
return client::ApiError(client::ErrorCode::ServiceUnavailable,
208+
"Service/Version not available for given HRN");
216209
}
217210

218-
auto client = api_response.MoveResult();
219-
if (client.GetBaseUrl().empty()) {
220-
OLP_SDK_LOG_WARNING_F(kLogTag, "LookupApi(%s/%s): %s - empty base URL",
221-
service.c_str(), service_version.c_str(),
222-
catalog.partition.c_str());
211+
const auto output_base_url = api_response.GetResult().front().GetBaseUrl();
212+
if (output_base_url.empty()) {
213+
OLP_SDK_LOG_WARNING_F(
214+
kLogTag, "LookupApiClient(%s/%s): %s - empty base URL", service.c_str(),
215+
service_version.c_str(), catalog.partition.c_str());
223216
}
224217

225-
if (cache) {
226-
const auto& base_url = client.GetBaseUrl();
218+
client::OlpClient output_client;
219+
output_client.SetBaseUrl(output_base_url);
227220

221+
if (cache) {
228222
constexpr time_t kExpiryTimeInSecs = 3600;
229-
if (cache->Put(cache_key, base_url, [base_url]() { return base_url; },
230-
kExpiryTimeInSecs)) {
223+
if (cache->Put(
224+
cache_key, output_base_url,
225+
[output_base_url]() { return output_base_url; },
226+
kExpiryTimeInSecs)) {
231227
OLP_SDK_LOG_TRACE_F(kLogTag, "Put '%s' to cache", cache_key.c_str());
232228
} else {
233229
OLP_SDK_LOG_WARNING_F(kLogTag, "Failed to put '%s' to cache",
234230
cache_key.c_str());
235231
}
236232
}
237233

238-
client.SetSettings(settings);
239-
return ApiClientResponse{std::move(client)};
234+
output_client.SetSettings(settings);
235+
return ApiClientResponse{std::move(output_client)};
240236
}
241237

242238
} // namespace write

‎olp-cpp-sdk-dataservice-write/src/generated/PlatformApi.cpp‎

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,17 +35,18 @@ namespace dataservice {
3535
namespace write {
3636
client::CancellationToken PlatformApi::GetApis(
3737
std::shared_ptr<client::OlpClient> client, const std::string& service,
38-
const std::string& serviceVersion, const ApisCallback& apisCallback) {
39-
std::multimap<std::string, std::string> headerParams;
40-
headerParams.insert(std::make_pair("Accept", "application/json"));
41-
std::multimap<std::string, std::string> queryParams;
42-
std::multimap<std::string, std::string> formParams;
38+
const std::string& service_version, const ApisCallback& apisCallback) {
39+
std::multimap<std::string, std::string> header_params;
40+
header_params.insert(std::make_pair("Accept", "application/json"));
41+
std::multimap<std::string, std::string> query_params;
42+
std::multimap<std::string, std::string> form_params;
4343

44-
std::string platformUrl = "/platform/apis/" + service + "/" + serviceVersion;
44+
std::string platform_url =
45+
"/platform/apis/" + service + "/" + service_version;
4546

4647
client::NetworkAsyncCallback callback = [apisCallback](
4748
client::HttpResponse response) {
48-
if (response.status != 200) {
49+
if (response.status != olp::http::HttpStatusCode::OK) {
4950
apisCallback(ApisResponse(
5051
client::ApiError(response.status, response.response.str())));
5152
} else {
@@ -55,8 +56,31 @@ client::CancellationToken PlatformApi::GetApis(
5556
}
5657
};
5758

58-
return client->CallApi(platformUrl, "GET", queryParams, headerParams,
59-
formParams, nullptr, "", callback);
59+
return client->CallApi(platform_url, "GET", query_params, header_params,
60+
form_params, nullptr, "", callback);
61+
}
62+
63+
PlatformApi::ApisResponse PlatformApi::GetApis(
64+
const client::OlpClient& client, const std::string& service,
65+
const std::string& service_version,
66+
client::CancellationContext cancel_context) {
67+
std::multimap<std::string, std::string> header_params;
68+
header_params.insert(std::make_pair("Accept", "application/json"));
69+
std::multimap<std::string, std::string> query_params;
70+
std::multimap<std::string, std::string> form_params;
71+
72+
std::string platform_url =
73+
"/platform/apis/" + service + "/" + service_version;
74+
75+
auto http_response =
76+
client.CallApi(std::move(platform_url), "GET", std::move(query_params),
77+
std::move(header_params), std::move(form_params), nullptr,
78+
"", cancel_context);
79+
if (http_response.status != olp::http::HttpStatusCode::OK) {
80+
return client::ApiError(http_response.status, http_response.response.str());
81+
}
82+
83+
return ApisResponse(parser::parse<model::Apis>(http_response.response));
6084
}
6185

6286
} // namespace write

‎olp-cpp-sdk-dataservice-write/src/generated/PlatformApi.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include <string>
2424

2525
#include <olp/core/client/ApiResponse.h>
26+
#include <olp/core/client/CancellationContext.h>
2627
#include "generated/model/Api.h"
2728
#include "olp/core/client/ApiError.h"
2829

@@ -54,6 +55,14 @@ class PlatformApi {
5455
static client::CancellationToken GetApis(
5556
std::shared_ptr<client::OlpClient> client, const std::string& service,
5657
const std::string& serviceVersion, const ApisCallback& callback);
58+
59+
/**
60+
* @brief Synchronous version of \c GetApis method.
61+
*/
62+
static ApisResponse GetApis(const client::OlpClient& client,
63+
const std::string& service,
64+
const std::string& service_version,
65+
client::CancellationContext context);
5766
};
5867

5968
} // namespace write

‎olp-cpp-sdk-dataservice-write/src/generated/ResourcesApi.cpp‎

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,18 +36,18 @@ client::CancellationToken ResourcesApi::GetApis(
3636
std::shared_ptr<client::OlpClient> client, const std::string& hrn,
3737
const std::string& service, const std::string& service_version,
3838
const ApisCallback& apisCallback) {
39-
std::multimap<std::string, std::string> headerParams;
40-
headerParams.insert(std::make_pair("Accept", "application/json"));
41-
std::multimap<std::string, std::string> queryParams;
42-
std::multimap<std::string, std::string> formParams;
39+
std::multimap<std::string, std::string> header_params;
40+
header_params.insert(std::make_pair("Accept", "application/json"));
41+
std::multimap<std::string, std::string> query_params;
42+
std::multimap<std::string, std::string> form_params;
4343

4444
// scan apis at resource endpoint
45-
std::string resourceUrl =
45+
std::string resource_url =
4646
"/resources/" + hrn + "/apis/" + service + "/" + service_version;
4747

4848
client::NetworkAsyncCallback callback = [apisCallback](
4949
client::HttpResponse response) {
50-
if (response.status != 200) {
50+
if (response.status != olp::http::HttpStatusCode::OK) {
5151
apisCallback(ApisResponse(
5252
client::ApiError(response.status, response.response.str())));
5353
} else {
@@ -56,8 +56,30 @@ client::CancellationToken ResourcesApi::GetApis(
5656
apisCallback(ApisResponse(parser::parse<model::Apis>(response.response)));
5757
}
5858
};
59-
return client->CallApi(resourceUrl, "GET", queryParams, headerParams,
60-
formParams, nullptr, "", callback);
59+
return client->CallApi(resource_url, "GET", query_params, header_params,
60+
form_params, nullptr, "", callback);
61+
}
62+
63+
ResourcesApi::ApisResponse ResourcesApi::GetApis(
64+
const client::OlpClient& client, const std::string& hrn,
65+
const std::string& service, const std::string& service_version,
66+
olp::client::CancellationContext cancel_context) {
67+
std::multimap<std::string, std::string> header_params;
68+
header_params.insert(std::make_pair("Accept", "application/json"));
69+
std::multimap<std::string, std::string> query_params;
70+
std::multimap<std::string, std::string> form_params;
71+
72+
// scan apis at resource endpoint
73+
std::string resource_url =
74+
"/resources/" + hrn + "/apis/" + service + "/" + service_version;
75+
auto http_response =
76+
client.CallApi(std::move(resource_url), "GET", std::move(query_params),
77+
std::move(header_params), std::move(form_params), nullptr,
78+
"", cancel_context);
79+
if (http_response.status != olp::http::HttpStatusCode::OK) {
80+
return client::ApiError(http_response.status, http_response.response.str());
81+
}
82+
return ApisResponse(parser::parse<model::Apis>(http_response.response));
6183
}
6284

6385
} // namespace write

‎olp-cpp-sdk-dataservice-write/src/generated/ResourcesApi.h‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include <string>
2424

2525
#include <olp/core/client/ApiResponse.h>
26+
#include <olp/core/client/CancellationContext.h>
2627
#include "generated/model/Api.h"
2728
#include "olp/core/client/ApiError.h"
2829

@@ -56,6 +57,15 @@ class ResourcesApi {
5657
std::shared_ptr<client::OlpClient> client, const std::string& hrn,
5758
const std::string& service, const std::string& service_version,
5859
const ApisCallback& callback);
60+
61+
/**
62+
* @brief Synchronous version of \c GetApis method.
63+
*/
64+
static ApisResponse GetApis(const client::OlpClient& client,
65+
const std::string& hrn,
66+
const std::string& service,
67+
const std::string& service_version,
68+
olp::client::CancellationContext cancel_context);
5969
};
6070

6171
} // namespace write

‎olp-cpp-sdk-dataservice-write/tests/StreamLayerClientImplTest.cpp‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -312,11 +312,7 @@ TEST_F(StreamLayerClientImplTest, FaliedPublishDataLessThanTwentyMib) {
312312
EXPECT_FALSE(response.IsSuccessful());
313313
EXPECT_EQ(olp::http::HttpStatusCode::BAD_REQUEST,
314314
response.GetError().GetHttpStatusCode());
315-
// For some reason the OlpClient return error string as:
316-
// 'Error occured. Please check HTTP status code.'
317-
// Maybe we should reconsider this...
318-
EXPECT_EQ("Error occured. Please check HTTP status code.",
319-
response.GetError().GetMessage());
315+
EXPECT_TRUE(response.GetError().GetMessage().empty());
320316
}
321317

322318
{

‎tests/functional/olp-cpp-sdk-dataservice-write/DataserviceWriteStreamLayerClientCacheTest.cpp‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,15 @@ class DataserviceWriteStreamLayerClientCacheTest : public ::testing::Test {
103103
if (disk_cache_) {
104104
disk_cache_->Close();
105105
}
106+
107+
// verify that no other thread contains network or task scheduler instance:
108+
auto network = network_;
109+
auto scheduler = task_scheduler_;
110+
network_.reset();
111+
task_scheduler_.reset();
112+
113+
ASSERT_EQ(network.use_count(), 1);
114+
ASSERT_EQ(scheduler.use_count(), 1);
106115
}
107116

108117
std::string GetTestCatalog() {
@@ -122,16 +131,18 @@ class DataserviceWriteStreamLayerClientCacheTest : public ::testing::Test {
122131
}
123132

124133
virtual std::shared_ptr<StreamLayerClient> CreateStreamLayerClient() {
125-
auto network = olp::client::OlpClientSettingsFactory::
134+
network_ = olp::client::OlpClientSettingsFactory::
126135
CreateDefaultNetworkRequestHandler();
136+
task_scheduler_ =
137+
olp::client::OlpClientSettingsFactory::CreateDefaultTaskScheduler(1u);
127138

128139
const auto app_id = CustomParameters::getArgument(kAppid);
129140
const auto secret = CustomParameters::getArgument(kSecret);
130141

131142
olp::authentication::Settings authentication_settings({app_id, secret});
132143
authentication_settings.token_endpoint_url =
133144
CustomParameters::getArgument(kEndpoint);
134-
authentication_settings.network_request_handler = network;
145+
authentication_settings.network_request_handler = network_;
135146

136147
olp::authentication::TokenProviderDefault provider(authentication_settings);
137148

@@ -140,9 +151,8 @@ class DataserviceWriteStreamLayerClientCacheTest : public ::testing::Test {
140151

141152
olp::client::OlpClientSettings settings;
142153
settings.authentication_settings = auth_client_settings;
143-
settings.network_request_handler = network;
144-
settings.task_scheduler =
145-
olp::client::OlpClientSettingsFactory::CreateDefaultTaskScheduler(1u);
154+
settings.network_request_handler = network_;
155+
settings.task_scheduler = task_scheduler_;
146156

147157
disk_cache_ = std::make_shared<olp::cache::DefaultCache>();
148158
EXPECT_EQ(disk_cache_->Open(),
@@ -168,7 +178,8 @@ class DataserviceWriteStreamLayerClientCacheTest : public ::testing::Test {
168178
protected:
169179
std::shared_ptr<StreamLayerClient> client_;
170180
std::shared_ptr<std::vector<unsigned char>> data_;
171-
181+
std::shared_ptr<olp::http::Network> network_;
182+
std::shared_ptr<olp::thread::TaskScheduler> task_scheduler_;
172183
std::shared_ptr<olp::cache::DefaultCache> disk_cache_;
173184
StreamLayerClientSettings stream_client_settings_;
174185
};

0 commit comments

Comments
 (0)