Commit a22b7af0 authored by Francis Dupont's avatar Francis Dupont
Browse files

[5351] Addressed nearly all comments

parent f004bcef
......@@ -1327,11 +1327,13 @@ AC_CONFIG_FILES([Makefile
src/share/database/scripts/mysql/upgrade_4.0_to_4.1.sh
src/share/database/scripts/mysql/upgrade_4.1_to_5.0.sh
src/share/database/scripts/mysql/upgrade_5.0_to_5.1.sh
src/share/database/scripts/mysql/upgrade_5.1_to_6.0.sh
src/share/database/scripts/pgsql/Makefile
src/share/database/scripts/pgsql/upgrade_1.0_to_2.0.sh
src/share/database/scripts/pgsql/upgrade_2.0_to_3.0.sh
src/share/database/scripts/pgsql/upgrade_3.0_to_3.1.sh
src/share/database/scripts/pgsql/upgrade_3.1_to_3.2.sh
src/share/database/scripts/pgsql/upgrade_3.2_to_4.0.sh
tools/Makefile
tools/path_replacer.sh
])
......
......@@ -171,11 +171,6 @@ Element::set(const std::string&, ConstElementPtr) {
throwTypeError("set(name, element) called on a non-map Element");
}
void
Element::combine_set(const std::string&, ConstElementPtr) {
throwTypeError("combine_set(name, element) called on a non-map Element");
}
void
Element::remove(const std::string&) {
throwTypeError("remove(string) called on a non-map Element");
......@@ -907,18 +902,6 @@ MapElement::set(const std::string& key, ConstElementPtr value) {
m[key] = value;
}
void
MapElement::combine_set(const std::string& key, ConstElementPtr value) {
auto previous = m.find(key);
if (previous == m.end()) {
m[key] = value;
return;
}
ElementPtr mutable_ = boost::const_pointer_cast<Element>(previous->second);
combine(mutable_, value);
m[key] = mutable_;
}
bool
MapElement::find(const std::string& id, ConstElementPtr& t) const {
try {
......@@ -1085,48 +1068,6 @@ merge(ElementPtr element, ConstElementPtr other) {
}
}
void
combine(ElementPtr element, ConstElementPtr other) {
if (element->getType() != Element::map ||
other->getType() != Element::map) {
isc_throw(TypeError, "combine arguments not MapElements");
}
const std::map<std::string, ConstElementPtr>& m = other->mapValue();
for (std::map<std::string, ConstElementPtr>::const_iterator it = m.begin();
it != m.end() ; ++it) {
if (isNull(it->second)) {
isc_throw(BadValue, "combine got a null pointer");
}
if (!element->contains(it->first)) {
element->set(it->first, it->second);
continue;
}
ConstElementPtr e = element->get(it->first);
if (isNull(e)) {
isc_throw(BadValue, "combine got a null pointer");
}
ElementPtr le = Element::createList();
size_t i;
if (e->getType() == Element::list) {
le = Element::createList(e->getPosition());
for (i = 0; i < e->size(); ++i) {
le->add(e->getNonConst(i));
}
} else {
le->add(boost::const_pointer_cast<Element>(e));
}
if (it->second->getType() == Element::list) {
for (i = 0; i < it->second->size(); ++i) {
le->add(it->second->getNonConst(i));
}
} else {
le->add(boost::const_pointer_cast<Element>(it->second));
}
element->set(it->first, le);
}
}
ElementPtr
copy(ConstElementPtr from, int level) {
if (isNull(from)) {
......
......@@ -321,12 +321,6 @@ public:
/// @param element The ElementPtr to set at the given key.
virtual void set(const std::string& name, ConstElementPtr element);
/// Sets the ElementPtr at the given key, if there was a previous
/// value it is combined with it
/// @param name The key of the Element to set
/// @param element The ElementPtr to set at the given key.
virtual void combine_set(const std::string& name, ConstElementPtr element);
/// Remove the ElementPtr at the given key
/// @param name The key of the Element to remove
virtual void remove(const std::string& name);
......@@ -673,7 +667,6 @@ public:
}
using Element::set;
void set(const std::string& key, ConstElementPtr value);
void combine_set(const std::string& key, ConstElementPtr value);
using Element::remove;
void remove(const std::string& s) { m.erase(s); }
bool contains(const std::string& s) const {
......@@ -739,23 +732,6 @@ ConstElementPtr removeIdentical(ConstElementPtr a, ConstElementPtr b);
/// Raises a TypeError if either ElementPtr is not a MapElement
void merge(ElementPtr element, ConstElementPtr other);
/// @brief Combines the data from other into element.
/// (on the first level). Both elements must be
/// MapElements.
/// Every string,value pair in other is copied into element
/// (the ElementPtr of value is copied, this is not a new object)
/// When there is already a value for a key in element if both are lists
/// the other list is appended to the element list, if only one is a
/// list the not list value is added to the list. If none is a list
/// a list is created, the element value is added and the other value
/// is added.
///
/// @param element target map
/// @param other the map to combine with element
/// @throw raises a TypeError if either ElementPtr is not a MapElement
/// @throw raises a BadValue is a null pointer occurs.
void combine(ElementPtr element, ConstElementPtr other);
/// @brief Copy the data up to a nesting level.
///
/// The copy is a deep copy so nothing is shared if it is not
......
......@@ -654,28 +654,6 @@ TEST(Element, MapElement) {
el->remove("value3");
EXPECT_TRUE(isNull(el->get("value3")));
EXPECT_TRUE(isNull(el->get("value4")));
ElementPtr em = Element::fromJSON("{ \"a\": 1 }");
el->combine_set("value4", em);
ASSERT_FALSE(isNull(el->get("value4")));
EXPECT_EQ(em, el->get("value4"));
em = Element::fromJSON("{ \"a\": 2 }");
el->combine_set("value4", em);
ASSERT_FALSE(isNull(el->get("value4")));
ASSERT_EQ(Element::map, el->get("value4")->getType());
EXPECT_EQ(1, el->get("value4")->size());
ASSERT_FALSE(isNull(el->get("value4")->get("a")));
ASSERT_EQ(Element::list, el->get("value4")->get("a")->getType());
EXPECT_EQ(2, el->get("value4")->get("a")->size());
ASSERT_EQ(Element::integer,
el->get("value4")->get("a")->get(0)->getType());
EXPECT_EQ(1, el->get("value4")->get("a")->get(0)->intValue());
ASSERT_EQ(Element::integer,
el->get("value4")->get("a")->get(1)->getType());
EXPECT_EQ(2, el->get("value4")->get("a")->get(1)->intValue());
EXPECT_EQ(el->find("value2/number")->intValue(), 42);
EXPECT_TRUE(isNull(el->find("value2/nothing/")));
......@@ -1007,89 +985,6 @@ TEST(Element, merge) {
}
// This test checks combine.
TEST(Element, combine) {
ElementPtr a = Element::createMap();
ElementPtr b = Element::createMap();
ConstElementPtr c = Element::createMap();
combine(a, b);
EXPECT_EQ(*a, *c);
a = Element::fromJSON("1");
b = Element::createMap();
EXPECT_THROW(combine(a, b), TypeError);
a = Element::createMap();
b = Element::fromJSON("{ \"a\": 1 }");
c = Element::fromJSON("{ \"a\": 1 }");
combine(a, b);
EXPECT_EQ(*a, *c);
a = Element::createMap();
b = Element::fromJSON("{ \"a\": 1 }");
c = Element::fromJSON("{ \"a\": 1 }");
combine(b, a);
EXPECT_EQ(*b, *c);
a = Element::fromJSON("{ \"a\": 1 }");
b = Element::fromJSON("{ \"a\": 2 }");
c = Element::fromJSON("{ \"a\": [ 1, 2 ] }");
combine(a, b);
EXPECT_EQ(*a, *c);
a = Element::fromJSON("{ \"a\": 1 }");
b = Element::fromJSON("{ \"a\": 2 }");
c = Element::fromJSON("{ \"a\": [ 2, 1 ] }");
combine(b, a);
EXPECT_EQ(*b, *c);
a = Element::fromJSON("{ \"a\": { \"b\": \"c\" } }");
b = Element::fromJSON("{ \"a\": { \"b\": \"d\" } }");
c = Element::fromJSON("{ \"a\": [ { \"b\": \"c\" } ,{ \"b\": \"d\" } ] }");
combine(a, b);
EXPECT_EQ(*a, *c);
a = Element::fromJSON("{ \"a\": { \"b\": \"c\" } }");
b = Element::fromJSON("{ \"a\": { \"b\": \"d\" } }");
c = Element::fromJSON("{ \"a\": [ { \"b\": \"d\" }, { \"b\": \"c\" } ] }");
combine(b, a);
EXPECT_EQ(*b, *c);
a = Element::fromJSON("{ \"a\": 1, \"b\": 2 }");
b = Element::fromJSON("{ \"a\": 2, \"b\": 1 }");
c = Element::fromJSON("{ \"a\": [ 1, 2 ], \"b\": [ 2, 1 ] }");
combine(a, b);
EXPECT_EQ(*a, *c);
a = Element::fromJSON("{ \"a\": [] }");
b = Element::fromJSON("{ \"a\": 1 }");
c = Element::fromJSON("{ \"a\": [ 1 ] }");
combine(a, b);
EXPECT_EQ(*a, *c);
combine(b, a);
EXPECT_EQ(*a, *c);
a = Element::fromJSON("{ \"a\": [] }");
b = Element::fromJSON("{ \"a\": [ 1 ] }");
c = Element::fromJSON("{ \"a\": [ 1 ] }");
combine(a, b);
EXPECT_EQ(*a, *c);
combine(b, a);
EXPECT_EQ(*a, *c);
a = Element::fromJSON("{ \"a\": [ 1 ] }");
b = Element::fromJSON("{ \"a\": [ 2 ] }");
c = Element::fromJSON("{ \"a\": [ 1, 2 ] }");
combine(a, b);
EXPECT_EQ(*a, *c);
a = Element::fromJSON("{ \"a\": [ 1, 2, 3 ] }");
b = Element::fromJSON("{ \"a\": [ 2, 3, 4 ] }");
c = Element::fromJSON("{ \"a\": [ 1, 2, 3, 2, 3, 4 ] }");
combine(a, b);
EXPECT_EQ(*a, *c);
}
// This test checks copy.
TEST(Element, copy) {
// Null pointer
......
......@@ -1803,7 +1803,6 @@ public:
}
// dhcp_client_class: VARCHAR(128) NULL
/// @todo Assign actual value to client class string.
client_class_len_ = client_class_.size();
bind_[7].buffer_type = MYSQL_TYPE_STRING;
bind_[7].buffer = const_cast<char*>(client_class_.c_str());
......
......@@ -17,9 +17,9 @@
namespace isc {
namespace dhcp {
/// @brief Define PostgreSQL backend version: 3.2
const uint32_t PG_SCHEMA_VERSION_MAJOR = 3;
const uint32_t PG_SCHEMA_VERSION_MINOR = 2;
/// @brief Define PostgreSQL backend version: 4.0
const uint32_t PG_SCHEMA_VERSION_MAJOR = 4;
const uint32_t PG_SCHEMA_VERSION_MINOR = 0;
// Maximum number of parameters that can be used a statement
// @todo This allows us to use an initializer list (since we can't
......
......@@ -116,7 +116,7 @@ TEST_F(CfgDUIDTest, setValues) {
ASSERT_NO_THROW(cfg_duid.setTime(32100));
ASSERT_NO_THROW(cfg_duid.setEnterpriseId(10));
ASSERT_NO_THROW(cfg_duid.setPersist(false));
std::string user_context = "{ \"comment\": \"foo\" }";
std::string user_context = "{ \"comment\": \"foo\", \"bar\": 1 }";
ASSERT_NO_THROW(cfg_duid.setContext(Element::fromJSON(user_context)));
// Check that values have been set correctly.
......@@ -136,7 +136,9 @@ TEST_F(CfgDUIDTest, setValues) {
" \"htype\": 100,\n"
" \"time\": 32100,\n"
" \"enterprise-id\": 10,\n"
" \"persist\": false }";
" \"persist\": false,\n"
" \"user-context\": { \"bar\": 1 }\n"
"}";
runToElementTest<CfgDUID>(expected, cfg_duid);
}
......
......@@ -369,14 +369,15 @@ TEST_F(CfgIfaceTest, unparse) {
EXPECT_NO_THROW(cfg4.use(AF_INET, "*"));
EXPECT_NO_THROW(cfg4.use(AF_INET, "eth0"));
EXPECT_NO_THROW(cfg4.use(AF_INET, "eth1/192.0.2.3"));
std::string comment = "{ \"comment\": \"foo\" }";
std::string comment = "{ \"comment\": \"foo\", \"bar\": 1 }";
EXPECT_NO_THROW(cfg4.setContext(Element::fromJSON(comment)));
// Check unparse
std::string expected =
"{ \"comment\": \"foo\", "
"\"interfaces\": [ \"*\", \"eth0\", \"eth1/192.0.2.3\" ], "
"\"re-detect\": false }";
"\"re-detect\": false, "
"\"user-context\": { \"bar\": 1 } }";
runToElementTest<CfgIface>(expected, cfg4);
// Now check IPv6
......@@ -384,13 +385,14 @@ TEST_F(CfgIfaceTest, unparse) {
EXPECT_NO_THROW(cfg6.use(AF_INET6, "*"));
EXPECT_NO_THROW(cfg6.use(AF_INET6, "eth1"));
EXPECT_NO_THROW(cfg6.use(AF_INET6, "eth0/2001:db8:1::1"));
comment = "{ \"comment\": \"bar\" }";
comment = "{ \"comment\": \"bar\", \"foo\": 2 }";
EXPECT_NO_THROW(cfg6.setContext(Element::fromJSON(comment)));
expected =
"{ \"comment\": \"bar\", "
"\"interfaces\": [ \"*\", \"eth1\", \"eth0/2001:db8:1::1\" ], "
"\"re-detect\": false }";
"\"re-detect\": false, "
"\"user-context\": { \"foo\": 2 } }";
runToElementTest<CfgIface>(expected, cfg6);
}
......
......@@ -257,7 +257,7 @@ TEST(CfgOptionDefTest, unparse) {
cfg.add(OptionDefinitionPtr(new
OptionDefinition("option-baz", 6, "uint16", "dns")), "isc");
OptionDefinitionPtr rec(new OptionDefinition("option-rec", 6, "record"));
std::string json = "{ \"comment\": \"foo\" }";
std::string json = "{ \"comment\": \"foo\", \"bar\": 1 }";
rec->setContext(data::Element::fromJSON(json));
rec->addRecordField("uint16");
rec->addRecordField("uint16");
......@@ -281,7 +281,8 @@ TEST(CfgOptionDefTest, unparse) {
" \"array\": false,\n"
" \"record-types\": \"uint16, uint16\",\n"
" \"encapsulate\": \"\",\n"
" \"space\": \"dns\"\n"
" \"space\": \"dns\",\n"
" \"user-context\": \"bar\": 1 }\n"
"},{\n"
" \"name\": \"option-foo\",\n"
" \"code\": 5,\n"
......
......@@ -600,7 +600,8 @@ TEST_F(CfgOptionTest, unparse) {
cfg.add(opt1, false, "dns");
OptionPtr opt2(new Option(Option::V6, 101, OptionBuffer(4, 12)));
OptionDescriptor desc2(opt2, false, "12, 12, 12, 12");
desc2.setContext(data::Element::fromJSON("{ \"comment\": \"foo\" }"));
std::string ctx = "{ \"comment\": \"foo\", \"bar\": 1 }";
desc2.setContext(data::Element::fromJSON(ctx));
cfg.add(desc2, "dns");
OptionPtr opt3(new Option(Option::V6, D6O_STATUS_CODE, OptionBuffer(2, 0)));
cfg.add(opt3, false, DHCP6_OPTION_SPACE);
......@@ -621,7 +622,8 @@ TEST_F(CfgOptionTest, unparse) {
" \"space\": \"dns\",\n"
" \"csv-format\": true,\n"
" \"data\": \"12, 12, 12, 12\",\n"
" \"always-send\": false\n"
" \"always-send\": false,\n"
" \"user-context\": { \"bar\": 1 }\n"
"},{\n"
" \"code\": 13,\n"
" \"name\": \"status-code\",\n"
......
......@@ -387,7 +387,8 @@ TEST(ClientClassDef, unparseDef) {
std::string test = "option[12].text == 'foo'";
cclass->setTest(test);
std::string comment = "bar";
std::string user_context = "{ \"comment\": \"" + comment + "\" }";
std::string user_context = "{ \"comment\": \"" + comment + "\", ";
user_context += "\"bar\": 1 }";
cclass->setContext(isc::data::Element::fromJSON(user_context));
std::string next_server = "1.2.3.4";
cclass->setNextServer(IOAddress(next_server));
......@@ -404,7 +405,8 @@ TEST(ClientClassDef, unparseDef) {
"\"next-server\": \"" + next_server + "\",\n"
"\"server-hostname\": \"" + sname + "\",\n"
"\"boot-file-name\": \"" + filename + "\",\n"
"\"option-data\": [ ] }\n";
"\"option-data\": [ ],\n"
"\"user-context\": { \"bar\": 1 } }\n";
runToElementTest<ClientClassDef>(expected, *cclass);
}
......
......@@ -103,7 +103,7 @@ TEST(D2ClientConfigTest, constructorsAndAccessors) {
ASSERT_TRUE(d2_client_config);
// Add user context
std::string user_context = "{ \"comment\": \"foo\" }";
std::string user_context = "{ \"comment\": \"bar\", \"foo\": 1 }";
EXPECT_FALSE(d2_client_config->getContext());
d2_client_config->setContext(Element::fromJSON(user_context));
......@@ -134,7 +134,7 @@ TEST(D2ClientConfigTest, constructorsAndAccessors) {
// Verify what toElement returns.
std::string expected = "{\n"
"\"comment\": \"foo\",\n"
"\"comment\": \"bar\",\n"
"\"enable-updates\": true,\n"
"\"server-ip\": \"127.0.0.1\",\n"
"\"server-port\": 477,\n"
......@@ -148,7 +148,8 @@ TEST(D2ClientConfigTest, constructorsAndAccessors) {
"\"override-client-update\": true,\n"
"\"replace-client-name\": \"when-present\",\n"
"\"generated-prefix\": \"the_prefix\",\n"
"\"qualifying-suffix\": \"the.suffix.\"\n"
"\"qualifying-suffix\": \"the.suffix.\",\n"
"\"user-context\": { \"foo\": 1 }\n"
"}\n";
runToElementTest<D2ClientConfig>(expected, *d2_client_config);
......
......@@ -195,7 +195,8 @@ TEST(SharedNetwork4Test, unparse) {
network->setValid(200);
network->setMatchClientId(false);
data::ElementPtr ctx = data::Element::fromJSON("{ \"comment\": \"foo\" }");
std::string uc = "{ \"comment\": \"bar\", \"foo\": 1}";
data::ElementPtr ctx = data::Element::fromJSON(uc);
network->setContext(ctx);
// Add several subnets.
......@@ -207,7 +208,7 @@ TEST(SharedNetwork4Test, unparse) {
network->add(subnet2);
std::string expected = "{\n"
" \"comment\": \"foo\",\n"
" \"comment\": \"bar\",\n"
" \"interface\": \"eth1\",\n"
" \"match-client-id\": false,\n"
" \"name\": \"frog\",\n"
......@@ -260,6 +261,7 @@ TEST(SharedNetwork4Test, unparse) {
" \"valid-lifetime\": 30\n"
" }\n"
" ],\n"
" \"user-context\": { \"foo\": 1 },\n"
" \"valid-lifetime\": 200\n"
"}\n";
......
......@@ -78,9 +78,6 @@ EP moveComments1(EP element) {
} else if (it->first == "user-context") {
// Do not traverse user-context entries
result->set("user-context", it->second);
} else if (it->first == "control-socket") {
// Do not traverse control-socke entries
result->set("control-socket", it->second);
} else {
// Not comment or user-context
try {
......@@ -102,7 +99,12 @@ EP moveComments1(EP element) {
ConstElementPtr comment = element->get("comment");
ElementPtr moved = Element::createMap();
moved->set("comment", comment);
result->combine_set("user-context", moved);
ConstElementPtr previous = element->get("user-context");
// If there is already a user context merge it
if (previous) {
merge(moved, previous);
}
result->set("user-context", moved);
}
return (result);
......@@ -136,7 +138,7 @@ namespace {
///
/// @tparam EP ElementPtr or ConstElementPtr (compiler will infer which one)
/// @param element the element to traverse
/// @return a modified copy where comment entries were moved to user-context
/// @return a modified copy where comment entries were moved from user-context
/// @throw UnModified with the unmodified value
template<typename EP>
EP extractComments1(EP element) {
......@@ -174,9 +176,6 @@ EP extractComments1(EP element) {
if (it->first == "comment") {
// Do not traverse comment entries
result->set("comment", it->second);
} else if (it->first == "control-socket") {
// Do not traverse control-socke entries
result->set("control-socket", it->second);
} else if (it->first == "user-context") {
if (it->second->contains("comment")) {
// Note there is a entry to move
......@@ -217,7 +216,7 @@ EP extractComments1(EP element) {
if (user_context->size() > 0) {
result->set("user-context", user_context);
}
result->combine_set("comment", copy(comment, 0));
result->set("comment", comment);
}
return (result);
......
......@@ -4,3 +4,4 @@
/upgrade_4.0_to_4.1.sh
/upgrade_4.1_to_5.0.sh
/upgrade_5.0_to_5.1.sh
/upgrade_5.1_to_6.0.sh
......@@ -9,6 +9,7 @@ sqlscripts_DATA += upgrade_3.0_to_4.0.sh
sqlscripts_DATA += upgrade_4.0_to_4.1.sh
sqlscripts_DATA += upgrade_4.1_to_5.0.sh
sqlscripts_DATA += upgrade_5.0_to_5.1.sh
sqlscripts_DATA += upgrade_5.1_to_6.0.sh
DISTCLEANFILES = upgrade_1.0_to_2.0.sh
......@@ -17,5 +18,6 @@ DISTCLEANFILES += upgrade_3.0_to_4.0.sh
DISTCLEANFILES += upgrade_4.0_to_4.1.sh
DISTCLEANFILES += upgrade_4.1_to_5.0.sh
DISTCLEANFILES += upgrade_5.0_to_5.1.sh
DISTCLEANFILES += upgrade_5.1_to_6.0.sh
EXTRA_DIST = ${sqlscripts_DATA}
......@@ -498,6 +498,11 @@ END
$$
DELIMITER ;
# Update the schema version number
UPDATE schema_version
SET version = '5', minor = '1';
# This line concludes database upgrade to version 5.1.
# Add user context into table holding hosts
ALTER TABLE hosts ADD COLUMN user_context TEXT NULL;
......@@ -507,8 +512,8 @@ ALTER TABLE dhcp6_options ADD COLUMN user_context TEXT NULL;
# Update the schema version number
UPDATE schema_version
SET version = '5', minor = '1';
# This line concludes database upgrade to version 5.1.
SET version = '6', minor = '0';
# This line concludes database upgrade to version 6.0.
# Notes:
#
......
......@@ -35,13 +35,6 @@ END
$$
DELIMITER ;
# Add user context into table holding hosts
ALTER TABLE hosts ADD COLUMN user_context TEXT NULL;
# Add user contexts into tables holding DHCP options
ALTER TABLE dhcp4_options ADD COLUMN user_context TEXT NULL;
ALTER TABLE dhcp6_options ADD COLUMN user_context TEXT NULL;
# Update the schema version number
UPDATE schema_version
SET version = '5', minor = '1';
......
#!/bin/sh
# Include utilities. Use installed version if available and
# use build version if it isn't.
if [ -e @datarootdir@/@PACKAGE_NAME@/scripts/admin-utils.sh ]; then
. @datarootdir@/@PACKAGE_NAME@/scripts/admin-utils.sh
else
. @abs_top_builddir@/src/bin/admin/admin-utils.sh
fi
VERSION=`mysql_version "$@"`
if [ "$VERSION" != "5.1" ]; then
printf "This script upgrades 5.1 to 6.0. Reported version is $VERSION. Skipping upgrade.\n"
exit 0
fi
mysql "$@" <<EOF
# Add user context into table holding hosts
ALTER TABLE hosts ADD COLUMN user_context TEXT NULL;
# Add user contexts into tables holding DHCP options
ALTER TABLE dhcp4_options ADD COLUMN user_context TEXT NULL;
ALTER TABLE dhcp6_options ADD COLUMN user_context TEXT NULL;