Write empty strings to hdf5

include/openmc/constants.h

@@ -26,7 +26,7 @@ using double_4dvec = vector<vector<vector<vector<double>>>>;
 constexpr int HDF5_VERSION[] {3, 0};
 
 // Version numbers for binary files
-constexpr array<int, 2> VERSION_STATEPOINT {18, 2};
+constexpr array<int, 2> VERSION_STATEPOINT {18, 3};
 constexpr array<int, 2> VERSION_PARTICLE_RESTART {2, 1};
 constexpr array<int, 2> VERSION_TRACK {3, 1};
 constexpr array<int, 2> VERSION_SUMMARY {6, 1};

include/openmc/hdf5_interface.h

@@ -1,13 +1,14 @@
 #ifndef OPENMC_HDF5_INTERFACE_H
 #define OPENMC_HDF5_INTERFACE_H
 
-#include <algorithm> // for min
+#include <algorithm> // for min, max, find
 #include <complex>
 #include <cstddef>
 #include <cstring> // for strlen
 #include <sstream>
 #include <string>
 #include <type_traits>
+#include <vector>
 
 #include "hdf5.h"
 #include "hdf5_hl.h"
@@ -185,12 +186,14 @@ inline void read_attribute(hid_t obj_id, const char* name, std::string& str)
 {
   // Create buffer to read data into
   auto n = attribute_typesize(obj_id, name);
-  char* buffer = new char[n];
+  std::vector<char> buffer(n, '\0');
 
   // Read attribute and set string
-  read_attr_string(obj_id, name, n, buffer);
-  str = std::string {buffer, n};
-  delete[] buffer;
+  read_attr_string(obj_id, name, n, buffer.data());
+
+  // As in read_dataset, the string ends at the first null character
+  auto end = std::find(buffer.begin(), buffer.end(), '\0');
+  str = std::string {buffer.begin(), end};
 }
 
 // overload for vector<std::string>
@@ -247,7 +250,11 @@ inline void read_dataset(
 
   // Read attribute and set string
   read_string(obj_id, name, n, buffer.data(), indep);
-  str = std::string {buffer.begin(), buffer.end()};
+
+  // Fixed-length strings are null-padded, and an empty string is stored as a
+  // single null byte, so the string ends at the first null character
+  auto end = std::find(buffer.begin(), buffer.end(), '\0');
+  str = std::string {buffer.begin(), end};
 }
 
 // array version

src/cell.cpp

@@ -285,9 +285,7 @@ void Cell::to_hdf5(hid_t cell_group) const
   // Create a group for this cell.
   auto group = create_group(cell_group, fmt::format("cell {}", id_));
 
-  if (!name_.empty()) {
-    write_string(group, "name", name_, false);
-  }
+  write_string(group, "name", name_, false);
 
   write_dataset(group, "universe", model::universes[universe_]->id_);
 

src/hdf5_interface.cpp

@@ -1,5 +1,6 @@
 #include "openmc/hdf5_interface.h"
 
+#include <algorithm> // for max
 #include <cstring>
 #include <stdexcept>
 #include <string>
@@ -587,17 +588,18 @@ void write_attr_int(hid_t obj_id, int ndim, const hsize_t* dims,
 
 void write_attr_string(hid_t obj_id, const char* name, const char* buffer)
 {
-  size_t n = strlen(buffer);
-  if (n > 0) {
-    // Set up appropriate datatype for a fixed-length string
-    hid_t datatype = H5Tcopy(H5T_C_S1);
-    H5Tset_size(datatype, n);
+  // As in write_string, an empty string is stored as a single null byte so that
+  // the attribute is always present in the file
+  size_t n = std::max(strlen(buffer), static_cast<size_t>(1));
 
-    write_attr(obj_id, 0, nullptr, name, datatype, buffer);
+  // Set up appropriate datatype for a fixed-length string
+  hid_t datatype = H5Tcopy(H5T_C_S1);
+  H5Tset_size(datatype, n);
 
-    // Free resources
-    H5Tclose(datatype);
-  }
+  write_attr(obj_id, 0, nullptr, name, datatype, buffer);
+
+  // Free resources
+  H5Tclose(datatype);
 }
 
 void write_dataset_lowlevel(hid_t group_id, int ndim, const hsize_t* dims,
@@ -662,17 +664,23 @@ void write_llong(hid_t group_id, int ndim, const hsize_t* dims,
 void write_string(hid_t group_id, int ndim, const hsize_t* dims, size_t slen,
   const char* name, const char* buffer, bool indep)
 {
-  if (slen > 0) {
-    // Set up appropriate datatype for a fixed-length string
-    hid_t datatype = H5Tcopy(H5T_C_S1);
-    H5Tset_size(datatype, slen);
+  // HDF5 has no zero-size string datatype, so an empty string is stored as a
+  // single null byte and stripped back off when read. Skipping the write
+  // entirely would leave the dataset absent from the file, which every reader
+  // that expects it then has to guard against.
+  const char null_char = '\0';
+  const char* data = (slen > 0) ? buffer : &null_char;
+  size_t size = std::max(slen, static_cast<size_t>(1));
 
-    write_dataset_lowlevel(
-      group_id, ndim, dims, name, datatype, H5S_ALL, indep, buffer);
+  // Set up appropriate datatype for a fixed-length string
+  hid_t datatype = H5Tcopy(H5T_C_S1);
+  H5Tset_size(datatype, size);
 
-    // Free resources
-    H5Tclose(datatype);
-  }
+  write_dataset_lowlevel(
+    group_id, ndim, dims, name, datatype, H5S_ALL, indep, data);
+
+  // Free resources
+  H5Tclose(datatype);
 }
 
 void write_string(

src/lattice.cpp

@@ -139,9 +139,7 @@ void Lattice::to_hdf5(hid_t lattices_group) const
   hid_t lat_group = create_group(lattices_group, group_name);
 
   // Write the name and outer universe.
-  if (!name_.empty()) {
-    write_string(lat_group, "name", name_, false);
-  }
+  write_string(lat_group, "name", name_, false);
 
   if (outer_ != NO_OUTER_UNIVERSE) {
     int32_t outer_id = model::universes[outer_]->id_;

src/surface.cpp

@@ -191,9 +191,7 @@ void Surface::to_hdf5(hid_t group_id) const
     }
   }
 
-  if (!name_.empty()) {
-    write_string(surf_group, "name", name_, false);
-  }
+  write_string(surf_group, "name", name_, false);
 
   to_hdf5_inner(surf_group);
 

tests/cpp_unit_tests/CMakeLists.txt

@@ -1,6 +1,7 @@
 set(TEST_NAMES
   test_distribution
   test_file_utils
+  test_hdf5_interface  
   test_tally
   test_interpolate
   test_math

tests/cpp_unit_tests/test_hdf5_interface.cpp

@@ -0,0 +1,213 @@
+#include <catch2/catch_test_macros.hpp>
+
+#include <cstdio>
+#include <string>
+
+#include "openmc/hdf5_interface.h"
+#include "openmc/vector.h"
+
+using namespace openmc;
+
+namespace {
+
+//! Scoped HDF5 file that is created on construction and deleted on destruction
+class TempFile {
+public:
+  explicit TempFile(const std::string& filename) : filename_(filename)
+  {
+    file_id_ = file_open(filename_, 'w');
+  }
+
+  ~TempFile()
+  {
+    if (file_id_ >= 0)
+      file_close(file_id_);
+    std::remove(filename_.c_str());
+  }
+
+  //! Close the file and reopen it for reading, so that tests exercise what
+  //! actually landed on disk rather than a cached in-memory value
+  hid_t reopen()
+  {
+    file_close(file_id_);
+    file_id_ = file_open(filename_, 'r');
+    return file_id_;
+  }
+
+  hid_t id() const { return file_id_; }
+
+private:
+  std::string filename_;
+  hid_t file_id_;
+};
+
+} // namespace
+
+TEST_CASE("String datasets round-trip")
+{
+  TempFile file("test_hdf5_string_dataset.h5");
+
+  const std::string empty {""};
+  const std::string nonempty {"pincell.exo"};
+  // A string whose length is exactly the datatype size, i.e. with no room for a
+  // trailing null character. Reading must not truncate it.
+  const std::string exact {"abc"};
+
+  write_dataset(file.id(), "empty", empty);
+  write_dataset(file.id(), "nonempty", nonempty);
+  write_dataset(file.id(), "exact", exact);
+  write_dataset(file.id(), "empty_literal", "");
+
+  hid_t file_id = file.reopen();
+
+  SECTION("An empty string is still written as a dataset")
+  {
+    // Regression test for openmc-dev/openmc#2285: writing nothing at all left
+    // the dataset absent from the file and broke readers downstream
+    REQUIRE(object_exists(file_id, "empty"));
+    REQUIRE(object_exists(file_id, "empty_literal"));
+
+    // Stored as a single null byte, since HDF5 has no zero-size string type
+    REQUIRE(dataset_typesize(file_id, "empty") == 1);
+  }
+
+  SECTION("An empty string reads back as empty")
+  {
+    std::string value {"not empty"};
+    read_dataset(file_id, "empty", value);
+    REQUIRE(value.empty());
+    REQUIRE(value == "");
+
+    value = "not empty";
+    read_dataset(file_id, "empty_literal", value);
+    REQUIRE(value.empty());
+  }
+
+  SECTION("A non-empty string is unchanged")
+  {
+    std::string value;
+    read_dataset(file_id, "nonempty", value);
+    REQUIRE(value == nonempty);
+    REQUIRE(dataset_typesize(file_id, "nonempty") == nonempty.size());
+  }
+
+  SECTION("A string with no room for a null terminator is not truncated")
+  {
+    std::string value;
+    read_dataset(file_id, "exact", value);
+    REQUIRE(value == exact);
+    REQUIRE(value.size() == 3);
+  }
+}
+
+TEST_CASE("String datasets round-trip within a group")
+{
+  TempFile file("test_hdf5_string_group.h5");
+
+  hid_t group = create_group(file.id(), "geometry");
+  write_dataset(group, "name", std::string {""});
+  write_dataset(group, "region", std::string {"1 -2 3"});
+  close_group(group);
+
+  hid_t file_id = file.reopen();
+  group = open_group(file_id, "geometry");
+
+  REQUIRE(object_exists(group, "name"));
+
+  std::string name {"stale"};
+  read_dataset(group, "name", name);
+  REQUIRE(name.empty());
+
+  std::string region;
+  read_dataset(group, "region", region);
+  REQUIRE(region == "1 -2 3");
+
+  close_group(group);
+}
+
+TEST_CASE("String attributes round-trip")
+{
+  TempFile file("test_hdf5_string_attribute.h5");
+
+  write_attribute(file.id(), "empty", std::string {""});
+  write_attribute(file.id(), "nonempty", std::string {"/path/to/inputs/"});
+  write_attribute(file.id(), "empty_literal", "");
+  write_attribute(file.id(), "exact", std::string {"abc"});
+
+  hid_t file_id = file.reopen();
+
+  SECTION("An empty attribute is still written")
+  {
+    // settings::path_input is empty unless -i is passed, so this is the common
+    // case for the "path" attribute of a statepoint file
+    REQUIRE(attribute_exists(file_id, "empty"));
+    REQUIRE(attribute_exists(file_id, "empty_literal"));
+    REQUIRE(attribute_typesize(file_id, "empty") == 1);
+  }
+
+  SECTION("An empty attribute reads back as empty")
+  {
+    std::string value {"not empty"};
+    read_attribute(file_id, "empty", value);
+    REQUIRE(value.empty());
+
+    value = "not empty";
+    read_attribute(file_id, "empty_literal", value);
+    REQUIRE(value.empty());
+  }
+
+  SECTION("A non-empty attribute is unchanged")
+  {
+    std::string value;
+    read_attribute(file_id, "nonempty", value);
+    REQUIRE(value == "/path/to/inputs/");
+
+    read_attribute(file_id, "exact", value);
+    REQUIRE(value == "abc");
+  }
+}
+
+TEST_CASE("Vectors of strings round-trip")
+{
+  TempFile file("test_hdf5_string_vector.h5");
+
+  const vector<std::string> mixed {"U235", "", "H1"};
+  const vector<std::string> all_empty {"", "", ""};
+  const vector<std::string> none {};
+
+  write_dataset(file.id(), "mixed", mixed);
+  write_dataset(file.id(), "all_empty", all_empty);
+  write_dataset(file.id(), "none", none);
+
+  hid_t file_id = file.reopen();
+
+  SECTION("Individual empty entries do not shrink the datatype")
+  {
+    REQUIRE(object_exists(file_id, "mixed"));
+    // Sized by the longest entry plus a null terminator
+    REQUIRE(dataset_typesize(file_id, "mixed") == 5);
+
+    hid_t dset = open_dataset(file_id, "mixed");
+    REQUIRE(object_shape(dset)[0] == 3);
+    close_dataset(dset);
+  }
+
+  SECTION("A vector of empty strings is written")
+  {
+    REQUIRE(object_exists(file_id, "all_empty"));
+    REQUIRE(dataset_typesize(file_id, "all_empty") == 1);
+
+    hid_t dset = open_dataset(file_id, "all_empty");
+    REQUIRE(object_shape(dset)[0] == 3);
+    close_dataset(dset);
+  }
+
+  SECTION("An empty vector is written as a zero-length dataset")
+  {
+    REQUIRE(object_exists(file_id, "none"));
+
+    hid_t dset = open_dataset(file_id, "none");
+    REQUIRE(object_shape(dset)[0] == 0);
+    close_dataset(dset);
+  }
+}