Skip to content

Commit

Permalink
feat!: RedisDataSource::Create should return unique_ptr instead of
Browse files Browse the repository at this point in the history
shared_ptr

We should default to single-ownership where possible. This makes it
easier for the caller to manage the returned value, either owning it or
converting it to a shared_ptr to pass into the SDK.
  • Loading branch information
cwaldren-ld committed Dec 15, 2023
1 parent 0a311e9 commit 536b098
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ class Redis;
}

namespace launchdarkly::server_side::integrations {

/**
* @brief RedisDataSource represents a data source for the Server-Side SDK
* backed by Redis. It is meant to be used in place of the standard
Expand Down Expand Up @@ -47,7 +46,7 @@ class RedisDataSource final : public ISerializedDataReader {
*
* @return A RedisDataSource, or an error if construction failed.
*/
static tl::expected<std::shared_ptr<RedisDataSource>, std::string> Create(
static tl::expected<std::unique_ptr<RedisDataSource>, std::string> Create(
std::string uri,
std::string prefix);

Expand All @@ -57,8 +56,6 @@ class RedisDataSource final : public ISerializedDataReader {
[[nodiscard]] std::string const& Identity() const override;
[[nodiscard]] bool Initialized() const override;

~RedisDataSource();

private:
RedisDataSource(std::unique_ptr<sw::redis::Redis> redis,
std::string prefix);
Expand All @@ -70,5 +67,4 @@ class RedisDataSource final : public ISerializedDataReader {
std::string const inited_key_;
std::unique_ptr<sw::redis::Redis> redis_;
};

} // namespace launchdarkly::server_side::integrations
9 changes: 2 additions & 7 deletions libs/server-sdk-redis-source/src/redis_source.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@
#include <redis++.h>

namespace launchdarkly::server_side::integrations {

tl::expected<std::shared_ptr<RedisDataSource>, std::string>
tl::expected<std::unique_ptr<RedisDataSource>, std::string>
RedisDataSource::Create(std::string uri, std::string prefix) {
try {
return std::shared_ptr<RedisDataSource>(new RedisDataSource(
return std::unique_ptr<RedisDataSource>(new RedisDataSource(
std::make_unique<sw::redis::Redis>(std::move(uri)),
std::move(prefix)));
} catch (sw::redis::Error const& e) {
Expand All @@ -26,8 +25,6 @@ RedisDataSource::RedisDataSource(std::unique_ptr<sw::redis::Redis> redis,
inited_key_(prefix_ + ":$inited"),
redis_(std::move(redis)) {}

RedisDataSource::~RedisDataSource() = default;

ISerializedDataReader::GetResult RedisDataSource::Get(
ISerializedItemKind const& kind,
std::string const& itemKey) const {
Expand All @@ -54,7 +51,6 @@ ISerializedDataReader::AllResult RedisDataSource::All(
items.emplace(key, SerializedItemDescriptor::Present(0, val));
}
return items;

} catch (sw::redis::Error const& e) {
return tl::make_unexpected(Error{e.what()});
}
Expand All @@ -72,5 +68,4 @@ bool RedisDataSource::Initialized() const {
return false;
}
}

} // namespace launchdarkly::server_side::integrations
11 changes: 9 additions & 2 deletions libs/server-sdk-redis-source/tests/redis_source_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,13 @@ TEST_F(RedisTests, FlagAndSegmentCanCoexistWithSameKey) {
serialize(boost::json::value_from(segment_in)));
}

TEST_F(RedisTests, CanConvertRedisDataSourceToDataReader) {
auto maybe_source = RedisDataSource::Create("tcp://foobar:1000", "prefix");
ASSERT_TRUE(maybe_source);

std::shared_ptr<ISerializedDataReader> reader = std::move(*maybe_source);
}

TEST(RedisErrorTests, InvalidURIs) {
std::vector<std::string> const uris = {"nope, not a redis URI",
"http://foo",
Expand Down Expand Up @@ -433,11 +440,11 @@ TEST(RedisErrorTests, ValidURIs) {
}

TEST(RedisErrorTests, GetReturnsErrorAndNoExceptionThrown) {
auto const maybe_source = RedisDataSource::Create(
auto maybe_source = RedisDataSource::Create(
"tcp://foobar:1000" /* no redis service here */, "prefix");
ASSERT_TRUE(maybe_source);

auto const source = *maybe_source;
auto const source = std::move(*maybe_source);

auto const get_initialized = source->Initialized();
ASSERT_FALSE(get_initialized);
Expand Down

0 comments on commit 536b098

Please sign in to comment.