Skip to content

Fix #4205 - WorkspaceObject::setString allows setting invalid names for ModelObjects #4206

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

Closed
jmarrec wants to merge 4 commits into from
Closed

Fix #4205 - WorkspaceObject::setString allows setting invalid names for ModelObjects #4206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e98db7
Add a test for #4205
jmarrec Feb 15, 2021
578d855
Fix #4205 - Prevent setString from creating duplicate names
jmarrec Feb 15, 2021
72180d1
clarify the intent a bit by explicitly calling .get() on optional<uns…
jmarrec Feb 15, 2021
90d1b62
Modify test, since the "" (empty) does work...
jmarrec Feb 15, 2021
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
4 changes: 2 additions & 2 deletions src/utilities/idf/IdfObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ namespace detail {
bool IdfObject_Impl::setString(unsigned index, const std::string& _value, bool checkValidity) {
std::string value = encodeString(_value);

if (m_iddObject.hasNameField() && (index == m_iddObject.nameFieldIndex())) {
if (m_iddObject.hasNameField() && (index == m_iddObject.nameFieldIndex().get())) {
Copy link
Contributor

@macumber macumber Feb 20, 2021

Choose a reason for hiding this comment

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

I missed this in my PR, I will update mine too

return IdfObject_Impl::setName(value, checkValidity).has_value();
}

Expand Down Expand Up @@ -579,7 +579,7 @@ namespace detail {
bool IdfObject_Impl::pushString(const std::string& value, bool checkValidity) {
// get new index
unsigned index = m_fields.size();
if (m_iddObject.hasNameField() && (index == m_iddObject.nameFieldIndex())) {
if (m_iddObject.hasNameField() && (index == m_iddObject.nameFieldIndex().get())) {
return IdfObject_Impl::setName(value, checkValidity).has_value();
}

Expand Down
23 changes: 23 additions & 0 deletions src/utilities/idf/Test/WorkspaceObject_GTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,3 +557,26 @@ TEST_F(IdfFixture, WorkspaceObject_SetDouble_NaN_and_Inf) {
EXPECT_FALSE(object2.pushExtensibleGroup(group).empty());
EXPECT_EQ(2u, object2.numExtensibleGroups());
}

TEST_F(IdfFixture, WorkspaceObject_setString) {

// Test for #4205 - setString on a WorkspaceObject should prevent duplicate names
Workspace ws(StrictnessLevel::Draft, IddFileType::OpenStudio);
WorkspaceObject space1 = ws.addObject(IdfObject(IddObjectType::OS_Space)).get();

WorkspaceObject space2 = ws.addObject(IdfObject(IddObjectType::OS_Space)).get();

unsigned nameIndex = 1;
EXPECT_TRUE(space1.setString(nameIndex, "Space 1"));
ASSERT_TRUE(space1.getString(nameIndex));
EXPECT_EQ("Space 1", space1.getString(nameIndex).get());

EXPECT_TRUE(space2.setString(nameIndex, "Space 1")); // Setting works, but it should modify it
ASSERT_TRUE(space2.getString(nameIndex));
EXPECT_NE("Space 1", space2.getString(nameIndex).get());

// That portion is accepted because the level is Draft, not Final
EXPECT_TRUE(space2.setString(nameIndex, ""));
ASSERT_TRUE(space2.getString(nameIndex));
EXPECT_EQ("", space2.getString(nameIndex).get());
}
2 changes: 1 addition & 1 deletion src/utilities/idf/WorkspaceObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ namespace detail {
}

// regular field -- name or data
if ((index == 0) && (iddObject().hasNameField())) {
if ((iddObject().hasNameField()) && (index == iddObject().nameFieldIndex().get())) {
return setName(value, checkValidity).has_value();
} // name

Expand Down