Support globs in maps
We need to remap sub folders. This is very annoying to do with one map
per folder. Recognize * and match it accordingly.
Change-Id: Ieae86dcec000206eadb913c29a8ee141e9e93f0f
diff --git a/aos/BUILD b/aos/BUILD
index 1d6d388..da558a4 100644
--- a/aos/BUILD
+++ b/aos/BUILD
@@ -481,6 +481,9 @@
"testdata/expected_multinode.json",
"testdata/good_multinode.json",
"testdata/good_multinode_hostnames.json",
+ "testdata/invalid_channel_name1.json",
+ "testdata/invalid_channel_name2.json",
+ "testdata/invalid_channel_name3.json",
"testdata/invalid_destination_node.json",
"testdata/invalid_nodes.json",
"testdata/invalid_source_node.json",
diff --git a/aos/configuration.cc b/aos/configuration.cc
index f250c93..0a34b02 100644
--- a/aos/configuration.cc
+++ b/aos/configuration.cc
@@ -1,11 +1,11 @@
#include "aos/configuration.h"
-#include <string.h>
-#include <stdlib.h>
-#include <sys/types.h>
-#include <netinet/in.h>
#include <arpa/inet.h>
#include <ifaddrs.h>
+#include <netinet/in.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/types.h>
#include <unistd.h>
#include <set>
@@ -80,8 +80,7 @@
namespace {
// Extracts the folder part of a path. Returns ./ if there is no path.
-std::string_view ExtractFolder(
- const std::string_view filename) {
+std::string_view ExtractFolder(const std::string_view filename) {
auto last_slash_pos = filename.find_last_of("/\\");
return last_slash_pos == std::string_view::npos
@@ -178,7 +177,7 @@
// Compares for equality (c == p) a channel, and a name, type tuple.
bool EqualsChannels(const Channel *c,
- ::std::pair<std::string_view, std::string_view> p) {
+ ::std::pair<std::string_view, std::string_view> p) {
return c->name()->string_view() == p.first &&
c->type()->string_view() == p.second;
}
@@ -195,8 +194,7 @@
// Maps name for the provided maps. Modifies name.
void HandleMaps(const flatbuffers::Vector<flatbuffers::Offset<aos::Map>> *maps,
- std::string_view *name, std::string_view type,
- const Node *node) {
+ std::string *name, std::string_view type, const Node *node) {
// For the same reason we merge configs in reverse order, we want to process
// maps in reverse order. That lets the outer config overwrite channels from
// the inner configs.
@@ -210,8 +208,16 @@
// Handle normal maps (now that we know that match and rename are filled
// out).
- if (i->match()->name()->string_view() != *name) {
- continue;
+ const std::string_view match_name = i->match()->name()->string_view();
+ if (match_name != *name) {
+ if (match_name.back() == '*' &&
+ std::string_view(*name).substr(
+ 0, std::min(name->size(), match_name.size() - 1)) ==
+ match_name.substr(0, match_name.size() - 1)) {
+ CHECK_EQ(match_name.find('*'), match_name.size() - 1);
+ } else {
+ continue;
+ }
}
// Handle type specific maps.
@@ -219,15 +225,19 @@
continue;
}
+ // Now handle node specific maps.
if (node != nullptr && i->match()->has_source_node() &&
i->match()->source_node()->string_view() !=
node->name()->string_view()) {
continue;
}
- VLOG(1) << "Renamed \"" << *name << "\" to \""
- << i->rename()->name()->string_view() << "\"";
- *name = i->rename()->name()->string_view();
+ std::string new_name(i->rename()->name()->string_view());
+ if (match_name.back() == '*') {
+ new_name += std::string(name->substr(match_name.size() - 1));
+ }
+ VLOG(1) << "Renamed \"" << *name << "\" to \"" << new_name << "\"";
+ *name = std::move(new_name);
}
}
@@ -298,8 +308,7 @@
{
::std::vector<flatbuffers::Offset<Channel>> channel_offsets;
for (const FlatbufferDetachedBuffer<Channel> &c : channels) {
- channel_offsets.emplace_back(
- CopyFlatBuffer<Channel>(&c.message(), &fbb));
+ channel_offsets.emplace_back(CopyFlatBuffer<Channel>(&c.message(), &fbb));
}
channels_offset = fbb.CreateVector(channel_offsets);
}
@@ -362,8 +371,36 @@
// Check that if there is a node list, all the source nodes are filled out and
// valid, and all the destination nodes are valid (and not the source). This
// is a basic consistency check.
- if (result.message().has_nodes() && config.message().has_channels()) {
- for (const Channel *c : *config.message().channels()) {
+ if (result.message().has_channels()) {
+ for (const Channel *c : *result.message().channels()) {
+ if (c->name()->string_view().back() == '/') {
+ LOG(FATAL) << "Channel names can't end with '/'";
+ }
+ if(c->name()->string_view().find("//")!= std::string_view::npos) {
+ LOG(FATAL) << ": Invalid channel name " << c->name()->string_view()
+ << ", can't use //.";
+ }
+ for (const char data : c->name()->string_view()) {
+ if (data >= '0' && data <= '9') {
+ continue;
+ }
+ if (data >= 'a' && data <= 'z') {
+ continue;
+ }
+ if (data >= 'A' && data <= 'Z') {
+ continue;
+ }
+ if (data == '-' || data == '_' || data == '/') {
+ continue;
+ }
+ LOG(FATAL) << "Invalid channel name " << c->name()->string_view()
+ << ", can only use [-a-zA-Z0-9_/]";
+ }
+ }
+ }
+
+ if (result.message().has_nodes() && result.message().has_channels()) {
+ for (const Channel *c : *result.message().channels()) {
CHECK(c->has_source_node()) << ": Channel " << FlatbufferToJson(c)
<< " is missing \"source_node\"";
CHECK(GetNode(&result.message(), c->source_node()->string_view()) !=
@@ -431,6 +468,7 @@
std::string_view type,
std::string_view application_name, const Node *node) {
const std::string_view original_name = name;
+ std::string mutable_name;
VLOG(1) << "Looking up { \"name\": \"" << name << "\", \"type\": \"" << type
<< "\" }";
@@ -443,14 +481,18 @@
if (application_iterator != config->applications()->cend() &&
EqualsApplications(*application_iterator, application_name)) {
if (application_iterator->has_maps()) {
- HandleMaps(application_iterator->maps(), &name, type, node);
+ mutable_name = std::string(name);
+ HandleMaps(application_iterator->maps(), &mutable_name, type, node);
+ name = std::string_view(mutable_name);
}
}
}
// Now do global maps.
if (config->has_maps()) {
- HandleMaps(config->maps(), &name, type, node);
+ mutable_name = std::string(name);
+ HandleMaps(config->maps(), &mutable_name, type, node);
+ name = std::string_view(mutable_name);
}
if (original_name != name) {
@@ -460,8 +502,7 @@
// Then look for the channel.
auto channel_iterator =
- std::lower_bound(config->channels()->cbegin(),
- config->channels()->cend(),
+ std::lower_bound(config->channels()->cbegin(), config->channels()->cend(),
std::make_pair(name, type), CompareChannels);
// Make sure we actually found it, and it matches.
@@ -514,7 +555,7 @@
// Search for a schema with a matching type.
const aos::FlatbufferString<reflection::Schema> *found_schema = nullptr;
- for (const aos::FlatbufferString<reflection::Schema> &schema: schemas) {
+ for (const aos::FlatbufferString<reflection::Schema> &schema : schemas) {
if (schema.message().root_table() != nullptr) {
if (schema.message().root_table()->name()->string_view() ==
c->type()->string_view()) {
@@ -771,7 +812,7 @@
}
bool ChannelMessageIsLoggedOnNode(const Channel *channel, const Node *node) {
- switch(channel->logger()) {
+ switch (channel->logger()) {
case LoggerConfig::LOCAL_LOGGER:
if (node == nullptr) {
// Single node world. If there is a local logger, then we want to use
diff --git a/aos/configuration.fbs b/aos/configuration.fbs
index c66da06..31d89e7 100644
--- a/aos/configuration.fbs
+++ b/aos/configuration.fbs
@@ -83,7 +83,9 @@
// Table to support renaming channel names.
table Map {
// Channel to match with. If the name in here matches, the name is replaced
- // with the name in rename.
+ // with the name in rename. If the name ends in *, it will be treated like a
+ // wildcard. Anything with the same prefix will match, and anything matching
+ // the * will get preserved on rename. This supports moving subfolders.
// Node specific matches are also supported.
match:Channel;
// The channel to merge in.
diff --git a/aos/configuration_test.cc b/aos/configuration_test.cc
index 0be7a46..aa6de8e 100644
--- a/aos/configuration_test.cc
+++ b/aos/configuration_test.cc
@@ -71,9 +71,9 @@
LOG(INFO) << "Read: " << FlatbufferToJson(config, true);
- EXPECT_EQ(FlatbufferToJson(GetChannel(config, ".aos.robot_state",
+ EXPECT_EQ(FlatbufferToJson(GetChannel(config, "/aos/robot_state",
"aos.RobotState", "app1", nullptr)),
- "{ \"name\": \".aos.robot_state\", \"type\": \"aos.RobotState\", "
+ "{ \"name\": \"/aos/robot_state\", \"type\": \"aos.RobotState\", "
"\"max_size\": 5 }");
}
@@ -87,6 +87,30 @@
kConfigPrefix + "config1_bad.json");
}
+// Tests that we reject invalid channel names. This means any channels with //
+// in their name, a trailing /, or regex characters.
+TEST_F(ConfigurationDeathTest, InvalidChannelName) {
+ EXPECT_DEATH(
+ {
+ FlatbufferDetachedBuffer<Configuration> config =
+ ReadConfig(kConfigPrefix + "invalid_channel_name1.json");
+ },
+ "Channel names can't end with '/'");
+ EXPECT_DEATH(
+ {
+ FlatbufferDetachedBuffer<Configuration> config =
+ ReadConfig(kConfigPrefix + "invalid_channel_name2.json");
+ },
+ "Invalid channel name");
+ EXPECT_DEATH(
+ {
+ FlatbufferDetachedBuffer<Configuration> config =
+ ReadConfig(kConfigPrefix + "invalid_channel_name3.json");
+ LOG(FATAL) << "Foo";
+ },
+ "Invalid channel name");
+}
+
// Tests that we can modify a config with a json snippet.
TEST_F(ConfigurationTest, MergeWithConfig) {
FlatbufferDetachedBuffer<Configuration> config =
@@ -203,6 +227,30 @@
kExpectedBazMultinodeLocation);
}
+// Tests that we can lookup a location with a glob
+TEST_F(ConfigurationTest, GetChannelGlob) {
+ FlatbufferDetachedBuffer<Configuration> config =
+ ReadConfig(kConfigPrefix + "good_multinode.json");
+ const Node *pi1 = GetNode(&config.message(), "pi1");
+
+ // Confirm that a glob with nothing after it matches.
+ const char *kExpectedMultinodeLocation =
+ "{ \"name\": \"/foo\", \"type\": \".aos.bar\", \"max_size\": 5, "
+ "\"source_node\": \"pi1\" }";
+ EXPECT_EQ(FlatbufferToJson(
+ GetChannel(config, "/magic/string", ".aos.bar", "app7", pi1)),
+ kExpectedMultinodeLocation);
+
+ // Now confirm that glob with something following it matches and renames
+ // correctly.
+ const char *kExpectedSubfolderMultinodeLocation =
+ "{ \"name\": \"/foo/subfolder\", \"type\": \".aos.bar\", \"max_size\": "
+ "5, \"source_node\": \"pi1\" }";
+ EXPECT_EQ(FlatbufferToJson(GetChannel(config, "/magic/string/subfolder",
+ ".aos.bar", "app7", pi1)),
+ kExpectedSubfolderMultinodeLocation);
+}
+
// Tests that we reject a configuration which has a nodes list, but has channels
// withoout source_node filled out.
TEST_F(ConfigurationDeathTest, InvalidSourceNode) {
diff --git a/aos/testdata/backwards.json b/aos/testdata/backwards.json
index 192005e..a39d5c7 100644
--- a/aos/testdata/backwards.json
+++ b/aos/testdata/backwards.json
@@ -1,12 +1,12 @@
{
"channels": [
{
- "name": ".aos.robot_state",
+ "name": "/aos/robot_state",
"type": "aos.RobotState",
"max_size": 5
},
{
- "name": ".aos.joystick_state",
+ "name": "/aos/joystick_state",
"type": "aos.JoystickState"
}
]
diff --git a/aos/testdata/good_multinode.json b/aos/testdata/good_multinode.json
index 94db924..52a53b0 100644
--- a/aos/testdata/good_multinode.json
+++ b/aos/testdata/good_multinode.json
@@ -7,6 +7,12 @@
"source_node": "pi1"
},
{
+ "name": "/foo/subfolder",
+ "type": ".aos.bar",
+ "max_size": 5,
+ "source_node": "pi1"
+ },
+ {
"name": "/batman",
"type": ".aos.baz",
"max_size": 5,
@@ -23,6 +29,14 @@
"rename": {
"name": "/foo"
}
+ },
+ {
+ "match": {
+ "name": "/magic/string*"
+ },
+ "rename": {
+ "name": "/foo"
+ }
}
],
"applications": [
diff --git a/aos/testdata/invalid_channel_name1.json b/aos/testdata/invalid_channel_name1.json
new file mode 100644
index 0000000..90c6d24
--- /dev/null
+++ b/aos/testdata/invalid_channel_name1.json
@@ -0,0 +1,9 @@
+{
+ "channels": [
+ {
+ "name": "/foo/",
+ "type": ".aos.bar",
+ "max_size": 5
+ }
+ ]
+}
diff --git a/aos/testdata/invalid_channel_name2.json b/aos/testdata/invalid_channel_name2.json
new file mode 100644
index 0000000..070c124
--- /dev/null
+++ b/aos/testdata/invalid_channel_name2.json
@@ -0,0 +1,9 @@
+{
+ "channels": [
+ {
+ "name": "/f*oo",
+ "type": ".aos.bar",
+ "max_size": 5
+ }
+ ]
+}
diff --git a/aos/testdata/invalid_channel_name3.json b/aos/testdata/invalid_channel_name3.json
new file mode 100644
index 0000000..efe8e1b
--- /dev/null
+++ b/aos/testdata/invalid_channel_name3.json
@@ -0,0 +1,9 @@
+{
+ "channels": [
+ {
+ "name": "/foo//bar",
+ "type": ".aos.bar",
+ "max_size": 5
+ }
+ ]
+}