Skip to content

Commit 8531125

Browse files
eddyashtoneddyashton
authored
Fix and test node status filter #2 (#2338)
1 parent 429a88e commit 8531125

5 files changed

Lines changed: 160 additions & 22 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
55
The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
66
and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
77

8+
## [0.19.3]
9+
10+
### Changed
11+
12+
- The status filter passed to `/node/network/nodes` now takes the correct CamelCased values (#2238).
13+
814
## [0.19.2]
915

1016
### Added
@@ -756,6 +762,7 @@ Some discrepancies with the TR remain, and are being tracked under https://githu
756762

757763
Initial pre-release
758764

765+
[0.19.3]: https://github.com/microsoft/CCF/releases/tag/ccf-0.19.3
759766
[0.19.2]: https://github.com/microsoft/CCF/releases/tag/ccf-0.19.2
760767
[0.19.1]: https://github.com/microsoft/CCF/releases/tag/ccf-0.19.1
761768
[0.19.0]: https://github.com/microsoft/CCF/releases/tag/ccf-0.19.0

‎src/ds/json.h‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -753,8 +753,46 @@ namespace std
753753
(POP2)(ADD_SCHEMA_COMPONENTS_OPTIONAL_WITH_RENAMES, TYPE, ##__VA_ARGS__); \
754754
}
755755

756+
// Enum conversion, based on NLOHMANN_JSON_SERIALIZE_ENUM, but less permissive
757+
// (throws on unknown JSON values)
756758
#define DECLARE_JSON_ENUM(TYPE, ...) \
757-
NLOHMANN_JSON_SERIALIZE_ENUM(TYPE, __VA_ARGS__) \
759+
template <typename BasicJsonType> \
760+
inline void to_json(BasicJsonType& j, const TYPE& e) \
761+
{ \
762+
static_assert(std::is_enum<TYPE>::value, #TYPE " must be an enum!"); \
763+
static const std::pair<TYPE, BasicJsonType> m[] = __VA_ARGS__; \
764+
auto it = std::find_if( \
765+
std::begin(m), \
766+
std::end(m), \
767+
[e](const std::pair<TYPE, BasicJsonType>& ej_pair) -> bool { \
768+
return ej_pair.first == e; \
769+
}); \
770+
if (it == std::end(m)) \
771+
{ \
772+
throw JsonParseError(fmt::format( \
773+
"Value {} in enum " #TYPE " has no specified JSON conversion", \
774+
(size_t)e)); \
775+
} \
776+
j = it->second; \
777+
} \
778+
template <typename BasicJsonType> \
779+
inline void from_json(const BasicJsonType& j, TYPE& e) \
780+
{ \
781+
static_assert(std::is_enum<TYPE>::value, #TYPE " must be an enum!"); \
782+
static const std::pair<TYPE, BasicJsonType> m[] = __VA_ARGS__; \
783+
auto it = std::find_if( \
784+
std::begin(m), \
785+
std::end(m), \
786+
[&j](const std::pair<TYPE, BasicJsonType>& ej_pair) -> bool { \
787+
return ej_pair.second == j; \
788+
}); \
789+
if (it == std::end(m)) \
790+
{ \
791+
throw JsonParseError( \
792+
fmt::format("{} is not convertible to " #TYPE, j.dump())); \
793+
} \
794+
e = it->first; \
795+
} \
758796
inline std::string schema_name(const TYPE&) \
759797
{ \
760798
return #TYPE; \

‎src/ds/test/json_schema.cpp‎

Lines changed: 73 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,8 @@ struct EnumStruct
374374
{
375375
One,
376376
Two,
377-
Three
377+
Three,
378+
Unconverted // Deliberately omitted from conversion
378379
};
379380

380381
SampleEnum se;
@@ -390,17 +391,81 @@ DECLARE_JSON_REQUIRED_FIELDS(EnumStruct, se);
390391

391392
TEST_CASE("enum")
392393
{
393-
EnumStruct es;
394-
es.se = EnumStruct::SampleEnum::Two;
394+
{
395+
INFO("Schema generation");
396+
EnumStruct es;
397+
es.se = EnumStruct::SampleEnum::Two;
398+
399+
nlohmann::json j = es;
400+
401+
REQUIRE(j["se"] == "two");
402+
403+
const auto schema = ds::json::build_schema<EnumStruct>("EnumStruct");
404+
405+
const nlohmann::json expected{"one", "two", "three"};
406+
REQUIRE(schema["properties"]["se"]["enum"] == expected);
407+
}
408+
409+
{
410+
INFO("from_json");
411+
412+
nlohmann::json j;
413+
414+
// Test good conversions
415+
j = "one";
416+
REQUIRE(j.get<EnumStruct::SampleEnum>() == EnumStruct::SampleEnum::One);
417+
418+
j = "two";
419+
REQUIRE(j.get<EnumStruct::SampleEnum>() == EnumStruct::SampleEnum::Two);
420+
421+
j = "three";
422+
REQUIRE(j.get<EnumStruct::SampleEnum>() == EnumStruct::SampleEnum::Three);
423+
424+
// Any other value will throw
425+
j = "One";
426+
REQUIRE_THROWS(j.get<EnumStruct::SampleEnum>());
395427

396-
nlohmann::json j = es;
428+
j = "two ";
429+
REQUIRE_THROWS(j.get<EnumStruct::SampleEnum>());
397430

398-
REQUIRE(j["se"] == "two");
431+
j = " three";
432+
REQUIRE_THROWS(j.get<EnumStruct::SampleEnum>());
399433

400-
const auto schema = ds::json::build_schema<EnumStruct>("EnumStruct");
434+
j = "penguin";
435+
REQUIRE_THROWS(j.get<EnumStruct::SampleEnum>());
401436

402-
const nlohmann::json expected{"one", "two", "three"};
403-
REQUIRE(schema["properties"]["se"]["enum"] == expected);
437+
j = 0;
438+
REQUIRE_THROWS(j.get<EnumStruct::SampleEnum>());
439+
440+
j = 1;
441+
REQUIRE_THROWS(j.get<EnumStruct::SampleEnum>());
442+
443+
j = nlohmann::json::object();
444+
REQUIRE_THROWS(j.get<EnumStruct::SampleEnum>());
445+
446+
j = nullptr;
447+
REQUIRE_THROWS(j.get<EnumStruct::SampleEnum>());
448+
}
449+
450+
{
451+
INFO("to_json");
452+
453+
nlohmann::json j;
454+
455+
j = EnumStruct::SampleEnum::One;
456+
REQUIRE(j.is_string());
457+
REQUIRE(j.get<std::string>() == "one");
458+
459+
j = EnumStruct::SampleEnum::Two;
460+
REQUIRE(j.is_string());
461+
REQUIRE(j.get<std::string>() == "two");
462+
463+
j = EnumStruct::SampleEnum::Three;
464+
REQUIRE(j.is_string());
465+
REQUIRE(j.get<std::string>() == "three");
466+
467+
REQUIRE_THROWS(j = EnumStruct::SampleEnum::Unconverted);
468+
}
404469
}
405470

406471
namespace examples

‎src/node/rpc/node_frontend.h‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -467,26 +467,20 @@ namespace ccf
467467
std::optional<NodeStatus> status;
468468
if (status_str.has_value())
469469
{
470-
const auto& s = status_str.value();
471-
if (s == "PENDING")
470+
// Convert the query argument to a JSON string, try to parse it as a
471+
// NodeStatus, return an error if this doesn't work
472+
try
472473
{
473-
status = NodeStatus::PENDING;
474+
status = nlohmann::json(status_str.value()).get<NodeStatus>();
474475
}
475-
else if (s == "TRUSTED")
476-
{
477-
status = NodeStatus::TRUSTED;
478-
}
479-
else if (s == "RETIRED")
480-
{
481-
status = NodeStatus::RETIRED;
482-
}
483-
else
476+
catch (const JsonParseError& e)
484477
{
485478
return ccf::make_error(
486479
HTTP_STATUS_BAD_REQUEST,
487480
ccf::errors::InvalidQueryParameterValue,
488481
fmt::format(
489-
"Query parameter '{}' is not a valid node status", s));
482+
"Query parameter '{}' is not a valid node status",
483+
status_str.value()));
490484
}
491485
}
492486

‎tests/reconfiguration.py‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,39 @@ def test_retire_primary(network, args):
145145
return network
146146

147147

148+
@reqs.description("Test node filtering by status")
149+
def test_node_filter(network, args):
150+
primary, _ = network.find_primary_and_any_backup()
151+
with primary.client() as c:
152+
trusted_before = c.get("/node/network/nodes?status=Trusted").body.json()
153+
pending_before = c.get("/node/network/nodes?status=Pending").body.json()
154+
retired_before = c.get("/node/network/nodes?status=Retired").body.json()
155+
new_node = network.create_and_add_pending_node(
156+
args.package, "local://localhost", args, target_node=primary
157+
)
158+
trusted_after = c.get("/node/network/nodes?status=Trusted").body.json()
159+
pending_after = c.get("/node/network/nodes?status=Pending").body.json()
160+
retired_after = c.get("/node/network/nodes?status=Retired").body.json()
161+
assert trusted_before == trusted_after, (trusted_before, trusted_after)
162+
assert len(pending_before["nodes"]) + 1 == len(pending_after["nodes"]), (
163+
pending_before,
164+
pending_after,
165+
)
166+
assert retired_before == retired_after, (retired_before, retired_after)
167+
168+
assert all(
169+
info["status"] == "Trusted" for info in trusted_after["nodes"]
170+
), trusted_after
171+
assert all(
172+
info["status"] == "Pending" for info in pending_after["nodes"]
173+
), pending_after
174+
assert all(
175+
info["status"] == "Retired" for info in retired_after["nodes"]
176+
), retired_after
177+
assert new_node
178+
return network
179+
180+
148181
def run(args):
149182
txs = app.LoggingTxs()
150183
with infra.network.network(
@@ -167,6 +200,7 @@ def run(args):
167200
test_add_node_from_snapshot(network, args)
168201
test_add_node_from_snapshot(network, args, from_backup=True)
169202
test_add_node_from_snapshot(network, args, copy_ledger_read_only=False)
203+
test_node_filter(network, args)
170204
errors, _ = network.get_joined_nodes()[-1].stop()
171205
if not any(
172206
"No snapshot found: Node will request all historical transactions" in s

0 commit comments

Comments
 (0)