Reject duplicate logger_nodes nodes
If someone tries merging Connections, there is a high likelihood that
they will end up with duplicate nodes in the list. Let's validate that
we end up with no configs with duplicates.
While we are here, do the same validation on the logger_nodes list too.
Change-Id: I05202eff8dcc8b8d09faf4383260b9e2fa8a0933
Signed-off-by: Austin Schuh <austin.schuh@bluerivertech.com>
diff --git a/aos/configuration.cc b/aos/configuration.cc
index 87abf15..314f82f 100644
--- a/aos/configuration.cc
+++ b/aos/configuration.cc
@@ -349,16 +349,43 @@
<< ", can only use [-a-zA-Z0-9_/]";
}
- // There is no good use case today for logging timestamps but not the
- // corresponding data. Instead of plumbing through all of this on the
- // reader side, let'd just disallow it for now.
- if (c->logger() == LoggerConfig::NOT_LOGGED &&
- c->has_destination_nodes()) {
+ if (c->has_logger_nodes()) {
+ // Confirm that we don't have duplicate logger nodes.
+ absl::btree_set<std::string_view> logger_nodes;
+ for (const flatbuffers::String *s : *c->logger_nodes()) {
+ logger_nodes.insert(s->string_view());
+ }
+ CHECK_EQ(static_cast<size_t>(logger_nodes.size()),
+ c->logger_nodes()->size())
+ << ": Found duplicate logger_nodes in "
+ << CleanedChannelToString(c);
+ }
+
+ if (c->has_destination_nodes()) {
+ // Confirm that we don't have duplicate timestamp logger nodes.
for (const Connection *d : *c->destination_nodes()) {
- CHECK(d->timestamp_logger() == LoggerConfig::NOT_LOGGED)
- << ": Logging timestamps without data is not supported. If you "
- "have a good use case, let's talk. "
- << CleanedChannelToString(c);
+ if (d->has_timestamp_logger_nodes()) {
+ absl::btree_set<std::string_view> timestamp_logger_nodes;
+ for (const flatbuffers::String *s : *d->timestamp_logger_nodes()) {
+ timestamp_logger_nodes.insert(s->string_view());
+ }
+ CHECK_EQ(static_cast<size_t>(timestamp_logger_nodes.size()),
+ d->timestamp_logger_nodes()->size())
+ << ": Found duplicate timestamp_logger_nodes in "
+ << CleanedChannelToString(c);
+ }
+ }
+
+ // There is no good use case today for logging timestamps but not the
+ // corresponding data. Instead of plumbing through all of this on the
+ // reader side, let'd just disallow it for now.
+ if (c->logger() == LoggerConfig::NOT_LOGGED) {
+ for (const Connection *d : *c->destination_nodes()) {
+ CHECK(d->timestamp_logger() == LoggerConfig::NOT_LOGGED)
+ << ": Logging timestamps without data is not supported. If "
+ "you have a good use case, let's talk. "
+ << CleanedChannelToString(c);
+ }
}
}
diff --git a/aos/configuration_test.cc b/aos/configuration_test.cc
index 0b842cb..a73a34a 100644
--- a/aos/configuration_test.cc
+++ b/aos/configuration_test.cc
@@ -876,6 +876,27 @@
"Logging timestamps without data");
}
+// Tests that we reject duplicate timestamp destination node configurations.
+TEST_F(ConfigurationDeathTest, DuplicateTimestampDestinationNodes) {
+ EXPECT_DEATH(
+ {
+ FlatbufferDetachedBuffer<Configuration> config = ReadConfig(
+ ArtifactPath("aos/testdata/duplicate_destination_nodes.json"));
+ },
+ "Found duplicate timestamp_logger_nodes in");
+}
+
+// Tests that we reject duplicate logger node configurations for a channel's
+// data.
+TEST_F(ConfigurationDeathTest, DuplicateLoggerNodes) {
+ EXPECT_DEATH(
+ {
+ FlatbufferDetachedBuffer<Configuration> config = ReadConfig(
+ ArtifactPath("aos/testdata/duplicate_logger_nodes.json"));
+ },
+ "Found duplicate logger_nodes in");
+}
+
} // namespace testing
} // namespace configuration
} // namespace aos
diff --git a/aos/testdata/BUILD b/aos/testdata/BUILD
index 51283c8..82e67f9 100644
--- a/aos/testdata/BUILD
+++ b/aos/testdata/BUILD
@@ -10,6 +10,8 @@
"config2.json",
"config2_multinode.json",
"config3.json",
+ "duplicate_destination_nodes.json",
+ "duplicate_logger_nodes.json",
"expected.json",
"expected_merge_with.json",
"expected_multinode.json",
diff --git a/aos/testdata/duplicate_destination_nodes.json b/aos/testdata/duplicate_destination_nodes.json
new file mode 100644
index 0000000..81814c3
--- /dev/null
+++ b/aos/testdata/duplicate_destination_nodes.json
@@ -0,0 +1,28 @@
+{
+ "channels": [
+ {
+ "name": "/foo",
+ "type": ".aos.bar",
+ "max_size": 5,
+ "source_node": "pi2",
+ "logger": "NOT_LOGGED",
+ "destination_nodes": [
+ {
+ "name": "pi1",
+ "timestamp_logger": "LOCAL_AND_REMOTE_LOGGER",
+ "timestamp_logger_nodes": ["pi2", "pi2"]
+ }
+ ]
+ }
+ ],
+ "nodes": [
+ {
+ "name": "pi1",
+ "hostname": "raspberrypi1"
+ },
+ {
+ "name": "pi2",
+ "hostname": "raspberrypi2"
+ }
+ ]
+}
diff --git a/aos/testdata/duplicate_logger_nodes.json b/aos/testdata/duplicate_logger_nodes.json
new file mode 100644
index 0000000..4eed4ee
--- /dev/null
+++ b/aos/testdata/duplicate_logger_nodes.json
@@ -0,0 +1,34 @@
+{
+ "channels": [
+ {
+ "name": "/foo",
+ "type": ".aos.bar",
+ "max_size": 5,
+ "source_node": "pi2",
+ "logger": "REMOTE_LOGGER",
+ "logger_nodes": ["pi1", "pi2", "pi2"],
+ "destination_nodes": [
+ {
+ "name": "pi1"
+ },
+ {
+ "name": "pi3"
+ }
+ ]
+ }
+ ],
+ "nodes": [
+ {
+ "name": "pi1",
+ "hostname": "raspberrypi1"
+ },
+ {
+ "name": "pi2",
+ "hostname": "raspberrypi2"
+ },
+ {
+ "name": "pi3",
+ "hostname": "raspberrypi3"
+ }
+ ]
+}
diff --git a/aos/testdata/invalid_logging_configuration.json b/aos/testdata/invalid_logging_configuration.json
index cb883b7..9213d8c 100644
--- a/aos/testdata/invalid_logging_configuration.json
+++ b/aos/testdata/invalid_logging_configuration.json
@@ -9,7 +9,8 @@
"destination_nodes": [
{
"name": "pi1",
- "timestamp_logger": "LOCAL_AND_REMOTE_LOGGER"
+ "timestamp_logger": "LOCAL_AND_REMOTE_LOGGER",
+ "timestamp_logger_nodes": ["pi2"]
}
]
},