Skip to content

Commit 8fa8ae8

Browse files
m-olkom-olko
authored
Changed UpdateFlags function to return absl::Status (#114)
## This PR Update the `FlagSync:UpdateFlags` interface to return `absl::Status` and return errors on validation problems. The change also includes updating all the tests to support that. ### Related Issues Fixes #113 Signed-off-by: Marcin Olko <molko@google.com>
1 parent b217075 commit 8fa8ae8

7 files changed

Lines changed: 69 additions & 53 deletions

File tree

‎providers/flagd/src/sync/grpc/grpc_sync.cpp‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,11 @@ GrpcSync::StreamResult GrpcSync::ProcessStream(
236236
continue;
237237
}
238238

239-
UpdateFlags(raw);
239+
absl::Status status = UpdateFlags(raw);
240+
if (!status.ok()) {
241+
LOG(ERROR) << "Failed to update flags: " << status;
242+
continue;
243+
}
240244

241245
if (!connected) {
242246
connected = true;

‎providers/flagd/src/sync/sync.cpp‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include <nlohmann/json.hpp>
77

88
#include "absl/log/log.h"
9+
#include "absl/strings/str_cat.h"
910
#include "embedded_schemas.h"
1011

1112
using Json = nlohmann::json;
@@ -38,13 +39,13 @@ class FlagSync::Validator {
3839
validator.set_root_schema(Json::parse(schema::schemas.at("flagd.json")));
3940
}
4041

41-
bool Validate(const Json& json) const {
42+
absl::Status Validate(const Json& json) const {
4243
try {
4344
validator.validate(json);
44-
return true;
45+
return absl::OkStatus();
4546
} catch (const std::exception& e) {
46-
LOG(ERROR) << "Flag configuration validation failed: " << e.what();
47-
return false;
47+
return absl::InvalidArgumentError(
48+
absl::StrCat("Flag configuration validation failed: ", e.what()));
4849
}
4950
}
5051
};
@@ -58,12 +59,11 @@ FlagSync::FlagSync()
5859

5960
FlagSync::~FlagSync() = default;
6061

61-
void FlagSync::UpdateFlags(const nlohmann::json& new_json) {
62+
absl::Status FlagSync::UpdateFlags(const nlohmann::json& new_json) {
6263
if (validator_) {
63-
if (!validator_->Validate(new_json)) {
64-
// Validation failed, do not update flags.
65-
LOG(ERROR) << "Flag configuration validation failed, skipping update.";
66-
return;
64+
absl::Status status = validator_->Validate(new_json);
65+
if (!status.ok()) {
66+
return status;
6767
}
6868
}
6969

@@ -83,6 +83,8 @@ void FlagSync::UpdateFlags(const nlohmann::json& new_json) {
8383
current_flags_ = std::move(new_flags_snapshot);
8484
global_metadata_ = std::move(new_metadata_snapshot);
8585
}
86+
87+
return absl::OkStatus();
8688
}
8789

8890
void FlagSync::ClearFlags() {

‎providers/flagd/src/sync/sync.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ class FlagSync {
2222
std::shared_ptr<const nlohmann::json> GetMetadata() const;
2323

2424
protected:
25-
void UpdateFlags(const nlohmann::json& new_json);
25+
absl::Status UpdateFlags(const nlohmann::json& new_json);
2626
void ClearFlags();
2727

2828
private:

‎providers/flagd/tests/evaluator/evaluator_test.cpp‎

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ class TestableSync : public flagd::FlagSync {
1616
}
1717
absl::Status Shutdown() override { return absl::OkStatus(); }
1818

19-
void TriggerUpdate(const nlohmann::json& new_json) {
20-
this->UpdateFlags(new_json);
19+
absl::Status TriggerUpdate(const nlohmann::json& new_json) {
20+
return this->UpdateFlags(new_json);
2121
}
2222
};
2323

@@ -46,7 +46,7 @@ TEST_F(EvaluatorTest, ResolveBooleanSuccess) {
4646
}
4747
})");
4848

49-
sync_->TriggerUpdate(flags);
49+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
5050

5151
openfeature::EvaluationContext ctx =
5252
openfeature::EvaluationContext::Builder().build();
@@ -63,7 +63,7 @@ TEST_F(EvaluatorTest, ResolveBooleanFlagNotFound) {
6363
nlohmann::json flags = nlohmann::json::parse(R"({
6464
"flags": {}
6565
})");
66-
sync_->TriggerUpdate(flags);
66+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
6767

6868
openfeature::EvaluationContext ctx =
6969
openfeature::EvaluationContext::Builder().build();
@@ -89,7 +89,7 @@ TEST_F(EvaluatorTest, ResolveBooleanTypeMismatch) {
8989
}
9090
})");
9191

92-
sync_->TriggerUpdate(flags);
92+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
9393

9494
openfeature::EvaluationContext ctx =
9595
openfeature::EvaluationContext::Builder().build();
@@ -123,7 +123,7 @@ TEST_F(EvaluatorTest, ResolveBooleanMetadata) {
123123
}
124124
})");
125125

126-
sync_->TriggerUpdate(flags);
126+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
127127

128128
openfeature::EvaluationContext ctx =
129129
openfeature::EvaluationContext::Builder().build();
@@ -154,7 +154,7 @@ TEST_F(EvaluatorTest, ResolveBooleanVariantNotFound) {
154154
}
155155
})");
156156

157-
sync_->TriggerUpdate(flags);
157+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
158158

159159
openfeature::EvaluationContext ctx =
160160
openfeature::EvaluationContext::Builder().build();
@@ -183,7 +183,7 @@ TEST_F(EvaluatorTest, ResolveStringSuccess) {
183183
}
184184
})");
185185

186-
sync_->TriggerUpdate(flags);
186+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
187187

188188
openfeature::EvaluationContext ctx =
189189
openfeature::EvaluationContext::Builder().build();
@@ -209,7 +209,7 @@ TEST_F(EvaluatorTest, ResolveIntegerSuccess) {
209209
}
210210
})");
211211

212-
sync_->TriggerUpdate(flags);
212+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
213213

214214
openfeature::EvaluationContext ctx =
215215
openfeature::EvaluationContext::Builder().build();
@@ -235,7 +235,7 @@ TEST_F(EvaluatorTest, ResolveDoubleSuccess) {
235235
}
236236
})");
237237

238-
sync_->TriggerUpdate(flags);
238+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
239239

240240
openfeature::EvaluationContext ctx =
241241
openfeature::EvaluationContext::Builder().build();
@@ -266,7 +266,7 @@ TEST_F(EvaluatorTest, ResolveObjectSuccess) {
266266
}
267267
})");
268268

269-
sync_->TriggerUpdate(flags);
269+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
270270

271271
openfeature::EvaluationContext ctx =
272272
openfeature::EvaluationContext::Builder().build();
@@ -302,7 +302,7 @@ TEST_F(EvaluatorTest, ResolveStringTypeMismatch) {
302302
}
303303
})");
304304

305-
sync_->TriggerUpdate(flags);
305+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
306306

307307
openfeature::EvaluationContext ctx =
308308
openfeature::EvaluationContext::Builder().build();
@@ -327,7 +327,7 @@ TEST_F(EvaluatorTest, ResolveIntegerTypeMismatch) {
327327
}
328328
})");
329329

330-
sync_->TriggerUpdate(flags);
330+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
331331

332332
openfeature::EvaluationContext ctx =
333333
openfeature::EvaluationContext::Builder().build();
@@ -352,7 +352,7 @@ TEST_F(EvaluatorTest, ResolveDoubleTypeMismatch) {
352352
}
353353
})");
354354

355-
sync_->TriggerUpdate(flags);
355+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
356356

357357
openfeature::EvaluationContext ctx =
358358
openfeature::EvaluationContext::Builder().build();
@@ -371,7 +371,7 @@ class EvaluatorDefaultVariantTest
371371
TEST_P(EvaluatorDefaultVariantTest, ResolveBooleanReturnsDefault) {
372372
nlohmann::json flags_json =
373373
nlohmann::json::parse(R"({"flags":)" + GetParam() + "}");
374-
sync_->TriggerUpdate(flags_json);
374+
EXPECT_TRUE(sync_->TriggerUpdate(flags_json).ok());
375375

376376
openfeature::EvaluationContext ctx =
377377
openfeature::EvaluationContext::Builder().build();
@@ -421,7 +421,7 @@ TEST_F(EvaluatorTest, ResolveBooleanDisabled) {
421421
}
422422
})");
423423

424-
sync_->TriggerUpdate(flags);
424+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
425425

426426
openfeature::EvaluationContext ctx =
427427
openfeature::EvaluationContext::Builder().build();
@@ -453,7 +453,7 @@ TEST_F(EvaluatorTest, ResolveStringTargetingSuccess) {
453453
}
454454
})");
455455

456-
sync_->TriggerUpdate(flags);
456+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
457457

458458
// Match targeting rule
459459
openfeature::EvaluationContext ctx_blue =
@@ -504,7 +504,7 @@ TEST_F(EvaluatorTest, ResolveIntegerComplexTargeting) {
504504
}
505505
})");
506506

507-
sync_->TriggerUpdate(flags);
507+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
508508

509509
openfeature::EvaluationContext ctx = openfeature::EvaluationContext::Builder()
510510
.WithAttribute("env", "prod")
@@ -538,7 +538,7 @@ TEST_F(EvaluatorTest, ResolveObjectNestedStructure) {
538538
}
539539
})");
540540

541-
sync_->TriggerUpdate(flags);
541+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
542542

543543
openfeature::EvaluationContext ctx =
544544
openfeature::EvaluationContext::Builder().build();
@@ -587,7 +587,7 @@ TEST_F(EvaluatorTest, ResolveBooleanFlagdSpecialVars) {
587587
}
588588
})");
589589

590-
sync_->TriggerUpdate(flags);
590+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
591591

592592
openfeature::EvaluationContext ctx =
593593
openfeature::EvaluationContext::Builder().build();
@@ -612,7 +612,7 @@ TEST_F(EvaluatorTest, ResolveBooleanTypeMismatchInTargeting) {
612612
}
613613
})");
614614

615-
sync_->TriggerUpdate(flags);
615+
EXPECT_TRUE(sync_->TriggerUpdate(flags).ok());
616616

617617
openfeature::EvaluationContext ctx = openfeature::EvaluationContext::Builder()
618618
.WithAttribute("color", "blue")

‎providers/flagd/tests/smoke/openfeature.cpp‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@ class MockSync : public flagd::FlagSync {
1616
}
1717
absl::Status Shutdown() override { return absl::OkStatus(); }
1818

19-
void SetFlags(const nlohmann::json& flags) { this->UpdateFlags(flags); }
19+
absl::Status SetFlags(const nlohmann::json& flags) {
20+
return this->UpdateFlags(flags);
21+
}
2022
};
2123

2224
class FlagdOpenFeatureTest : public ::testing::Test {
@@ -54,7 +56,7 @@ TEST_F(FlagdOpenFeatureTest, BooleanEvaluation) {
5456
}
5557
}
5658
})");
57-
sync_->SetFlags(flags);
59+
EXPECT_TRUE(sync_->SetFlags(flags).ok());
5860

5961
EXPECT_TRUE(client_->GetBooleanValue("bool-flag", false));
6062
EXPECT_FALSE(client_->GetBooleanValue("non-existent", false));
@@ -73,7 +75,7 @@ TEST_F(FlagdOpenFeatureTest, StringEvaluation) {
7375
}
7476
}
7577
})");
76-
sync_->SetFlags(flags);
78+
EXPECT_TRUE(sync_->SetFlags(flags).ok());
7779

7880
EXPECT_EQ(client_->GetStringValue("string-flag", "default"), "value2");
7981
}
@@ -91,7 +93,7 @@ TEST_F(FlagdOpenFeatureTest, IntegerEvaluation) {
9193
}
9294
}
9395
})");
94-
sync_->SetFlags(flags);
96+
EXPECT_TRUE(sync_->SetFlags(flags).ok());
9597

9698
EXPECT_EQ(client_->GetIntegerValue("int-flag", 0), 1);
9799
}
@@ -109,7 +111,7 @@ TEST_F(FlagdOpenFeatureTest, DoubleEvaluation) {
109111
}
110112
}
111113
})");
112-
sync_->SetFlags(flags);
114+
EXPECT_TRUE(sync_->SetFlags(flags).ok());
113115

114116
EXPECT_DOUBLE_EQ(client_->GetDoubleValue("double-flag", 0.0), 2.2);
115117
}
@@ -128,7 +130,7 @@ TEST_F(FlagdOpenFeatureTest, ObjectEvaluation) {
128130
}
129131
}
130132
})");
131-
sync_->SetFlags(flags);
133+
EXPECT_TRUE(sync_->SetFlags(flags).ok());
132134

133135
openfeature::Value val =
134136
client_->GetObjectValue("obj-flag", openfeature::Value());
@@ -159,7 +161,7 @@ TEST_F(FlagdOpenFeatureTest, EvaluationContextTargeting) {
159161
}
160162
}
161163
})");
162-
sync_->SetFlags(flags);
164+
EXPECT_TRUE(sync_->SetFlags(flags).ok());
163165

164166
// Without context, should return defaultVariant "red"
165167
EXPECT_EQ(client_->GetStringValue("targeting-flag", "default"), "red-value");

‎providers/flagd/tests/smoke/sync.cpp‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,8 @@ class MockSync : public flagd::FlagSync {
1818
(override));
1919
MOCK_METHOD(absl::Status, Shutdown, (), (override));
2020

21-
void TriggerUpdate(const nlohmann::json& new_json) {
22-
this->UpdateFlags(new_json);
21+
absl::Status TriggerUpdate(const nlohmann::json& new_json) {
22+
return this->UpdateFlags(new_json);
2323
}
2424
};
2525

@@ -55,7 +55,7 @@ TEST_F(FlagSyncTest, HelperMethodsUpdateAndRetrieveFlags) {
5555
}
5656
})"_json;
5757

58-
sync_.TriggerUpdate(expected_flags);
58+
EXPECT_TRUE(sync_.TriggerUpdate(expected_flags).ok());
5959

6060
result_ptr = sync_.GetFlags();
6161

@@ -82,7 +82,7 @@ TEST_F(FlagSyncTest, ThreadSafetyReadersAndWriters) {
8282
const int k_iterations = 5000;
8383
std::atomic<bool> start_flag{false};
8484

85-
auto writer_func = [&]() {
85+
auto writer_func = [&] {
8686
while (!start_flag.load());
8787

8888
for (int i = 0; i < k_iterations; ++i) {
@@ -99,11 +99,11 @@ TEST_F(FlagSyncTest, ThreadSafetyReadersAndWriters) {
9999
"metadata": {}
100100
})"_json;
101101
update["flags"]["myFlag"]["variants"]["iteration"] = i;
102-
sync_.TriggerUpdate(update);
102+
EXPECT_TRUE(sync_.TriggerUpdate(update).ok());
103103
}
104104
};
105105

106-
auto reader_func = [&]() {
106+
auto reader_func = [&] {
107107
while (!start_flag.load());
108108

109109
for (int i = 0; i < k_iterations; ++i) {

0 commit comments

Comments
 (0)