Skip to content
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

fix: refactor Variation methods for correctness & clarity #203

Merged
merged 57 commits into from
Aug 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
607024a
add <array> header
cwaldren-ld Aug 16, 2023
f49d534
add another missing <array>
cwaldren-ld Aug 16, 2023
53c8c54
rename sdk-contract-tests to client-contract-tests
cwaldren-ld Jul 20, 2023
d14350b
copy client-side contract test server to server-side
cwaldren-ld Jul 21, 2023
f1ebea7
refactor contract tests data model into dedicated library
cwaldren-ld Jul 21, 2023
aa85920
fix client name and version in contract test binaries
cwaldren-ld Aug 15, 2023
709c995
update evaluation/evaluation detail commands to deserialize contexts
cwaldren-ld Aug 15, 2023
5406b59
re-enable client-entity
cwaldren-ld Aug 15, 2023
e45741b
value-init data model types during deserialization
cwaldren-ld Aug 16, 2023
298c6d8
add server Github action workflow
cwaldren-ld Aug 16, 2023
d1976df
update client Github action workflow to fix contract test binary path
cwaldren-ld Aug 16, 2023
dbb541d
be accepting of an empty PUT path
cwaldren-ld Aug 16, 2023
0b3f00a
more suppressions
cwaldren-ld Aug 17, 2023
ac96b10
fix all custom event contract tests
cwaldren-ld Aug 17, 2023
2d4e5b8
attempt to make CI run
cwaldren-ld Aug 17, 2023
0829bca
fix some event tests
cwaldren-ld Aug 18, 2023
e2d01df
remove a bunch of suppressions
cwaldren-ld Aug 18, 2023
37e604a
add missing version to feature events
cwaldren-ld Aug 18, 2023
22911a2
fix debug event tests
cwaldren-ld Aug 18, 2023
2ddf672
fix some feature event emission
cwaldren-ld Aug 18, 2023
a41423f
remove old PR branch from server.yml
cwaldren-ld Aug 21, 2023
d5ce5af
update comment
cwaldren-ld Aug 21, 2023
0145069
add server.yml
cwaldren-ld Aug 17, 2023
57f5f9c
initial implementation of AllFlagsState method and structs
cwaldren-ld Aug 18, 2023
e9c068d
begin adding unit tests
cwaldren-ld Aug 18, 2023
e578ad1
unsupress contract tests due to lack of AllFlagsState API
cwaldren-ld Aug 18, 2023
9b9a984
cleanup AllFlagsState api and types
cwaldren-ld Aug 21, 2023
796ff70
doc comments on AllFlagsState
cwaldren-ld Aug 21, 2023
3713595
renamings
cwaldren-ld Aug 21, 2023
144d705
refactor AllFlagsStateOptions into enum nested in AllFlagsState struct
cwaldren-ld Aug 21, 2023
059b21a
fix build
cwaldren-ld Aug 21, 2023
43f27cb
fix regressed test
cwaldren-ld Aug 21, 2023
07663f7
revert change to DetailsOnlyForTrackedFlags unit test
cwaldren-ld Aug 21, 2023
daef5cb
only return empty AllFlagsState if not initialized and memory store u…
cwaldren-ld Aug 22, 2023
47ad8bb
additional suppressions for bucketing
cwaldren-ld Aug 22, 2023
ca9a74e
refactor variation methods to separate out event generation
cwaldren-ld Aug 22, 2023
d5cd0fa
refactor IsExperimentationEnabled into data_model::Flag
cwaldren-ld Aug 22, 2023
219d7a9
remove a bunch of evaluation suppressions
cwaldren-ld Aug 22, 2023
33944c7
remove some event suppressions
cwaldren-ld Aug 22, 2023
41a9e4c
remove more event suppressions
cwaldren-ld Aug 23, 2023
14e0c14
remove obsolete methods
cwaldren-ld Aug 23, 2023
1a3a405
better encapsulation of EventScope
cwaldren-ld Aug 23, 2023
d8754a5
re-add variation call logging
cwaldren-ld Aug 23, 2023
405693f
const correctness for EventScope
cwaldren-ld Aug 23, 2023
1cf9666
add const to LogVariationCall
cwaldren-ld Aug 23, 2023
4ce9dc9
refactor event scope to pass IEventProcessor in callback
cwaldren-ld Aug 23, 2023
63e3ad1
remove disabled bool from EventScope and rely on polymorphism
cwaldren-ld Aug 23, 2023
275b238
Merge branch 'server-side' into cw/sc-213848/refactor-variation-methods
cwaldren-ld Aug 23, 2023
2e49214
rename EventScope::Get to Send
cwaldren-ld Aug 23, 2023
5b7e08e
remove NullEventProcessor from server-side SDK and document EventScope
cwaldren-ld Aug 23, 2023
a6589e7
feat: implement prerequisite event recording (#204)
cwaldren-ld Aug 23, 2023
6b1f647
add unit tests for EventScope
cwaldren-ld Aug 23, 2023
15f362e
fix evaluator tests
cwaldren-ld Aug 23, 2023
b1d1398
add some event factory tests
cwaldren-ld Aug 23, 2023
8b0fbac
typo
cwaldren-ld Aug 23, 2023
73b8900
Merge branch 'server-side' into cw/sc-213848/refactor-variation-methods
cwaldren-ld Aug 24, 2023
1cf17bb
fix a lint
cwaldren-ld Aug 24, 2023
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
112 changes: 0 additions & 112 deletions contract-tests/server-contract-tests/test-suppressions.txt
Original file line number Diff line number Diff line change
@@ -1,111 +1,3 @@
# SC-214431
evaluation/parameterized/bad attribute reference errors - clause with no attribute/test-flag/evaluate flag with detail
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woohoo! Just streaming issues left.

evaluation/parameterized/bad attribute reference errors - empty path component/test-flag/evaluate flag with detail
evaluation/parameterized/bad attribute reference errors - tilde followed by invalid escape character/test-flag/evaluate flag with detail
evaluation/parameterized/bad attribute reference errors - tilde followed by nothing/test-flag/evaluate flag with detail
evaluation/parameterized/evaluation failures (bool)/off variation too low/off variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (bool)/off variation too high/off variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (bool)/flag is off but has no off variation/flag is off but has no off variation/evaluate flag with detail
evaluation/parameterized/evaluation failures (bool)/fallthrough variation too low/fallthrough variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (bool)/fallthrough variation too high/fallthrough variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (bool)/target variation too low/target variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (bool)/target variation too high/target variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (bool)/rule variation too low/rule variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (bool)/rule variation too high/rule variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (int)/off variation too low/off variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (int)/off variation too high/off variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (int)/flag is off but has no off variation/flag is off but has no off variation/evaluate flag with detail
evaluation/parameterized/evaluation failures (int)/fallthrough variation too low/fallthrough variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (int)/fallthrough variation too high/fallthrough variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (int)/target variation too low/target variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (int)/target variation too high/target variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (int)/rule variation too low/rule variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (int)/rule variation too high/rule variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (double)/off variation too low/off variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (double)/off variation too high/off variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (double)/flag is off but has no off variation/flag is off but has no off variation/evaluate flag with detail
evaluation/parameterized/evaluation failures (double)/fallthrough variation too low/fallthrough variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (double)/fallthrough variation too high/fallthrough variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (double)/target variation too low/target variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (double)/target variation too high/target variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (double)/rule variation too low/rule variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (double)/rule variation too high/rule variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (string)/off variation too low/off variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (string)/off variation too high/off variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (string)/flag is off but has no off variation/flag is off but has no off variation/evaluate flag with detail
evaluation/parameterized/evaluation failures (string)/fallthrough variation too low/fallthrough variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (string)/fallthrough variation too high/fallthrough variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (string)/target variation too low/target variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (string)/target variation too high/target variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (string)/rule variation too low/rule variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (string)/rule variation too high/rule variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (any)/off variation too low/off variation too low/evaluate flag without detail
evaluation/parameterized/evaluation failures (any)/off variation too low/off variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (any)/off variation too high/off variation too high/evaluate flag without detail
evaluation/parameterized/evaluation failures (any)/off variation too high/off variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (any)/flag is off but has no off variation/flag is off but has no off variation/evaluate flag without detail
evaluation/parameterized/evaluation failures (any)/flag is off but has no off variation/flag is off but has no off variation/evaluate flag with detail
evaluation/parameterized/evaluation failures (any)/fallthrough variation too low/fallthrough variation too low/evaluate flag without detail
evaluation/parameterized/evaluation failures (any)/fallthrough variation too low/fallthrough variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (any)/fallthrough variation too high/fallthrough variation too high/evaluate flag without detail
evaluation/parameterized/evaluation failures (any)/fallthrough variation too high/fallthrough variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (any)/target variation too low/target variation too low/evaluate flag without detail
evaluation/parameterized/evaluation failures (any)/target variation too low/target variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (any)/target variation too high/target variation too high/evaluate flag without detail
evaluation/parameterized/evaluation failures (any)/target variation too high/target variation too high/evaluate flag with detail
evaluation/parameterized/evaluation failures (any)/rule variation too low/rule variation too low/evaluate flag without detail
evaluation/parameterized/evaluation failures (any)/rule variation too low/rule variation too low/evaluate flag with detail
evaluation/parameterized/evaluation failures (any)/rule variation too high/rule variation too high/evaluate flag without detail
evaluation/parameterized/evaluation failures (any)/rule variation too high/rule variation too high/evaluate flag with detail
evaluation/parameterized/prerequisites/prerequisite cycle is detected at top level, recursion stops/prerequisite cycle is detected at top level, recursion stops/evaluate flag with detail
evaluation/parameterized/prerequisites/prerequisite cycle is detected at top level, recursion stops/prerequisite cycle is detected at top level, recursion stops/evaluate all flags
evaluation/parameterized/prerequisites/prerequisite cycle is detected at deeper level, recursion stops/prerequisite cycle is detected at deeper level, recursion stops/evaluate flag with detail
evaluation/parameterized/prerequisites/prerequisite cycle is detected at deeper level, recursion stops/prerequisite cycle is detected at deeper level, recursion stops/evaluate all flags
evaluation/parameterized/rollout or experiment - error for empty variations list in rollout/fallthrough rollout/fallthrough rollout/evaluate flag with detail
evaluation/parameterized/rollout or experiment - error for empty variations list in rollout/rule rollout/rule rollout/evaluate flag with detail
evaluation/parameterized/segment recursion/cycle is detected at top level, recursion stops/cycle is detected at top level, recursion stops/evaluate flag with detail
evaluation/parameterized/segment recursion/cycle is detected below top level, recursion stops/cycle is detected below top level, recursion stops/evaluate flag with detail
events/summary events/prerequisites
events/feature events/full feature event for tracked flag/without reason/single kind default/malformed flag/type: bool
events/feature events/full feature event for tracked flag/without reason/single kind default/malformed flag/type: int
events/feature events/full feature event for tracked flag/without reason/single kind default/malformed flag/type: double
events/feature events/full feature event for tracked flag/without reason/single kind default/malformed flag/type: string
events/feature events/full feature event for tracked flag/without reason/single kind default/malformed flag/type: any
events/feature events/full feature event for tracked flag/without reason/single kind non-default/valid flag/type: bool
events/feature events/full feature event for tracked flag/without reason/single kind non-default/malformed flag/type: bool
events/feature events/full feature event for tracked flag/without reason/single kind non-default/malformed flag/type: int
events/feature events/full feature event for tracked flag/without reason/single kind non-default/malformed flag/type: double
events/feature events/full feature event for tracked flag/without reason/single kind non-default/malformed flag/type: string
events/feature events/full feature event for tracked flag/without reason/single kind non-default/malformed flag/type: any
events/feature events/full feature event for tracked flag/without reason/multi-kind/valid flag/type: bool
events/feature events/full feature event for tracked flag/without reason/multi-kind/malformed flag/type: bool
events/feature events/full feature event for tracked flag/without reason/multi-kind/malformed flag/type: int
events/feature events/full feature event for tracked flag/without reason/multi-kind/malformed flag/type: double
events/feature events/full feature event for tracked flag/without reason/multi-kind/malformed flag/type: string
events/feature events/full feature event for tracked flag/without reason/multi-kind/malformed flag/type: any
events/feature events/full feature event for tracked flag/with reason/single kind default/valid flag/type: bool
events/feature events/full feature event for tracked flag/with reason/single kind default/malformed flag/type: bool
events/feature events/full feature event for tracked flag/with reason/single kind default/malformed flag/type: int
events/feature events/full feature event for tracked flag/with reason/single kind default/malformed flag/type: double
events/feature events/full feature event for tracked flag/with reason/single kind default/malformed flag/type: string
events/feature events/full feature event for tracked flag/with reason/single kind default/malformed flag/type: any
events/feature events/full feature event for tracked flag/with reason/single kind non-default/valid flag/type: bool
events/feature events/full feature event for tracked flag/with reason/single kind non-default/malformed flag/type: bool
events/feature events/full feature event for tracked flag/with reason/single kind non-default/malformed flag/type: int
events/feature events/full feature event for tracked flag/with reason/single kind non-default/malformed flag/type: double
events/feature events/full feature event for tracked flag/with reason/single kind non-default/malformed flag/type: string
events/feature events/full feature event for tracked flag/with reason/single kind non-default/malformed flag/type: any
events/feature events/full feature event for tracked flag/with reason/multi-kind/valid flag/type: bool
events/feature events/full feature event for tracked flag/with reason/multi-kind/malformed flag/type: bool
events/feature events/full feature event for tracked flag/with reason/multi-kind/malformed flag/type: int
events/feature events/full feature event for tracked flag/with reason/multi-kind/malformed flag/type: double
events/feature events/full feature event for tracked flag/with reason/multi-kind/malformed flag/type: string
events/feature events/full feature event for tracked flag/with reason/multi-kind/malformed flag/type: any
events/feature events/evaluating all flags generates no events
events/feature prerequisite events/without reasons
events/feature prerequisite events/with reasons
events/experimentation/experiment in rule
events/experimentation/experiment in fallthrough
streaming/updates/flag patch with same version is not applied
streaming/updates/segment patch with same version is not applied
streaming/updates/flag patch with lower version is not applied
Expand Down Expand Up @@ -162,7 +54,3 @@ context type/convert/old user to context/{"key": "a", "avatar": "b"}
context type/convert/old user to context/{"key": "a", "avatar": null}
context type/convert/old user to context/{"key": "a", "ip": "b"}
context type/convert/old user to context/{"key": "a", "ip": null}

# These appear to only fail in CI, but not locally on Mac. Requires investigation.
evaluation/bucketing/bucket by non-key attribute/in rollouts/invalid value type
evaluation/bucketing/bucket by non-key attribute/in rollouts/attribute not found
4 changes: 4 additions & 0 deletions libs/internal/include/launchdarkly/data_model/flag.hpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#include <launchdarkly/data/evaluation_reason.hpp>
#include <launchdarkly/data_model/context_aware_reference.hpp>
#include <launchdarkly/data_model/context_kind.hpp>
#include <launchdarkly/data_model/rule_clause.hpp>
Expand All @@ -21,7 +22,7 @@
using FlagVersion = std::uint64_t;
using Date = std::uint64_t;

struct Rollout {

Check warning on line 25 in libs/internal/include/launchdarkly/data_model/flag.hpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/include/launchdarkly/data_model/flag.hpp:25:12 [cppcoreguidelines-pro-type-member-init]

constructor does not initialize these fields: kind
enum class Kind {
kUnrecognized = 0,
kExperiment = 1,
Expand Down Expand Up @@ -104,5 +105,8 @@
* @return Version of this flag.
*/
[[nodiscard]] inline std::uint64_t Version() const { return version; }

[[nodiscard]] bool IsExperimentationEnabled(
std::optional<EvaluationReason> const& reason) const;
};
} // namespace launchdarkly::data_model
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ struct FeatureEventParams {
std::optional<Reason> reason;
bool require_full_event;
std::optional<Date> debug_events_until_date;
std::optional<std::string> prereq_of;
};

struct FeatureEventBase {
Expand All @@ -72,6 +73,7 @@ struct FeatureEventBase {
Value value;
std::optional<Reason> reason;
Value default_;
std::optional<std::string> prereq_of;

explicit FeatureEventBase(FeatureEventParams const& params);
};
Expand Down
22 changes: 22 additions & 0 deletions libs/internal/src/data_model/flag.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
Flag::Weight weight_)
: WeightedVariation(variation_, weight_, false) {}

Flag::Rollout::WeightedVariation::WeightedVariation(Flag::Variation variation_,

Check warning on line 9 in libs/internal/src/data_model/flag.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/src/data_model/flag.cpp:9:53 [bugprone-easily-swappable-parameters]

2 adjacent parameters of 'WeightedVariation' of similar type are easily swapped by mistake
Flag::Weight weight_,
bool untracked_)
: variation(variation_), weight(weight_), untracked(untracked_) {}
Expand All @@ -26,4 +26,26 @@
bucketBy("key"),
contextKind("user") {}

bool Flag::IsExperimentationEnabled(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moving this from where the AllFlagsState needed it internally into a public place, since it's used more broadly now.

std::optional<EvaluationReason> const& reason) const {
if (!reason) {
return false;
}
if (reason->InExperiment()) {
return true;
}
switch (reason->Kind()) {
case EvaluationReason::Kind::kFallthrough:
return this->trackEventsFallthrough;
case EvaluationReason::Kind::kRuleMatch:
if (!reason->RuleIndex() ||
reason->RuleIndex() >= this->rules.size()) {
return false;
}
return this->rules.at(*reason->RuleIndex()).trackEvents;

Check warning on line 45 in libs/internal/src/data_model/flag.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/src/data_model/flag.cpp:45:36 [bugprone-unchecked-optional-access]

unchecked access to optional value
default:
return false;
}
}

} // namespace launchdarkly::data_model
3 changes: 2 additions & 1 deletion libs/internal/src/events/common_events.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ FeatureEventBase::FeatureEventBase(FeatureEventParams const& params)
variation(params.variation),
value(params.value),
reason(params.reason),
default_(params.default_) {}
default_(params.default_),
prereq_of(params.prereq_of) {}
} // namespace launchdarkly::events
3 changes: 3 additions & 0 deletions libs/internal/src/serialization/events/json_events.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
obj.emplace("reason", boost::json::value_from(*event.reason));
}
obj.emplace("default", boost::json::value_from(event.default_));
if (event.prereq_of) {
obj.emplace("prereqOf", *event.prereq_of);
}
}

void tag_invoke(boost::json::value_from_tag const& tag,
Expand All @@ -53,7 +56,7 @@

namespace launchdarkly::events::server_side {

void tag_invoke(boost::json::value_from_tag const&,

Check warning on line 59 in libs/internal/src/serialization/events/json_events.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/src/serialization/events/json_events.cpp:59:51 [readability-named-parameter]

all parameters should be named in a function
boost::json::value& json_value,
IndexEvent const& event) {
auto& obj = json_value.emplace_object();
Expand Down
1 change: 1 addition & 0 deletions libs/server-sdk/src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ add_library(${LIBNAME}
data_store/persistent/expiration_tracker.hpp
data_store/persistent/persistent_data_store.cpp
data_store/persistent/expiration_tracker.cpp
events/event_factory.cpp
)

if (MSVC OR (NOT BUILD_SHARED_LIBS))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@

namespace launchdarkly::server_side {

bool IsExperimentationEnabled(data_model::Flag const& flag,
std::optional<EvaluationReason> const& reason);

bool IsSet(AllFlagsState::Options options, AllFlagsState::Options flag);
bool NotSet(AllFlagsState::Options options, AllFlagsState::Options flag);

Expand Down
Loading
Loading