From d78870da901c2e91d5ce3cae4de1c748d8967843 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Tue, 26 Mar 2024 12:54:09 +0100 Subject: [PATCH 1/4] Fix #5125 - Add versionModified, repo, org, releaseTag to BCLSEarchResult + shush some annoying warnings due to trying to parse VersionString with empty string --- src/utilities/bcl/BCL.cpp | 74 ++++++++++++++++++++++++------- src/utilities/bcl/BCL.hpp | 10 +++++ src/utilities/bcl/BCLXML.cpp | 86 ++++++++++++++++++------------------ 3 files changed, 110 insertions(+), 60 deletions(-) diff --git a/src/utilities/bcl/BCL.cpp b/src/utilities/bcl/BCL.cpp index 98020c780eb..115bf1152ef 100644 --- a/src/utilities/bcl/BCL.cpp +++ b/src/utilities/bcl/BCL.cpp @@ -10,6 +10,7 @@ #include "../core/Assert.hpp" #include "../core/StringHelpers.hpp" +#include "../time/DateTime.hpp" #include @@ -128,24 +129,33 @@ BCLFile::BCLFile(const pugi::xml_node& fileElement) { m_softwareProgram = softwareProgramElement.text().as_string(); m_identifier = identifierElement.text().as_string(); - if (!minCompatibleElement) { - try { - // if minCompatibleVersion not explicitly set, assume identifier is min - m_minCompatibleVersion = VersionString(m_identifier); - } catch (const std::exception&) { + + // Avoids gettings lots of 'Could not parse '' as a version string' + auto parseVersionIfExistsAndNotEmpty = [](const pugi::xml_node& vElement) -> boost::optional { + if (!vElement) { + return boost::none; } - } else { - try { - m_minCompatibleVersion = VersionString(minCompatibleElement.text().as_string()); - } catch (const std::exception&) { + const std::string vstr = vElement.text().as_string(); + if (vstr.empty()) { + return boost::none; } - } - if (maxCompatibleElement) { + boost::optional result; + try { - m_maxCompatibleVersion = VersionString(maxCompatibleElement.text().as_string()); + result = VersionString(vstr); } catch (const std::exception&) { } + return result; + }; + + if (!minCompatibleElement) { + m_minCompatibleVersion = parseVersionIfExistsAndNotEmpty(identifierElement); + } else { + m_minCompatibleVersion = parseVersionIfExistsAndNotEmpty(minCompatibleElement); } + + m_maxCompatibleVersion = parseVersionIfExistsAndNotEmpty(maxCompatibleElement); + m_filename = filenameElement.text().as_string(); m_url = urlElement.text().as_string(); m_filetype = filetypeElement.text().as_string(); @@ -350,7 +360,7 @@ BCLSearchResult::BCLSearchResult(const pugi::xml_node& componentElement) : m_com auto provenanceElement = provenancesElement.child("provenance"); while (provenanceElement) { if (provenanceElement.first_child() != nullptr) { - m_provenances.push_back(BCLProvenance(provenanceElement)); + m_provenances.emplace_back(provenanceElement); } else { break; } @@ -359,7 +369,7 @@ BCLSearchResult::BCLSearchResult(const pugi::xml_node& componentElement) : m_com auto provenanceRequiredElement = provenancesElement.child("provenance_required"); if (provenanceRequiredElement) { std::string required = provenanceRequiredElement.text().as_string(); - m_provenanceRequired = (required == "true") ? true : false; + m_provenanceRequired = (required == "true"); } auto tagElement = tagsElement.child("tag"); @@ -418,7 +428,7 @@ BCLSearchResult::BCLSearchResult(const pugi::xml_node& componentElement) : m_com auto fileElement = filesElement.child("file"); while (fileElement) { if (fileElement.first_child() != nullptr) { - m_files.push_back(BCLFile(fileElement)); + m_files.emplace_back(fileElement); // BCLFile } else { break; } @@ -429,13 +439,32 @@ BCLSearchResult::BCLSearchResult(const pugi::xml_node& componentElement) : m_com auto costElement = costsElement.child("cost"); while (costElement) { if (costElement.first_child() != nullptr) { - m_costs.push_back(BCLCost(costElement)); + m_costs.emplace_back(costElement); // BCLCost } else { break; } costElement = costElement.next_sibling("cost"); } } + + if (auto orgElement = componentElement.child("org")) { + m_org = orgElement.text().as_string(); + } + if (auto repoElement = componentElement.child("repo")) { + m_repo = repoElement.text().as_string(); + } + if (auto release_tagElement = componentElement.child("release_tag")) { + m_releaseTag = release_tagElement.text().as_string(); + } + if (auto versionModifiedElement = componentElement.child("version_modified")) { + const std::string versionModified = versionModifiedElement.text().as_string(); + if (!versionModified.empty()) { + // fromXsdDateTime forwards to fromISO8601 and handles both formats + if (auto dt_ = openstudio::DateTime::fromXsdDateTime(versionModified)) { + m_versionModified = dt_->toISO8601(); + } + } + } } std::string BCLSearchResult::uid() const { @@ -490,6 +519,19 @@ std::vector BCLSearchResult::costs() const { return m_costs; } +std::string BCLSearchResult::org() const { + return m_org; +} +std::string BCLSearchResult::repo() const { + return m_repo; +} +std::string BCLSearchResult::releaseTag() const { + return m_releaseTag; +} +std::string BCLSearchResult::versionModified() const { + return m_versionModified; +} + BCL::BCL() = default; boost::optional getComponent(const std::string& uid, const std::string& versionId) { diff --git a/src/utilities/bcl/BCL.hpp b/src/utilities/bcl/BCL.hpp index f3fbc763abf..a7e6002e384 100644 --- a/src/utilities/bcl/BCL.hpp +++ b/src/utilities/bcl/BCL.hpp @@ -180,6 +180,11 @@ class UTILITIES_API BCLSearchResult std::vector files() const; std::vector costs() const; + std::string org() const; + std::string repo() const; + std::string releaseTag() const; + std::string versionModified() const; + private: REGISTER_LOGGER("openstudio.BCLSearchResult"); @@ -196,6 +201,11 @@ class UTILITIES_API BCLSearchResult std::vector m_attributes; std::vector m_files; std::vector m_costs; + + std::string m_org; + std::string m_repo; + std::string m_releaseTag; + std::string m_versionModified; // ISO8601 string, or empty }; /// This is a generic interface that can be used for searching either the local or remote bcl. diff --git a/src/utilities/bcl/BCLXML.cpp b/src/utilities/bcl/BCLXML.cpp index d27a2d9cbd8..f3c6d06fb06 100644 --- a/src/utilities/bcl/BCLXML.cpp +++ b/src/utilities/bcl/BCLXML.cpp @@ -64,17 +64,27 @@ BCLXML::BCLXML(const openstudio::path& xmlPath) : m_path(openstudio::filesystem: } } - // get schema version to see if we need to upgrade anything - // added in schema version 3 - VersionString startingVersion("2.0"); - auto subelement = element.child("schema_version"); - if (subelement) { + // Avoids gettings lots of 'Could not parse '' as a version string' + auto parseVersionIfExistsAndNotEmpty = [](const pugi::xml_node& vElement) -> boost::optional { + if (!vElement) { + return boost::none; + } + const std::string vstr = vElement.text().as_string(); + if (vstr.empty()) { + return boost::none; + } + boost::optional result; + try { - startingVersion = VersionString(subelement.text().as_string()); + result = VersionString(vstr); } catch (const std::exception&) { - // Yuck } - } + return result; + }; + + // get schema version to see if we need to upgrade anything + // added in schema version 3 + const VersionString startingVersion = parseVersionIfExistsAndNotEmpty(element.child("schema_version")).get_value_or(VersionString("2.0")); // validate the gbxml prior to reverse translation auto bclXMLValidator = XMLValidator::bclXMLValidator(m_bclXMLType, startingVersion); @@ -101,12 +111,14 @@ BCLXML::BCLXML(const openstudio::path& xmlPath) : m_path(openstudio::filesystem: m_error = errorElement.text().as_string(); } - subelement = element.child("version_modified"); - if (subelement) { + if (auto subelement = element.child("version_modified")) { m_versionModified = subelement.text().as_string(); - if (!DateTime::fromXsdDateTime(m_versionModified)) { - // not an allowable date time - m_versionModified = ""; + if (m_versionModified.empty()) { + // fromXsdDateTime forwards to fromISO8601 and handles both formats + if (!DateTime::fromXsdDateTime(m_versionModified)) { + // not an allowable date time + m_versionModified = ""; + } } } @@ -127,8 +139,7 @@ BCLXML::BCLXML(const openstudio::path& xmlPath) : m_path(openstudio::filesystem: if (m_bclXMLType == BCLXMLType::MeasureXML) { m_modelerDescription = decodeString(element.child("modeler_description").text().as_string()); - subelement = element.child("arguments"); - if (subelement) { + if (auto subelement = element.child("arguments")) { for (auto& arg : subelement.children("argument")) { try { m_arguments.emplace_back(arg); @@ -138,8 +149,7 @@ BCLXML::BCLXML(const openstudio::path& xmlPath) : m_path(openstudio::filesystem: } } - subelement = element.child("outputs"); - if (subelement) { + if (auto subelement = element.child("outputs")) { for (auto& outputElement : subelement.children("output")) { if (outputElement.first_child() != nullptr) { try { @@ -152,8 +162,7 @@ BCLXML::BCLXML(const openstudio::path& xmlPath) : m_path(openstudio::filesystem: } } - subelement = element.child("files"); - if (subelement) { + if (auto subelement = element.child("files")) { for (auto& fileElement : subelement.children("file")) { if (fileElement.first_child() != nullptr) { @@ -163,32 +172,23 @@ BCLXML::BCLXML(const openstudio::path& xmlPath) : m_path(openstudio::filesystem: boost::optional maxCompatibleVersion; auto versionElement = fileElement.child("version"); if (versionElement) { - softwareProgram = versionElement.child("software_program").text().as_string(); - softwareProgramVersion = versionElement.child("identifier").text().as_string(); + auto softwareProgramElement = versionElement.child("software_program"); + softwareProgram = softwareProgramElement.text().as_string(); + auto softwareProgramVersionElement = versionElement.child("identifier"); + softwareProgramVersion = softwareProgramVersionElement.text().as_string(); // added in schema version 3 auto minCompatibleVersionElement = versionElement.child("min_compatible"); + if (!minCompatibleVersionElement) { - try { - // if minCompatibleVersion not explicitly set, assume softwareProgramVersion is min - minCompatibleVersion = VersionString(softwareProgramVersion); - } catch (const std::exception&) { - } + minCompatibleVersion = parseVersionIfExistsAndNotEmpty(softwareProgramVersionElement); } else { - try { - minCompatibleVersion = VersionString(minCompatibleVersionElement.text().as_string()); - } catch (const std::exception&) { - } + minCompatibleVersion = parseVersionIfExistsAndNotEmpty(minCompatibleVersionElement); } // added in schema version 3 auto maxCompatibleVersionElement = versionElement.child("max_compatible"); - if (maxCompatibleVersionElement) { - try { - maxCompatibleVersion = VersionString(maxCompatibleVersionElement.text().as_string()); - } catch (const std::exception&) { - } - } + maxCompatibleVersion = parseVersionIfExistsAndNotEmpty(maxCompatibleVersionElement); } const std::string fileName = fileElement.child("filename").text().as_string(); //std::string fileType = fileElement.firstChildElement("filetype").firstChild().nodeValue().toStdString(); @@ -229,16 +229,15 @@ BCLXML::BCLXML(const openstudio::path& xmlPath) : m_path(openstudio::filesystem: } } - subelement = element.child("attributes"); - if (subelement) { + if (auto subelement = element.child("attributes")) { for (auto& attributeElement : subelement.children("attribute")) { if (attributeElement.first_child() != nullptr) { - std::string name = attributeElement.child("name").text().as_string(); - std::string value = attributeElement.child("value").text().as_string(); - std::string datatype = attributeElement.child("datatype").text().as_string(); + const std::string name = attributeElement.child("name").text().as_string(); + const std::string value = attributeElement.child("value").text().as_string(); + const std::string datatype = attributeElement.child("datatype").text().as_string(); // Units are optional - std::string units = attributeElement.child("units").text().as_string(); + const std::string units = attributeElement.child("units").text().as_string(); if (datatype == "float") { if (units.empty()) { @@ -273,8 +272,7 @@ BCLXML::BCLXML(const openstudio::path& xmlPath) : m_path(openstudio::filesystem: } } - subelement = element.child("tags"); - if (subelement) { + if (auto subelement = element.child("tags")) { for (auto& tagElement : subelement.children("tag")) { auto text = tagElement.text(); if (!text.empty()) { From 80b5d88e6c57f2c8e3f648b49eefe05128c3649e Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Tue, 26 Mar 2024 13:37:31 +0100 Subject: [PATCH 2/4] Use an optional DateTime directly instead of storing a std::string --- src/utilities/bcl/BCL.cpp | 4 ++-- src/utilities/bcl/BCL.hpp | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/utilities/bcl/BCL.cpp b/src/utilities/bcl/BCL.cpp index 115bf1152ef..8748a5be739 100644 --- a/src/utilities/bcl/BCL.cpp +++ b/src/utilities/bcl/BCL.cpp @@ -461,7 +461,7 @@ BCLSearchResult::BCLSearchResult(const pugi::xml_node& componentElement) : m_com if (!versionModified.empty()) { // fromXsdDateTime forwards to fromISO8601 and handles both formats if (auto dt_ = openstudio::DateTime::fromXsdDateTime(versionModified)) { - m_versionModified = dt_->toISO8601(); + m_versionModified = *dt_; } } } @@ -528,7 +528,7 @@ std::string BCLSearchResult::repo() const { std::string BCLSearchResult::releaseTag() const { return m_releaseTag; } -std::string BCLSearchResult::versionModified() const { +boost::optional BCLSearchResult::versionModified() const { return m_versionModified; } diff --git a/src/utilities/bcl/BCL.hpp b/src/utilities/bcl/BCL.hpp index a7e6002e384..278ed1c3d2b 100644 --- a/src/utilities/bcl/BCL.hpp +++ b/src/utilities/bcl/BCL.hpp @@ -12,6 +12,7 @@ #include "../core/Path.hpp" #include "../core/Logger.hpp" #include "../data/Attribute.hpp" +#include "../time/DateTime.hpp" namespace pugi { class xml_node; @@ -183,7 +184,7 @@ class UTILITIES_API BCLSearchResult std::string org() const; std::string repo() const; std::string releaseTag() const; - std::string versionModified() const; + boost::optional versionModified() const; private: REGISTER_LOGGER("openstudio.BCLSearchResult"); @@ -205,7 +206,7 @@ class UTILITIES_API BCLSearchResult std::string m_org; std::string m_repo; std::string m_releaseTag; - std::string m_versionModified; // ISO8601 string, or empty + boost::optional m_versionModified; }; /// This is a generic interface that can be used for searching either the local or remote bcl. From c36bfaee4182424715e459ff64450a3c41752c9e Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Tue, 26 Mar 2024 14:49:50 +0100 Subject: [PATCH 3/4] Add test for new BCLSearchResult (and existing) members --- src/utilities/bcl/test/BCL_GTest.cpp | 42 ++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/utilities/bcl/test/BCL_GTest.cpp b/src/utilities/bcl/test/BCL_GTest.cpp index 028adf2ad95..a405279d5af 100644 --- a/src/utilities/bcl/test/BCL_GTest.cpp +++ b/src/utilities/bcl/test/BCL_GTest.cpp @@ -417,3 +417,45 @@ TEST_F(BCLFixture, RemoteBCL_EncodingURI) { std::vector responses = remoteBCL.searchComponentLibrary("ashrae 4A", 127); ASSERT_GT(responses.size(), 0u); } + +TEST_F(BCLFixture, RemoteBCL_BCLSearchResult) { + RemoteBCL remoteBCL; + + const std::string openstudio_results_uid = "a25386cd-60e4-46bc-8b11-c755f379d916"; + // get openstudio_results + // std::vector responses = remoteBCL.searchMeasureLibrary("openstudio_results", 980); + std::vector responses = remoteBCL.searchMeasureLibrary(openstudio_results_uid, 980); + + ASSERT_EQ(1, responses.size()); + auto& response = responses.front(); + + EXPECT_FALSE(response.name().empty()); + EXPECT_EQ("Openstudio results", response.name()); + + EXPECT_FALSE(response.uid().empty()); + EXPECT_EQ(openstudio_results_uid, response.uid()); + + EXPECT_FALSE(response.versionId().empty()); + EXPECT_FALSE(response.description().empty()); + EXPECT_FALSE(response.modelerDescription().empty()); + EXPECT_TRUE(response.fidelityLevel().empty()); + EXPECT_EQ("measure", response.componentType()); + + EXPECT_TRUE(response.provenanceRequired()); + EXPECT_TRUE(response.provenances().empty()); + EXPECT_FALSE(response.tags().empty()); + EXPECT_FALSE(response.attributes().empty()); + EXPECT_FALSE(response.files().empty()); + EXPECT_TRUE(response.costs().empty()); + + EXPECT_FALSE(response.org().empty()); + EXPECT_EQ("NREL", response.org()); + EXPECT_FALSE(response.repo().empty()); + EXPECT_EQ("openstudio-common-measures-gem", response.repo()); + EXPECT_FALSE(response.releaseTag().empty()); + + auto dt_ = response.versionModified(); + ASSERT_TRUE(dt_); + const openstudio::DateTime dateTime(Date(MonthOfYear::Nov, 14, 2022)); + EXPECT_GT(*dt_, dateTime); +} From 8dc8a9c65ea3748c302c887dcb5ff85ec2df0f9e Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Wed, 27 Mar 2024 15:37:02 +0100 Subject: [PATCH 4/4] Adjust test --- src/utilities/bcl/test/BCL_GTest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utilities/bcl/test/BCL_GTest.cpp b/src/utilities/bcl/test/BCL_GTest.cpp index a405279d5af..3a5bc20495a 100644 --- a/src/utilities/bcl/test/BCL_GTest.cpp +++ b/src/utilities/bcl/test/BCL_GTest.cpp @@ -441,7 +441,7 @@ TEST_F(BCLFixture, RemoteBCL_BCLSearchResult) { EXPECT_TRUE(response.fidelityLevel().empty()); EXPECT_EQ("measure", response.componentType()); - EXPECT_TRUE(response.provenanceRequired()); + EXPECT_FALSE(response.provenanceRequired()); EXPECT_TRUE(response.provenances().empty()); EXPECT_FALSE(response.tags().empty()); EXPECT_FALSE(response.attributes().empty());