Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 55 additions & 8 deletions libs/common/include/launchdarkly/attributes_builder.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,37 @@ class AttributesBuilder final {

public:
/**
* Create an attributes builder with the given key.
* @param key A unique string identifying a context.
* Create an attributes builder with the given kind and key.
* @param builder The context builder associated with this attributes
* builder.
* @param kind The kind being added.
* @param key The key for the kind.
*/
AttributesBuilder(BuilderReturn& builder, std::string kind, std::string key)
: key_(std::move(key)), kind_(std::move(kind)), builder_(builder) {}

/**
* Crate an attributes builder with the specified kind, and pre-populated
* with the given attributes.
* @param builder The context builder associated with this attributes
* builder.
* @param kind The kind being added.
* @param attributes Attributes to populate the builder with.
*/
AttributesBuilder(BuilderReturn& builder,
std::string kind,
Attributes const& attributes)
: key_(attributes.Key()),
kind_(std::move(kind)),
builder_(builder),
name_(attributes.Name()),
anonymous_(attributes.Anonymous()),
private_attributes_(attributes.PrivateAttributes()) {
for (auto& pair : attributes.CustomAttributes().AsObject()) {
values_[pair.first] = pair.second;
}
}

/**
* The attributes builder should never be copied. We depend on a stable
* reference stored in the context builder.
Expand Down Expand Up @@ -118,9 +143,9 @@ class AttributesBuilder final {
* This action only affects analytics events that involve this particular
* Context. To mark some (or all) Context attributes as private for all
* contexts, use the overall configuration for the SDK. See
* launchdarkly::config::shared::builders::EventsBuilder< SDK >::AllAttributesPrivate
* launchdarkly::config::shared::builders::EventsBuilder<SDK>::AllAttributesPrivate
* and
* launchdarkly::config::shared::builders::EventsBuilder< SDK >::PrivateAttribute.
* launchdarkly::config::shared::builders::EventsBuilder<SDK>::PrivateAttribute.
*
* The attributes "kind" and "key", and the "_meta" attributes cannot be
* made private.
Expand Down Expand Up @@ -155,17 +180,39 @@ class AttributesBuilder final {
return *this;
}

/**
* Start adding a kind to the context.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wording nit: I think you are adding a new context with the specified kind to the builder ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I don't really think we are, or that if we are it isn't meaningful.

builder.Set("address", "111 Street")

I am adding an address to my context?

The address isn't an "address" though. It is a string, which I have added to my context with the name "address".

A kind is just the key for a set of attributes in a context, which we also happen to call a context.

I think we are adding a kind.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The kind happens to be a context, like the address happens to be a string, or the name is a string.)

*
* If you call this function multiple times with the same kind, then
* the same builder will be returned each time. If you previously called
* the function with the same kind, but different key, then the key
* will be updated.
*
* @param kind The kind being added.
* @param key The key for the kind.
* @return A builder which allows adding attributes for the kind.
*/
AttributesBuilder& Kind(std::string kind, std::string key) {
return builder_.Kind(kind, key);
}

/**
* Build the context. This method should not be called more than once.
* It moves the builder content into the built context.
* Start updating an existing kind.
*
* @param kind The kind to start updating.
Comment on lines +200 to +202
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, I think you're updating the existing context with the given kind

* @return A builder which allows adding attributes for the kind, or
* nullptr if the kind doesn't already exist.
*/
AttributesBuilder* Kind(std::string const& kind) {
return builder_.Kind(kind);
}

/**
* Build the context.
*
* @return The built context.
*/
[[nodiscard]] BuildType Build() { return builder_.Build(); }
[[nodiscard]] BuildType Build() const { return builder_.Build(); }

private:
BuilderReturn& builder_;
Expand All @@ -177,7 +224,7 @@ class AttributesBuilder final {
*/
void Key(std::string key) { key_ = std::move(key); }

Attributes BuildAttributes();
Attributes BuildAttributes() const;

AttributesBuilder& Set(std::string name,
launchdarkly::Value value,
Expand Down
32 changes: 24 additions & 8 deletions libs/common/include/launchdarkly/context_builder.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,19 @@ class ContextBuilder final {
friend AttributesBuilder<ContextBuilder, Context>;

public:
ContextBuilder() = default;

/**
* Create a new context builder from the given context. The created builder
* will have all the kinds and attributes of the original context.
*
* If the original context is not valid, then this builder will
* be created in a default state.
*
* @param context The context to base the builder on.
*/
ContextBuilder(Context const& context);

/**
* Start adding a kind to the context.
*
Expand All @@ -79,24 +92,27 @@ class ContextBuilder final {
std::string key);

/**
* Build a context. This should only be called once, because
* the contents of the builder are moved into the created context.
* Start updating an existing kind.
*
* After the context is build, then this builder, and any kind builders
* associated with it, should no longer be used.
* @param kind The kind to start updating.
Comment on lines -83 to +97
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

I think the reason I find the current wording a little confusing is because in the UI a context kind is an entity that you can create and define (Contexts list --> Kind tab --> Create kind). It's separate from any context where it's used, and this is definitely not working with that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, adding a "org" to a context is different than adding an "org" to your launchdarky account. The "org" you put into a context has attributes, and you cannot even make it without any attributes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both are kinds though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think got hung up on the two uses of "kind":

  • the entity you add to your LD account, can (but don't have to) use when defining a new context in the SDK, and can get a list of using the REST API, etc.
  • the thing that, as you say above, happens to be a context, like the address happens to be a string, or the name is a string

* @return A builder which allows adding attributes for the kind, or
* nullptr if the kind doesn't already exist.
*/
AttributesBuilder<ContextBuilder, Context>* Kind(std::string const& kind);

/**
* Build a context. The same builder instance may be used to build multiple
* contexts.
*
* You MUST add at least one kind before building a context. Not doing
* so will result in an invalid context.
*
* @return The built context.
*/
Context Build();
Context Build() const;

private:
std::map<std::string, AttributesBuilder<ContextBuilder, Context>> builders_;
std::map<std::string, Attributes> kinds_;
bool valid_ = true;
std::string errors_;
};

} // namespace launchdarkly
7 changes: 3 additions & 4 deletions libs/common/src/attributes_builder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ AttributesBuilder<ContextBuilder, Context>::Set(std::string name, Value value) {
template <>
AttributesBuilder<ContextBuilder, Context>&
AttributesBuilder<ContextBuilder, Context>::SetPrivate(std::string name,
Value value) {
Value value) {
return Set(std::move(name), std::move(value), true);
}

Expand All @@ -56,9 +56,8 @@ AttributesBuilder<ContextBuilder, Context>::AddPrivateAttribute(
}

template <>
Attributes AttributesBuilder<ContextBuilder, Context>::BuildAttributes() {
return {std::move(key_), std::move(name_), anonymous_, std::move(values_),
std::move(private_attributes_)};
Attributes AttributesBuilder<ContextBuilder, Context>::BuildAttributes() const {
return {key_, name_, anonymous_, values_, private_attributes_};
}

} // namespace launchdarkly
53 changes: 38 additions & 15 deletions libs/common/src/context_builder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,18 @@ static bool ValidKind(std::string_view kind) {
});
}

ContextBuilder::ContextBuilder(Context const& context) {
if (!context.Valid()) {
return;
}

for (const auto& kind : context.kinds_) {
const auto& attributes = context.Attributes(kind);
builders_.emplace(kind, AttributesBuilder<ContextBuilder, Context>(
*this, kind, attributes));
}
}

AttributesBuilder<ContextBuilder, Context>& ContextBuilder::Kind(
std::string const& kind,
std::string key) {
Expand All @@ -41,13 +53,17 @@ AttributesBuilder<ContextBuilder, Context>& ContextBuilder::Kind(
return builders_.at(kind);
}

Context ContextBuilder::Build() {
Context ContextBuilder::Build() const {
bool valid = true;
std::string errors;
std::map<std::string, Attributes> kinds;

if (builders_.empty()) {
valid_ = false;
if (!errors_.empty()) {
errors_.append(", ");
valid = false;
if (!errors.empty()) {
errors.append(", ");
}
errors_.append(ContextErrors::kMissingKinds);
errors.append(ContextErrors::kMissingKinds);
}
// We need to validate all the kinds. Being as kinds could be updated
// we cannot do this validation in the `kind` method.
Expand All @@ -56,9 +72,9 @@ Context ContextBuilder::Build() {
bool kind_valid = ValidKind(kind);
bool key_valid = !kind_builder.second.key_.empty();
if (!kind_valid || !key_valid) {
valid_ = false;
auto append = errors_.length() != 0;
std::stringstream stream(errors_);
valid = false;
auto append = errors.length() != 0;
std::stringstream stream(errors);
if (append) {
stream << ", ";
}
Expand All @@ -72,18 +88,25 @@ Context ContextBuilder::Build() {
stream << kind << ": " << ContextErrors::kInvalidKey;
}
stream.flush();
errors_ = stream.str();
errors = stream.str();
}
}
if (valid_) {
if (valid) {
for (auto& kind_builder : builders_) {
kinds_.emplace(kind_builder.first,
kind_builder.second.BuildAttributes());
kinds.emplace(kind_builder.first,
kind_builder.second.BuildAttributes());
}
builders_.clear();
return {std::move(kinds_)};
return {std::move(kinds)};
}
return {std::move(errors)};
}

AttributesBuilder<ContextBuilder, Context>* ContextBuilder::Kind(
std::string const& kind) {
if (builders_.count(kind)) {
return &builders_.at(kind);
}
return {std::move(errors_)};
return nullptr;
}

} // namespace launchdarkly
69 changes: 67 additions & 2 deletions libs/common/tests/context_builder_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ TEST(ContextBuilderTests, CanMakeSingleContextWithCustomAttributes) {
EXPECT_EQ("bobby-bobberson", context.Attributes("user").Key());
EXPECT_TRUE(context.Attributes("user").Anonymous());
EXPECT_EQ(1, context.Attributes("user").PrivateAttributes().size());
EXPECT_EQ(1,
context.Attributes("user").PrivateAttributes().count("email"));
EXPECT_EQ(1, context.Attributes("user").PrivateAttributes().count("email"));
}

TEST(ContextBuilderTests, CanBuildComplexMultiContext) {
Expand Down Expand Up @@ -176,4 +175,70 @@ TEST(ContextBuilderTests, AccessKindBuilderMultipleTimes) {
EXPECT_TRUE(context.Get("user", "isCat").AsBool());
}

TEST(ContextBuilderTests, AddAttributeToExistingContext) {
auto context = ContextBuilder()
.Kind("user", "potato")
.Name("Bob")
.Set("city", "Reno")
.SetPrivate("private", "a")
.Build();

auto builder = ContextBuilder(context);
if (auto updater = builder.Kind("user")) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmrj Adding "state" and "sneaky" to the context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could I also do:

updater->Set("city", "Winnemucca")

and overwrite the Reno value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

updater->Set("state", "Nevada");
updater->SetPrivate("sneaky", true);
}
auto updated_context = builder.Build();

EXPECT_EQ("potato", updated_context.Get("user", "key").AsString());
EXPECT_EQ("Bob", updated_context.Get("user", "name").AsString());
EXPECT_EQ("Reno", updated_context.Get("user", "city").AsString());
EXPECT_EQ("Nevada", updated_context.Get("user", "state").AsString());

EXPECT_EQ(2, updated_context.Attributes("user").PrivateAttributes().size());
EXPECT_EQ(1, updated_context.Attributes("user").PrivateAttributes().count("private"));
EXPECT_EQ(1, updated_context.Attributes("user").PrivateAttributes().count("sneaky"));
}

TEST(ContextBuilderTests, AddKindToExistingContext) {
auto context = ContextBuilder()
.Kind("user", "potato")
.Name("Bob")
.Set("city", "Reno")
.Build();

auto updated_context =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmrj Adding an "org" to the context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. Do I need to call IdentifyAsync with updated_context after this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this just makes a context. You need to identify with that context to get flag values for it.

ContextBuilder(context).Kind("org", "org-key").Build();

EXPECT_EQ("potato", updated_context.Get("user", "key").AsString());
EXPECT_EQ("Bob", updated_context.Get("user", "name").AsString());
EXPECT_EQ("Reno", updated_context.Get("user", "city").AsString());

EXPECT_EQ("org-key", updated_context.Get("org", "key"));
}

TEST(ContextBuilderTests, UseTheSameBuilderToBuildMultipleContexts) {
auto builder = ContextBuilder();

auto context_a =
builder.Kind("user", "potato").Name("Bob").Set("city", "Reno").Build();

auto context_b = builder.Build();

auto context_c = builder.Kind("bob", "tomato").Build();

EXPECT_EQ("potato", context_a.Get("user", "key").AsString());
EXPECT_EQ("Bob", context_a.Get("user", "name").AsString());
EXPECT_EQ("Reno", context_a.Get("user", "city").AsString());

EXPECT_EQ("potato", context_b.Get("user", "key").AsString());
EXPECT_EQ("Bob", context_b.Get("user", "name").AsString());
EXPECT_EQ("Reno", context_b.Get("user", "city").AsString());

EXPECT_EQ("potato", context_c.Get("user", "key").AsString());
EXPECT_EQ("Bob", context_c.Get("user", "name").AsString());
EXPECT_EQ("Reno", context_c.Get("user", "city").AsString());
EXPECT_EQ("tomato", context_c.Get("bob", "key").AsString());
}

// NOLINTEND cppcoreguidelines-avoid-magic-numbers