diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fdde48..e555823 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -333,6 +333,13 @@ a file shorter than the end of file its superblock records is refused ("truncated file"), and nothing past that end is read. `File`, `LazyFile` and `MmapFile` do this. + - the writer: `FileWriter::finish()` / `FileBuilder::finish()` refuse a + datatype the reader would refuse (`FormatError::SerializationError`, + "datatype cannot be written: ..."), such as a compound with a repeated + field name or no fields, or an enum member with an empty name + (`CompoundTypeBuilder` and `EnumTypeBuilder` build them without + complaint). These were never valid HDF5 — h5py refuses them — and + clawhdf5 wrote them, which made files it could not read back. Checks newer libhdf5 releases make but HDF5 2.0 does not (bit-field offsets, the variable-length kind, array sizes) are left out, so files diff --git a/crates/clawhdf5-format/src/datatype.rs b/crates/clawhdf5-format/src/datatype.rs index 980575d..703f44a 100644 --- a/crates/clawhdf5-format/src/datatype.rs +++ b/crates/clawhdf5-format/src/datatype.rs @@ -1141,9 +1141,23 @@ impl Datatype { } /// Check that this datatype can be written: every part of it has an - /// on-disk encoding. [`Self::serialize`] cannot report errors, so the - /// writer calls this first. + /// on-disk encoding, and the encoding is one the reader (and libhdf5) + /// accepts. [`Self::serialize`] cannot report errors, so the writer calls + /// this first. A compound with no fields or a repeated field name, or an + /// enum member with an empty name, is refused here: libhdf5 and h5py + /// refuse such types, and so does [`Self::parse`], so writing one made a + /// file that could not be read back. pub fn check_encodable(&self) -> Result<(), FormatError> { + self.check_encodable_parts()?; + Self::parse(&self.serialize()).map_err(|e| { + FormatError::SerializationError(format!( + "datatype cannot be written: HDF5 readers refuse it ({e})" + )) + })?; + Ok(()) + } + + fn check_encodable_parts(&self) -> Result<(), FormatError> { match self { Datatype::Opaque { tag, .. } if opaque_tag_text(tag).len() > MAX_OPAQUE_TAG_LEN => { Err(FormatError::SerializationError(format!( @@ -1156,10 +1170,10 @@ impl Datatype { )), Datatype::Compound { members, .. } => members .iter() - .try_for_each(|m| m.datatype.check_encodable()), + .try_for_each(|m| m.datatype.check_encodable_parts()), Datatype::Enumeration { base_type, .. } | Datatype::VariableLength { base_type, .. } - | Datatype::Array { base_type, .. } => base_type.check_encodable(), + | Datatype::Array { base_type, .. } => base_type.check_encodable_parts(), _ => Ok(()), } } diff --git a/crates/clawhdf5/tests/legacy_writer_files.rs b/crates/clawhdf5/tests/legacy_writer_files.rs index 3e8a3b9..e826064 100644 --- a/crates/clawhdf5/tests/legacy_writer_files.rs +++ b/crates/clawhdf5/tests/legacy_writer_files.rs @@ -81,3 +81,61 @@ fn every_object_of_a_v2_7_0_file_reads() { let paged = File::open(fixture("written_by_v2_7_0_paged.h5")).unwrap(); assert_eq!(paged.dataset("d").unwrap().read_f32().unwrap(), [1.0, 2.0]); } + +/// Datatypes libhdf5 (and this reader) refuse — a compound with a repeated +/// field name or no fields, an enum member with an empty name — must not be +/// written: they made files that did not read back. Valid neighbours of each +/// still write and read back. +#[test] +fn datatypes_the_reader_refuses_are_not_written() { + use clawhdf5::{CompoundTypeBuilder, EnumTypeBuilder, FileBuilder}; + + let write_compound = |dt, raw: Vec| { + let mut b = FileBuilder::new(); + b.create_dataset("d").with_compound_data(dt, raw, 1); + b.finish() + }; + let write_enum = |dt| { + let mut b = FileBuilder::new(); + b.create_dataset("d").with_enum_u8_data(dt, &[0, 1]); + b.finish() + }; + let refused = |r: Result, clawhdf5::Error>, what: &str| { + let err = r.expect_err("written"); + let msg = err.to_string(); + assert!(msg.contains("datatype cannot be written"), "{msg}"); + assert!(msg.contains(what), "{msg}"); + }; + + let dup = CompoundTypeBuilder::new() + .f64_field("x") + .f64_field("x") + .build(); + refused( + write_compound(dup, vec![0; 16]), + "duplicated compound field name 'x'", + ); + refused( + write_compound(CompoundTypeBuilder::new().build(), vec![]), + "invalid", + ); + let empty_name = EnumTypeBuilder::u8_based() + .u8_value("A", 0) + .u8_value("", 1) + .build(); + refused(write_enum(empty_name), "0 length enum name"); + + let ok = CompoundTypeBuilder::new() + .f64_field("x") + .f64_field("y") + .build(); + let bytes = write_compound(ok, vec![0; 16]).unwrap(); + let file = File::from_bytes(bytes).unwrap(); + file.dataset("d").unwrap().dtype().unwrap(); + let ok = EnumTypeBuilder::u8_based() + .u8_value("A", 0) + .u8_value("B", 1) + .build(); + let file = File::from_bytes(write_enum(ok).unwrap()).unwrap(); + file.dataset("d").unwrap().dtype().unwrap(); +}