Add Id to Trigger
Currently, Trigger is using Name as unique identifier. By adding Id, we
can be compliant with redfish specification:
- Id will be used as unique identifier
- Name will be used as human readable, non-unique name
AddTrigger dbus method is now requiring both id and name. Each of them
can be passed as empty string and the service will fill them with
correct values. If only id is an empty string, name will be used to
generate its value.
Dbus object path and persistent storage filename are now be based on id,
instead of name.
Added validation for AddTrigger:
- correct characters in id
- max id length
Added Name property for Trigger object, which can be modified from dbus.
Testing done:
- Unit test added and passing
- Trigger was added using dbus, without errors
- Id generation is working properly
- Name property is accessible and writable from dbus
Signed-off-by: Szymon Dompke <szymon.dompke@intel.com>
Change-Id: Ibb553586eaf51943044c93a35bc0725e6ef67ce9
diff --git a/meson.build b/meson.build
index d06d8fa..331926a 100644
--- a/meson.build
+++ b/meson.build
@@ -68,8 +68,8 @@
get_option('max-reading-parameters').to_string(),
'-DTELEMETRY_MIN_INTERVAL=' + get_option('min-interval').to_string(),
'-DTELEMETRY_MAX_TRIGGERS=' + get_option('max-triggers').to_string(),
- '-DTELEMETRY_MAX_REPORT_NAME_LENGTH=' +
- get_option('max-report-name-length').to_string(),
+ '-DTELEMETRY_MAX_DBUS_PATH_LENGTH=' +
+ get_option('max-dbus-path-length').to_string(),
language: 'cpp'
)
diff --git a/meson_options.txt b/meson_options.txt
index 1a7896e..04ed22e 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -7,5 +7,5 @@
description: 'Minimal value of interval in milliseconds')
option('max-triggers', type: 'integer', min: 1, value: 10,
description: 'Max number of Triggers')
-option('max-report-name-length', type: 'integer', min: 1, value: 4095,
- description: 'Max length of Report name')
+option('max-dbus-path-length', type: 'integer', min: 1, value: 4095,
+ description: 'Max length of dbus object path')
diff --git a/src/interfaces/trigger.hpp b/src/interfaces/trigger.hpp
index 328e581..4ec5070 100644
--- a/src/interfaces/trigger.hpp
+++ b/src/interfaces/trigger.hpp
@@ -10,7 +10,7 @@
public:
virtual ~Trigger() = default;
- virtual std::string getName() const = 0;
+ virtual std::string getId() const = 0;
virtual std::string getPath() const = 0;
};
diff --git a/src/interfaces/trigger_factory.hpp b/src/interfaces/trigger_factory.hpp
index 7a39aeb..f39d555 100644
--- a/src/interfaces/trigger_factory.hpp
+++ b/src/interfaces/trigger_factory.hpp
@@ -20,7 +20,8 @@
virtual ~TriggerFactory() = default;
virtual std::unique_ptr<interfaces::Trigger> make(
- const std::string& name, const std::vector<std::string>& triggerActions,
+ const std::string& id, const std::string& name,
+ const std::vector<std::string>& triggerActions,
const std::vector<std::string>& reportNames,
interfaces::TriggerManager& triggerManager,
interfaces::JsonStorage& triggerStorage,
diff --git a/src/report.hpp b/src/report.hpp
index 26cc130..18574cf 100644
--- a/src/report.hpp
+++ b/src/report.hpp
@@ -50,8 +50,8 @@
static void timerProc(boost::system::error_code, Report& self);
void scheduleTimer(Milliseconds interval);
- const std::string name;
- const std::string path;
+ std::string name;
+ std::string path;
std::string reportingType;
Milliseconds interval;
bool emitsReadingsUpdate;
diff --git a/src/report_manager.cpp b/src/report_manager.cpp
index c8103a6..64c587e 100644
--- a/src/report_manager.cpp
+++ b/src/report_manager.cpp
@@ -101,7 +101,7 @@
{
throw sdbusplus::exception::SdBusError(
static_cast<int>(std::errc::invalid_argument),
- "Report name exceed maximum length");
+ "Report name exceeds maximum length");
}
}
diff --git a/src/report_manager.hpp b/src/report_manager.hpp
index f056f78..9c21c5d 100644
--- a/src/report_manager.hpp
+++ b/src/report_manager.hpp
@@ -4,6 +4,7 @@
#include "interfaces/report.hpp"
#include "interfaces/report_factory.hpp"
#include "interfaces/report_manager.hpp"
+#include "report.hpp"
#include <systemd/sd-bus-protocol.h>
@@ -59,7 +60,8 @@
static constexpr size_t maxReports{TELEMETRY_MAX_REPORTS};
static constexpr size_t maxReadingParams{TELEMETRY_MAX_READING_PARAMS};
static constexpr size_t maxReportNameLength{
- TELEMETRY_MAX_REPORT_NAME_LENGTH};
+ TELEMETRY_MAX_DBUS_PATH_LENGTH -
+ std::string_view(Report::reportDir).length()};
static constexpr Milliseconds minInterval{TELEMETRY_MIN_INTERVAL};
static constexpr const char* reportManagerIfaceName =
"xyz.openbmc_project.Telemetry.ReportManager";
diff --git a/src/trigger.cpp b/src/trigger.cpp
index 92e1472..ff229b8 100644
--- a/src/trigger.cpp
+++ b/src/trigger.cpp
@@ -1,5 +1,6 @@
#include "trigger.hpp"
+#include "trigger_manager.hpp"
#include "types/report_types.hpp"
#include "types/trigger_types.hpp"
#include "utils/conversion_trigger.hpp"
@@ -10,19 +11,21 @@
Trigger::Trigger(
boost::asio::io_context& ioc,
const std::shared_ptr<sdbusplus::asio::object_server>& objServer,
- const std::string& nameIn, const std::vector<std::string>& triggerActionsIn,
+ const std::string& idIn, const std::string& nameIn,
+ const std::vector<std::string>& triggerActionsIn,
const std::vector<std::string>& reportNamesIn,
const std::vector<LabeledSensorInfo>& LabeledSensorsInfoIn,
const LabeledTriggerThresholdParams& labeledThresholdParamsIn,
std::vector<std::shared_ptr<interfaces::Threshold>>&& thresholdsIn,
interfaces::TriggerManager& triggerManager,
interfaces::JsonStorage& triggerStorageIn) :
- name(nameIn),
- triggerActions(std::move(triggerActionsIn)), path(triggerDir + name),
- reportNames(reportNamesIn), labeledSensorsInfo(LabeledSensorsInfoIn),
+ id(idIn),
+ name(nameIn), triggerActions(std::move(triggerActionsIn)),
+ path(triggerDir + id), reportNames(reportNamesIn),
+ labeledSensorsInfo(LabeledSensorsInfoIn),
labeledThresholdParams(labeledThresholdParamsIn),
thresholds(std::move(thresholdsIn)),
- fileName(std::to_string(std::hash<std::string>{}(name))),
+ fileName(std::to_string(std::hash<std::string>{}(id))),
triggerStorage(triggerStorageIn)
{
deleteIface = objServer->add_unique_interface(
@@ -89,6 +92,15 @@
return isTriggerThresholdDiscrete(labeledThresholdParams);
});
+ dbusIface.register_property_rw(
+ "Name", std::string(),
+ sdbusplus::vtable::property_::emits_change,
+ [this](auto newVal, auto& oldVal) {
+ name = oldVal = newVal;
+ return true;
+ },
+ [this](const auto&) { return name; });
+
dbusIface.register_property_r("TriggerActions", triggerActions,
sdbusplus::vtable::property_::const_,
[this](const auto& x) { return x; });
@@ -107,6 +119,7 @@
nlohmann::json data;
data["Version"] = triggerVersion;
+ data["Id"] = id;
data["Name"] = name;
data["ThresholdParamsDiscriminator"] = labeledThresholdParams.index();
data["TriggerActions"] = triggerActions;
diff --git a/src/trigger.hpp b/src/trigger.hpp
index 904f520..e1874e7 100644
--- a/src/trigger.hpp
+++ b/src/trigger.hpp
@@ -16,7 +16,7 @@
public:
Trigger(boost::asio::io_context& ioc,
const std::shared_ptr<sdbusplus::asio::object_server>& objServer,
- const std::string& name,
+ const std::string& id, const std::string& name,
const std::vector<std::string>& triggerActions,
const std::vector<std::string>& reportNames,
const std::vector<LabeledSensorInfo>& LabeledSensorsInfoIn,
@@ -30,9 +30,9 @@
Trigger& operator=(const Trigger&) = delete;
Trigger& operator=(Trigger&&) = delete;
- std::string getName() const override
+ std::string getId() const override
{
- return name;
+ return id;
}
std::string getPath() const override
@@ -43,9 +43,10 @@
bool storeConfiguration() const;
private:
- const std::string name;
+ std::string id;
+ std::string name;
std::vector<std::string> triggerActions;
- const std::string path;
+ std::string path;
bool persistent = false;
std::vector<std::string> reportNames;
std::vector<LabeledSensorInfo> labeledSensorsInfo;
diff --git a/src/trigger_factory.cpp b/src/trigger_factory.cpp
index 98cfefe..8980cc9 100644
--- a/src/trigger_factory.cpp
+++ b/src/trigger_factory.cpp
@@ -21,7 +21,8 @@
{}
std::unique_ptr<interfaces::Trigger> TriggerFactory::make(
- const std::string& name, const std::vector<std::string>& triggerActionsIn,
+ const std::string& id, const std::string& name,
+ const std::vector<std::string>& triggerActionsIn,
const std::vector<std::string>& reportNames,
interfaces::TriggerManager& triggerManager,
interfaces::JsonStorage& triggerStorage,
@@ -100,9 +101,9 @@
}
return std::make_unique<Trigger>(
- bus->get_io_context(), objServer, name, triggerActionsIn, reportNames,
- labeledSensorsInfo, labeledThresholdParams, std::move(thresholds),
- triggerManager, triggerStorage);
+ bus->get_io_context(), objServer, id, name, triggerActionsIn,
+ reportNames, labeledSensorsInfo, labeledThresholdParams,
+ std::move(thresholds), triggerManager, triggerStorage);
}
std::pair<Sensors, std::vector<std::string>> TriggerFactory::getSensors(
diff --git a/src/trigger_factory.hpp b/src/trigger_factory.hpp
index e7f8fb0..757d663 100644
--- a/src/trigger_factory.hpp
+++ b/src/trigger_factory.hpp
@@ -16,7 +16,7 @@
interfaces::ReportManager& reportManager);
std::unique_ptr<interfaces::Trigger>
- make(const std::string& name,
+ make(const std::string& id, const std::string& name,
const std::vector<std::string>& triggerActions,
const std::vector<std::string>& reportNames,
interfaces::TriggerManager& triggerManager,
diff --git a/src/trigger_manager.cpp b/src/trigger_manager.cpp
index da4797f..ac0fba5 100644
--- a/src/trigger_manager.cpp
+++ b/src/trigger_manager.cpp
@@ -20,7 +20,7 @@
triggerManagerPath, triggerManagerIfaceName, [this](auto& iface) {
iface.register_method(
"AddTrigger",
- [this](boost::asio::yield_context& yield,
+ [this](boost::asio::yield_context& yield, const std::string& id,
const std::string& name,
const std::vector<std::string>& triggerActions,
const SensorsInfo& sensors,
@@ -34,8 +34,8 @@
std::vector<LabeledSensorInfo> labeledSensorsInfo =
triggerFactory->getLabeledSensorsInfo(yield, sensors);
- return addTrigger(name, triggerActions, labeledSensorsInfo,
- reportNames,
+ return addTrigger(id, name, triggerActions,
+ labeledSensorsInfo, reportNames,
labeledTriggerThresholdParams)
.getPath();
});
@@ -50,7 +50,8 @@
triggers.end());
}
-void TriggerManager::verifyAddTrigger(const std::string& triggerName)
+void TriggerManager::verifyAddTrigger(const std::string& triggerId,
+ const std::string& triggerName) const
{
if (triggers.size() >= maxTriggers)
{
@@ -59,9 +60,12 @@
"Reached maximal trigger count");
}
+ verifyTriggerIdLength(triggerId);
+ verifyIdCharacters(triggerId);
+
for (const auto& trigger : triggers)
{
- if (trigger->getName() == triggerName)
+ if (trigger->getId() == triggerId)
{
throw sdbusplus::exception::SdBusError(
static_cast<int>(std::errc::file_exists), "Duplicate trigger");
@@ -69,18 +73,79 @@
}
}
+void TriggerManager::verifyTriggerIdLength(const std::string& triggerId)
+{
+ if (triggerId.length() > maxTriggerIdLength)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument),
+ "Trigger id exceeds maximum length");
+ }
+}
+
+void TriggerManager::verifyIdCharacters(const std::string& triggerId)
+{
+ if (triggerId.find_first_not_of(allowedCharactersInId) != std::string::npos)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument),
+ "Invalid character in trigger id");
+ }
+}
+
+std::string TriggerManager::generateId(const std::string& prefix,
+ const std::string& triggerName) const
+{
+ verifyIdCharacters(prefix);
+ std::string strippedId(triggerName);
+ strippedId.erase(std::remove_if(strippedId.begin(), strippedId.end(),
+ [](char c) {
+ return c == '/' ||
+ allowedCharactersInId.find(c) ==
+ std::string_view::npos;
+ }),
+ strippedId.end());
+
+ strippedId = prefix + strippedId;
+ strippedId = strippedId.substr(
+ 0, maxTriggerIdLength - std::to_string(maxTriggers - 1).length());
+
+ size_t idx = 0;
+ std::string tmpId(strippedId);
+ while (std::find_if(triggers.begin(), triggers.end(),
+ [&tmpId](const auto& trigger) {
+ return trigger->getId() == tmpId;
+ }) != triggers.end())
+ {
+ tmpId = strippedId + std::to_string(idx++);
+ }
+ return tmpId;
+}
+
interfaces::Trigger& TriggerManager::addTrigger(
- const std::string& triggerName,
+ const std::string& triggerIdIn, const std::string& triggerNameIn,
const std::vector<std::string>& triggerActions,
const std::vector<LabeledSensorInfo>& labeledSensorsInfo,
const std::vector<std::string>& reportNames,
const LabeledTriggerThresholdParams& labeledThresholdParams)
{
- verifyAddTrigger(triggerName);
+ std::string triggerName = triggerNameIn;
+ if (triggerName.empty())
+ {
+ triggerName = triggerNameDefault;
+ }
+
+ std::string triggerId = triggerIdIn;
+ if (triggerId.empty() || triggerId.ends_with('/'))
+ {
+ triggerId = generateId(triggerId, triggerName);
+ }
+
+ verifyAddTrigger(triggerId, triggerName);
triggers.emplace_back(triggerFactory->make(
- triggerName, triggerActions, reportNames, *this, *triggerStorage,
- labeledThresholdParams, labeledSensorsInfo));
+ triggerId, triggerName, triggerActions, reportNames, *this,
+ *triggerStorage, labeledThresholdParams, labeledSensorsInfo));
return *triggers.back();
}
@@ -104,6 +169,7 @@
{
throw std::runtime_error("Invalid version");
}
+ const std::string& id = data->at("Id").get_ref<std::string&>();
const std::string& name = data->at("Name").get_ref<std::string&>();
int thresholdParamsDiscriminator =
data->at("ThresholdParamsDiscriminator").get<int>();
@@ -130,8 +196,8 @@
auto labeledSensorsInfo =
data->at("Sensors").get<std::vector<LabeledSensorInfo>>();
- addTrigger(name, triggerActions, labeledSensorsInfo, reportNames,
- labeledThresholdParams);
+ addTrigger(id, name, triggerActions, labeledSensorsInfo,
+ reportNames, labeledThresholdParams);
}
catch (const std::exception& e)
{
diff --git a/src/trigger_manager.hpp b/src/trigger_manager.hpp
index c3acd36..4837062 100644
--- a/src/trigger_manager.hpp
+++ b/src/trigger_manager.hpp
@@ -3,6 +3,7 @@
#include "interfaces/report_manager.hpp"
#include "interfaces/trigger_factory.hpp"
#include "interfaces/trigger_manager.hpp"
+#include "trigger.hpp"
#include <sdbusplus/asio/object_server.hpp>
@@ -30,10 +31,15 @@
std::unique_ptr<sdbusplus::asio::dbus_interface> managerIface;
std::vector<std::unique_ptr<interfaces::Trigger>> triggers;
- void verifyAddTrigger(const std::string& triggerName);
+ void verifyAddTrigger(const std::string& triggerId,
+ const std::string& triggerName) const;
+ std::string generateId(const std::string& prefix,
+ const std::string& triggerName) const;
+ static void verifyTriggerIdLength(const std::string& triggerId);
+ static void verifyIdCharacters(const std::string& triggerId);
interfaces::Trigger&
- addTrigger(const std::string& triggerName,
+ addTrigger(const std::string& triggerId, const std::string& triggerName,
const std::vector<std::string>& triggerActions,
const std::vector<LabeledSensorInfo>& labeledSensors,
const std::vector<std::string>& reportNames,
@@ -42,8 +48,15 @@
public:
static constexpr size_t maxTriggers{TELEMETRY_MAX_TRIGGERS};
+ static constexpr size_t maxTriggerIdLength{
+ TELEMETRY_MAX_DBUS_PATH_LENGTH -
+ std::string_view(Trigger::triggerDir).length()};
static constexpr const char* triggerManagerIfaceName =
"xyz.openbmc_project.Telemetry.TriggerManager";
static constexpr const char* triggerManagerPath =
"/xyz/openbmc_project/Telemetry/Triggers";
+
+ static constexpr std::string_view allowedCharactersInId =
+ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_/";
+ static constexpr const char* triggerNameDefault = "Trigger";
};
diff --git a/tests/src/dbus_environment.hpp b/tests/src/dbus_environment.hpp
index 0ddf241..087daef 100644
--- a/tests/src/dbus_environment.hpp
+++ b/tests/src/dbus_environment.hpp
@@ -1,6 +1,7 @@
#pragma once
#include "types/duration_type.hpp"
+#include "utils/set_exception.hpp"
#include <sdbusplus/asio/object_server.hpp>
#include <sdbusplus/asio/property.hpp>
@@ -92,6 +93,44 @@
static bool waitForFutures(std::string_view name,
Milliseconds timeout = std::chrono::seconds(10));
+ template <class T>
+ static T getProperty(const std::string& path,
+ const std::string& interfaceName,
+ const std::string& property)
+ {
+ auto propertyPromise = std::promise<T>();
+ auto propertyFuture = propertyPromise.get_future();
+ sdbusplus::asio::getProperty<T>(
+ *DbusEnvironment::getBus(), DbusEnvironment::serviceName(), path,
+ interfaceName, property,
+ [&propertyPromise](const boost::system::error_code& ec, T t) {
+ if (ec)
+ {
+ utils::setException(propertyPromise, "GetProperty failed");
+ return;
+ }
+ propertyPromise.set_value(t);
+ });
+ return DbusEnvironment::waitForFuture(std::move(propertyFuture));
+ }
+
+ template <class T>
+ static boost::system::error_code
+ setProperty(const std::string& path, const std::string& interfaceName,
+ const std::string& property, const T& newValue)
+ {
+ auto setPromise = std::promise<boost::system::error_code>();
+ auto future = setPromise.get_future();
+ sdbusplus::asio::setProperty(
+ *DbusEnvironment::getBus(), DbusEnvironment::serviceName(), path,
+ interfaceName, property, std::move(newValue),
+ [setPromise =
+ std::move(setPromise)](boost::system::error_code ec) mutable {
+ setPromise.set_value(ec);
+ });
+ return DbusEnvironment::waitForFuture(std::move(future));
+ }
+
private:
static std::future<bool> getFuture(std::string_view name);
diff --git a/tests/src/mocks/trigger_factory_mock.hpp b/tests/src/mocks/trigger_factory_mock.hpp
index 3df567e..ab7510d 100644
--- a/tests/src/mocks/trigger_factory_mock.hpp
+++ b/tests/src/mocks/trigger_factory_mock.hpp
@@ -15,14 +15,14 @@
{
using namespace testing;
- ON_CALL(*this, make(A<const std::string&>(), _, _, _, _, _, _))
- .WillByDefault(WithArgs<0>(Invoke([](const std::string& name) {
- return std::make_unique<NiceMock<TriggerMock>>(name);
+ ON_CALL(*this, make(A<const std::string&>(), _, _, _, _, _, _, _))
+ .WillByDefault(WithArgs<0>(Invoke([](const std::string& id) {
+ return std::make_unique<NiceMock<TriggerMock>>(id);
})));
}
MOCK_METHOD(std::unique_ptr<interfaces::Trigger>, make,
- (const std::string& name,
+ (const std::string& id, const std::string& name,
const std::vector<std::string>& triggerActions,
const std::vector<std::string>& reportNames,
interfaces::TriggerManager& triggerManager,
@@ -53,7 +53,7 @@
.WillByDefault(Return(params.sensors()));
return EXPECT_CALL(
- *this, make(params.name(), params.triggerActions(),
+ *this, make(params.id(), params.name(), params.triggerActions(),
params.reportNames(), tm, triggerStorage,
params.thresholdParams(), params.sensors()));
}
@@ -68,7 +68,7 @@
.WillByDefault(Return(dummy));
return EXPECT_CALL(*this,
- make(_, _, _, tm, triggerStorage, _, dummy));
+ make(_, _, _, _, tm, triggerStorage, _, dummy));
}
}
};
diff --git a/tests/src/mocks/trigger_mock.hpp b/tests/src/mocks/trigger_mock.hpp
index d8eddd2..be40a65 100644
--- a/tests/src/mocks/trigger_mock.hpp
+++ b/tests/src/mocks/trigger_mock.hpp
@@ -7,12 +7,12 @@
class TriggerMock : public interfaces::Trigger
{
public:
- TriggerMock(std::string name)
+ TriggerMock(std::string id)
{
using namespace testing;
- ON_CALL(*this, getName).WillByDefault([name] { return name; });
- ON_CALL(*this, getPath).WillByDefault([name] { return "/" + name; });
+ ON_CALL(*this, getId).WillByDefault([id] { return id; });
+ ON_CALL(*this, getPath).WillByDefault([id] { return "/" + id; });
EXPECT_CALL(*this, Die).Times(AnyNumber());
}
@@ -21,7 +21,7 @@
Die();
}
- MOCK_METHOD(std::string, getName, (), (const, override));
+ MOCK_METHOD(std::string, getId, (), (const, override));
MOCK_METHOD(std::string, getPath, (), (const, override));
MOCK_METHOD(void, Die, ());
};
diff --git a/tests/src/params/trigger_params.hpp b/tests/src/params/trigger_params.hpp
index f2cac0e..62acd7a 100644
--- a/tests/src/params/trigger_params.hpp
+++ b/tests/src/params/trigger_params.hpp
@@ -11,6 +11,17 @@
class TriggerParams
{
public:
+ TriggerParams& id(std::string val)
+ {
+ idProperty = std::move(val);
+ return *this;
+ }
+
+ const std::string& id() const
+ {
+ return idProperty;
+ }
+
TriggerParams& name(std::string val)
{
nameProperty = std::move(val);
@@ -55,7 +66,8 @@
}
private:
- std::string nameProperty = "Trigger1";
+ std::string idProperty = "Trigger1";
+ std::string nameProperty = "My Numeric Trigger";
std::vector<std::string> triggerActionsProperty = {"UpdateReport"};
std::vector<LabeledSensorInfo> labeledSensorsProperty = {
diff --git a/tests/src/test_report.cpp b/tests/src/test_report.cpp
index 79e66ff..f5f84c1 100644
--- a/tests/src/test_report.cpp
+++ b/tests/src/test_report.cpp
@@ -7,7 +7,6 @@
#include "report.hpp"
#include "report_manager.hpp"
#include "utils/conv_container.hpp"
-#include "utils/set_exception.hpp"
#include "utils/tstring.hpp"
#include <sdbusplus/exception.hpp>
@@ -80,20 +79,17 @@
template <class T>
static T getProperty(const std::string& path, const std::string& property)
{
- auto propertyPromise = std::promise<T>();
- auto propertyFuture = propertyPromise.get_future();
- sdbusplus::asio::getProperty<T>(
- *DbusEnvironment::getBus(), DbusEnvironment::serviceName(), path,
- Report::reportIfaceName, property,
- [&propertyPromise](const boost::system::error_code& ec, T t) {
- if (ec)
- {
- utils::setException(propertyPromise, "GetProperty failed");
- return;
- }
- propertyPromise.set_value(t);
- });
- return DbusEnvironment::waitForFuture(std::move(propertyFuture));
+ return DbusEnvironment::getProperty<T>(path, Report::reportIfaceName,
+ property);
+ }
+
+ template <class T>
+ static boost::system::error_code setProperty(const std::string& path,
+ const std::string& property,
+ const T& newValue)
+ {
+ return DbusEnvironment::setProperty<T>(path, Report::reportIfaceName,
+ property, newValue);
}
boost::system::error_code call(const std::string& path,
@@ -114,23 +110,6 @@
return call(path, Report::reportIfaceName, "Update");
}
- template <class T>
- static boost::system::error_code setProperty(const std::string& path,
- const std::string& property,
- const T& newValue)
- {
- auto setPromise = std::promise<boost::system::error_code>();
- auto future = setPromise.get_future();
- sdbusplus::asio::setProperty(
- *DbusEnvironment::getBus(), DbusEnvironment::serviceName(), path,
- Report::reportIfaceName, property, std::move(newValue),
- [setPromise =
- std::move(setPromise)](boost::system::error_code ec) mutable {
- setPromise.set_value(ec);
- });
- return DbusEnvironment::waitForFuture(std::move(future));
- }
-
boost::system::error_code deleteReport(const std::string& path)
{
return call(path, Report::deleteIfaceName, "Delete");
diff --git a/tests/src/test_report_manager.cpp b/tests/src/test_report_manager.cpp
index e0705ca..eeedd96 100644
--- a/tests/src/test_report_manager.cpp
+++ b/tests/src/test_report_manager.cpp
@@ -6,7 +6,6 @@
#include "report.hpp"
#include "report_manager.hpp"
#include "utils/conversion.hpp"
-#include "utils/set_exception.hpp"
#include "utils/transform.hpp"
using namespace testing;
@@ -69,33 +68,11 @@
}
template <class T>
- static T getProperty(std::string property)
+ static T getProperty(const std::string& property)
{
- auto propertyPromise = std::promise<T>();
- auto propertyFuture = propertyPromise.get_future();
- sdbusplus::asio::getProperty<T>(
- *DbusEnvironment::getBus(), DbusEnvironment::serviceName(),
+ return DbusEnvironment::getProperty<T>(
ReportManager::reportManagerPath,
- ReportManager::reportManagerIfaceName, property,
- [&propertyPromise](const boost::system::error_code& ec, T t) {
- if (ec)
- {
- utils::setException(propertyPromise, "Get property failed");
- return;
- }
- propertyPromise.set_value(t);
- });
- return DbusEnvironment::waitForFuture(std::move(propertyFuture));
- }
-
- static std::string prepareReportNameWithLength(size_t length)
- {
- std::stringstream reportNameStream;
- for (size_t i = 0; i < length; ++i)
- {
- reportNameStream << "z";
- }
- return reportNameStream.str();
+ ReportManager::reportManagerIfaceName, property);
}
};
@@ -124,8 +101,7 @@
TEST_F(TestReportManager, addReportWithMaxLengthName)
{
- std::string reportName =
- prepareReportNameWithLength(ReportManager::maxReportNameLength);
+ std::string reportName(ReportManager::maxReportNameLength, 'z');
reportParams.reportName(reportName);
reportFactoryMock.expectMake(reportParams, Ref(*sut), Ref(storageMock));
@@ -141,7 +117,7 @@
.Times(0);
reportParams.reportName(
- prepareReportNameWithLength(ReportManager::maxReportNameLength + 1));
+ std::string(ReportManager::maxReportNameLength + 1, 'z'));
auto [ec, path] = addReport(reportParams);
diff --git a/tests/src/test_trigger.cpp b/tests/src/test_trigger.cpp
index 3a9004f..a074e7a 100644
--- a/tests/src/test_trigger.cpp
+++ b/tests/src/test_trigger.cpp
@@ -4,8 +4,8 @@
#include "mocks/trigger_manager_mock.hpp"
#include "params/trigger_params.hpp"
#include "trigger.hpp"
+#include "trigger_manager.hpp"
#include "utils/conversion_trigger.hpp"
-#include "utils/set_exception.hpp"
#include "utils/transform.hpp"
#include "utils/tstring.hpp"
@@ -22,7 +22,8 @@
TriggerParams triggerParams;
TriggerParams triggerDiscreteParams =
TriggerParams()
- .name("Trigger2")
+ .id("DiscreteTrigger")
+ .name("My Discrete Trigger")
.thresholdParams(std::vector<discrete::LabeledThresholdParam>{
discrete::LabeledThresholdParam{
"userId", discrete::Severity::warning,
@@ -55,8 +56,8 @@
{
return std::make_unique<Trigger>(
DbusEnvironment::getIoc(), DbusEnvironment::getObjServer(),
- params.name(), params.triggerActions(), params.reportNames(),
- params.sensors(), params.thresholdParams(),
+ params.id(), params.name(), params.triggerActions(),
+ params.reportNames(), params.sensors(), params.thresholdParams(),
std::vector<std::shared_ptr<interfaces::Threshold>>{},
*triggerManagerMockPtr, storageMock);
}
@@ -70,20 +71,8 @@
template <class T>
static T getProperty(const std::string& path, const std::string& property)
{
- auto propertyPromise = std::promise<T>();
- auto propertyFuture = propertyPromise.get_future();
- sdbusplus::asio::getProperty<T>(
- *DbusEnvironment::getBus(), DbusEnvironment::serviceName(), path,
- Trigger::triggerIfaceName, property,
- [&propertyPromise](const boost::system::error_code& ec, T t) {
- if (ec)
- {
- utils::setException(propertyPromise, "GetProperty failed");
- return;
- }
- propertyPromise.set_value(t);
- });
- return DbusEnvironment::waitForFuture(std::move(propertyFuture));
+ return DbusEnvironment::getProperty<T>(path, Trigger::triggerIfaceName,
+ property);
}
template <class T>
@@ -91,17 +80,8 @@
const std::string& property,
const T& newValue)
{
- auto setPromise = std::promise<boost::system::error_code>();
- auto setFuture = setPromise.get_future();
-
- sdbusplus::asio::setProperty(
- *DbusEnvironment::getBus(), DbusEnvironment::serviceName(), path,
- Trigger::triggerIfaceName, property, std::move(newValue),
- [setPromise =
- std::move(setPromise)](boost::system::error_code ec) mutable {
- setPromise.set_value(ec);
- });
- return DbusEnvironment::waitForFuture(std::move(setFuture));
+ return DbusEnvironment::setProperty<T>(path, Trigger::triggerIfaceName,
+ property, newValue);
}
boost::system::error_code deleteTrigger(const std::string& path)
@@ -119,6 +99,8 @@
TEST_F(TestTrigger, checkIfPropertiesAreSet)
{
+ EXPECT_THAT(getProperty<std::string>(sut->getPath(), "Name"),
+ Eq(triggerParams.name()));
EXPECT_THAT(getProperty<bool>(sut->getPath(), "Persistent"), Eq(true));
EXPECT_THAT(
getProperty<std::vector<std::string>>(sut->getPath(), "TriggerActions"),
@@ -134,6 +116,14 @@
triggerParams.thresholdParams())));
}
+TEST_F(TestTrigger, setPropertyNameToCorrectValue)
+{
+ std::string name = "custom name 1234 %^#5";
+ EXPECT_THAT(setProperty(sut->getPath(), "Name", name),
+ Eq(boost::system::errc::success));
+ EXPECT_THAT(getProperty<std::string>(sut->getPath(), "Name"), Eq(name));
+}
+
TEST_F(TestTrigger, checkIfNumericCoversionsAreGood)
{
const auto& labeledParamsBase =
@@ -188,7 +178,7 @@
TEST_F(TestTrigger, deleteTrigger)
{
- EXPECT_CALL(storageMock, remove(to_file_path(sut->getName())));
+ EXPECT_CALL(storageMock, remove(to_file_path(sut->getId())));
EXPECT_CALL(*triggerManagerMockPtr, removeTrigger(sut.get()));
auto ec = deleteTrigger(sut->getPath());
EXPECT_THAT(ec, Eq(boost::system::errc::success));
@@ -202,7 +192,7 @@
TEST_F(TestTrigger, settingPersistencyToFalseRemovesTriggerFromStorage)
{
- EXPECT_CALL(storageMock, remove(to_file_path(sut->getName())));
+ EXPECT_CALL(storageMock, remove(to_file_path(sut->getId())));
bool persistent = false;
EXPECT_THAT(setProperty(sut->getPath(), "Persistent", persistent),
@@ -230,11 +220,11 @@
EXPECT_THAT(getProperty<bool>(sut->getPath(), "Persistent"), Eq(false));
}
-TEST_F(TestTriggerErrors, creatingTriggerThrowsExceptionWhenNameIsInvalid)
+TEST_F(TestTriggerErrors, creatingTriggerThrowsExceptionWhenIdIsInvalid)
{
EXPECT_CALL(storageMock, store(_, _)).Times(0);
- EXPECT_THROW(makeTrigger(triggerParams.name("inv?lidName")),
+ EXPECT_THROW(makeTrigger(triggerParams.id("inv?lidId")),
sdbusplus::exception::SdBusError);
}
@@ -262,6 +252,11 @@
ASSERT_THAT(storedConfiguration.at("Version"), Eq(expectedTriggerVersion));
}
+TEST_F(TestTriggerStore, settingPersistencyToTrueStoresTriggerId)
+{
+ ASSERT_THAT(storedConfiguration.at("Id"), Eq(triggerParams.id()));
+}
+
TEST_F(TestTriggerStore, settingPersistencyToTrueStoresTriggerName)
{
ASSERT_THAT(storedConfiguration.at("Name"), Eq(triggerParams.name()));
diff --git a/tests/src/test_trigger_manager.cpp b/tests/src/test_trigger_manager.cpp
index 540bcbc..c813e97 100644
--- a/tests/src/test_trigger_manager.cpp
+++ b/tests/src/test_trigger_manager.cpp
@@ -28,7 +28,7 @@
addTriggerPromise.set_value({ec, path});
},
DbusEnvironment::serviceName(), TriggerManager::triggerManagerPath,
- TriggerManager::triggerManagerIfaceName, "AddTrigger",
+ TriggerManager::triggerManagerIfaceName, "AddTrigger", params.id(),
params.name(), params.triggerActions(), sensorInfos,
params.reportNames(),
std::visit(utils::FromLabeledThresholdParamConversion(),
@@ -55,7 +55,7 @@
std::make_unique<NiceMock<TriggerFactoryMock>>();
TriggerFactoryMock& triggerFactoryMock = *triggerFactoryMockPtr;
std::unique_ptr<TriggerMock> triggerMockPtr =
- std::make_unique<NiceMock<TriggerMock>>(TriggerParams().name());
+ std::make_unique<NiceMock<TriggerMock>>(TriggerParams().id());
TriggerMock& triggerMock = *triggerMockPtr;
std::unique_ptr<TriggerManager> sut;
MockFunction<void(std::string)> checkPoint;
@@ -110,6 +110,93 @@
EXPECT_THAT(path, Eq(std::string()));
}
+TEST_F(TestTriggerManager, DISABLED_failToAddTriggerWithInvalidId)
+{
+ triggerFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ auto [ec, path] = addTrigger(TriggerParams().id("not valid?"));
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
+TEST_F(TestTriggerManager, addTriggerWithoutIdAndName)
+{
+ triggerFactoryMock
+ .expectMake(TriggerParams()
+ .id(TriggerManager::triggerNameDefault)
+ .name(TriggerManager::triggerNameDefault),
+ Ref(*sut), Ref(storageMock))
+ .WillOnce(Return(ByMove(std::move(triggerMockPtr))));
+
+ auto [ec, path] = addTrigger(TriggerParams().id("").name(""));
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Not(Eq("")));
+}
+
+TEST_F(TestTriggerManager, addTriggerWithPrefixId)
+{
+ triggerFactoryMock
+ .expectMake(TriggerParams()
+ .id("TelemetryService/HackyName")
+ .name("Hacky/Name!@#$"),
+ Ref(*sut), Ref(storageMock))
+ .WillOnce(Return(ByMove(std::move(triggerMockPtr))));
+
+ auto [ec, path] = addTrigger(
+ TriggerParams().id("TelemetryService/").name("Hacky/Name!@#$"));
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Not(Eq("")));
+}
+
+TEST_F(TestTriggerManager, addTriggerWithoutIdTwice)
+{
+ addTrigger(TriggerParams().id(""));
+
+ auto [ec, path] = addTrigger(TriggerParams().id(""));
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Not(Eq("")));
+}
+
+TEST_F(TestTriggerManager, addTriggerWithoutIdAndWithLongNameTwice)
+{
+ addTrigger(TriggerParams().id("").name(
+ std::string(2 * TriggerManager::maxTriggerIdLength, 'z')));
+
+ auto [ec, path] = addTrigger(TriggerParams().id("").name(
+ std::string(2 * TriggerManager::maxTriggerIdLength, 'z')));
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Not(Eq("")));
+}
+
+TEST_F(TestTriggerManager, addTriggerWithMaxLengthId)
+{
+ auto triggerId = std::string(TriggerManager::maxTriggerIdLength, 'z');
+ auto triggerParams = TriggerParams().id(triggerId);
+
+ triggerFactoryMock.expectMake(triggerParams, Ref(*sut), Ref(storageMock))
+ .WillOnce(Return(ByMove(std::move(triggerMockPtr))));
+
+ auto [ec, path] = addTrigger(triggerParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Eq(triggerMock.getPath()));
+}
+
+TEST_F(TestTriggerManager, DISABLED_failToAddTriggerWithTooLongId)
+{
+ auto triggerId = std::string(TriggerManager::maxTriggerIdLength + 1, 'z');
+ auto triggerParams = TriggerParams().id(triggerId);
+
+ triggerFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ auto [ec, path] = addTrigger(triggerParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
TEST_F(TestTriggerManager, DISABLED_failToAddTriggerWhenMaxTriggerIsReached)
{
auto triggerParams = TriggerParams();
@@ -119,14 +206,14 @@
for (size_t i = 0; i < TriggerManager::maxTriggers; i++)
{
- triggerParams.name(TriggerParams().name() + std::to_string(i));
+ triggerParams.id(TriggerParams().id() + std::to_string(i));
auto [ec, path] = addTrigger(triggerParams);
EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
}
- triggerParams.name(TriggerParams().name() +
- std::to_string(TriggerManager::maxTriggers));
+ triggerParams.id(TriggerParams().id() +
+ std::to_string(TriggerManager::maxTriggers));
auto [ec, path] = addTrigger(triggerParams);
EXPECT_THAT(ec.value(), Eq(boost::system::errc::too_many_files_open));
EXPECT_THAT(path, Eq(std::string()));
@@ -192,13 +279,15 @@
ON_CALL(storageMock, load(FilePath("trigger1")))
.WillByDefault(InvokeWithoutArgs([this] { return data1; }));
- data2["Name"] = "Trigger2";
+ data2["Id"] = "Trigger2";
+ data2["Name"] = "Second Trigger";
ON_CALL(storageMock, load(FilePath("trigger2")))
.WillByDefault(InvokeWithoutArgs([this] { return data2; }));
}
nlohmann::json data1 = nlohmann::json{
{"Version", Trigger::triggerVersion},
+ {"Id", TriggerParams().id()},
{"Name", TriggerParams().name()},
{"ThresholdParamsDiscriminator",
TriggerParams().thresholdParams().index()},
@@ -214,8 +303,9 @@
TEST_F(TestTriggerManagerStorage, triggerManagerCtorAddTriggerFromStorage)
{
triggerFactoryMock.expectMake(TriggerParams(), _, Ref(storageMock));
- triggerFactoryMock.expectMake(TriggerParams().name("Trigger2"), _,
- Ref(storageMock));
+ triggerFactoryMock.expectMake(
+ TriggerParams().id("Trigger2").name("Second Trigger"), _,
+ Ref(storageMock));
EXPECT_CALL(storageMock, remove(_)).Times(0);
sut = makeTriggerManager();
@@ -254,8 +344,9 @@
{
data1["Version"] = Trigger::triggerVersion - 1;
- triggerFactoryMock.expectMake(TriggerParams().name("Trigger2"), _,
- Ref(storageMock));
+ triggerFactoryMock.expectMake(
+ TriggerParams().id("Trigger2").name("Second Trigger"), _,
+ Ref(storageMock));
EXPECT_CALL(storageMock, remove(FilePath("trigger1")));
sut = makeTriggerManager();