-
Notifications
You must be signed in to change notification settings - Fork 187
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.
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
#4748 #4817 - Validate BCLXML with schema when loading + make sorting of files in measure.xml consistent when saving #4827
Conversation
@@ -177,7 +177,7 @@ if(NOT CONAN_OPENSTUDIO_ALREADY_RUN) | |||
"cpprestsdk/2.10.18#df2f6ac88e47cadd9c9e8e0971e00d89" | |||
"websocketpp/0.8.2#3fd704c4c5388d9c08b11af86f79f616" | |||
"geographiclib/1.52#76536a9315a003ef3511919310b2fe37" | |||
"swig/4.0.2#9fcccb1e39eed9acd53a4363d8129be5" | |||
"swig/4.1.0#f4419c5ab88f877272cbaa36dd0237ff" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to bump swig because it crashes when you add a spaceship operator<=>
(took me a bit to figure that was the problem)
@@ -504,6 +505,7 @@ add_library(openstudio_utilities_minimal | |||
core/StringStreamLogSink.hpp | |||
core/StringStreamLogSink_Impl.hpp | |||
core/StringStreamLogSink.cpp | |||
bcl/BCLEnums.hpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the enums and put them into their own file, which is part of openstudio_utilities_minimal
} | ||
|
||
return fmt::format("{:04d}-{:02d}-{:02d}T{:02d}:{:02d}:{:02d}{}{:02d}:{:02d}", 2005, 2, 27, m_time.hours(), m_time.minutes(), m_time.seconds(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apparently am to blame for leaving these placeholders/debug variables of 2005, 2, 27 when I ported this code to fmt.
// 2016-07-13T16:08:43Z | ||
|
||
const boost::posix_time::ptime pt(m_date.impl(), m_time.impl()); | ||
std::string result = boost::posix_time::to_iso_extended_string(pt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well use the to_iso_extended_string
EXPECT_EQ("19970716T192000+0100", test->toISO8601()); | ||
EXPECT_EQ("1997-07-16T19:20:00+01:00", test->toXsdDateTime()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test ISO/XSD
src/utilities/xml/XMLValidator.hpp
Outdated
@@ -76,6 +77,8 @@ class UTILITIES_API XMLValidator | |||
|
|||
static XMLValidator gbxmlValidator(); | |||
|
|||
static XMLValidator bclXMLValidator(openstudio::BCLXMLType bclXMLType = openstudio::BCLXMLType::MeasureXML, int schemaVersion = 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new xml validator
set -x | ||
|
||
tag=v0.7.1 | ||
|
||
curl -sL -o component_v2.xsd https://raw.githubusercontent.com/NREL/bcl-gem/$tag/schemas/v2/component_2012_11_08.xsd | ||
curl -sL -o measure_v2.xsd https://raw.githubusercontent.com/NREL/bcl-gem/$tag/schemas/v2/measure_2013_3_26.xsd | ||
|
||
curl -sL -o component_v3.xsd https://raw.githubusercontent.com/NREL/bcl-gem/$tag/schemas/v3/component_v3.xsd | ||
curl -sL -o measure_v3.xsd https://raw.githubusercontent.com/NREL/bcl-gem/$tag/schemas/v3/measure_v3.xsd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I downloaded the measure/component schemas from bcl-gem, and I made a few adjustment that I'm upstreaming: NREL/bcl-gem#27
src/utilities/xml/XMLValidator.cpp
Outdated
XMLValidator XMLValidator::bclXMLValidator(openstudio::BCLXMLType bclXMLType, int schemaVersion) { | ||
const auto tmpDir = openstudio::filesystem::create_temporary_directory("xmlvalidation"); | ||
if (tmpDir.empty()) { | ||
LOG_AND_THROW("Failed to create a temporary directory for extracting the embedded path"); | ||
} | ||
|
||
if (schemaVersion < 2 || schemaVersion > 3) { | ||
LOG_AND_THROW("Unknown schema version " << schemaVersion << ", accepted = [2, 3]"); | ||
} | ||
|
||
std::string schemaName; | ||
if (bclXMLType == BCLXMLType::ComponentXML) { | ||
schemaName = "component"; | ||
} else if (bclXMLType == BCLXMLType::MeasureXML) { | ||
schemaName = "measure"; | ||
} else { | ||
LOG_AND_THROW("Unknown BCLXMLType " << bclXMLType.valueName()); | ||
} | ||
schemaName = fmt::format("{}_v{}.xsd", schemaName, schemaVersion); | ||
|
||
const bool quiet = true; | ||
::openstudio::embedded_files::extractFile(fmt::format(":/xml/resources/bcl/{}", schemaName), openstudio::toString(tmpDir), quiet); | ||
return XMLValidator(tmpDir / schemaName); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the implementation, load up the required schema
src/utilities/bcl/BCLXML.cpp
Outdated
// validate the gbxml prior to reverse translation | ||
auto bclXMLValidator = XMLValidator::bclXMLValidator(m_bclXMLType, startingVersion.major()); | ||
if (!bclXMLValidator.validate(xmlPath)) { | ||
LOG(Warn, "Failed to validate measure.xml at " << xmlPath); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When loading a BCLXML, validate it. This will spit warnings only.
namespace openstudio { | ||
|
||
BCLXML::BCLXML(const BCLXMLType& bclXMLType) | ||
: m_bclXMLType(bclXMLType), | ||
m_uid{removeBraces(openstudio::createUUID())}, | ||
m_versionId{removeBraces(openstudio::createUUID())}, | ||
m_versionModified{DateTime::nowUTC().toISO8601()} {} | ||
m_versionModified{DateTime::nowUTC().toXsdDateTime()} {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema defines that it needs xs:dateTime
, so I had to adjust the way it writes it to the xml in a few places (and this is how I found the mistake in DateTime::toXsdDateTime)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kflemin here is the new usage of xs:datetime
f93e94e
to
2b36856
Compare
tested the validator on the entire openstudio-common-measures-gem repo and all measures are failing validation with the following error: xsdValidate.parseFileError: XML error 17.1824: Element 'version_modified': '20221115T223604Z' is not a valid value of the atomic type 'xs:dateTime'. I think it will be a problem if all of our measures fail validation and we have to update all of them... |
@kflemin any change that triggers a new measure.xml will become valid though. I have changed BCLXML to use toXsdDatetime. |
@jmarrec We can change our measures to this new format, but right now every single one is wrong and it's a large effort to update (but not impossible). Just wanted to raise the concern :) Perhaps we could plan to make this change with the ruby version update? Since we know we have to update all measure repos for that change, we could roll in this change as well? |
@kflemin what I propose is to have the v2 and more importantly v3 schema not as a - <xs:element name="version_modified" type="xs:dateTime">
+ <xs:element name="version_modified">
<xs:annotation>
<xs:documentation>timestamp at which the current version_id was created</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:string">
+ <xs:pattern value="[0-9]{4}(0[1-9]|1[0-2])(0[1-9]|[1-2][0-9]|3[01])T(([01][0-9]|2[0-3])([0-5][0-9])([0-5][0-9])(\.[0-9]+)?|(24:00:00(\.0+)?))(Z|(\+|-)((0[0-9]|1[0-3]):[0-5][0-9]|14:00))?"/>
+ <xs:pattern value="[0-9]{4}-(0[1-9]|1[0-2])-(0[1-9]|[1-2][0-9]|3[01])T(([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9])(\.[0-9]+)?|(24:00:00(\.0+)?))(Z|(\+|-)((0[0-9]|1[0-3]):[0-5][0-9]|14:00))?"/>
+ </xs:restriction>
+ </xs:simpleType>
</xs:element> Then create a new v3.1 schema (and use that one upon writting a new BCLXML to disk) that uses the current implementation (= use As a side note, I think it'd be a good idea to add some automated testing on the bcl-gem repo to ensure the XSD schemas are actually validating correctly (because they are not right now). Some python test code for the above patterns: from lxml import etree
schema_root = etree.XML('''
<xs:schema xmlns:xs="http://www.w3.org/2001/XMLSchema" elementFormDefault="qualified" attributeFormDefault="unqualified">
<xs:element name="version_modified">
<xs:annotation>
<xs:documentation>timestamp at which the current version_id was created</xs:documentation>
</xs:annotation>
<xs:simpleType>
<xs:restriction base="xs:string">
<xs:pattern value="[0-9]{4}(0[1-9]|1[0-2])(0[1-9]|[1-2][0-9]|3[01])T(([01][0-9]|2[0-3])([0-5][0-9])([0-5][0-9])(\.[0-9]+)?|(24:00:00(\.0+)?))(Z|(\+|-)((0[0-9]|1[0-3]):[0-5][0-9]|14:00))?"/>
<xs:pattern value="[0-9]{4}-(0[1-9]|1[0-2])-(0[1-9]|[1-2][0-9]|3[01])T(([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9])(\.[0-9]+)?|(24:00:00(\.0+)?))(Z|(\+|-)((0[0-9]|1[0-3]):[0-5][0-9]|14:00))?"/>
</xs:restriction>
</xs:simpleType>
</xs:element>
<xs:element name="measure">
<xs:annotation>
<xs:documentation>root element defining a measure</xs:documentation>
</xs:annotation>
<xs:complexType>
<xs:sequence>
<xs:element ref="version_modified" minOccurs="0"/>
</xs:sequence>
</xs:complexType>
</xs:element>
</xs:schema>
''')
schema = etree.XMLSchema(schema_root)
parser = etree.XMLParser(schema=schema)
tests = ['20160426T215418Z', '20160426T215418-06:00',
'2016-07-13T16:08:43-06:00', '2016-07-13T16:08:43Z']
schema = etree.XMLSchema(schema_root)
parser = etree.XMLParser(schema=schema)
tests = ['20160426T215418Z', '20160426T215418-06:00',
'2016-07-13T16:08:43-06:00', '2016-07-13T16:08:43Z']
for t in tests:
try:
root = etree.fromstring(
f'''
<measure>
<version_modified>{t}</version_modified>
</measure>'''
, parser)
except etree.XMLSyntaxError as e:
print(f"Failed for {t}: {e}") |
2d523b0
to
37e553f
Compare
Done the changes in the last three commits: https://github.com/NREL/OpenStudio/pull/4827/files/79068d017b4aac1e6bc6ec0f653dc292bee8ef4f..37e553f424e659126800a50a9290b68622275174 |
VersionString BCLXML::currentSchemaVersion() { | ||
return {3, 1}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new static method for clarity / ease of upgrade.
const std::string schemaVersion = BCLXML::currentSchemaVersion().str(); | ||
text.set(schemaVersion.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use that new method
static XMLValidator bclXMLValidator(openstudio::BCLXMLType bclXMLType = openstudio::BCLXMLType::MeasureXML, | ||
const VersionString& schemaVersion = VersionString(3, 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change XMLValidator::bclXMLValidator to use a VersionString instead of a single int versionMajor
<xs:element name="datetime"> | ||
<xs:annotation> | ||
<xs:documentation>date component was uploaded</xs:documentation> | ||
</xs:annotation> | ||
<xs:simpleType> | ||
<xs:restriction base="xs:string"> | ||
<xs:pattern value="[0-9]{4}(0[1-9]|1[0-2])(0[1-9]|[1-2][0-9]|3[01])T(([01][0-9]|2[0-3])([0-5][0-9])([0-5][0-9])(\.[0-9]+)?|(24:00:00(\.0+)?))(Z|(\+|-)((0[0-9]|1[0-3]):[0-5][0-9]|14:00))?"/> | ||
<xs:pattern value="[0-9]{4}-(0[1-9]|1[0-2])-(0[1-9]|[1-2][0-9]|3[01])T(([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9])(\.[0-9]+)?|(24:00:00(\.0+)?))(Z|(\+|-)((0[0-9]|1[0-3]):[0-5][0-9]|14:00))?"/> | ||
</xs:restriction> | ||
</xs:simpleType> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for 2.0 and 3.0, we replace any xs:dateTime
with a xs:string
with a regex pattern that will match any ISO8601 or XSD Datetime object.
@@ -4,7 +4,7 @@ | |||
<xs:annotation> | |||
<xs:documentation>Component.xsd describes the components that are in the building component library or NREL analysis platform library.</xs:documentation> | |||
</xs:annotation> | |||
<xs:element name="schema_version" fixed="3.0"> | |||
<xs:element name="schema_version" fixed="3.1"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New 3.1 schema is the former 3.0. Uses a strict xs:dateTime
format since this is now what we correctly write on a new BCLXML::save / saveAs
TEST_F(BCLFixture, BCLXML_validation_historical) { | ||
|
||
// This has an ISO8601 datetime instead of a valid xs:dateTime | ||
// <version_modified>20170316T161836Z</version_modified> | ||
const openstudio::path xmlPath = resourcesPath() / toPath("/utilities/BCL/Measures/v3/IncreaseRoofRValue/measure.xml"); | ||
const boost::optional<BCLXML> bclXML = BCLXML::load(xmlPath); | ||
ASSERT_TRUE(bclXML); | ||
|
||
// Validate with 3.0 schema: it should work | ||
{ | ||
auto bclXMLValidator = XMLValidator::bclXMLValidator(BCLXMLType::MeasureXML, VersionString(3, 0)); | ||
EXPECT_TRUE(bclXMLValidator.validate(xmlPath)); | ||
EXPECT_EQ(0, bclXMLValidator.errors().size()) << [&bclXMLValidator]() { | ||
std::string s; | ||
for (auto& logMessage : bclXMLValidator.errors()) { | ||
s += logMessage.logMessage() + "\n"; | ||
} | ||
return s; | ||
}(); | ||
EXPECT_EQ(0, bclXMLValidator.warnings().size()); | ||
} | ||
|
||
// Validate with 3.1 schema: it shouldn't work | ||
{ | ||
auto bclXMLValidator = XMLValidator::bclXMLValidator(BCLXMLType::MeasureXML, VersionString(3, 1)); | ||
EXPECT_FALSE(bclXMLValidator.validate(xmlPath)); | ||
|
||
EXPECT_EQ(2, bclXMLValidator.errors().size()) << [&bclXMLValidator]() { | ||
std::string s; | ||
for (auto& logMessage : bclXMLValidator.errors()) { | ||
s += logMessage.logMessage() + "\n"; | ||
} | ||
return s; | ||
}(); | ||
EXPECT_TRUE(checkLogMessagesContain(bclXMLValidator.errors(), | ||
{ | ||
"Element 'schema_version': The actual value '3.0' does not match the fixed value constraint '3.1'", | ||
"Element 'version_modified': '20170316T161836Z' is not a valid value of the atomic type 'xs:dateTime", | ||
})); | ||
EXPECT_EQ(0, bclXMLValidator.warnings().size()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New test for validating an incorrect measure v3, ensuring we don't spit warnings when loading this historical one (unless we specifically try to validate using the 3.1 strict xs:dateTime one)
// Validate any newly written XML | ||
boost::optional<BCLXML> copy = BCLXML::load(xmlPath); | ||
ASSERT_TRUE(copy); | ||
|
||
auto bclXMLValidator = XMLValidator::bclXMLValidator(BCLXMLType::MeasureXML, 3); | ||
auto bclXMLValidator = XMLValidator::bclXMLValidator(BCLXMLType::MeasureXML, BCLXML::currentSchemaVersion()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new XML written is valid per the 3.1 schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works great from what I can tell! I ran it on the openstudio-common-measures repo and didn't get any errors or warnings! thanks!
37e553f
to
e42a359
Compare
…_utilities_minimal
… optional, so don't initialize the missing ones, or on save you end up with nothing in there. Loading and saving `resources/Examples/compact_osw/measures/IncreaseWallRValue` with develop you end up with this ```diff + <description></description> <type>Double</type> + <units></units> <required>true</required> <model_dependent>false</model_dependent> <default_value>30</default_value> + <min_value></min_value> + <max_value></max_value> ```
…erators for BCLFileReference
…r<=>, add comments
…eed GCC 10+, Apple Clang 13 at least), let's avoid it for now
e42a359
to
3e0f599
Compare
@jmarrec I'm seeing a unit test failure (OpenStudioCLI.Labs.test_logger_py ) across all platforms. Could you take a look? |
…dio.Logger.instance()` is what I should have used to begin with.
@@ -12,7 +12,7 @@ | |||
handler.setFormatter(formatter) | |||
logger.addHandler(handler) | |||
|
|||
openstudio.Logger_instance().standardOutLogger().setLogLevel(openstudio.Error) | |||
openstudio.Logger.instance().standardOutLogger().setLogLevel(openstudio.Error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swig bump cause the openstudio.Logger_instance() to be gone. openstudio.Logger.instance()
is what I should have used to begin with. @tijcolem
I'm not sure what's going on with the Windows build. Locally it builds just fine on my win10 Virtual Machine. |
@jmarrec I tried replaying the windows build and running a full build. Hitting the same built errors. https://ci.openstudio.net/blue/organizations/jenkins/openstudio-develop-full/detail/openstudio-develop-full/254/pipeline/ Could you take another look at it? |
I have no clue what's going on. Left is my local python_OpenStudioUtilitiesBCL_wrap.cxx, right is the build box: The order here seems completely messed up OpenStudio/src/utilities/CMakeLists.txt Lines 562 to 573 in 53176d7
Idd.i includes all of the THe order is also completely different from OpenStudio/src/utilities/Utilities.i Lines 15 to 26 in 53176d7
And we do string a specific Unit.i much much later... That doesn't make sense, and I don't know:
|
And it's still obscure why this happens
1fe4e54
to
c07e7a2
Compare
CI Results for c07e7a2:
|
@jmarrec CI is happy now. Thanks for addressing! |
Pull request overview
Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.