Skip to content

fix: handle setting protected/type-enforced attributes correctly #433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
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
50 changes: 26 additions & 24 deletions libs/common/include/launchdarkly/attributes_builder.hpp
Copy link
Member

Choose a reason for hiding this comment

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

The various docs need updated for this.

     * This method cannot be used to set the key, kind, name, or anonymous
     * property of a context. The specific methods on the context builder, or
     * attributes builder, should be used.

There are probably some other specific methods that need doc comment updates.

Copy link
Member

Choose a reason for hiding this comment

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

@kinyoklion I had Devin take a run at updating these docs. Seem fine to me, but can you take a look as well since this is more your area?

Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
#pragma once

Check notice on line 1 in libs/common/include/launchdarkly/attributes_builder.hpp

GitHub Actions / cpp-linter

Run clang-format on libs/common/include/launchdarkly/attributes_builder.hpp

File libs/common/include/launchdarkly/attributes_builder.hpp does not conform to Custom style guidelines. (lines 23, 34, 154, 218, 234, 245)

#include <string>

#include <launchdarkly/attribute_reference.hpp>
#include <launchdarkly/attributes.hpp>
#include <launchdarkly/value.hpp>

namespace launchdarkly {
#include <map>
#include <string>

namespace launchdarkly {
class ContextBuilder;

/**
@@ -22,7 +22,7 @@
class AttributesBuilder final {
friend class ContextBuilder;

public:
public:
/**
* Create an attributes builder with the given kind and key.
* @param builder The context builder associated with this attributes
@@ -31,7 +31,8 @@
* @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) {}
: builder_(builder), kind_(std::move(kind)), key_(std::move(key)) {
}

/**
* Crate an attributes builder with the specified kind, and pre-populated
@@ -44,13 +45,13 @@
AttributesBuilder(BuilderReturn& builder,
std::string kind,
Attributes const& attributes)
: key_(attributes.Key()),
: builder_(builder),
kind_(std::move(kind)),
builder_(builder),
key_(attributes.Key()),
name_(attributes.Name()),
anonymous_(attributes.Anonymous()),
private_attributes_(attributes.PrivateAttributes()) {
for (auto& pair : attributes.CustomAttributes().AsObject()) {

Check warning on line 54 in libs/common/include/launchdarkly/attributes_builder.hpp

GitHub Actions / cpp-linter

/libs/common/include/launchdarkly/attributes_builder.hpp:54:14 [readability-qualified-auto]

'auto &pair' can be declared as 'const auto &pair'
values_[pair.first] = pair.second;
}
}
@@ -65,7 +66,7 @@

// This cannot be noexcept because of:
// https://developercommunity.visualstudio.com/t/bug-in-stdmapstdpair-implementation-with-move-only/840554
AttributesBuilder(AttributesBuilder&& builder) = default;

Check warning on line 69 in libs/common/include/launchdarkly/attributes_builder.hpp

GitHub Actions / cpp-linter

/libs/common/include/launchdarkly/attributes_builder.hpp:69:5 [performance-noexcept-move-constructor]

move constructors should be marked noexcept
~AttributesBuilder() = default;

/**
@@ -90,36 +91,36 @@
/**
* Add or update an attribute in the context.
*
* This method cannot be used to set the key, kind, name, anonymous, or
* _meta property of a context. The specific methods on the context builder,
* or attributes builder, should be used.
* This method cannot be used to set the key, kind, or _meta property
* of a context. The anonymous and name properties can be set, but must
* be of the correct types (boolean and string respectively). The specific
* methods on the context builder or attributes builder can also be used
* for these properties.
*
* @param name The name of the attribute.
* @param value The value for the attribute.
* @param private_attribute If the attribute should be considered private:
* that is, the value will not be sent to LaunchDarkly in analytics events.
* @return A reference to the current builder.
*/
AttributesBuilder& Set(std::string name, launchdarkly::Value value);
AttributesBuilder& Set(std::string name, Value value);

/**
* Add or update a private attribute in the context.
*
* This method cannot be used to set the key, kind, name, or anonymous
* property of a context. The specific methods on the context builder, or
* attributes builder, should be used.
* This method cannot be used to set the key, kind, or _meta property
* of a context. The anonymous and name properties can be set, but must
* be of the correct types (boolean and string respectively). The specific
* methods on the context builder or attributes builder can also be used
* for these properties.
*
* Once you have set an attribute private it will remain in the private
* list even if you call `set` afterward. This method is just a convenience
* which also adds the attribute to the `PrivateAttributes`.
*
* @param name The name of the attribute.
* @param value The value for the attribute.
* @param private_attribute If the attribute should be considered private:
* that is, the value will not be sent to LaunchDarkly in analytics events.
* @return A reference to the current builder.
*/
AttributesBuilder& SetPrivate(std::string name, launchdarkly::Value value);
AttributesBuilder& SetPrivate(std::string name, Value value);

/**
* Designate a context attribute, or properties within them, as private:
@@ -150,7 +151,7 @@
* launchdarkly::config::shared::builders::EventsBuilder<SDK>::PrivateAttribute.
*
* The attributes "kind" and "key", and the "_meta" attributes cannot be
* made private.
* made private. The "anonymous" attribute can be marked private, but will not be redacted.
*
* In this example, firstName is marked as private, but lastName is not:
*
@@ -216,7 +217,7 @@
*/
[[nodiscard]] BuildType Build() const { return builder_.Build(); }

private:
private:
BuilderReturn& builder_;

/**
@@ -226,18 +227,19 @@
*/
void Key(std::string key) { key_ = std::move(key); }

Attributes BuildAttributes() const;

Check warning on line 230 in libs/common/include/launchdarkly/attributes_builder.hpp

GitHub Actions / cpp-linter

/libs/common/include/launchdarkly/attributes_builder.hpp:230:5 [modernize-use-nodiscard]

function 'BuildAttributes' should be marked [[nodiscard]]

AttributesBuilder& Set(std::string name,
launchdarkly::Value value,
Value value,
bool private_attribute);


std::string kind_;
std::string key_;
std::string name_;
bool anonymous_ = false;

std::map<std::string, launchdarkly::Value> values_;
std::map<std::string, Value> values_;
AttributeReference::SetType private_attributes_;
};
} // namespace launchdarkly
} // namespace launchdarkly
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/** @file */

Check notice on line 1 in libs/common/include/launchdarkly/bindings/c/context_builder.h

GitHub Actions / cpp-linter

Run clang-format on libs/common/include/launchdarkly/bindings/c/context_builder.h

File libs/common/include/launchdarkly/bindings/c/context_builder.h does not conform to Custom style guidelines. (lines 98, 182)
// NOLINTBEGIN modernize-use-using

#pragma once
@@ -94,8 +94,8 @@
* A subsequent call to @ref LDContextBuilder_Attributes_Set, for the same
* attribute, will not remove the private status.
*
* This method cannot be used to set the `key`, `kind`, `name`, or `anonymous`
* property of a context.
* This method cannot be used to set the `key`, `kind`, or `_meta`
* property of a context. The `anonymous` and `name` properties can be set, but must be of the correct types.
*
* Adding a @ref LDValue to the builder will consume that value.
*
@@ -179,7 +179,7 @@
* - @ref LDClientConfigBuilder_Events_PrivateAttribute
*
* The attributes "kind" and "key", and the "_meta" attributes cannot be
* made private.
* made private. The "anonymous" attribute can be marked private, but will not be redacted.
*
* @param builder The builder. Must not be NULL.
* @param kind The kind to set the attribute as private for. Must not be NULL.
94 changes: 84 additions & 10 deletions libs/common/src/attributes_builder.cpp
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
#include <launchdarkly/attributes_builder.hpp>

Check notice on line 1 in libs/common/src/attributes_builder.cpp

GitHub Actions / cpp-linter

Run clang-format on libs/common/src/attributes_builder.cpp

File libs/common/src/attributes_builder.cpp does not conform to Custom style guidelines. (lines 4, 27, 28, 29, 30, 40, 42, 43, 44, 46, 50, 51, 52, 53, 54, 57, 58, 59, 60, 61, 62, 63, 64, 65, 67, 68, 69, 70, 71, 72, 73, 76, 92, 111, 112, 119, 133, 137)
#include <launchdarkly/context_builder.hpp>

namespace launchdarkly {
#include <set>
#include <unordered_map>
#include <functional>

namespace launchdarkly {
template <>
AttributesBuilder<ContextBuilder, Context>&
AttributesBuilder<ContextBuilder, Context>::Name(std::string name) {
@@ -17,33 +20,104 @@
return *this;
}

// These attributes values cannot be set via the public Set/SetPrivate methods.
// 'key' and 'kind' are defined in the AttributesBuilder constructor, whereas
// '_meta' is set internally by the SDK.
std::set<std::string> const& ProtectedAttributes() {
static std::set<std::string> protectedAttrs = {
"key",
"kind",
"_meta"
};
return protectedAttrs;
}

// Utility struct to enforce the type-safe setting of a particular attribute
// via a method on AttributesBuilder.
struct TypedAttribute {
enum Value::Type type;
std::function<void(AttributesBuilder<ContextBuilder, Context>& builder,
Value)> setter;

TypedAttribute(enum Value::Type type,
std::function<void(
AttributesBuilder<ContextBuilder, Context>& builder,
Value const&)> setter)
: type(type), setter(std::move(setter)) {
}
};

// These attribute values, while able to be set via public Set/SetPrivate methods,
// are regarded as special and are type-enforced. Instead of being stored in the
// internal AttributesBuilder::values map, they are stored as individual fields.
// This is because they have special meaning/usage within the LaunchDarkly Platform.
std::unordered_map<std::string, TypedAttribute> const&
TypedAttributes() {
static std::unordered_map<std::string, TypedAttribute> typedAttrs = {
{
"anonymous", {Value::Type::kBool,
[](AttributesBuilder<
ContextBuilder, Context>
& builder,
Value const& value) {
builder.Anonymous(
value.AsBool());
}}
},
{"name", {Value::Type::kString,
[](AttributesBuilder<
ContextBuilder, Context>&
builder,
Value const& value) {
builder.Name(value.AsString());
}}}
};
return typedAttrs;
}


template <>
AttributesBuilder<ContextBuilder, Context>&
AttributesBuilder<ContextBuilder, Context>::Set(std::string name,
Value value,
bool private_attribute) {
if (name == "key" || name == "kind" || name == "anonymous" ||
name == "name" || name == "_meta") {
// Protected attributes cannot be set at all.
if (ProtectedAttributes().count(name) > 0) {
return *this;
}

// Typed attributes can be set only if the value is of the correct type;
// the setting is forwarded to one of AttributeBuilder's dedicated methods.
auto const typed_attr = TypedAttributes().find(name);
const bool is_typed = typed_attr != TypedAttributes().end();
if (is_typed && value.Type() != typed_attr->second.type) {
return *this;
}

if (is_typed) {
typed_attr->second.setter(*this, value);
} else {
values_[name] = std::move(value);
}

if (private_attribute) {
this->private_attributes_.insert(name);
this->private_attributes_.insert(std::move(name));
}
values_[std::move(name)] = std::move(value);
return *this;
}

template <>
AttributesBuilder<ContextBuilder, Context>&
AttributesBuilder<ContextBuilder, Context>::Set(std::string name, Value value) {
AttributesBuilder<ContextBuilder, Context>::Set(
std::string name,
Value value) {
return Set(std::move(name), std::move(value), false);
}

template <>
AttributesBuilder<ContextBuilder, Context>&
AttributesBuilder<ContextBuilder, Context>::SetPrivate(std::string name,
Value value) {
Value value) {
return Set(std::move(name), std::move(value), true);
}

@@ -56,8 +130,8 @@
}

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

} // namespace launchdarkly
} // namespace launchdarkly
Loading