From: Francis Dupont Date: Mon, 17 Sep 2018 20:29:11 +0000 (+0200) Subject: [65-libyang-adaptor] Addressed comments X-Git-Tag: 30-implement-control-socket-for-ddns_base~1^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a9e8d32ef554a6599064b8288ac6babd2a8a487d;p=thirdparty%2Fkea.git [65-libyang-adaptor] Addressed comments --- diff --git a/src/lib/yang/adaptor.cc b/src/lib/yang/adaptor.cc index 7ed4ad78ca..1113f3dab3 100644 --- a/src/lib/yang/adaptor.cc +++ b/src/lib/yang/adaptor.cc @@ -82,16 +82,22 @@ Adaptor::toParent(const string& name, ElementPtr parent, parent->set(name, param); } } - + namespace { /// @brief Apply insert. /// +/// This function applies an insert action at the given scope: +/// - in a map it adds the value at the key when the key does not exist +/// - in a list it appends the value +/// - in other scope type it does nothing +/// @note a copy of the value is used to avoid unwanted sharing/side effects. +/// /// @param key The key of the modification. /// @param value The value of the modification. /// @param scope The place to apply the insert. -void apply_insert(ConstElementPtr key, ConstElementPtr value, - ElementPtr scope) { +void applyInsert(ConstElementPtr key, ConstElementPtr value, + ElementPtr scope) { if (scope->getType() == Element::map) { if (!key || !value || (key->getType() != Element::string)) { return; @@ -109,13 +115,13 @@ void apply_insert(ConstElementPtr key, ConstElementPtr value, /// @brief Apply replace. /// -/// For maps same than insert but the new value is set even if the key -/// already exists. +/// This function works only on map scopes and acts as insert at the +/// exception the new value is set even if the key already exists. /// /// @param key The key of the modification. /// @param value The value of the modification. /// @param scope The place to apply the replace. -void apply_replace(ConstElementPtr key, ConstElementPtr value, +void applyReplace(ConstElementPtr key, ConstElementPtr value, ElementPtr scope) { if ((scope->getType() != Element::map) || !key || !value || (key->getType() != Element::string)) { @@ -129,32 +135,40 @@ void apply_replace(ConstElementPtr key, ConstElementPtr value, /// @brief Apply delete. /// -/// @param last The last item of the path. +/// This function deletes the value designed by the key from the given scope: +/// - in a map the key is the key of the value to remove +/// - in a list the key is: +/// * when it is an integer the index of the value to remove +/// * when it is a map the key / value pair which belongs to the value +/// to remove +/// - in other scope type it does nothing +/// +/// @param key The key item of the path. /// @param scope The place to apply the delete. -void apply_delete(ConstElementPtr last, ElementPtr scope) { +void applyDelete(ConstElementPtr key, ElementPtr scope) { if (scope->getType() == Element::map) { - if (!last || (last->getType() != Element::string)) { + if (!key || (key->getType() != Element::string)) { return; } - string name = last->stringValue(); + string name = key->stringValue(); if (!name.empty()) { scope->remove(name); } } else if (scope->getType() == Element::list) { - if (!last) { + if (!key) { return; - } else if (last->getType() == Element::integer) { - int index = last->intValue(); + } else if (key->getType() == Element::integer) { + int index = key->intValue(); if ((index >= 0) && (index < scope->size())) { scope->remove(index); } - } else if (last->getType() == Element::map) { - ConstElementPtr key = last->get("key"); - ConstElementPtr value = last->get("value"); - if (!key || !value || (key->getType() != Element::string)) { + } else if (key->getType() == Element::map) { + ConstElementPtr entry = key->get("key"); + ConstElementPtr value = key->get("value"); + if (!entry || !value || (entry->getType() != Element::string)) { return; } - string name = key->stringValue(); + string name = entry->stringValue(); if (name.empty()) { return; } @@ -175,10 +189,13 @@ void apply_delete(ConstElementPtr last, ElementPtr scope) { /// @brief Apply action. /// +/// This function applies one action (insert, replace or delete) using +/// applyInsert, applyReplace or applyDelete. +/// /// @param actions The action list. /// @param scope The current scope. /// @param next The index of the next action. -void apply_action(ConstElementPtr actions, ElementPtr scope, size_t next) { +void applyAction(ConstElementPtr actions, ElementPtr scope, size_t next) { if (next == actions->size()) { return; } @@ -186,33 +203,41 @@ void apply_action(ConstElementPtr actions, ElementPtr scope, size_t next) { ++next; if (!action || (action->getType() != Element::map) || !action->contains("action")) { - apply_action(actions, scope, next); + applyAction(actions, scope, next); return; } string name = action->get("action")->stringValue(); if (name == "insert") { - apply_insert(action->get("key"), action->get("value"), scope); + applyInsert(action->get("key"), action->get("value"), scope); } else if (name == "replace") { - apply_replace(action->get("key"), action->get("value"), scope); + applyReplace(action->get("key"), action->get("value"), scope); } else if (name == "delete") { - apply_delete(action->get("last"), scope); + applyDelete(action->get("key"), scope); } - apply_action(actions, scope, next); + applyAction(actions, scope, next); } -/// @brief Modify down. +/// @brief Apply recursively actions following the path and going down +/// in the to modify structure. +/// +/// The recursive call when the end of the path is not reached is done: +/// - in a map on the value at the key +/// - in a list the next path item is: +/// * if it is a positive integer on the value at the index +/// * if it is -1 on all elements of the list +/// * if a map on the element where the key / value pair belongs to /// /// @param path The search list. /// @param actions The action list. /// @param scope The current scope. /// @param next The index of the next item to use in the path. -void path_down(ConstElementPtr path, ConstElementPtr actions, ElementPtr scope, +void applyDown(ConstElementPtr path, ConstElementPtr actions, ElementPtr scope, size_t next) { if (!scope) { return; } if (next == path->size()) { - apply_action(actions, scope, 0); + applyAction(actions, scope, 0); return; } ConstElementPtr step = path->get(next); @@ -227,7 +252,7 @@ void path_down(ConstElementPtr path, ConstElementPtr actions, ElementPtr scope, } ElementPtr down = boost::const_pointer_cast(scope->get(name)); if (down) { - path_down(path, actions, down, next); + applyDown(path, actions, down, next); } } else if (scope->getType() == Element::list) { if (!step) { @@ -250,7 +275,7 @@ void path_down(ConstElementPtr path, ConstElementPtr actions, ElementPtr scope, } ConstElementPtr compare = down->get(name); if (compare && value->equals(*compare)) { - path_down(path, actions, down, next); + applyDown(path, actions, down, next); return; } } @@ -260,20 +285,21 @@ void path_down(ConstElementPtr path, ConstElementPtr actions, ElementPtr scope, int index = step->intValue(); if (index == -1) { for (ElementPtr down : downs) { - path_down(path, actions, down, next); + applyDown(path, actions, down, next); } } else if ((index >= 0) && (index < scope->size())) { - path_down(path, actions, scope->getNonConst(index), next); + applyDown(path, actions, scope->getNonConst(index), next); } } } } // end of anonymous namespace +/// Apply recursively starting at the beginning of the path. void Adaptor::modify(ConstElementPtr path, ConstElementPtr actions, ElementPtr config) { - path_down(path, actions, config, 0); + applyDown(path, actions, config, 0); } }; // end of namespace isc::yang diff --git a/src/lib/yang/adaptor.h b/src/lib/yang/adaptor.h index 8d2089c6a4..90c2bd9bf0 100644 --- a/src/lib/yang/adaptor.h +++ b/src/lib/yang/adaptor.h @@ -76,7 +76,7 @@ public: isc::data::ElementPtr parent, isc::data::ConstElementPtr list); - /// @brief Modify. + /// @brief Modify a configuration in its JSON element format. /// /// Smart merging tool, e.g. completing a from yang configuration. /// @@ -88,12 +88,39 @@ public: /// * special value -1: current scope is a list, apply to all items. /// * map { "": }: current scope is a list, go down to /// the item using the key / value pair. - /// - an action can be: insert, replace or delete. + /// - an action can be: insert, replace or delete: + /// * insert adds a value at a key: + /// . in a map the key is the new entry key + /// . in a list an integer key is the new element index + /// . in a list a map key is the key / value pair the to modify + /// element contains + /// * replace adds or replaces if it already exists a value at + /// an entry key in a map. + /// * delete removes a value designed by a key + /// + /// For instance to add a control-socket entry in a configuration + /// from a generic (vs Kea) model: + /// @code + /// path := [ ] + /// actions := [ { + /// "action": "insert", + /// "key": "Dhcp4", + /// "value": + /// { + /// "control-socket": + /// { + /// "socket-type": "unix", + /// "socket-name": "/tmp/kea4-ctrl-socket" + /// } + /// } + /// } + /// @endcode /// /// @param path The search list to follow down to the place to /// apply the action list. /// @param actions The action list /// @param config The configuration (JSON map) to modify. + /// @note modify does not throw but returns on unexpected conditions. static void modify(isc::data::ConstElementPtr path, isc::data::ConstElementPtr actions, isc::data::ElementPtr config); diff --git a/src/lib/yang/tests/adaptor_unittests.cc b/src/lib/yang/tests/adaptor_unittests.cc index 816b848e9e..7ddf419e68 100644 --- a/src/lib/yang/tests/adaptor_unittests.cc +++ b/src/lib/yang/tests/adaptor_unittests.cc @@ -18,7 +18,8 @@ using namespace isc::yang; namespace { -// Test get context. +// Test that get context works as expected i.e. moves a comment into +// the user context creating it if not exists. TEST(AdaptorTest, getContext) { // Empty. string config = "{\n" @@ -82,7 +83,8 @@ TEST(AdaptorTest, getContext) { EXPECT_EQ("{ \"bar\": 2, \"comment\": \"a comment\" }", context->str()); } -// Test from parent. +// Test that fromParent works as expected i.e. moves parameters from the +// parent to children and not overwrite them. TEST(AdaptorTest, fromParent) { string config = "{\n" " \"param1\": 123,\n" @@ -118,7 +120,8 @@ TEST(AdaptorTest, fromParent) { EXPECT_TRUE(json->equals(*Element::fromJSON(expected))); } -// Test to parent. +// Test that toParent works as expected i.e. moves parameters from children +// to the parent throwing when a value differs between two children. TEST(AdaptorTest, toParent) { string config = "{\n" " \"list\": [\n" @@ -156,7 +159,7 @@ TEST(AdaptorTest, toParent) { EXPECT_NO_THROW(Adaptor::toParent("param2",json, json->get("list"))); EXPECT_TRUE(json->equals(*Element::fromJSON(expected))); - // param1 has different value so it should throw. + // param[345] have different values so it should throw. EXPECT_THROW(Adaptor::toParent("param3",json, json->get("list")), BadValue); EXPECT_THROW(Adaptor::toParent("param4",json, json->get("list")), @@ -249,7 +252,7 @@ TEST(AdaptorTest, modifyMapDelete) { string sactions = "[\n" "{\n" " \"action\": \"delete\",\n" - " \"last\": \"test\"\n" + " \"key\": \"test\"\n" "}]\n"; ConstElementPtr actions; ASSERT_NO_THROW(actions = Element::fromJSON(sactions)); @@ -341,10 +344,10 @@ TEST(AdaptorTest, modifyListDelete) { string sactions = "[\n" "{\n" " \"action\": \"delete\",\n" - " \"last\": 2\n" + " \"key\": 2\n" "},{\n" " \"action\": \"delete\",\n" - " \"last\": { \"key\": \"foo\", \"value\": \"bar\" }\n" + " \"key\": { \"key\": \"foo\", \"value\": \"bar\" }\n" "}]\n"; ConstElementPtr actions; ASSERT_NO_THROW(actions = Element::fromJSON(sactions)); @@ -365,7 +368,8 @@ TEST(AdaptorTest, modifyListAllDelete) { "]]]\n"; ElementPtr json; ASSERT_NO_THROW(json = Element::fromJSON(config)); - // The only change from the previous unit test is 0 -> -1. + // The main change from the previous unit test is key 0 -> -1 so + // modify() applies the delete to all elements vs only the first one. string spath = "[ -1 ]"; ConstElementPtr path; ASSERT_NO_THROW(path = Element::fromJSON(spath)); @@ -373,10 +377,10 @@ TEST(AdaptorTest, modifyListAllDelete) { string sactions = "[\n" "{\n" " \"action\": \"delete\",\n" - " \"last\": 2\n" + " \"key\": 2\n" "},{\n" " \"action\": \"delete\",\n" - " \"last\": { \"key\": \"foo\", \"value\": \"bar\" }\n" + " \"key\": { \"key\": \"foo\", \"value\": \"bar\" }\n" "}]\n"; ConstElementPtr actions; ASSERT_NO_THROW(actions = Element::fromJSON(sactions));