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

macumber · 2021-02-14T19:13:45Z

Issue overview

WorkspaceObject::setString allows setting invalid names for ModelObjects

Current Behavior

WorkspaceObject::setString assumes name index is always 0, for ModelObjects name index is 1. This allows user to set invalid names using WorkspaceObject::setString.

Expected Behavior

The following test should pass:

  Model model;
  Space space1(model);
  Space space2(model);
  unsigned nameIndex = 1;

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

  EXPECT_FALSE(space2.setString(nameIndex, "Space 1"));
  ASSERT_TRUE(space2.getString(nameIndex));
  EXPECT_NE("Space 1", space2.getString(nameIndex).get());

  EXPECT_FALSE(space2.setString(nameIndex, ""));
  ASSERT_TRUE(space2.getString(nameIndex));
  EXPECT_NE("", space2.getString(nameIndex).get());

The text was updated successfully, but these errors were encountered:

jmarrec · 2021-02-15T11:02:09Z

Why would you call setString on a ModelObject though? Isn't setName better? (I'm just curious)

jmarrec · 2021-02-15T11:06:46Z

Interesting, the IdfObject does the right thing.

OpenStudio/src/utilities/idf/IdfObject.cpp

Lines 442 to 445 in 1de7dfe

    
           if (m_iddObject.hasNameField() && (index == m_iddObject.nameFieldIndex())) { 
        
             return IdfObject_Impl::setName(value, checkValidity).has_value(); 
        
           }

The WorkspaceObject does hardcode to zero:

OpenStudio/src/utilities/idf/WorkspaceObject.cpp

Lines 433 to 436 in 1de7dfe

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

jmarrec · 2021-02-15T13:05:19Z

Well, IdfObject isn't doing the right thing I think. I don't understand why we don't have a warning poping up for this...

OpenStudio/src/utilities/idf/IdfObject.cpp

Lines 442 to 445 in 1de7dfe

    
           if (m_iddObject.hasNameField() && (index == m_iddObject.nameFieldIndex())) { 
        
             return IdfObject_Impl::setName(value, checkValidity).has_value(); 
        
           }

m_iddObject.nameFieldIndex() returns an optional, so it's missing a cast...

OpenStudio/src/utilities/idd/IddObject.cpp

Lines 250 to 255 in 1de7dfe

    
           boost::optional<unsigned> IddObject_Impl::nameFieldIndex() const { 
        
             if (hasNameField()) { 
        
               return m_nameFieldCache->second; 
        
             } 
        
             return boost::none; 
        
           }

jmarrec · 2021-02-15T13:24:52Z

Ok after testing on Compiler explorer and looking at the assembly (see https://godbolt.org/z/8aGbxE) this works as intended, but I still find it confusing.

jmarrec · 2021-02-15T13:49:50Z

setName("") is allowed though...

[11] OS-build-release(main)> m = Model.new
=> #<OpenStudio::Model::Model:0x0000560687d68ce0 @__swigtype__="_p_openstudio__model__Model">
[12] OS-build-release(main)> s = Space.new(m)
=> #<OpenStudio::Model::Space:0x0000560687b06948 @__swigtype__="_p_openstudio__model__Space">
[13] OS-build-release(main)> s.setName("")
=> #<OpenStudio::OptionalString:0x0000560687a70c90 @__swigtype__="_p_boost__optionalT_std__string_t">
[14] OS-build-release(main)> puts s
OS:Space,
  {7cf6aaae-e3ed-4931-9dba-fb3704d7b5a4}, !- Handle
  ;                                       !- Name

Can anyone justify this behavior? Should this block be (level >= StrictnessLevel::Draft) instead?

OpenStudio/src/utilities/idf/WorkspaceObject.cpp

Lines 361 to 364 in 1de7dfe

    
           // do not set if would violate field NullAndRequired 
        
           if ((level > StrictnessLevel::Draft) && newName.empty() && iddObject().isRequiredField(*index)) { 
        
             return boost::none; 
        
           }

jmarrec · 2021-02-15T14:19:45Z

If you change it to >= instead of >, you get the following failures:

The following tests FAILED:
	132 - IdfFixture.WorkspaceObject_Lights_Strictness_Draft (Failed)
	143 - IdfFixture.WorkspaceObjectWatcher_DataFieldChanges (Failed)
	1516 - ModelFixture.MeterConstructor (Failed)

Click to see verbose log

(py39)julien@OS-build-release$ ctest -VV --rerun-failed -j 16 -E "^(ModelFixture.Space_intersectSurfaces_degenerate1|ModelFixture.Space_intersectSurfaces_degenerate2|ModelFixture.Space_intersectSurfaces_degenerate3|ModelFixture.ThreeJSReverseTranslator_FloorplanJS_SimAUD_Paper|CLITest-test_encodings-encoding|CLITest-test_encodings-encoding2)$"
UpdateCTestConfiguration  from :/home/julien/Software/Others/OS-build-release/DartConfiguration.tcl
Parse Config file:/home/julien/Software/Others/OS-build-release/DartConfiguration.tcl
UpdateCTestConfiguration  from :/home/julien/Software/Others/OS-build-release/DartConfiguration.tcl
Parse Config file:/home/julien/Software/Others/OS-build-release/DartConfiguration.tcl
Test project /home/julien/Software/Others/OS-build-release
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 132
    Start  132: IdfFixture.WorkspaceObject_Lights_Strictness_Draft

132: Test command: /home/julien/Software/Others/OS-build-release/Products/openstudio_utilities_tests "--gtest_filter=IdfFixture.WorkspaceObject_Lights_Strictness_Draft"
132: Environment variables: 
132:  PATH=/home/julien/Virtualenvs/py39/bin:/home/julien/.pyenv/plugins/pyenv-virtualenvwrapper/shims:/home/julien/.pyenv/libexec:/home/julien/.pyenv/plugins/python-build/bin:/home/julien/.pyenv/plugins/pyenv-virtualenvwrapper/bin:/home/julien/.pyenv/plugins/python-build/bin:/home/julien/.pyenv/plugins/pyenv-virtualenvwrapper/bin:/home/julien/.pyenv/shims:~/.pyenv/bin:/home/julien/Qt/QtIFW-4.0.0/bin/:~/.config/composer/vendor/bin:/home/julien/.local/bin:/home/julien/.rvm/gems/ruby-2.5.5/bin:/home/julien/.rvm/gems/ruby-2.5.5@global/bin:/home/julien/.rvm/rubies/ruby-2.5.5/bin:/home/julien/.rvm/gems/ruby-2.5.5/bin:/home/julien/.rvm/gems/ruby-2.5.5@global/bin:/home/julien/.rvm/rubies/ruby-2.5.5/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/julien/.dotnet/tools:/home/julien/.rvm/bin:/home/julien/.dotnet/tools
132: Test timeout computed to be: 660
test 143
    Start  143: IdfFixture.WorkspaceObjectWatcher_DataFieldChanges

143: Test command: /home/julien/Software/Others/OS-build-release/Products/openstudio_utilities_tests "--gtest_filter=IdfFixture.WorkspaceObjectWatcher_DataFieldChanges"
143: Environment variables: 
143:  PATH=/home/julien/Virtualenvs/py39/bin:/home/julien/.pyenv/plugins/pyenv-virtualenvwrapper/shims:/home/julien/.pyenv/libexec:/home/julien/.pyenv/plugins/python-build/bin:/home/julien/.pyenv/plugins/pyenv-virtualenvwrapper/bin:/home/julien/.pyenv/plugins/python-build/bin:/home/julien/.pyenv/plugins/pyenv-virtualenvwrapper/bin:/home/julien/.pyenv/shims:~/.pyenv/bin:/home/julien/Qt/QtIFW-4.0.0/bin/:~/.config/composer/vendor/bin:/home/julien/.local/bin:/home/julien/.rvm/gems/ruby-2.5.5/bin:/home/julien/.rvm/gems/ruby-2.5.5@global/bin:/home/julien/.rvm/rubies/ruby-2.5.5/bin:/home/julien/.rvm/gems/ruby-2.5.5/bin:/home/julien/.rvm/gems/ruby-2.5.5@global/bin:/home/julien/.rvm/rubies/ruby-2.5.5/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/julien/.dotnet/tools:/home/julien/.rvm/bin:/home/julien/.dotnet/tools
143: Test timeout computed to be: 660
test 1516
    Start 1516: ModelFixture.MeterConstructor

1516: Test command: /home/julien/Software/Others/OS-build-release/Products/openstudio_model_tests "--gtest_filter=ModelFixture.MeterConstructor"
1516: Environment variables: 
1516:  PATH=/home/julien/Virtualenvs/py39/bin:/home/julien/.pyenv/plugins/pyenv-virtualenvwrapper/shims:/home/julien/.pyenv/libexec:/home/julien/.pyenv/plugins/python-build/bin:/home/julien/.pyenv/plugins/pyenv-virtualenvwrapper/bin:/home/julien/.pyenv/plugins/python-build/bin:/home/julien/.pyenv/plugins/pyenv-virtualenvwrapper/bin:/home/julien/.pyenv/shims:~/.pyenv/bin:/home/julien/Qt/QtIFW-4.0.0/bin/:~/.config/composer/vendor/bin:/home/julien/.local/bin:/home/julien/.rvm/gems/ruby-2.5.5/bin:/home/julien/.rvm/gems/ruby-2.5.5@global/bin:/home/julien/.rvm/rubies/ruby-2.5.5/bin:/home/julien/.rvm/gems/ruby-2.5.5/bin:/home/julien/.rvm/gems/ruby-2.5.5@global/bin:/home/julien/.rvm/rubies/ruby-2.5.5/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/julien/.dotnet/tools:/home/julien/.rvm/bin:/home/julien/.dotnet/tools
1516: Test timeout computed to be: 660
132: Running main() from gmock_main.cc
132: Note: Google Test filter = IdfFixture.WorkspaceObject_Lights_Strictness_Draft
132: [==========] Running 1 test from 1 test suite.
132: [----------] Global test environment set-up.
132: [----------] 1 test from IdfFixture
143: Running main() from gmock_main.cc
143: Note: Google Test filter = IdfFixture.WorkspaceObjectWatcher_DataFieldChanges
143: [==========] Running 1 test from 1 test suite.
143: [----------] Global test environment set-up.
143: [----------] 1 test from IdfFixture
1516: Running main() from gmock_main.cc
1516: Note: Google Test filter = ModelFixture.MeterConstructor
1516: [==========] Running 1 test from 1 test suite.
1516: [----------] Global test environment set-up.
1516: [----------] 1 test from ModelFixture
1516: [ RUN      ] ModelFixture.MeterConstructor
1516: /home/julien/Software/Others/OpenStudio/src/model/test/OutputMeter_GTest.cpp:196: Failure
1516: Value of: meter.specificInstallLocation()
1516:   Actual: true
1516: Expected: false
1516: [  FAILED  ] ModelFixture.MeterConstructor (3 ms)
1516: [----------] 1 test from ModelFixture (3 ms total)
1516: 
1516: [----------] Global test environment tear-down
1516: [==========] 1 test from 1 test suite ran. (4 ms total)
1516: [  PASSED  ] 0 tests.
1516: [  FAILED  ] 1 test, listed below:
1516: [  FAILED  ] ModelFixture.MeterConstructor
1516: 
1516:  1 FAILED TEST
1/3 Test #1516: ModelFixture.MeterConstructor ........................***Failed    0.02 sec
143: [ RUN      ] IdfFixture.WorkspaceObjectWatcher_DataFieldChanges
143: /home/julien/Software/Others/OpenStudio/src/utilities/idf/Test/WorkspaceObjectWatcher_GTest.cpp:88: Failure
143: Value of: lights.setName("")
143:   Actual: false
143: Expected: true
143: /home/julien/Software/Others/OpenStudio/src/utilities/idf/Test/WorkspaceObjectWatcher_GTest.cpp:89: Failure
143: Value of: watcher.dirty()
143:   Actual: false
143: Expected: true
143: /home/julien/Software/Others/OpenStudio/src/utilities/idf/Test/WorkspaceObjectWatcher_GTest.cpp:91: Failure
143: Value of: watcher.nameChanged()
143:   Actual: false
143: Expected: true
143: /home/julien/Software/Others/OpenStudio/src/utilities/idf/Test/WorkspaceObjectWatcher_GTest.cpp:94: Failure
143: Value of: lights.createName(false)
143:   Actual: false
143: Expected: true
143: /home/julien/Software/Others/OpenStudio/src/utilities/idf/Test/WorkspaceObjectWatcher_GTest.cpp:95: Failure
143: Value of: watcher.dirty()
143:   Actual: false
143: Expected: true
143: /home/julien/Software/Others/OpenStudio/src/utilities/idf/Test/WorkspaceObjectWatcher_GTest.cpp:97: Failure
143: Value of: watcher.nameChanged()
143:   Actual: false
143: Expected: true
143: [  FAILED  ] IdfFixture.WorkspaceObjectWatcher_DataFieldChanges (0 ms)
143: [----------] 1 test from IdfFixture (0 ms total)
143: 
143: [----------] Global test environment tear-down
143: [==========] 1 test from 1 test suite ran. (104 ms total)
143: [  PASSED  ] 0 tests.
143: [  FAILED  ] 1 test, listed below:
143: [  FAILED  ] IdfFixture.WorkspaceObjectWatcher_DataFieldChanges
143: 
143:  1 FAILED TEST
132: [ RUN      ] IdfFixture.WorkspaceObject_Lights_Strictness_Draft
132: /home/julien/Software/Others/OpenStudio/src/utilities/idf/Test/WorkspaceObject_GTest.cpp:237: Failure
132: Value of: light->setString(LightsFields::Name, "")
132:   Actual: false
132: Expected: true
132: [utilities.idf.IdfObject] <1> Could not convert 'Hi' to double
132: [utilities.idf.IdfObject] <1> Could not convert 'Hi' to double
2/3 Test  #143: IdfFixture.WorkspaceObjectWatcher_DataFieldChanges ...***Failed    0.12 sec
132: [  FAILED  ] IdfFixture.WorkspaceObject_Lights_Strictness_Draft (114 ms)
132: [----------] 1 test from IdfFixture (114 ms total)
132: 
132: [----------] Global test environment tear-down
132: [==========] 1 test from 1 test suite ran. (221 ms total)
132: [  PASSED  ] 0 tests.
132: [  FAILED  ] 1 test, listed below:
132: [  FAILED  ] IdfFixture.WorkspaceObject_Lights_Strictness_Draft
132: 
132:  1 FAILED TEST
3/3 Test  #132: IdfFixture.WorkspaceObject_Lights_Strictness_Draft ...***Failed    0.24 sec

0% tests passed, 3 tests failed out of 3

Total Test time (real) =   0.44 sec

The following tests FAILED:
	132 - IdfFixture.WorkspaceObject_Lights_Strictness_Draft (Failed)
	143 - IdfFixture.WorkspaceObjectWatcher_DataFieldChanges (Failed)
	1516 - ModelFixture.MeterConstructor (Failed)

see #4205 (comment)

macumber · 2021-02-16T00:28:36Z

I should have told you I was working on this locally. This is related to openstudiocoalition/OpenStudioApplication#314

macumber · 2021-02-16T00:30:24Z

I have some code I will push up once I get things sorted, it's a pain because Meter uses name to store the E+ meter name and breaks the idea of what a name should be

jmarrec · 2021-02-16T08:39:59Z

Yeah I saw the test failing and understood why. I would probably consider changing the OutputMeter API (and IDD) to be more sane.

macumber · 2021-02-17T03:46:19Z

At first I was going to suggest changing 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 so I am looking at other options now.

see NREL#4205 (comment)

macumber added the Triage Issue needs to be assessed and labeled, further information on reported might be needed label Feb 14, 2021

jmarrec added a commit that referenced this issue Feb 15, 2021

Add a test for #4205

9e98db7

jmarrec added component - Utilities Other severity - Minor Bug and removed Triage Issue needs to be assessed and labeled, further information on reported might be needed labels Feb 15, 2021

jmarrec self-assigned this Feb 15, 2021

jmarrec mentioned this issue Feb 15, 2021

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

Closed

20 tasks

jmarrec added a commit that referenced this issue Feb 15, 2021

Fix #4205 - Prevent setString from creating duplicate names

578d855

jmarrec added a commit that referenced this issue Feb 15, 2021

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

90d1b62

see #4205 (comment)

macumber mentioned this issue Feb 20, 2021

Fix set string index 1 for ModelObjects #4211

Merged

13 tasks

jmarrec added a commit to openstudiocoalition/OpenStudio that referenced this issue Feb 22, 2021

Add a test for NREL#4205

7ddfd92

jmarrec added a commit to openstudiocoalition/OpenStudio that referenced this issue Feb 22, 2021

Fix NREL#4205 - Prevent setString from creating duplicate names

67cfea6

jmarrec added a commit to openstudiocoalition/OpenStudio that referenced this issue Feb 22, 2021

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

bd04b80

see NREL#4205 (comment)

kbenne closed this as completed in #4211 Mar 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

macumber commented Feb 14, 2021

jmarrec commented Feb 15, 2021 •

edited

jmarrec commented Feb 15, 2021

jmarrec commented Feb 15, 2021

jmarrec commented Feb 15, 2021 •

edited

jmarrec commented Feb 15, 2021

jmarrec commented Feb 15, 2021

macumber commented Feb 16, 2021

macumber commented Feb 16, 2021

jmarrec commented Feb 16, 2021

macumber commented Feb 17, 2021

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

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

Comments

macumber commented Feb 14, 2021

Issue overview

Current Behavior

Expected Behavior

jmarrec commented Feb 15, 2021 • edited

jmarrec commented Feb 15, 2021

jmarrec commented Feb 15, 2021

jmarrec commented Feb 15, 2021 • edited

jmarrec commented Feb 15, 2021

jmarrec commented Feb 15, 2021

macumber commented Feb 16, 2021

macumber commented Feb 16, 2021

jmarrec commented Feb 16, 2021

macumber commented Feb 17, 2021

jmarrec commented Feb 15, 2021 •

edited

jmarrec commented Feb 15, 2021 •

edited