Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax Alfalfa Point ID Requirements #5296

Merged
merged 6 commits into from
Nov 8, 2024
Merged

Relax Alfalfa Point ID Requirements #5296

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
912124c
remove most point id requirements
TShapinsky Nov 7, 2024
257d8d3
remove unnecessary setting of point Id
TShapinsky Nov 7, 2024
25e61d0
Update src/alfalfa/AlfalfaJSON.cpp
TShapinsky Nov 8, 2024
175fd88
Update src/alfalfa/test/AlfalfaJSON_GTest.cpp
TShapinsky Nov 8, 2024
56565e9
use std::replace instead of regex
TShapinsky Nov 8, 2024
d5c11a6
fix formatting
TShapinsky Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/alfalfa/AlfalfaJSON.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,10 @@ namespace alfalfa {

Json::Value AlfalfaJSON_Impl::toJSON() const {
Json::Value root;
for (const auto& point : m_points) {
int i = 0;
for (const auto& point : points()) {
TShapinsky marked this conversation as resolved.
Show resolved Hide resolved
// No guard here as the toJSON call will throw an exception if the id does not exist.
root[point.id().get()] = point.toJSON();
root[i++] = point.toJSON();
}
return root;
}
Expand Down
43 changes: 9 additions & 34 deletions src/alfalfa/AlfalfaPoint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
namespace openstudio {
namespace alfalfa {

static constexpr std::string_view ID_VALID_CHARS_MSG = "IDs can only contain letters, numbers, and the following special characters _-[]():";

namespace detail {

AlfalfaPoint_Impl::AlfalfaPoint_Impl(const std::string& display_name) {
Expand All @@ -22,11 +20,7 @@ namespace alfalfa {
Json::Value AlfalfaPoint_Impl::toJSON() const {
Json::Value point;

if (auto id_ = id()) {
point["id"] = *id_;
} else {
throw std::runtime_error(fmt::format("Point requires a valid ID for export. {}", ID_VALID_CHARS_MSG));
}
point["id"] = id();
point["name"] = displayName();
if (auto input_ = input()) {
point["input"]["type"] = input_->typeName();
Expand Down Expand Up @@ -80,37 +74,24 @@ namespace alfalfa {
}

void AlfalfaPoint_Impl::setId(const std::string& id) {
if (isValidId(id)) {
m_id = id;
} else {
throw std::runtime_error(ID_VALID_CHARS_MSG.data());
if (id.empty()) {
throw std::runtime_error("Id must have non-zero length");
}
m_id = toIdString(id);
}

boost::optional<std::string> AlfalfaPoint_Impl::id() const {
boost::optional<std::string> result = m_id;
if (!result.is_initialized()) {
std::string id = toIdString(displayName());
if (isValidId(id)) {
result = id;
} else {
LOG(Warn, fmt::format("Display name '{}' does not produce a valid point ID. Manually set a valid ID or export will fail.", displayName()));
}
std::string AlfalfaPoint_Impl::id() const {
if (m_id.empty()) {
return toIdString(displayName());
}
return result;
return m_id;
}

void AlfalfaPoint_Impl::setDisplayName(const std::string& display_name) {
if (display_name.empty()) {
throw std::runtime_error("Display name must have non-zero length");
}
m_display_name = display_name;
if (!m_id.is_initialized()) {
const std::string id = toIdString(display_name);
if (!isValidId(id)) {
LOG(Warn, fmt::format("Display name '{}' does not produce a valid point ID. Manually set a valid ID or export will fail.", display_name));
}
}
}

std::string AlfalfaPoint_Impl::displayName() const {
Expand All @@ -125,17 +106,11 @@ namespace alfalfa {
return m_optional;
}

bool AlfalfaPoint_Impl::isValidId(const std::string& id) {
return !id.empty() && boost::regex_match(id, boost::regex(R"(^[A-Za-z0-9_\-\[\]:()]*$)"));
}
Comment on lines -128 to -130
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TShapinsky can you clarify what you meant by "onerous"?

Was it just annoying / too restrictive?
Or was is slow?

Boost (and std) regexes are awful in terms of performance, but using a static regex makes it better (we use Meyers singletons in IddRegex.hpp for eg)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was mostly annoying / too restrictive. For some background, the point ids become path parameters when interacting with the Alfalfa/BOPTest REST API. Originally I thought it would be neat to restrict the character set to avoid needing a lot of percent encoded characters. However, it's pretty common for names in open studio to include various different characters and since the requirement isn't hard I decided to get rid of it in the interest of making the alfalfa interface more user friendly.


std::string AlfalfaPoint_Impl::toIdString(const std::string& str) {
return boost::regex_replace(str, boost::regex(" "), "_");
Copy link
Collaborator

@jmarrec jmarrec Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a side note, but this is probably much faster, not that it matters much probably

#include <algorithm>
std::replace( str.begin(), str.end(), ' ', '_')

Edit: It is in fact, 150 times faster: https://quick-bench.com/q/LBF5Z_hhF14xxmjJ8IGs7UhGAUY

}
} // namespace detail

// AlfalfaPoint::AlfalfaPoint(const AlfalfaPoint& point) : m_impl(point.m_impl) {}

AlfalfaPoint::AlfalfaPoint(const std::string& display_name) : m_impl(std::make_shared<detail::AlfalfaPoint_Impl>(display_name)) {}

void AlfalfaPoint::setInput(const AlfalfaComponent& component) {
Expand All @@ -162,7 +137,7 @@ namespace alfalfa {
return m_impl->units();
}

boost::optional<std::string> AlfalfaPoint::id() const {
std::string AlfalfaPoint::id() const {
return m_impl->id();
}

Expand Down
2 changes: 1 addition & 1 deletion src/alfalfa/AlfalfaPoint.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ namespace alfalfa {
/**
* Get id of point. By default this will be a version of the display name with spaces removed.
*/
boost::optional<std::string> id() const;
std::string id() const;

/**
* Set id of point. This is the component which will uniquely identify the point in the API.
Expand Down
6 changes: 2 additions & 4 deletions src/alfalfa/AlfalfaPoint_Impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ namespace alfalfa {

boost::optional<std::string> units() const;

boost::optional<std::string> id() const;
std::string id() const;

void setId(const std::string& id);

Expand All @@ -47,13 +47,11 @@ namespace alfalfa {
private:
static std::string toIdString(const std::string& str);

static bool isValidId(const std::string& id);

boost::optional<AlfalfaComponent> m_input;
boost::optional<AlfalfaComponent> m_output;
std::string m_display_name;
boost::optional<std::string> m_units;
boost::optional<std::string> m_id;
std::string m_id;
bool m_optional = true;

// configure logging
Expand Down
59 changes: 15 additions & 44 deletions src/alfalfa/test/AlfalfaJSON_GTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <fmt/core.h>
#include <gtest/gtest.h>
#include <stdexcept>
#include <vector>

#include "../AlfalfaJSON.hpp"
Expand Down Expand Up @@ -194,75 +195,47 @@ TEST(AlfalfaJSON, json_serialization) {
const bool parsing_success = Json::parseFromStream(r_builder, ifs, &root, &formatted_errors);
EXPECT_TRUE(parsing_success);
EXPECT_EQ(alfalfa.toJSON(), root);
for (const AlfalfaPoint& point : alfalfa.points()) {
EXPECT_EQ(root[point.id().get()], point.toJSON());
int i = 0;
for (const auto& point : alfalfa.points()) {
EXPECT_EQ(root[i++], point.toJSON());
TShapinsky marked this conversation as resolved.
Show resolved Hide resolved
}
}

TEST(AlfalfaJSON, point_exceptions_logging) {
const std::string ID_VALID_CHARS_MSG = "IDs can only contain letters, numbers, and the following special characters _-[]():";
const std::string DISPLAY_NAME_VALID_CHARS_MSG =
"Display name '{}' does not produce a valid point ID. Manually set a valid ID or export will fail.";
const std::string DISPLAY_NAME_EMPTY_ERROR = "Display name must have non-zero length";
const std::string ID_EMPTY_ERROR = "Id must have non-zero length";
const std::string LOG_CHANNEL = "openstudio.AlfalfaPoint";
StringStreamLogSink ss;
ss.setLogLevel(Warn);

const AlfalfaPoint point("Point");
AlfalfaPoint point("Point");
ASSERT_EQ(0, ss.logMessages().size());
point.id();
ASSERT_EQ(0, ss.logMessages().size());

//Test logging in constructor
AlfalfaPoint invalid_point("Point$$$");
ASSERT_EQ(1, ss.logMessages().size());
LogMessage invalid_id_msg = ss.logMessages().at(0);
EXPECT_EQ(invalid_id_msg.logMessage(), fmt::format(fmt::runtime(DISPLAY_NAME_VALID_CHARS_MSG), "Point$$$"));
EXPECT_EQ(invalid_id_msg.logLevel(), Warn);
EXPECT_EQ(invalid_id_msg.logChannel(), LOG_CHANNEL);
ss.resetStringStream();
ASSERT_EQ(ss.logMessages().size(), 0);

// Test logging when getting id()
const boost::optional<std::string> invalid_point_id = invalid_point.id();
EXPECT_FALSE(invalid_point_id.is_initialized());
ASSERT_EQ(ss.logMessages().size(), 1);
invalid_id_msg = ss.logMessages().at(0);
EXPECT_EQ(invalid_id_msg.logMessage(), fmt::format(fmt::runtime(DISPLAY_NAME_VALID_CHARS_MSG), "Point$$$"));
EXPECT_EQ(invalid_id_msg.logLevel(), Warn);
EXPECT_EQ(invalid_id_msg.logChannel(), LOG_CHANNEL);
ss.resetStringStream();
ASSERT_EQ(ss.logMessages().size(), 0);

// Test exception handling in setId()
EXPECT_THROW(
{
try {
invalid_point.setId("Point_123_$$$");
const AlfalfaPoint invalid_point("");
} catch (const std::runtime_error& error) {
EXPECT_EQ(error.what(), ID_VALID_CHARS_MSG);
EXPECT_EQ(error.what(), DISPLAY_NAME_EMPTY_ERROR);
throw;
}
},
std::runtime_error);
ASSERT_EQ(ss.logMessages().size(), 0);

// Test exception handling in toJSON()
// Test exception handling in setId()
EXPECT_THROW(
{
try {
Json::Value root = invalid_point.toJSON();
point.setId("");
} catch (const std::runtime_error& error) {
EXPECT_EQ(error.what(), "Point requires a valid ID for export. " + ID_VALID_CHARS_MSG);
EXPECT_EQ(error.what(), ID_EMPTY_ERROR);
throw;
}
},
std::runtime_error);
ASSERT_EQ(ss.logMessages().size(), 1);
invalid_id_msg = ss.logMessages().at(0);
EXPECT_EQ(invalid_id_msg.logMessage(), fmt::format(fmt::runtime(DISPLAY_NAME_VALID_CHARS_MSG), "Point$$$"));
EXPECT_EQ(invalid_id_msg.logLevel(), Warn);
EXPECT_EQ(invalid_id_msg.logChannel(), LOG_CHANNEL);
ss.resetStringStream();
ASSERT_EQ(ss.logMessages().size(), 0);

//Test Calls work when provided with legal input
Expand All @@ -278,8 +251,7 @@ TEST(AlfalfaJSON, point_exceptions_logging) {
// Test that changing display name changes the ID if an ID has not been set.
valid_point.setDisplayName("Another Good Point");
ASSERT_EQ(ss.logMessages().size(), 0);
ASSERT_TRUE(valid_point.id().is_initialized());
ASSERT_EQ(valid_point.id().get(), "Another_Good_Point");
ASSERT_EQ(valid_point.id(), "Another_Good_Point");
ASSERT_EQ(ss.logMessages().size(), 0);

Json::Value valid_json = valid_point.toJSON();
Expand All @@ -289,16 +261,15 @@ TEST(AlfalfaJSON, point_exceptions_logging) {
const std::string new_id = "Another_Valid_Point(123)";
valid_point.setId(new_id);
ASSERT_EQ(ss.logMessages().size(), 0);
ASSERT_TRUE(valid_point.id().is_initialized());
ASSERT_EQ(valid_point.id().get(), new_id);
ASSERT_EQ(valid_point.id(), new_id);

valid_json = valid_point.toJSON();
ASSERT_EQ(ss.logMessages().size(), 0);

// Test that once an ID is set, setting a new display name won't throw a warning.
valid_point.setDisplayName("Valid Name, but Invalid Id $$$");
ASSERT_EQ(ss.logMessages().size(), 0);
ASSERT_EQ(valid_point.id().get(), new_id);
ASSERT_EQ(valid_point.id(), new_id);

valid_json = valid_point.toJSON();
ASSERT_EQ(ss.logMessages().size(), 0);
Expand Down