Fix set string index 1 for ModelObjects by macumber · Pull Request #4211 · NatLabRockies/OpenStudio

macumber · 2021-02-17T03:59:26Z

Pull request overview

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

Please read OpenStudio Pull Requests to better understand the OpenStudio Pull Request protocol.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Checked behavior in OpenStudioApplication, adjusted policies as needed (src/openstudio_lib/library/OpenStudioPolicy.xml)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

macumber · 2021-02-17T04:00:38Z

Relates to openstudiocoalition/OpenStudioApplication#314

macumber · 2021-02-17T04:01:09Z

src/model/test/SwimmingPoolIndoor_GTest.cpp

  ASSERT_TRUE(space2);
  auto surfaces2 = space2->surfaces();
-  auto floorSurfaceIt2 = std::find_if(std::begin(surfaces), std::end(surfaces), [](const auto& surface) { return surface.surfaceType() == "Floor"; });
+  auto floorSurfaceIt2 = std::find_if(std::begin(surfaces2), std::end(surfaces2), [](const auto& surface) { return surface.surfaceType() == "Floor"; });


This is a bug in this test unrelated to this pull request

macumber · 2021-02-17T04:02:14Z

src/utilities/idf/WorkspaceObject.cpp

    }

    // regular field -- name or data
-    if ((index == 0) && (iddObject().hasNameField())) {


Could add additional check if this field is a handle here and route that to setName as well to preserve existing behavior.

macumber · 2021-02-17T04:03:09Z

src/utilities/idf/WorkspaceObject.cpp


      // check collection NameConflict
-      if (!uniquelyIdentifiableByName()) {
+      if (!newName.empty() && iddObject().isRequiredField(*index) && !uniquelyIdentifiableByName()) {


This change is mostly to support OutputMeter having an empty name on construction. If we change to OutputMeter Meter Name we can revert this line.

This change created #4247

OS:Coil:Heating:Electric does not have \required-field on its Name field, and now unicity of name isn't enforced anymore.

macumber · 2021-02-17T04:03:45Z

src/utilities/idf/WorkspaceObject.cpp


      // do not set if would violate field NullAndRequired
-      if ((level > StrictnessLevel::Draft) && newName.empty() && iddObject().isRequiredField(*index)) {
+      if ((level >= StrictnessLevel::Draft) && newName.empty() && iddObject().isRequiredField(*index)) {


I thought this was enforced at Draft strictness, we don't use Final strictness for anything that I know of

macumber · 2021-02-17T04:04:29Z

src/model/test/ModelObject_GTest.cpp

              == newSurface.construction().get().cast<LayeredConstruction>().layers());
 }
+
+TEST_F(ModelFixture, ModelObject_SetString) {


This is a new test to verify the bug fix

Merge from #4206 adds a very similar one for at Workspace level, I think both are relevant and I don't see a problem testing twice.

macumber · 2021-02-20T20:15:50Z

Looks like the Python bindings might have issues accessing PyPi, possibly due to not allowing access to secrets on a pull request from a fork?

https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/

NOTE: Try --verbose to see response content.
HTTPError: 403 Forbidden from https://test.pypi.org/legacy/
Invalid or non-existent authentication information. See https://test.pypi.org/help/#invalid-auth for more information.
Error: Process completed with exit code 1.

BTW, it is so cool to be able to see the build output in GitHub Actions!

…ames to empty string because it is wrongly assuming that the name string for WorkspaceObject's is always index 0. For ModelObject's the index is 1 because handle is 0. Changing this results in some breakage of OutputMeter which uses/abuses the name field to be the E+ meter name. We may want to change OutputMeter's `Name` field to be `Meter Name`. That seems clear to me but it might break any measure that depends on `meter.name` to give the E+ name. In this commit I kept OutputMeter's `Name` field but marked it as not required (since it is empty) when a meter is first constructed. This might not be ideal though, we may want to change the field to `Meter Name` to prevent Workspace naming logic from introducing further bugs. Finally, the Workspace naming logic was protecting the handle field of ModelObjects. By moving this protection to the name field we may be losing some protection of the handle field. We may want to add functionality to consider both handle and name fields if they exist? As with any change to this Workspace level stuff a good review is required.

…igned>

see NatLabRockies#4205 (comment)

jmarrec · 2021-02-22T18:11:19Z

src/utilities/idf/WorkspaceObject.cpp

+    if (iddObject().hasNameField()) {
+      boost::optional<unsigned> nameIndex = iddObject().nameFieldIndex(); 
+      if (nameIndex && (*nameIndex == index)) {
+        return setName(value, checkValidity).has_value();
+      }


Suggested change

if (iddObject().hasNameField()) {

boost::optional<unsigned> nameIndex = iddObject().nameFieldIndex();

if (nameIndex && (*nameIndex == index)) {

return setName(value, checkValidity).has_value();

}

// regular field -- name or data

if ((iddObject().hasNameField()) && (index == iddObject().nameFieldIndex().get())) {

return setName(value, checkValidity).has_value();

} // name

if hasNameField, I don't think you also need to check if nameIndex exits.

jmarrec · 2021-02-22T18:13:40Z

@macumber I rebased this branch onto develop and merged #4206 into this (after rebasing it onto develop as well so I could resolve conflicts properly)

jmarrec · 2021-02-22T18:14:50Z

Looks like the Python bindings might have issues accessing PyPi, possibly due to not allowing access to secrets on a pull request from a fork?

The repo/fork is the one that should have the proper TESTPYPI_TOKEN, so it's expected that it fails. don't worry about it.

jmarrec · 2021-03-02T10:50:22Z

oops, seems like this never got built by CI and didn't notice it broke https://github.com/NREL/OpenStudio/blob/3650208f2f09905ba612ea122737035bc6577f56/src/utilities/idf/Test/WorkspaceObject_GTest.cpp#L578-L581

cf #4211 (comment)

macumber commented Feb 17, 2021

View reviewed changes

macumber mentioned this pull request Feb 20, 2021

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

Closed

20 tasks

macumber and others added 8 commits February 22, 2021 19:09

Fix error in test, comparing iterators for wrong array

6fe1380

Additional checks from NatLabRockies#4206

82ff56e

Add a test for NatLabRockies#4205

7ddfd92

Fix NatLabRockies#4205 - Prevent setString from creating duplicate names

67cfea6

clarify the intent a bit by explicitly calling .get() on optional<uns…

4b18053

…igned>

Modify test, since the "" (empty) does work...

bd04b80

see NatLabRockies#4205 (comment)

Merge branch '4205_SetString_WorkspaceObject' into fix_set_string

53bfdb7

jmarrec force-pushed the fix_set_string branch from 141d728 to 53bfdb7 Compare February 22, 2021 18:12

jmarrec reviewed Feb 22, 2021

View reviewed changes

kbenne merged commit 3650208 into NatLabRockies:develop Mar 1, 2021

jmarrec deleted the fix_set_string branch March 2, 2021 10:52

jmarrec added a commit that referenced this pull request Mar 2, 2021

Fix unrelated test due to #4211

f0af0e8

cf #4211 (comment)

jmarrec added a commit that referenced this pull request Mar 5, 2021

Fix unrelated test due to #4211

0be7e99

cf #4211 (comment)

jmarrec mentioned this pull request Mar 18, 2021

OS v3.2 UnitarySystem FT regression? #4247

Closed

jmarrec mentioned this pull request Aug 22, 2023

Inspector dialog allows setting invalid object names openstudiocoalition/OpenStudioApplication#314

Closed

Conversation

macumber commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request overview

Pull Request Author

Review Checklist

Uh oh!

macumber commented Feb 17, 2021

Uh oh!

macumber Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

macumber Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

macumber Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

jmarrec Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

macumber Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

macumber Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

jmarrec Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

macumber commented Feb 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarrec Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

jmarrec commented Feb 22, 2021

Uh oh!

jmarrec commented Feb 22, 2021

Uh oh!

jmarrec commented Mar 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

macumber commented Feb 17, 2021 •

edited

Loading

macumber Feb 17, 2021 •

edited

Loading

macumber commented Feb 20, 2021 •

edited

Loading