Fixes for BeerXML, BeerJSON export. Plus more on Windows binary signing

.github/workflows/windows.yml

@@ -248,7 +248,9 @@ jobs:
 
 
       #
-      # Sign the Windows binaries using our Signpath certificate
+      # Sign the Windows binaries using our Signpath certificate.  Note that this involves sending the binary to the
+      # remote SignPath service where the signing actually happens.  We need to have an account and credentials with
+      # that service to use it, so this step can't be done in forked repositories.
       #
       # Various settings here have to align with the "brewtarget" project in the "Brewtarget [OSS]" organisation
       # registered at https://app.signpath.io/.
@@ -311,7 +313,15 @@ jobs:
           signing-policy-slug: '${{ env.SIGNPATH_SIGNING_POLICY_SLUG }}'
           github-artifact-id: '${{steps.upload-unsigned-artifact.outputs.artifact-id}}'
           wait-for-completion: true
-          output-artifact-directory: 'C:/_/mbuild/packages/windows/signed'
+          #
+          # From trial and error, it seems output-artifact-directory must be a relative directory, because it will get
+          # appended to some base directory (possibly on the remote server where the signing is actually done).  Eg, if
+          # we set:
+          #    output-artifact-directory: 'C:/_/mbuild/packages/windows/signed'
+          # We get error:
+          #    ENOENT: no such file or directory, mkdir 'D:\a\brewtarget\brewtarget\C:\_\mbuild\packages\windows\signed'
+          #
+          output-artifact-directory: 'signed'
           github-extended-verification-token: '${{ secrets.EXTENDED_VERIFICATION_TOKEN }}'
 
       #

.signpath/artifact-configurations/default.xml

@@ -5,32 +5,59 @@
    See .github/workflows/windows.yml for more general info on how we use SignPath to sign the Windows binaries
 
    See https://about.signpath.io/documentation/artifact-configuration/ for the syntax for this file
+
+   +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+   + NOTE: Although, as is done in the https://github.com/SignPath/github-actions-extended-demo/ SignPath demo
+   +       application, we include this Artifact Configuration file in our source tree, it is not directly picked up
+   +       by the build process.  Whenever you change this file, you need to manually upload the revised version to
+   +       the SignPath Brewtarget project, as that's where the configuration is read from during the signing process.
+   +
+   +       The upside of doing things this way that the configuration file gets validated when you upload it (including
+   +       to confirm you are only using features permitted by your subscription).
+   +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 -->
 <artifact-configuration xmlns="http://signpath.io/artifact-configuration/v1">
   <!--
   Note, per https://github.com/SignPath/github-action-submit-signing-request, that "the used artifact configuration must
   have a zip-file element at its root, as all artifacts are packaged as ZIP archives on GitHub by default."
+
+  Prior to the signing step, our build will have generated two files (where x.y.z is the version number - eg 4.0.4):
+    Brewtarget x.y.z Installer.exe
+    Brewtarget x.y.z Installer.exe.sha256sum
+  GitHub will then have zipped these up into a file called brewtarget-dev-MINGW64.xip
+
+  We want to create a signed version of the first file, and then generate a new checksum for it.
+
+  Fortunately we don't have to work this out from scratch.  By manually uploading an installer to sign, SignPath will
+  also generate a sample artifact-configuration, which we can then edit as needed.
+
+  Because we are only signing a single installer, our configuration is actually very simple.
   -->
   <zip-file>
     <!--
-    Prior to the signing step, our build will have generated two files (where x.y.z is the version number - eg 4.0.4):
-      Brewtarget x.y.z Installer.exe
-      Brewtarget x.y.z Installer.exe.sha256sum
-    We want to create a signed version of the first file, and then generate a new checksum for it.
+    pe-file tag is for "Portable Executable (PE) files: EXE, DLL, and other executable files"
 
-    Fortunately we don't have to work this out from scratch.  By manually uploading an installer to sign, SignPath will
-    also generate a sample artifact-configuration, which we can then edit as needed.
-
-    Because we are only signing a single installer, our configuration is actually very simple.
+    NB: File and directory names in path attributes are case-insensitive. You may use slash / or backslash \ as
+        directory separators.
     -->
-    <pe-file>
+    <pe-file path="Brewtarget *.*.* Installer.exe">
       <!--
-      This means do the signing with Microsoft Authenticode, which is "the primary signing method on the Windows
-      platform".  This is equivalent to using Microsoft’s SignTool.exe.
+      authenticode-sign means do the signing with Microsoft Authenticode, which is "the primary signing method on the
+      Windows platform".  This is equivalent to using Microsoft’s SignTool.exe.
+
+      In theory, we can specify hash-algorithm="sha256" as an attribute on the authenticode-sign tag.  However, if we
+      try that, we get an error:
+
+         "You're not allowed to use the attribute 'hash-algorithm' on the element 'authenticode-sign' with your current
+         subscription type."
+
+      Fortunately sha256 is the default algorithm, so we can just omit that attribute.
+
+      Similarly, our subscription does not allow us to use the 'description' or 'description-url' attributes here,
+      otherwise we would specify:  description="Brewtarget Windows Installer"
+                                   description-url="https://www.brewtarget.beer"
       -->
-      <authenticode-sign hash-algorithm="sha256"
-                         description="Brewtarget Windows Installer"
-                         description-url="https://www.brewtarget.beer" />
+      <authenticode-sign />
     </pe-file>
   </zip-file>
 </artifact-configuration>

CHANGES.markdown

@@ -22,7 +22,8 @@ Minor bug fixes for the 4.0.4 release (ie bugs in 4.0.4 are fixed in this 4.0.5
 
 ### Bug Fixes
 * Crash on new ingredient import (brewtarget v4.0.4) at startup time [828](https://github.com/Brewtarget/brewtarget/issues/828)
-*  Crash on new recipe creation [829](https://github.com/Brewtarget/brewtarget/issues/829)
+* Crash on new recipe creation [829](https://github.com/Brewtarget/brewtarget/issues/829)
+* BeerXML/BeerJSON Recipe export does not match selected type [835](https://github.com/Brewtarget/brewtarget/issues/835)
 
 ### Release Timestamp
 Tue, 24 Sep 2024 04:00:05 +0100

src/serialization/ImportExport.cpp

@@ -81,20 +81,40 @@ namespace {
          Q_ASSERT(importOrExport == ImportOrExport::EXPORT);
          fileChooser.setAcceptMode(QFileDialog::AcceptSave);
          fileChooser.setFileMode(QFileDialog::AnyFile);
-         // If the user doesn't specify a suffix
-         fileChooser.setDefaultSuffix(QString("xml"));
-         // TBD what's the difference between filter and nameFilter?
-//         QString filterStr = "BeerXML files (*.xml)";
-//         fileChooser.setNameFilter(filterStr);
+         //
+         // If the user doesn't specify a suffix, we choose one for them.  By default it's "json" to match the first
+         // filter in the list.  If the user changes the filter, then we get a signal and change the default suffix
+         // accordingly.
+         //
+         // I think in practice QFileDialog::filterSelected will always get sent when exec() is run below, so we don't
+         // need the call to setDefaultSuffix here as the one in the lambda below will suffice.  But it doesn't hurt to
+         // have this initial one, so we'll leave it for now.
+         //
+         fileChooser.setDefaultSuffix(QString("json"));
+         fileChooser.connect(
+            &fileChooser,
+            &QFileDialog::filterSelected,
+            [&fileChooser](QString const & filter) {
+               // The Qt docs are a bit silent about what the parameter is for the QFileDialog::filterSelected signal.
+               // By adding a logging statement here, we discover that we get either "*.json" or "*.xml".  So we just
+               // need to chop off the first two characters to get default suffix.
+               //
+               // In Qt 6, we can replace `mid` with `sliced` which is faster apparently.
+               qDebug() << Q_FUNC_INFO << "Export filter is:" << filter;
+               fileChooser.setDefaultSuffix(filter.mid(2));
+               return;
+            }
+         );
       }
 
       if (!fileChooser.exec() ) {
          // User clicked cancel, so nothing more to do
          return std::nullopt;
       }
 
-      qDebug() << Q_FUNC_INFO << "Selected " << fileChooser.selectedFiles().length() << " files";
-      qDebug() << Q_FUNC_INFO << "Directory " << fileChooser.directory();
+      qDebug() <<
+         Q_FUNC_INFO << "Selected" << fileChooser.selectedFiles().length() << "file(s) (from directory" <<
+         fileChooser.directory() << "):" << fileChooser.selectedFiles();
 
       // Remember the directory for next time
       fileChooserDirectory = fileChooser.directory().canonicalPath();

src/serialization/json/JsonMeasureableUnitsMapping.cpp

@@ -97,3 +97,37 @@ Measurement::Unit const * JsonMeasureableUnitsMapping::findUnit(std::string_view
 Measurement::Unit const * JsonMeasureableUnitsMapping::defaultUnit() const {
    return this->nameToUnit.cbegin()->second;
 }
+
+template<class S>
+S & JsonMeasureableUnitsMapping::writeToStream(S & stream) const {
+   stream <<
+      "Unit Field:" << this->unitField << "; Value Field:" << this->valueField << "; Map:\n";
+   for (auto const & ii : this->nameToUnit) {
+      stream << QString::fromStdString(std::string{ii.first}) << "->" << *ii.second << "\n";
+   }
+   return stream;
+}
+
+template<class S>
+S & operator<<(S & stream, JsonMeasureableUnitsMapping const & jmum) {
+   return jmum.writeToStream(stream);
+}
+
+template<class S>
+S & operator<<(S & stream, JsonMeasureableUnitsMapping const * jmum) {
+   if (jmum) {
+      stream << *jmum;
+   } else {
+      stream << "NULL";
+   }
+   return stream;
+}
+
+//
+// Instantiate the above template functions for the types that are going to use them
+// (This is all just a trick to allow the template definition to be here in the .cpp file and not in the header.)
+//
+template QDebug &      operator<<(QDebug &      stream, JsonMeasureableUnitsMapping const & jmum);
+template QTextStream & operator<<(QTextStream & stream, JsonMeasureableUnitsMapping const & jmum);
+template QDebug &      operator<<(QDebug &      stream, JsonMeasureableUnitsMapping const * jmum);
+template QTextStream & operator<<(QTextStream & stream, JsonMeasureableUnitsMapping const * jmum);

src/serialization/json/JsonMeasureableUnitsMapping.h

@@ -89,6 +89,21 @@ class JsonMeasureableUnitsMapping {
     *         unit matches our canonical metric ones.)
     */
    Measurement::Unit const * defaultUnit() const;
+
+   template<class S> S & writeToStream(S & stream) const;
 };
 
+/**
+ * \brief Convenience function to allow output of \c JsonMeasureableUnitsMapping to \c QDebug or \c QTextStream stream
+ */
+template<class S>
+S & operator<<(S & stream, JsonMeasureableUnitsMapping const & jmum);
+
+/**
+ * \brief Convenience function to allow output of \c JsonMeasureableUnitsMapping to \c QDebug or \c QTextStream stream
+ */
+template<class S>
+S & operator<<(S & stream, JsonMeasureableUnitsMapping const * jmum);
+
+
 #endif

src/serialization/json/JsonRecord.cpp

@@ -1081,6 +1081,7 @@ void JsonRecord::insertValue(JsonRecordDefinition::FieldDefinition const & field
             JsonMeasureableUnitsMapping const * const unitsMapping =
                std::get<JsonMeasureableUnitsMapping const *>(fieldDefinition.valueDecoder);
             Q_ASSERT(unitsMapping);
+            qDebug() << Q_FUNC_INFO << *unitsMapping;
             Measurement::Unit const * const aUnit = unitsMapping->defaultUnit();
             Measurement::Unit const & canonicalUnit = aUnit->getCanonical();
             qDebug() << Q_FUNC_INFO << canonicalUnit;

src/serialization/json/JsonXPath.cpp

@@ -496,7 +496,7 @@ std::string JsonXPath::makePointerToLeaf(boost::json::value ** valuePointer) con
    return std::get<JsonXPath::JsonKey>(priorNode);
 }
 
-std::string_view JsonXPath::asKey() const {
+std::string JsonXPath::asKey() const {
    // It's a coding error if there is more than one path part
    Q_ASSERT(this->m_pathParts.size() == 1);
 
@@ -505,9 +505,8 @@ std::string_view JsonXPath::asKey() const {
 
    // We need to get the first path part (m_pathParts[0]) and then strip the beginning '/' character from it
    // (.substr(1)) before we pass it to the string_view constructor.  (Maybe there is a slick way to skip over the '/'
-   // in the string_view constructor, but I didn't yet find it.)
-   std::string_view key{std::get<JsonXPath::JsonKey>(this->m_pathParts[0]).substr(1)};
-   return key;
+   // in the std::string constructor, but I didn't yet find it.)
+   return std::get<JsonXPath::JsonKey>(this->m_pathParts[0]).substr(1);
 }
 
 char const * JsonXPath::asXPath_c_str() const {

src/serialization/json/JsonXPath.h

@@ -122,7 +122,7 @@ class JsonXPath {
     * \brief For a trivial path, return it without the leading slash (as a \c string_view because that's what we're
     *        going to pass to Boost.JSON).  Caller's responsibility to ensure this is indeed a trivial path.
     */
-   std::string_view asKey() const;
+   std::string asKey() const;
 
    /**
     * \brief This returns a C-style string as that's most universally usable for logging