From f57d515d921b334c56115795e90628affb51e3b5 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:33 +0300 Subject: [PATCH 01/16] fix(crypt4gh): wipe aead and blake2 state on drop chacha20poly1305 and blake2 0.11 only zeroize behind a feature the workspace never enabled, so their copies of the keys stayed in freed memory. A compile-time test now catches the feature going missing. Rewrap also returns an error instead of panicking when the random source fails, and two comments that described the wrong mechanism are fixed. --- Cargo.lock | 4 +++ Cargo.toml | 4 +-- crates/core/fuzz/Cargo.lock | 4 +++ crates/core/src/crypt4gh/body.rs | 8 +++--- crates/core/src/crypt4gh/header.rs | 4 +-- crates/core/src/crypt4gh/mod.rs | 40 +++++++++++++++++++++++++++--- 6 files changed, 51 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fcaf8cf..690b921 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -379,6 +379,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d2f6c7dbe95a6ed67ad9f18e57daf93a2f034c524b99fd2b76d18fdfeb6660aa" dependencies = [ "hybrid-array", + "zeroize", ] [[package]] @@ -485,6 +486,7 @@ dependencies = [ "cipher", "cpufeatures", "rand_core 0.10.1", + "zeroize", ] [[package]] @@ -497,6 +499,7 @@ dependencies = [ "chacha20", "cipher", "poly1305", + "zeroize", ] [[package]] @@ -888,6 +891,7 @@ dependencies = [ "const-oid", "crypto-common 0.2.2", "ctutils", + "zeroize", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 4470bdf..7675543 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -164,8 +164,8 @@ arrow-select = { version = "=60.0.0", default-features = false } sha2 = "=0.11.0" # crypt4gh codec: the pure-Rust RustCrypto stack, so nothing here needs `unsafe` or C. x25519-dalek = { version = "=3.0.0", default-features = false, features = ["static_secrets", "zeroize", "getrandom"] } -chacha20poly1305 = { version = "=0.11.0", default-features = false, features = ["alloc"] } -blake2 = { version = "=0.11.0", default-features = false } +chacha20poly1305 = { version = "=0.11.0", default-features = false, features = ["alloc", "zeroize"] } +blake2 = { version = "=0.11.0", default-features = false, features = ["zeroize"] } getrandom = { version = "=0.4.3", default-features = false } zeroize = { version = "=1.9.0", default-features = false, features = ["alloc"] } tempfile = "=3.27.0" diff --git a/crates/core/fuzz/Cargo.lock b/crates/core/fuzz/Cargo.lock index 387a442..87abe10 100644 --- a/crates/core/fuzz/Cargo.lock +++ b/crates/core/fuzz/Cargo.lock @@ -215,6 +215,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d2f6c7dbe95a6ed67ad9f18e57daf93a2f034c524b99fd2b76d18fdfeb6660aa" dependencies = [ "hybrid-array", + "zeroize", ] [[package]] @@ -302,6 +303,7 @@ dependencies = [ "cfg-if", "cipher", "cpufeatures", + "zeroize", ] [[package]] @@ -314,6 +316,7 @@ dependencies = [ "chacha20", "cipher", "poly1305", + "zeroize", ] [[package]] @@ -491,6 +494,7 @@ dependencies = [ "const-oid", "crypto-common", "ctutils", + "zeroize", ] [[package]] diff --git a/crates/core/src/crypt4gh/body.rs b/crates/core/src/crypt4gh/body.rs index 20d2524..c3e77e0 100644 --- a/crates/core/src/crypt4gh/body.rs +++ b/crates/core/src/crypt4gh/body.rs @@ -102,11 +102,9 @@ pub(crate) fn decrypt_body( .map(|k| ChaCha20Poly1305::new(k.into())) .collect(); - // Reusable scratch for the multi-key retry path. An in-place trial decrypt XORs the - // keystream into the buffer before the tag is checked, so a wrong-key attempt scrambles - // it; with more than one candidate key the trial therefore runs against this copy and a - // failure never consumes the original ciphertext. Allocated once, and empty in the - // single-key case, which decrypts in place. + // Scratch for the multi-key retry path: each trial decrypts this copy, so a failed + // attempt leaves the ciphertext alone without relying on the AEAD checking its tag + // first. Allocated once; empty in the single-key case, which decrypts in place. // // `Zeroizing` because both buffers hold plaintext after decryption, which on the // identity-restore path is the node's own secret key material. A plain `Vec` would leave diff --git a/crates/core/src/crypt4gh/header.rs b/crates/core/src/crypt4gh/header.rs index e660def..1bb51b3 100644 --- a/crates/core/src/crypt4gh/header.rs +++ b/crates/core/src/crypt4gh/header.rs @@ -82,8 +82,8 @@ fn derive_shared_key( sender_pk: &PublicKey, recipient_pk: &PublicKey, ) -> Option> { - // `StaticSecret` stores the clamped scalar. Expose its raw bytes to compute the bare DH - // point, matching libsodium's `crypto_scalarmult`. + // `x25519` clamps the raw secret bytes as libsodium's `crypto_scalarmult` does, so the + // bare DH point matches the reference implementation. let scalar = Zeroizing::new(local_sk.0.to_bytes()); let shared_point = Zeroizing::new(x25519(*scalar, *peer_pk.as_bytes())); diff --git a/crates/core/src/crypt4gh/mod.rs b/crates/core/src/crypt4gh/mod.rs index cd8d2f3..8be21d5 100644 --- a/crates/core/src/crypt4gh/mod.rs +++ b/crates/core/src/crypt4gh/mod.rs @@ -499,8 +499,8 @@ pub fn recover_writer_keys( /// [`CoreError::DecryptFailed`] if the magic/version is wrong, the header is /// malformed, or no identity in `identities` can decrypt any header packet, /// [`CoreError::InvalidManifest`] if the stream carries a non-data-encryption header packet, -/// such as an edit list, that rewrap cannot faithfully re-encode, or [`CoreError::Io`] on a -/// read or write failure. +/// such as an edit list, that rewrap cannot faithfully re-encode, [`CoreError::Io`] on a +/// read or write failure, or [`CoreError::InternalError`] if the system random source fails. pub fn rewrap_header( reader: &mut R, writer: &mut W, @@ -525,7 +525,7 @@ pub fn rewrap_header( /// # Errors /// /// As [`rewrap_header`]: empty `new_recipients`, an unreadable/undecryptable header, a -/// non-data-encryption header packet, or a write failure. +/// non-data-encryption header packet, a write failure, or a failed random source. pub fn rewrap_header_as( reader: &mut R, writer: &mut W, @@ -596,7 +596,10 @@ pub fn rewrap_header_as( let sender_sk = if let Some(sk) = writer_sk { sk } else { - ephemeral_sk = generate_keypair().0; + // Drawn like the session key, so a failed random source is an error, not a panic. + let mut seed = Zeroizing::new([0u8; 32]); + fill_random(seed.as_mut())?; + ephemeral_sk = SecretKey::from_bytes(*seed); &ephemeral_sk }; let mut packets: Vec> = Vec::with_capacity(session_keys.len() * new_recipients.len()); @@ -662,6 +665,19 @@ mod tests { reason = "sender/recipient sk/pk are the standard, clearest crypto naming" )] + /// Every key-holding type here wipes itself on drop. RustCrypto makes that an opt-in + /// `zeroize` feature, so a bump that loses it fails to compile here rather than leaving + /// keys in freed memory. `Blake2b` has no marker of its own, so its state and block + /// buffer, which hold the shared point, are named instead. + #[test] + fn key_holders_wipe_themselves_on_drop() { + fn wipes_on_drop() {} + wipes_on_drop::(); + wipes_on_drop::(); + wipes_on_drop::(); + wipes_on_drop::>(); + } + #[test] fn the_header_trial_decrypt_budget_bounds_packets_times_identities() { // `MAX_HEADER_PACKETS` bounds the packet count, not the asymmetric work: every @@ -1171,6 +1187,22 @@ mod tests { provider_pk.as_bytes(), "the plain rewrap must NOT coincidentally stamp the provider key" ); + + // And a fresh one on every rewrap, not a constant key. + let mut again = Vec::new(); + rewrap_header( + &mut &to_a[..], + &mut again, + std::slice::from_ref(&sk_a), + std::slice::from_ref(&pk_b), + ) + .unwrap(); + let eph_again = recover_writer_keys(&mut &again[..], std::slice::from_ref(&sk_b)).unwrap(); + assert_ne!( + eph[0].as_bytes(), + eph_again[0].as_bytes(), + "each plain rewrap mints its own sender" + ); } #[test] From c6124fc0f614e0ace39dddb3f612abde983a0721 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:33 +0300 Subject: [PATCH 02/16] fix(parquet): refuse footer schemas over 64 elements parquet reserves column metadata for every schema leaf, so an 8 MB footer with a flat schema of a million leaves made it hold over 400 MB before refusing the file. Ours has a dozen elements. parquet 60 no longer panics on the old fixtures, which left the read-path and readiness panic boundaries untested. A new malformed file and a fault point reach them again, and a file with no page index at all is covered. --- crates/core/src/faults.rs | 4 ++ crates/core/src/parquet_bounds.rs | 62 ++++++++++++++---- crates/core/src/parquet_io.rs | 56 +++++++++++++++- .../malformed/negative_chunk_start.parquet | Bin 0 -> 3151 bytes crates/core/tests/it/parquet_probes.rs | 34 +++++++++- .../gdi-dataset-tool/tests/it/validate_e2e.rs | 35 ++++++++++ 6 files changed, 177 insertions(+), 14 deletions(-) create mode 100644 crates/core/tests/fixtures/malformed/negative_chunk_start.parquet diff --git a/crates/core/src/faults.rs b/crates/core/src/faults.rs index 0c0a89a..1adc6ac 100644 --- a/crates/core/src/faults.rs +++ b/crates/core/src/faults.rs @@ -80,6 +80,10 @@ pub enum FaultPoint { /// before the purge makes it recoverable — the next boot's `reap_deleting` finishes /// the erasure. The [`guard`] key is the dataset id. PostStatusPurge, + /// Inside the readiness probe's panic boundary, just before it opens a stored file's + /// footer. A panic here stands in for a decoder panic that no known file triggers this + /// early. The [`guard`] key is the file path. + FooterProbe, } #[cfg(not(feature = "fault-injection"))] diff --git a/crates/core/src/parquet_bounds.rs b/crates/core/src/parquet_bounds.rs index 71e99a8..ccb77ad 100644 --- a/crates/core/src/parquet_bounds.rs +++ b/crates/core/src/parquet_bounds.rs @@ -11,23 +11,25 @@ //! - a field is not in the schema below, or has a different Thrift type; //! - a list declares more elements than bytes remain or than [`MAX_LIST_ELEMENTS`], or a //! value runs past its blob; -//! - a schema element other than the root declares children, or the root declares more -//! than the schema holds; +//! - the schema declares more than [`MAX_SCHEMA_ELEMENTS`] elements, a schema element other +//! than the root declares children, or the root declares more than the schema holds; //! - the footer is over [`MAX_FOOTER_BYTES`], or a page-index entry is empty, outside the //! data, or adds up to more than [`MAX_PAGE_INDEX_BYTES`]. //! //! The types matter because `parquet` reads a known field by its schema type and ignores the //! type on the wire, so bytes we would skip as an `i32` can be a list header to it. When every //! wire type matches, both readers meet the same list headers. `parquet` reserves under 128 -//! bytes per list element, so the element cap holds a list to 128 MiB. It builds the schema -//! tree recursively; our writer's schema is flat, so only the root may have children. Real -//! metadata is small: the gnomAD chr21 corpus slice has 2.2 KB of footer and 1.5 KB of page -//! index, and a 1 GiB file from our writer has a few thousand row groups. +//! bytes per list element, so the element cap holds each list to 128 MiB. It also reserves +//! column metadata per schema leaf, which is why the schema has a cap of its own. It builds +//! the schema tree recursively; our writer's schema is flat, so only the root may have +//! children. Real metadata is small: the gnomAD chr21 corpus slice has 2.2 KB of footer and +//! 1.5 KB of page index, and a 1 GiB file from our writer has a few thousand row groups. //! -//! The schema is the part of `parquet-format` that `parquet` 60 reads here, minus the -//! encryption fields and geospatial statistics, which plaintext files from our writer never -//! carry. Encrypted footers (`PARE`) are not walked: `parquet` reads them only with the node's -//! key, so only the node's own store gets that far. +//! The schema is the part of `parquet-format` that `parquet` 60 reads here, minus what +//! plaintext files from our writer never carry: the encryption fields, geospatial statistics, +//! the `File` logical type (19) and the third column order (3). A file that carries one is +//! refused. Encrypted footers (`PARE`) are not walked: `parquet` reads them only with the +//! node's key, so only the node's own store gets that far. use std::fs::File; use std::io::{Read, Seek, SeekFrom}; @@ -48,6 +50,10 @@ pub(crate) const MAX_PAGE_INDEX_BYTES: u64 = 64 * 1024 * 1024; /// The most elements a list may declare. `parquet` reserves them all before reading one. pub(crate) const MAX_LIST_ELEMENTS: u64 = 1 << 20; +/// The most elements the schema may declare. Ours has a dozen, and `parquet` reserves column +/// metadata for every leaf, far more per element than the list cap allows for. +pub(crate) const MAX_SCHEMA_ELEMENTS: u64 = 64; + /// The file ends with the footer length (a little-endian `u32`) and the magic. const TAIL_BYTES: u64 = 8; const MAGIC_PLAINTEXT_FOOTER: &[u8] = b"PAR1"; @@ -300,7 +306,8 @@ static SCHEMA_ELEMENT: Shape = Shape { ], }; -/// A union: one member field is set. Members without a payload are empty structs. +/// A union: one member field is set. Members without a payload are empty structs. The walk +/// also passes none or several, which `parquet` then refuses with an error. static LOGICAL_TYPE: Shape = Shape { name: "LogicalType", fields: &[ @@ -647,6 +654,13 @@ impl<'a> Thrift<'a> { "a list declares {count} elements, over the {MAX_LIST_ELEMENTS}-element cap" ))); } + if matches!(elem, Elem::Struct(shape) if std::ptr::eq(shape, &raw const SCHEMA_ELEMENT)) + && count > MAX_SCHEMA_ELEMENTS + { + return Err(refused(format!( + "the schema declares {count} elements, over the {MAX_SCHEMA_ELEMENTS}-element cap" + ))); + } for i in 0..count { match elem { Elem::Bool => self.skip(1)?, @@ -863,6 +877,32 @@ mod tests { } } + #[test] + fn a_schema_over_the_element_cap_is_refused() { + // `parquet` reserves column metadata for every leaf of the schema, so a flat schema + // of a million leaves in 8 MB of footer made it hold over 400 MB before refusing + // the file. + let schema = |elements: u64| { + let empty_elements = vec![0; usize::try_from(elements).unwrap()]; + [ + vec![field(2, LIST)], + list_header(STRUCT, elements), + empty_elements, + vec![0], + ] + .concat() + }; + assert!(check_metadata_bounds(&parquet_file(&[], &schema(MAX_SCHEMA_ELEMENTS))).is_ok()); + let msg = refusal(&parquet_file(&[], &schema(MAX_SCHEMA_ELEMENTS + 1))); + assert!( + msg.contains(&format!( + "the schema declares {} elements", + MAX_SCHEMA_ELEMENTS + 1 + )), + "{msg}" + ); + } + #[test] fn malformed_thrift_is_refused() { let overlong_varint = [vec![field(1, I32)], vec![0x80; 10], vec![0]].concat(); diff --git a/crates/core/src/parquet_io.rs b/crates/core/src/parquet_io.rs index 4692160..aafd196 100644 --- a/crates/core/src/parquet_io.rs +++ b/crates/core/src/parquet_io.rs @@ -994,7 +994,13 @@ pub fn probe_dataset_readable( // no partially-mutated state is observed. The guard also marks the panic expected, // so the process-level hook downgrades its output instead of logging raw panic text. crate::panic_guard::catch_decode_panic( - || open_reader_builder(file, decryptor, path.as_ref()).map(|_| ()), + || { + crate::faults::guard( + crate::faults::FaultPoint::FooterProbe, + &path.to_string_lossy(), + )?; + open_reader_builder(file, decryptor, path.as_ref()).map(|_| ()) + }, || { invalid_parquet(format!( "parquet footer probe panicked on {} (malformed file)", @@ -1464,6 +1470,30 @@ mod tests { ); } + #[test] + fn read_matching_rows_isolates_a_decode_panic() { + // `negative_chunk_start.parquet` is the COVID corpus file with its first column + // chunk's data page moved to offset -1 and no page index. `parquet` asserts on the + // offset as it reads the chunk, so the boundary must turn that panic into a clean + // `InvalidParquet`. + let path = std::path::Path::new("tests/fixtures/malformed/negative_chunk_start.parquet"); + let err = read_matching_rows( + path, + &ParquetCaps::default(), + PosWindow { + lo: 0, + hi: i64::MAX, + }, + &|_, _, _, _| true, + &DatasetDecryptor::plaintext(), + ) + .expect_err("a decode panic must come back as an error"); + assert!( + format!("{err}").contains("parquet decode panicked"), + "expected the panic boundary's error, got {err}" + ); + } + #[test] fn pme_transient_decode_error_is_reclassified_transient() { // A Vault transient carried via `PME_TRANSIENT_MARKER` re-surfaces as `Transient`, @@ -2585,6 +2615,30 @@ mod probe_tests { ); } + /// No known file panics `parquet` while it only opens the footer, so a fault stands in + /// for one. A panic escaping the probe would latch `/health/ready` at 503. + #[cfg(feature = "fault-injection")] + #[test] + #[serial_test::serial(faults)] + fn probe_dataset_readable_isolates_a_decode_panic() { + let dir = tempfile::tempdir().unwrap(); + let path = dir + .path() + .join("allele-freq.chr1.0.br10000000.0123456789abcdef.parquet"); + write_valid(&path); + let _fault = crate::faults::arm_panic( + crate::faults::FaultPoint::FooterProbe, + &path.to_string_lossy(), + 1, + ); + let err = probe_dataset_readable(dir.path(), &DatasetDecryptor::plaintext()) + .expect_err("a decode panic must come back as an error"); + assert!( + format!("{err}").contains("footer probe panicked"), + "expected the panic boundary's error, got {err}" + ); + } + #[test] fn probe_dataset_readable_rejects_a_malformed_arrow_schema() { // The readiness probe must return a clean `Err` on a malformed `ARROW:schema`. A diff --git a/crates/core/tests/fixtures/malformed/negative_chunk_start.parquet b/crates/core/tests/fixtures/malformed/negative_chunk_start.parquet new file mode 100644 index 0000000000000000000000000000000000000000..cdada3d674638c2ef362dfff40fc175821beeebe GIT binary patch literal 3151 zcmbVPe{54#6u$4Z>$*|8uJm4BNfQPMFB{p`0d6uv)!Wi-9o^P$Z3jdG8(B65HrU1n z`iJ9(Gf{)t0=S3_Fh&rUnV_Ohf^#YgA!IQGf5aFwhfzXeA{q?=@tpf!OS^zuyn*)K zbME(@bH01dmW6oj5^AJoIxk3NO6-K>zURJ^V_ZN;dhthK0Vm6+3bj!aHR)bCn-HRa zCzHBTU}<@TNZ^b7Pn>TWoR#LJ#7i$F!~pT?szcKtPf2u-6ofhwB}PA&NaLp&*3lz| zl7deQq@W!-N>qY)d0u10w{5+eq6}LKf|o$odm~o~Iqdp3#Ftp6tnE9+Ulmr_6iU zQ~UcVPvPC;o`r86_B>iV=uvO?dnN<@o*9Q)ml|r3L}Y_Gd%St}&|;EXUtqDl@ncR) zUR}-HqaWVd_3h{v?A-opnE-tbT{^WA+ipT+d}Ar4o;9>GNZCCJ^RE#yCYvmy@6UVY z*`8M>z4kvHSw~-fY3~bPU-+$a*38Hoo_kY>wT8x0$*Dxf_N3d!=bk6;$gz{HhVhIS z7xd-T?A^ZO=FPDSb2gN_rJ0538hBlVu+fDbrg9WBW`K zh6ZxlWv61qDb&BH>}0xGiZ~T1l5FsS!Ixw(z)Uz5{7f>+e!o*mi?nq%Y)P^ju_qx0 zwq>$;EffysC!rIBIZq{uo)!EgKo{ya)m5jta_@rz52A$JP>VHBwPxys-x!7KvH2;4 zF7^mr4mA^}iw#R+>tv9Fiw(-jr+$NeszBcjBdtK8SS6muOyY#w9y5*TY$F+oZO^R4 zw%?MtJ26|cLV1@eEDi1LZQUC>HnlWuZJ-QlUPKf_sHTok3+`(DWs6AA1&L~B9*iWP zN9i0W<^IQ?iM7m`JCQ^-5~LMZGf1RbtH*1FXQn#%W-R z2|I)|m#5OmygIJS_mVm>Tm8-=GQmMtj5zg4nG7MtK09Q>hM{p)GaI%#aQGDqudS`A zU!e`4GH!C?S{C+k&cpB@*J{qYxv!a5q6xk^_X;~4;mmmePhg6F!>I#JVGehfa6d3~ zgWKmv#u%P`czqQz@mBmO7uzD-3Tl4@l2|U{m!lC^I1t6JiqOMu_b%LtC9Z3sz1|4W zYzhH%r|_*!+-X57Z@}H1@gA!*FnmU<65GI2_ Date: Wed, 7 Oct 2026 12:04:33 +0300 Subject: [PATCH 03/16] perf(fairdp): prune orphaned statements in one walk The prune walked the record's subject once per statement, so a catalog of 3,000 datasets took seconds to render. It now visits each node once, and also removes a named node that a drop has cut loose. --- crates/fairdp/src/publish.rs | 67 ++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 26 deletions(-) diff --git a/crates/fairdp/src/publish.rs b/crates/fairdp/src/publish.rs index aa8f515..ed604c4 100644 --- a/crates/fairdp/src/publish.rs +++ b/crates/fairdp/src/publish.rs @@ -5,7 +5,8 @@ //! distributions, per dataset kind), then `rename_properties`, `replace_values` and //! `type_values` over the whole record. Every dataset on this node is `aggregated`, so //! datasets and distributions get both their `all` and their `aggregated` block. Last, every -//! blank node no named node reaches, which a drop (`[]`) can leave, is removed. +//! statement no path from the record's subject reaches, which a drop (`[]`) can leave, is +//! removed. //! //! [`Publish::compile`] resolves every name once. The node's startup preflight runs it, so //! an unknown prefix or a malformed IRI stops the node at start instead of failing a @@ -17,8 +18,7 @@ use std::fmt; use gdi_node_standalone_core::config::{AddValue, FairdpPublish, PublishParts}; use gdi_node_standalone_core::error::CoreError; use oxrdf::{ - BlankNode, Graph, Literal, NamedNode, NamedOrBlankNode, NamedOrBlankNodeRef, Term, TermRef, - Triple, TripleRef, + BlankNode, Graph, Literal, NamedNode, NamedOrBlankNode, Term, TermRef, Triple, TripleRef, }; use crate::serialize::PREFIXES; @@ -280,36 +280,31 @@ impl Publish { } } - remove_orphans(graph); + remove_orphans(graph, &subject); } } -/// Removes the statements of every blank node that no named node reaches. A drop leaves -/// such nodes behind, and they would still be served, just unlinked. -fn remove_orphans(graph: &mut Graph) { - let mut reached = HashSet::new(); - let mut stack: Vec = graph - .iter() - .filter_map(|t| match t.subject { - NamedOrBlankNodeRef::NamedNode(n) => Some(n.into_owned().into()), - NamedOrBlankNodeRef::BlankNode(_) => None, - }) - .collect(); +/// Removes every statement that no path from the record's `subject` reaches. A drop can cut +/// a node loose, blank or named, and its statements would still be served, just unlinked. +/// Each node is walked once, so this is linear in the record. +fn remove_orphans(graph: &mut Graph, subject: &NamedOrBlankNode) { + let mut reached = HashSet::from([subject.clone()]); + let mut stack = vec![subject.clone()]; while let Some(node) = stack.pop() { for triple in graph.triples_for_subject(&node) { - if let TermRef::BlankNode(b) = triple.object - && reached.insert(b.into_owned()) - { - stack.push(b.into_owned().into()); + let object: NamedOrBlankNode = match triple.object { + TermRef::NamedNode(n) => n.into_owned().into(), + TermRef::BlankNode(b) => b.into_owned().into(), + TermRef::Literal(_) => continue, + }; + if reached.insert(object.clone()) { + stack.push(object); } } } let orphans: Vec = graph .iter() - .filter(|t| match t.subject { - NamedOrBlankNodeRef::BlankNode(b) => !reached.contains(&b.into_owned()), - NamedOrBlankNodeRef::NamedNode(_) => false, - }) + .filter(|t| !reached.contains(&t.subject.into_owned())) .map(TripleRef::into_owned) .collect(); for triple in &orphans { @@ -454,7 +449,8 @@ impl<'a> Names<'a> { .map_err(|_| PublishError(format!("{setting}: `{name}` is not a valid IRI"))) } - /// `old = new` or `old = [new, …]`, as in `rename_properties` and `replace_values`. + /// `old = [new, …]`, as in `rename_properties` and `replace_values`; `= []` maps to + /// nothing. fn mapping( &self, setting: &str, @@ -746,6 +742,25 @@ mod tests { graph } + #[test] + fn a_drop_takes_a_named_node_cut_loose_along() { + // A dropped property's value can be an IRI with statements of its own; once nothing + // reaches that node, its statements go too. + let licence = "https://licences.example/cc-by-4.0"; + let publish = FairdpPublish { + rename_properties: BTreeMap::from([("dct:license".to_owned(), Vec::new())]), + ..FairdpPublish::default() + }; + let mut graph = dataset_with(&format!("{DCT}license"), iri(licence)); + graph.insert(&Triple::new( + iri(licence), + iri("http://www.w3.org/2000/01/rdf-schema#label"), + Literal::new_simple_literal("CC BY 4.0"), + )); + apply(&publish, &mut graph); + assert!(graph.is_empty(), "{}", turtle(&graph)); + } + #[test] fn values_are_typed_by_their_shape() { let publish = dataset_add(&[ @@ -758,7 +773,7 @@ mod tests { ("dct:rightsHolder", text("gdi@example.org")), ("dct:issued", text("2026-09-24")), ("dct:modified", text("2026-09-24T10:00:00Z")), - ("dct:subject", text("unknown:prefix stays text")), + ("dct:subject", text("Note: prose with a colon stays text")), ]); let mut graph = Graph::new(); apply(&publish, &mut graph); @@ -773,7 +788,7 @@ mod tests { "dct:rightsHolder ", "dct:issued \"2026-09-24\"^^xsd:date", "dct:modified \"2026-09-24T10:00:00Z\"^^xsd:dateTime", - "dct:subject \"unknown:prefix stays text\"", + "dct:subject \"Note: prose with a colon stays text\"", ] { assert!(out.contains(expected), "missing `{expected}` in\n{out}"); } From 45ebcc369cc21e149d68f396153e8fcc2f66d095 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:33 +0300 Subject: [PATCH 04/16] test(fairdp): check the age range, provenance and health theme The render fixture left these fields empty, so a min/max swap would only have shown up as a changed snapshot. --- crates/fairdp/benches/render.rs | 13 ++-- crates/fairdp/tests/render.rs | 65 +++++++++++++++++-- .../snapshots/render__dataset_ntriples.snap | 26 +++++--- 3 files changed, 86 insertions(+), 18 deletions(-) diff --git a/crates/fairdp/benches/render.rs b/crates/fairdp/benches/render.rs index 819e560..b37b95b 100644 --- a/crates/fairdp/benches/render.rs +++ b/crates/fairdp/benches/render.rs @@ -79,10 +79,15 @@ fn covid_entry(dataset_id: &str) -> DatasetEntry { schema_agency: Some("DataCite".to_owned()), name: Some("Example identifier".to_owned()), }]), - min_typical_age: None, - max_typical_age: None, - provenance: None, - health_theme: None, + min_typical_age: Some(18), + max_typical_age: Some(90), + provenance: Some(LocalizedText::Plain( + "Whole-genome sequencing of the cohort, aggregated per population.".to_owned(), + )), + health_theme: Some(vec![ + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS" + .to_owned(), + ]), contact_point: Some(ContactPoint { fn_: Some("Data team".to_owned()), has_email: Some("mailto:data@example.org".to_owned()), diff --git a/crates/fairdp/tests/render.rs b/crates/fairdp/tests/render.rs index 9be2480..6ecb4d8 100644 --- a/crates/fairdp/tests/render.rs +++ b/crates/fairdp/tests/render.rs @@ -66,10 +66,15 @@ fn covid_entry() -> DatasetEntry { schema_agency: Some("DataCite".to_owned()), name: Some("Example identifier".to_owned()), }]), - min_typical_age: None, - max_typical_age: None, - provenance: None, - health_theme: None, + min_typical_age: Some(18), + max_typical_age: Some(90), + provenance: Some(LocalizedText::Plain( + "Whole-genome sequencing of the cohort, aggregated per population.".to_owned(), + )), + health_theme: Some(vec![ + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS" + .to_owned(), + ]), contact_point: Some(ContactPoint { fn_: Some("Data team".to_owned()), has_email: Some("mailto:data@example.org".to_owned()), @@ -256,6 +261,57 @@ fn dataset_turtle_contains_key_triples() { ); } +/// The age range, provenance and health theme, each under its own predicate. The golden +/// alone would let a min/max swap through on a re-snapshot. +#[test] +fn dataset_carries_its_ages_provenance_and_health_theme() { + let fairdp = fairdp_config(); + let ctx = FdpContext::new(BASE_URL, BEACON_PATH, &fairdp); + let graph = dataset_graph(&covid_entry(), &ctx); + let dataset_iri = format!("{BASE_URL}/fairdp/dataset/{DATASET_ID}"); + + assert_eq!( + literals(&graph, &dataset_iri, &format!("{HEALTH}minTypicalAge")), + ["18"] + ); + assert_eq!( + literals(&graph, &dataset_iri, &format!("{HEALTH}maxTypicalAge")), + ["90"] + ); + assert_eq!( + iri_objects(&graph, &dataset_iri, &format!("{HEALTH}healthTheme")), + ["https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS"] + ); + + // A dct:ProvenanceStatement node whose label is the text. + let statement = blank_object( + &graph, + NamedNodeRef::new_unchecked(&dataset_iri), + &format!("{DCT}provenance"), + ); + assert_eq!( + iri_objects_of( + &graph, + statement, + "http://www.w3.org/1999/02/22-rdf-syntax-ns#type" + ), + [format!("{DCT}ProvenanceStatement")] + ); + let label = NamedNodeRef::new_unchecked("http://www.w3.org/2000/01/rdf-schema#label"); + let labels: Vec<&str> = graph + .triples_for_subject(statement) + .filter(|t| t.predicate == label) + .filter_map(|t| match t.object { + TermRef::Literal(l) => Some(l.value()), + _ => None, + }) + .collect(); + assert_eq!( + labels, + ["Whole-genome sequencing of the cohort, aggregated per population."] + ); +} + /// A deterministic golden representation of a graph: canonicalize blank nodes, then emit /// sorted N-Triples, one fully-qualified triple per line. /// @@ -858,6 +914,7 @@ const XSD_DATE_TIME: &str = "http://www.w3.org/2001/XMLSchema#dateTime"; const FDP_O: &str = "https://w3id.org/fdp/fdp-o#"; const DCT: &str = "http://purl.org/dc/terms/"; const DCAT: &str = "http://www.w3.org/ns/dcat#"; +const HEALTH: &str = "http://healthdataportal.eu/ns/health#"; const LDP_CONTAINS: &str = "http://www.w3.org/ns/ldp#contains"; const DCATAP_LEG: &str = "http://data.europa.eu/r5r/applicableLegislation"; const HEALTHDCATAP_HDAB: &str = "http://healthdataportal.eu/ns/health#hdab"; diff --git a/crates/fairdp/tests/snapshots/render__dataset_ntriples.snap b/crates/fairdp/tests/snapshots/render__dataset_ntriples.snap index 73aeb75..98f4f3b 100644 --- a/crates/fairdp/tests/snapshots/render__dataset_ntriples.snap +++ b/crates/fairdp/tests/snapshots/render__dataset_ntriples.snap @@ -5,6 +5,9 @@ expression: nt _:c14n6 + + "90"^^ + "18"^^ "123456"^^ "1234"^^ @@ -17,7 +20,8 @@ expression: nt "2026-04-09T14:30:52.837Z"^^ - _:c14n8 + _:c14n7 + _:c14n9 "GoE Eesti koondsagedused"@et "GoE Estonia aggregated AFs"@en @@ -48,16 +52,18 @@ _:c14n4 _:c14n5 _:c14n5 "Data team" _:c14n5 -_:c14n6 _:c14n7 +_:c14n6 _:c14n8 _:c14n6 _:c14n6 _:c14n1 _:c14n6 _:c14n6 "Estonian HDAB" -_:c14n7 "hdab@example.org" -_:c14n7 -_:c14n8 _:c14n0 -_:c14n8 -_:c14n8 _:c14n4 -_:c14n8 -_:c14n8 -_:c14n8 "University of Tartu" +_:c14n7 +_:c14n7 "Whole-genome sequencing of the cohort, aggregated per population." +_:c14n8 "hdab@example.org" +_:c14n8 +_:c14n9 _:c14n0 +_:c14n9 +_:c14n9 _:c14n4 +_:c14n9 +_:c14n9 +_:c14n9 "University of Tartu" From 2c867bc0565060dbc926d00976c026a3bdfc0cd7 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:34 +0300 Subject: [PATCH 05/16] feat: put the validator's rules into the package schemas The manifest and overlay schemas now state what the validator enforces: the contact point's fields and e-mail pattern, the age cap, the bounds on localized text and the closed vocabularies. A test checks that the schemas and the node refuse the same values. A closed list now also refuses a value listed twice, and lint and validate share one list of recommended fields, so lint flags a missing health theme too. --- crates/core/src/model/manifest.rs | 30 ++- crates/core/src/model/metadata.rs | 44 +++- crates/core/src/model/overlay.rs | 32 ++- crates/core/src/validate_pkg.rs | 194 +++++++++++++---- .../gdi-dataset-tool/src/commands/cmd_lint.rs | 29 ++- crates/gdi-node-standalone/tests/it/main.rs | 1 + .../tests/it/package_schema.rs | 195 ++++++++++++++++++ docs/gdi-dataset-tool.md | 3 +- docs/manifest.schema.json | 109 +++++++--- docs/metadata-overlay.schema.json | 97 +++++++-- docs/package-format.md | 7 +- 11 files changed, 631 insertions(+), 110 deletions(-) create mode 100644 crates/gdi-node-standalone/tests/it/package_schema.rs diff --git a/crates/core/src/model/manifest.rs b/crates/core/src/model/manifest.rs index 42c11a9..1945ae1 100644 --- a/crates/core/src/model/manifest.rs +++ b/crates/core/src/model/manifest.rs @@ -146,6 +146,10 @@ pub struct ManifestMetadata { /// Creating agents (>= 1). pub creator: Vec, /// GDI health category IRIs (>= 1). + #[cfg_attr( + feature = "schema", + schemars(schema_with = "crate::validate_pkg::health_category_schema") + )] pub health_category: Vec, /// Tags for discovery (recommended). #[serde(skip_serializing_if = "Option::is_none")] @@ -154,10 +158,18 @@ pub struct ManifestMetadata { #[serde(skip_serializing_if = "Option::is_none")] pub number_of_unique_individuals: Option, /// Standards-compliance IRIs. - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default, skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars(schema_with = "crate::validate_pkg::conforms_to_schema") + )] pub conforms_to: Option>, /// Dataset type IRIs, set only for synthetic datasets. - #[serde(rename = "type", skip_serializing_if = "Option::is_none")] + #[serde(rename = "type", default, skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars(schema_with = "crate::validate_pkg::dataset_type_schema") + )] pub type_: Option>, /// DPV legal basis IRIs (for real personal data). #[serde(skip_serializing_if = "Option::is_none")] @@ -176,15 +188,27 @@ pub struct ManifestMetadata { pub contact_point: Option, /// Typical age of the youngest people in the dataset, in years. #[serde(skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars(range(max = crate::validate_pkg::MAX_TYPICAL_AGE)) + )] pub min_typical_age: Option, /// Typical age of the oldest people in the dataset, in years. #[serde(skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars(range(max = crate::validate_pkg::MAX_TYPICAL_AGE)) + )] pub max_typical_age: Option, /// How the data was produced (plain string or language map). #[serde(skip_serializing_if = "Option::is_none")] pub provenance: Option, /// Health themes: the disease or healthcare areas, as EU health-theme IRIs (recommended). - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default, skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars(schema_with = "crate::validate_pkg::health_theme_schema") + )] pub health_theme: Option>, /// Computed: distinct `(chromosome, POS, REF, ALT)` variants the dataset serves. /// diff --git a/crates/core/src/model/metadata.rs b/crates/core/src/model/metadata.rs index a6db6c1..ac05555 100644 --- a/crates/core/src/model/metadata.rs +++ b/crates/core/src/model/metadata.rs @@ -8,15 +8,38 @@ use std::collections::BTreeMap; /// Either a plain string or a BCP-47 language map (e.g. `{en: "...", et: "..."}`). /// -/// Used for the localized `title` and `description` fields. +/// Used for the localized `title`, `description` and `provenance` fields. +// +// Not a doc comment: the published schema allows every value up to the longest field's cap, +// and the validator applies each field's own. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] #[serde(untagged)] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] pub enum LocalizedText { /// Single untagged value. - Plain(String), + Plain( + #[cfg_attr( + feature = "schema", + schemars(length(min = 1, max = crate::validate_pkg::MAX_DESCRIPTION_LEN)) + )] + String, + ), /// Language-tagged values, keyed by BCP-47 tag. - Map(BTreeMap), + Map( + #[cfg_attr( + feature = "schema", + schemars(extend( + "minProperties" = 1, + "maxProperties" = crate::validate_pkg::MAX_LOCALIZED_ENTRIES, + "additionalProperties" = { + "type": "string", + "minLength": 1, + "maxLength": crate::validate_pkg::MAX_DESCRIPTION_LEN + } + )) + )] + BTreeMap, + ), } /// An agent (creator / contributor). Maps to `foaf:Agent`. @@ -35,9 +58,24 @@ pub struct Agent { pub struct ContactPoint { /// `vcard:fn` (required). #[serde(rename = "fn", skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars( + required, + length(min = 1, max = crate::validate_pkg::MAX_CONTACT_FN_LEN) + ) + )] pub fn_: Option, /// `vcard:hasEmail`, a `mailto:` IRI (required). #[serde(skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars( + required, + length(max = crate::validate_pkg::MAX_EMAIL_LEN), + regex(pattern = crate::validate_pkg::MAILTO_EMAIL_PATTERN) + ) + )] pub has_email: Option, /// `vcard:hasURL` (optional; recommended sub-field). #[serde(rename = "hasURL", skip_serializing_if = "Option::is_none")] diff --git a/crates/core/src/model/overlay.rs b/crates/core/src/model/overlay.rs index b356de8..6ed5504 100644 --- a/crates/core/src/model/overlay.rs +++ b/crates/core/src/model/overlay.rs @@ -36,7 +36,11 @@ pub struct MetadataOverlay { #[serde(skip_serializing_if = "Option::is_none")] pub creator: Option>, /// GDI health category IRIs (>= 1). - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default, skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars(schema_with = "crate::validate_pkg::optional_health_category_schema") + )] pub health_category: Option>, /// Tags for discovery (recommended). #[serde(skip_serializing_if = "Option::is_none")] @@ -45,10 +49,18 @@ pub struct MetadataOverlay { #[serde(skip_serializing_if = "Option::is_none")] pub number_of_unique_individuals: Option, /// Standards-compliance IRIs. - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default, skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars(schema_with = "crate::validate_pkg::conforms_to_schema") + )] pub conforms_to: Option>, /// Dataset type IRIs, set only for synthetic datasets. - #[serde(rename = "type", skip_serializing_if = "Option::is_none")] + #[serde(rename = "type", default, skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars(schema_with = "crate::validate_pkg::dataset_type_schema") + )] pub type_: Option>, /// DPV legal basis IRIs (for real personal data). #[serde(skip_serializing_if = "Option::is_none")] @@ -64,15 +76,27 @@ pub struct MetadataOverlay { pub contact_point: Option, /// Typical age of the youngest people in the dataset, in years. #[serde(skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars(range(max = crate::validate_pkg::MAX_TYPICAL_AGE)) + )] pub min_typical_age: Option, /// Typical age of the oldest people in the dataset, in years. #[serde(skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars(range(max = crate::validate_pkg::MAX_TYPICAL_AGE)) + )] pub max_typical_age: Option, /// How the data was produced (plain string or language map). #[serde(skip_serializing_if = "Option::is_none")] pub provenance: Option, /// Health themes: the disease or healthcare areas, as EU health-theme IRIs (recommended). - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default, skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "schema", + schemars(schema_with = "crate::validate_pkg::health_theme_schema") + )] pub health_theme: Option>, } diff --git a/crates/core/src/validate_pkg.rs b/crates/core/src/validate_pkg.rs index f19894e..9ee60c2 100644 --- a/crates/core/src/validate_pkg.rs +++ b/crates/core/src/validate_pkg.rs @@ -75,7 +75,7 @@ pub const MAX_PROVENANCE_LEN: usize = MAX_DESCRIPTION_LEN; /// `minTypicalAge` / `maxTypicalAge` cap, in years. pub const MAX_TYPICAL_AGE: u32 = 150; /// Maximum language entries in a localized field. -const MAX_LOCALIZED_ENTRIES: usize = 24; +pub const MAX_LOCALIZED_ENTRIES: usize = 24; /// `keyword` (each) cap. pub const MAX_KEYWORD_LEN: usize = 64; /// `keywords` count cap. @@ -88,6 +88,11 @@ const MAX_CREATORS_COUNT: usize = 64; pub const MAX_CONTACT_FN_LEN: usize = 255; /// `email` cap (RFC 5321). pub const MAX_EMAIL_LEN: usize = 254; +/// [`is_mailto_email`] and the IRI-safe character rule as one pattern for the published +/// schemas; the validator runs the code. The node crate's `package_schema` tests check that +/// both refuse the same values. +pub const MAILTO_EMAIL_PATTERN: &str = + r#"^mailto:[^@<>"{}|^`\\\x00-\x20]+@[^.<>"{}|^`\\\x00-\x20]+\.[^<>"{}|^`\\\x00-\x20]+$"#; /// URL cap (`hasURL`, `afSourceReference`, ...). const MAX_URL_LEN: usize = 2048; /// IRI cap (every IRI metadata field). @@ -240,6 +245,63 @@ pub const HEALTH_THEMES: &[(&str, &str)] = &[ ), ]; +/// The schema form of [`validate_closed_list`]: values from `allowed`, each at most once, +/// at least `min_items` of them, and `null` too when `nullable`. +/// +/// An `Option` field using one of the wrappers below needs `#[serde(default)]`: under +/// `schema_with` that is what keeps it out of the schema's `required` list. +#[cfg(feature = "schema")] +fn closed_list_schema(allowed: &[&str], min_items: usize, nullable: bool) -> schemars::Schema { + let kind = if nullable { + serde_json::json!(["array", "null"]) + } else { + serde_json::json!("array") + }; + let mut schema = schemars::json_schema!({ + "type": kind, + "items": {"type": "string", "enum": allowed}, + "uniqueItems": true, + "maxItems": allowed.len() + }); + if min_items > 0 { + schema.insert("minItems".to_owned(), min_items.into()); + } + schema +} + +/// `healthCategory` in the manifest schema: at least one [`HEALTH_CATEGORIES`] IRI. +#[cfg(feature = "schema")] +pub(crate) fn health_category_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schema { + closed_list_schema(HEALTH_CATEGORIES, 1, false) +} + +/// `healthCategory` in the overlay schema, where it may be left out. +#[cfg(feature = "schema")] +pub(crate) fn optional_health_category_schema( + _: &mut schemars::SchemaGenerator, +) -> schemars::Schema { + closed_list_schema(HEALTH_CATEGORIES, 1, true) +} + +/// `healthTheme` in the published schemas: a list of [`HEALTH_THEMES`] IRIs. +#[cfg(feature = "schema")] +pub(crate) fn health_theme_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schema { + let themes: Vec<&str> = HEALTH_THEMES.iter().map(|&(iri, _)| iri).collect(); + closed_list_schema(&themes, 0, true) +} + +/// `conformsTo` in the published schemas: a list of [`CONFORMS_TO`] IRIs. +#[cfg(feature = "schema")] +pub(crate) fn conforms_to_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schema { + closed_list_schema(CONFORMS_TO, 0, true) +} + +/// `type` in the published schemas: at least one [`DATASET_TYPES`] IRI. +#[cfg(feature = "schema")] +pub(crate) fn dataset_type_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schema { + closed_list_schema(DATASET_TYPES, 1, true) +} + /// `conformsTo` IRIs: the closed set gdi-metadata's `DatasetShape` enumerates /// (`sh:in`, 3 values; vendored `Dataset.ttl`). /// @@ -457,7 +519,7 @@ fn validate_catalog( } /// Validate the mandatory metadata: title/description, access-rights, legislation, -/// license, creator, and health-category. +/// license, creator, health-category and the presence of contactPoint. /// /// Pushes the absent-EHDS advisory ([`ehds_absent_warning`]) onto `warnings`; every other /// finding here is a hard error. @@ -515,23 +577,36 @@ fn validate_core_metadata(m: &PackageMetadata, warnings: &mut Vec) -> Co /// Validate the recommended-tier fields, pushing a naming warning when absent. fn validate_recommended(m: &PackageMetadata, warnings: &mut Vec) -> CoreResult<()> { - match &m.keywords { - None => warnings.push("recommended field \"keywords\" is absent".to_owned()), - Some(kws) => { - check_max_count("keywords", kws.len(), MAX_KEYWORDS_COUNT)?; - for kw in kws { - check_max_chars("a keyword", kw, MAX_KEYWORD_LEN)?; - } + for field in absent_recommended_fields(m) { + warnings.push(format!("recommended field \"{field}\" is absent")); + } + if let Some(kws) = &m.keywords { + check_max_count("keywords", kws.len(), MAX_KEYWORDS_COUNT)?; + for kw in kws { + check_max_chars("a keyword", kw, MAX_KEYWORD_LEN)?; } } + if let Some(themes) = &m.health_theme { + validate_health_themes(themes)?; + } + Ok(()) +} + +/// The recommended fields `m` leaves out, by their wire names; an empty list counts as left +/// out. `validate` warns about each and `build --strict` fails on them; `lint` lists them. +#[must_use] +pub fn absent_recommended_fields(m: &PackageMetadata) -> Vec<&'static str> { + let mut absent = Vec::new(); + if m.keywords.as_ref().is_none_or(Vec::is_empty) { + absent.push("keywords"); + } if m.number_of_unique_individuals.is_none() { - warnings.push("recommended field \"numberOfUniqueIndividuals\" is absent".to_owned()); + absent.push("numberOfUniqueIndividuals"); } - match &m.health_theme { - Some(themes) if !themes.is_empty() => validate_health_themes(themes)?, - _ => warnings.push("recommended field \"healthTheme\" is absent".to_owned()), + if m.health_theme.as_ref().is_none_or(Vec::is_empty) { + absent.push("healthTheme"); } - Ok(()) + absent } /// Validate `creator`: at least one agent, each with a bounded non-empty name. @@ -554,23 +629,31 @@ fn validate_health_categories(categories: &[String]) -> CoreResult<()> { if categories.is_empty() { return Err(invalid("healthCategory must have at least one entry")); } - for iri in categories { + validate_closed_list("healthCategory", categories, |iri| { validate_iri("healthCategory", iri)?; - validate_health_category(iri)?; - } - Ok(()) + validate_health_category(iri) + }) } /// Validate `healthTheme`: every IRI must be in [`HEALTH_THEMES`]. fn validate_health_themes(themes: &[String]) -> CoreResult<()> { - for iri in themes { - if !HEALTH_THEMES.iter().any(|&(theme, _)| theme == iri) { - return Err(invalid(&format!( + validate_closed_list("healthTheme", themes, |iri| { + if HEALTH_THEMES.iter().any(|&(theme, _)| theme == iri) { + Ok(()) + } else { + Err(invalid(&format!( "healthTheme value {iri:?} is not allowed" - ))); + ))) } - } - Ok(()) + }) +} + +/// Validate `conformsTo`: each IRI from the closed [`CONFORMS_TO`] set. +fn validate_conforms_to_list(iris: &[String]) -> CoreResult<()> { + validate_closed_list("conformsTo", iris, |iri| { + validate_iri("conformsTo", iri)?; + validate_conforms_to(iri) + }) } /// Validate the optional metadata fields (validated when present, silent when @@ -581,10 +664,7 @@ fn validate_health_themes(themes: &[String]) -> CoreResult<()> { /// See the module docs for the tier split. fn validate_optional_metadata(m: &PackageMetadata, notes: &mut Vec) -> CoreResult<()> { if let Some(conforms) = &m.conforms_to { - for iri in conforms { - validate_iri("conformsTo", iri)?; - validate_conforms_to(iri)?; - } + validate_conforms_to_list(conforms)?; } if let Some(types) = &m.type_ { validate_types(types)?; @@ -721,6 +801,25 @@ fn validate_iri_list(field: &str, iris: &[String]) -> CoreResult<()> { Ok(()) } +/// Validate a list drawn from a closed vocabulary: each value by `check`, and none twice. +/// +/// A repeat publishes nothing new (RDF keeps one triple) but would be stored as given. +/// Refusing it also caps the list at the vocabulary's size. +fn validate_closed_list( + field: &str, + values: &[String], + check: impl Fn(&str) -> CoreResult<()>, +) -> CoreResult<()> { + let mut seen = std::collections::HashSet::new(); + for value in values { + check(value)?; + if !seen.insert(value.as_str()) { + return Err(invalid(&format!("{field} lists {value:?} twice"))); + } + } + Ok(()) +} + /// The placeholder prefix `init` writes for every required field the provider must supply /// (e.g. `title: "REPLACE: Dataset title"`). const REPLACE_MARKER: &str = "REPLACE:"; @@ -898,16 +997,15 @@ pub fn validate_enum(field: &str, value: &str, allowed: &[&str]) -> CoreResult<( /// # Errors /// /// Returns [`CoreError::InvalidManifest`] for an empty list, or for a value that is not -/// in the set or not an IRI. +/// in the set, not an IRI, or listed twice. pub fn validate_types(types: &[String]) -> CoreResult<()> { if types.is_empty() { return Err(invalid("type must list at least one IRI")); } - for iri in types { + validate_closed_list("type", types, |iri| { validate_enum("type", iri, DATASET_TYPES)?; - validate_iri("type", iri)?; - } - Ok(()) + validate_iri("type", iri) + }) } /// Validate `healthCategory`: one of the closed [`HEALTH_CATEGORIES`] set @@ -1329,10 +1427,7 @@ pub fn validate_patch(patch: &crate::model::MetadataOverlay) -> CoreResult<()> { } } if let Some(conforms) = conforms_to { - for iri in conforms { - validate_iri("conformsTo", iri)?; - validate_conforms_to(iri)?; - } + validate_conforms_to_list(conforms)?; } if let Some(t) = type_ { validate_types(t)?; @@ -1649,6 +1744,31 @@ mod tests { assert!(format!("{err}").contains("healthTheme"), "{err}"); } + #[test] + fn a_closed_list_refuses_a_value_listed_twice() { + let twice = |iri: &str| vec![iri.to_owned(), iri.to_owned()]; + let mut category = sample_package(); + category.metadata.health_category = twice(HEALTH_CATEGORIES[0]); + let mut theme = sample_package(); + theme.metadata.health_theme = Some(twice(HEALTH_THEMES[0].0)); + let mut conforms = sample_package(); + conforms.metadata.conforms_to = Some(twice(CONFORMS_TO[0])); + let mut types = sample_package(); + types.metadata.type_ = Some(twice(DATASET_TYPES[0])); + for (p, field) in [ + (category, "healthCategory"), + (theme, "healthTheme"), + (conforms, "conformsTo"), + (types, "type"), + ] { + let err = validate_package(&p, Some(&node_catalogs())).unwrap_err(); + assert!( + format!("{err}").contains(&format!("{field} lists ")), + "{err}" + ); + } + } + #[test] fn missing_contact_point_rejected() { let mut p = sample_package(); diff --git a/crates/gdi-dataset-tool/src/commands/cmd_lint.rs b/crates/gdi-dataset-tool/src/commands/cmd_lint.rs index a477138..bfed377 100644 --- a/crates/gdi-dataset-tool/src/commands/cmd_lint.rs +++ b/crates/gdi-dataset-tool/src/commands/cmd_lint.rs @@ -13,13 +13,14 @@ use std::collections::{BTreeMap, BTreeSet}; use std::path::Path; use gdi_node_standalone_core::convert::MAX_POPULATIONS; -use gdi_node_standalone_core::model::{Manifest, ManifestMetadata}; +use gdi_node_standalone_core::model::Manifest; use gdi_node_standalone_core::parquet_io::{ AlleleRow, DatasetDecryptor, PosWindow, read_matching_rows, }; use gdi_node_standalone_core::popfield::TOTAL_POPULATION; use gdi_node_standalone_core::s3_layout::is_data_file_name; use gdi_node_standalone_core::validate_parquet::ParquetCaps; +use gdi_node_standalone_core::validate_pkg::absent_recommended_fields; use crate::ToolError; use crate::cli::{LintArgs, OutputFormat}; @@ -133,10 +134,6 @@ fn suppression_from(manifest: &Manifest) -> (Option, Vec bool { - meta.keywords.as_ref().is_some_and(|k| !k.is_empty()) -} - /// Streaming fold of allele-frequency rows into the [`LintReport`] counters, so `lint` /// can process a dataset one parquet file at a time instead of materialising every row /// of every file at once (an unbounded peak on a multi-million-variant dataset). @@ -188,13 +185,10 @@ impl LintAccumulator { /// Finalize into the report, joining the folded counters with the manifest fields. fn finalize(self, manifest: &Manifest) -> LintReport { let meta = &manifest.metadata; - let mut recommended_absent = Vec::new(); - if !has_keywords(meta) { - recommended_absent.push("keywords".to_owned()); - } - if meta.number_of_unique_individuals.is_none() { - recommended_absent.push("numberOfUniqueIndividuals".to_owned()); - } + let recommended_absent = absent_recommended_fields(&meta.as_package_metadata()) + .into_iter() + .map(str::to_owned) + .collect(); let total_only = self.populations.len() == 1 && self.populations.contains_key(TOTAL_POPULATION); let (suppression, ignored_info_fields) = suppression_from(manifest); @@ -419,6 +413,7 @@ fn lint_dir(args: &LintArgs, dir: &Path) -> Result<(), ToolError> { mod tests { #![expect(clippy::unwrap_used, reason = "unwrap is permitted in test code")] use super::*; + use gdi_node_standalone_core::model::ManifestMetadata; use gdi_node_standalone_core::model::{Assembly, DatasetMode, LocalizedText, ManifestConfig}; use gdi_node_standalone_core::variant::Vt; @@ -628,7 +623,7 @@ mod tests { row("FI_F", Vt::Del, 1.0, Some(40), Some(40)), // af≈1 + AC==AN row("EE_M", Vt::Snp, 0.02, Some(2), Some(100)), // low-AC (2 < 5) ]; - // keywords present, individuals absent -> one recommended field flagged. + // keywords present; individuals and health theme absent -> those two flagged. let m = manifest(Some(vec!["covid".to_owned()]), None); let r = analyze(&m, &rows); @@ -636,7 +631,10 @@ mod tests { assert_eq!(r.rows, 4); assert_eq!( r.recommended_absent, - vec!["numberOfUniqueIndividuals".to_owned()] + vec![ + "numberOfUniqueIndividuals".to_owned(), + "healthTheme".to_owned() + ] ); assert_eq!(r.populations.len(), 4); assert_eq!(r.populations.get("FI_M"), Some(&1)); @@ -685,7 +683,8 @@ mod tests { r.recommended_absent, vec![ "keywords".to_owned(), - "numberOfUniqueIndividuals".to_owned() + "numberOfUniqueIndividuals".to_owned(), + "healthTheme".to_owned() ] ); assert_eq!(r.rows, 0); diff --git a/crates/gdi-node-standalone/tests/it/main.rs b/crates/gdi-node-standalone/tests/it/main.rs index e50b9f4..67d8be7 100644 --- a/crates/gdi-node-standalone/tests/it/main.rs +++ b/crates/gdi-node-standalone/tests/it/main.rs @@ -47,6 +47,7 @@ mod metrics_endpoint; mod not_found_shapes; mod operating_doc_cli; mod operator_flows; +mod package_schema; mod public_cors_fallback; mod query_stats_route; mod reingest_route; diff --git a/crates/gdi-node-standalone/tests/it/package_schema.rs b/crates/gdi-node-standalone/tests/it/package_schema.rs new file mode 100644 index 0000000..4f61ef4 --- /dev/null +++ b/crates/gdi-node-standalone/tests/it/package_schema.rs @@ -0,0 +1,195 @@ +//! The published manifest and overlay schemas refuse what the validator refuses. +//! +//! `docs/package-format.md` makes the schemas the contract a consumer generates code from, so +//! a rule only the validator knows is one a consumer silently lacks. Each case here is a +//! value the node refuses at ingest; the schema must refuse it too, and a value at the limit +//! must pass both. +#![expect(clippy::unwrap_used, reason = "unwrap is permitted in test code")] + +use gdi_node_standalone_core::model::{Manifest, MetadataOverlay}; +use gdi_node_standalone_core::validate_pkg::{ + CONFORMS_TO, DATASET_TYPES, HEALTH_CATEGORIES, MAX_CONTACT_FN_LEN, MAX_PROVENANCE_LEN, + MAX_TYPICAL_AGE, validate_overlay_result, validate_patch, +}; +use serde_json::{Value, json}; + +const THEME: &str = + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS"; + +fn schema(file: &str) -> jsonschema::Validator { + let path = format!("{}/../../docs/{file}", env!("CARGO_MANIFEST_DIR")); + let schema: Value = serde_json::from_str(&std::fs::read_to_string(path).unwrap()).unwrap(); + jsonschema::validator_for(&schema).unwrap() +} + +fn golden_manifest() -> Value { + let path = format!( + "{}/../core/tests/fixtures/manifest.json", + env!("CARGO_MANIFEST_DIR") + ); + serde_json::from_str(&std::fs::read_to_string(path).unwrap()).unwrap() +} + +/// The golden manifest with `metadata.` set to `value`. +fn manifest_with(field: &str, value: Value) -> Value { + let mut manifest = golden_manifest(); + manifest["metadata"][field] = value; + manifest +} + +fn node_accepts_manifest(manifest: &Value) -> bool { + serde_json::from_value::(manifest.clone()) + .is_ok_and(|m| validate_overlay_result(&m.metadata).is_ok()) +} + +fn node_accepts_overlay(overlay: &Value) -> bool { + serde_json::from_value::(overlay.clone()) + .is_ok_and(|o| validate_patch(&o).is_ok()) +} + +fn long(n: usize) -> String { + "a".repeat(n) +} + +/// Field values the node refuses, each with a value at the limit that it accepts. +fn cases() -> Vec<(&'static str, &'static str, Value, Value)> { + let contact = |fn_: Value, email: Value| json!({"fn": fn_, "hasEmail": email}); + let email = json!("mailto:data@example.org"); + vec![ + ( + "contact point without a name or e-mail", + "contactPoint", + json!({}), + contact(json!("Data team"), email.clone()), + ), + ( + "contact name too long", + "contactPoint", + contact(json!(long(MAX_CONTACT_FN_LEN + 1)), email.clone()), + contact(json!(long(MAX_CONTACT_FN_LEN)), email), + ), + ( + "e-mail without mailto:", + "contactPoint", + contact(json!("Data team"), json!("data@example.org")), + contact(json!("Data team"), json!("mailto:data@example.org")), + ), + ( + "e-mail domain without a dot", + "contactPoint", + contact(json!("Data team"), json!("mailto:data@example")), + contact(json!("Data team"), json!("mailto:data@ex.ample.org")), + ), + ( + "e-mail with a space", + "contactPoint", + contact(json!("Data team"), json!("mailto:da ta@example.org")), + contact(json!("Data team"), json!("mailto:da.ta@example.org")), + ), + ( + "minimum age too high", + "minTypicalAge", + json!(MAX_TYPICAL_AGE + 1), + json!(MAX_TYPICAL_AGE), + ), + ( + "maximum age too high", + "maxTypicalAge", + json!(MAX_TYPICAL_AGE + 1), + json!(MAX_TYPICAL_AGE), + ), + ("empty provenance", "provenance", json!(""), json!("P")), + ( + "provenance too long", + "provenance", + json!(long(MAX_PROVENANCE_LEN + 1)), + json!(long(MAX_PROVENANCE_LEN)), + ), + ( + "empty provenance map", + "provenance", + json!({}), + json!({"en": "P"}), + ), + ( + "health theme outside the vocabulary", + "healthTheme", + json!(["https://example.org/GENOMICS"]), + json!([THEME]), + ), + ( + "health theme listed twice", + "healthTheme", + json!([THEME, THEME]), + json!([THEME]), + ), + ( + "health category outside the vocabulary", + "healthCategory", + json!(["https://example.org/GENOMICS"]), + json!([HEALTH_CATEGORIES[0]]), + ), + ( + "conformsTo outside the vocabulary", + "conformsTo", + json!(["https://example.org/compliant"]), + json!([CONFORMS_TO[0]]), + ), + ( + "dataset type outside the vocabulary", + "type", + json!(["https://example.org/REAL_DATA"]), + json!([DATASET_TYPES[0]]), + ), + ] +} + +#[test] +fn the_manifest_schema_refuses_what_the_node_refuses() { + let schema = schema("manifest.schema.json"); + assert!( + schema.is_valid(&golden_manifest()), + "the golden manifest is valid" + ); + for (case, field, refused, accepted) in cases() { + let bad = manifest_with(field, refused); + assert!( + !node_accepts_manifest(&bad), + "{case}: the node must refuse it" + ); + assert!(!schema.is_valid(&bad), "{case}: the schema must refuse it"); + + let good = manifest_with(field, accepted); + assert!( + node_accepts_manifest(&good), + "{case}: the node accepts the limit" + ); + assert!( + schema.is_valid(&good), + "{case}: the schema accepts the limit" + ); + } +} + +#[test] +fn the_overlay_schema_refuses_what_the_node_refuses() { + let schema = schema("metadata-overlay.schema.json"); + for (case, field, refused, accepted) in cases() { + let bad = json!({ field: refused }); + assert!( + !node_accepts_overlay(&bad), + "{case}: the node must refuse it" + ); + assert!(!schema.is_valid(&bad), "{case}: the schema must refuse it"); + + let good = json!({ field: accepted }); + assert!( + node_accepts_overlay(&good), + "{case}: the node accepts the limit" + ); + assert!( + schema.is_valid(&good), + "{case}: the schema accepts the limit" + ); + } +} diff --git a/docs/gdi-dataset-tool.md b/docs/gdi-dataset-tool.md index 986cd8c..daa5064 100644 --- a/docs/gdi-dataset-tool.md +++ b/docs/gdi-dataset-tool.md @@ -1421,7 +1421,8 @@ gdi-dataset-tool lint [--format text|json] | `--format` | `text` | `text` (human card) or `json` (the same report, for machine consumers). | The report covers recommended-metadata coverage (`keywords`, -`numberOfUniqueIndividuals`), per-population and per-variant-type site counts, and an +`numberOfUniqueIndividuals`, `healthTheme`; the fields `validate` warns about), per-population +and per-variant-type site counts, and an allele-frequency sanity scan: `AF=0`, `AF≈1`, `AC==AN` saturation, and low-AC or rare-variant exposure against a threshold. Every line is a per-signal count, not a verdict. Reading the numbers against your own thresholds is the provider's call. diff --git a/docs/manifest.schema.json b/docs/manifest.schema.json index d03e605..a76386c 100644 --- a/docs/manifest.schema.json +++ b/docs/manifest.schema.json @@ -405,18 +405,24 @@ } }, "LocalizedText": { - "description": "Either a plain string or a BCP-47 language map (e.g. `{en: \"...\", et: \"...\"}`).\n\nUsed for the localized `title` and `description` fields.", + "description": "Either a plain string or a BCP-47 language map (e.g. `{en: \"...\", et: \"...\"}`).\n\nUsed for the localized `title`, `description` and `provenance` fields.", "anyOf": [ { "description": "Single untagged value.", - "type": "string" + "type": "string", + "maxLength": 10000, + "minLength": 1 }, { "description": "Language-tagged values, keyed by BCP-47 tag.", "type": "object", "additionalProperties": { - "type": "string" - } + "type": "string", + "maxLength": 10000, + "minLength": 1 + }, + "maxProperties": 24, + "minProperties": 1 } ] }, @@ -515,8 +521,15 @@ "null" ], "items": { - "type": "string" - } + "type": "string", + "enum": [ + "http://data.gdi.eu/core/p2/ExternallyGoverned", + "http://data.gdi.eu/core/p2/1MGCompliant", + "http://data.gdi.eu/core/p2/1MGCohort" + ] + }, + "maxItems": 3, + "uniqueItems": true }, "contactPoint": { "description": "Who to write to about the dataset. Required.", @@ -524,17 +537,15 @@ "properties": { "fn": { "description": "`vcard:fn` (required).", - "type": [ - "string", - "null" - ] + "type": "string", + "maxLength": 255, + "minLength": 1 }, "hasEmail": { "description": "`vcard:hasEmail`, a `mailto:` IRI (required).", - "type": [ - "string", - "null" - ] + "type": "string", + "maxLength": 254, + "pattern": "^mailto:[^@<>\"{}|^`\\\\\\x00-\\x20]+@[^.<>\"{}|^`\\\\\\x00-\\x20]+\\.[^<>\"{}|^`\\\\\\x00-\\x20]+$" }, "hasURL": { "description": "`vcard:hasURL` (optional; recommended sub-field).", @@ -543,7 +554,11 @@ "null" ] } - } + }, + "required": [ + "fn", + "hasEmail" + ] }, "creator": { "description": "Creating agents (>= 1).", @@ -561,14 +576,20 @@ "anyOf": [ { "description": "Single untagged value.", - "type": "string" + "type": "string", + "maxLength": 10000, + "minLength": 1 }, { "description": "Language-tagged values, keyed by BCP-47 tag.", "type": "object", "additionalProperties": { - "type": "string" - } + "type": "string", + "maxLength": 10000, + "minLength": 1 + }, + "maxProperties": 24, + "minProperties": 1 } ] }, @@ -576,8 +597,16 @@ "description": "GDI health category IRIs (>= 1).", "type": "array", "items": { - "type": "string" - } + "type": "string", + "enum": [ + "http://data.gdi.eu/core/p2/HealthCategoryHumanGenetic", + "http://data.gdi.eu/core/p2/HealthCategoryHumanEpigenomic", + "http://data.gdi.eu/core/p2/HealthCategoryHumanGenomic" + ] + }, + "maxItems": 3, + "minItems": 1, + "uniqueItems": true }, "healthTheme": { "description": "Health themes: the disease or healthcare areas, as EU health-theme IRIs (recommended).", @@ -586,8 +615,32 @@ "null" ], "items": { - "type": "string" - } + "type": "string", + "enum": [ + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/ANTIMICROBIAL_CONTROL", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/BLOOD_INFECTIONS", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/CANCER_DISEASE", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/CLIMATE_HEALTH", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/EMERGENCY_SETTINGS", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/ENTERIC_INFECTIONS", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/ENVIRONMENTAL_HEALTH", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_SYSTEMS", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/IMMUNIZATION_DISEASES", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/INJURY_PREVENTION", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/LIFECOURSE_HEALTH", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/MENTAL_HEALTH", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/NONCOMMUNICABLE_DISEASES", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/NUTRITION_SECURITY", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/REPRODUCTIVE_HEALTH", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/RESPIRATORY_DISEASES", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/SENSORY_HEALTH", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/TROPICAL_DISEASES", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/VECTOR_DISEASES" + ] + }, + "maxItems": 20, + "uniqueItems": true }, "isReferencedBy": { "description": "Publication DOI IRIs.", @@ -630,6 +683,7 @@ "null" ], "format": "uint32", + "maximum": 150, "minimum": 0 }, "minTypicalAge": { @@ -639,6 +693,7 @@ "null" ], "format": "uint32", + "maximum": 150, "minimum": 0 }, "numberOfRecords": { @@ -701,8 +756,14 @@ "null" ], "items": { - "type": "string" - } + "type": "string", + "enum": [ + "https://publications.europa.eu/resource/authority/dataset-type/SYNTHETIC_DATA" + ] + }, + "maxItems": 1, + "minItems": 1, + "uniqueItems": true } }, "required": [ diff --git a/docs/metadata-overlay.schema.json b/docs/metadata-overlay.schema.json index 275346d..968b445 100644 --- a/docs/metadata-overlay.schema.json +++ b/docs/metadata-overlay.schema.json @@ -28,8 +28,15 @@ "null" ], "items": { - "type": "string" - } + "type": "string", + "enum": [ + "http://data.gdi.eu/core/p2/ExternallyGoverned", + "http://data.gdi.eu/core/p2/1MGCompliant", + "http://data.gdi.eu/core/p2/1MGCohort" + ] + }, + "maxItems": 3, + "uniqueItems": true }, "contactPoint": { "description": "Who to write to about the dataset.", @@ -70,8 +77,16 @@ "null" ], "items": { - "type": "string" - } + "type": "string", + "enum": [ + "http://data.gdi.eu/core/p2/HealthCategoryHumanGenetic", + "http://data.gdi.eu/core/p2/HealthCategoryHumanEpigenomic", + "http://data.gdi.eu/core/p2/HealthCategoryHumanGenomic" + ] + }, + "maxItems": 3, + "minItems": 1, + "uniqueItems": true }, "healthTheme": { "description": "Health themes: the disease or healthcare areas, as EU health-theme IRIs (recommended).", @@ -80,8 +95,32 @@ "null" ], "items": { - "type": "string" - } + "type": "string", + "enum": [ + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/ANTIMICROBIAL_CONTROL", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/BLOOD_INFECTIONS", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/CANCER_DISEASE", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/CLIMATE_HEALTH", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/EMERGENCY_SETTINGS", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/ENTERIC_INFECTIONS", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/ENVIRONMENTAL_HEALTH", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_SYSTEMS", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/IMMUNIZATION_DISEASES", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/INJURY_PREVENTION", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/LIFECOURSE_HEALTH", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/MENTAL_HEALTH", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/NONCOMMUNICABLE_DISEASES", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/NUTRITION_SECURITY", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/REPRODUCTIVE_HEALTH", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/RESPIRATORY_DISEASES", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/SENSORY_HEALTH", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/TROPICAL_DISEASES", + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/VECTOR_DISEASES" + ] + }, + "maxItems": 20, + "uniqueItems": true }, "isReferencedBy": { "description": "Publication DOI IRIs.", @@ -127,6 +166,7 @@ "null" ], "format": "uint32", + "maximum": 150, "minimum": 0 }, "minTypicalAge": { @@ -136,6 +176,7 @@ "null" ], "format": "uint32", + "maximum": 150, "minimum": 0 }, "numberOfUniqueIndividuals": { @@ -186,8 +227,14 @@ "null" ], "items": { - "type": "string" - } + "type": "string", + "enum": [ + "https://publications.europa.eu/resource/authority/dataset-type/SYNTHETIC_DATA" + ] + }, + "maxItems": 1, + "minItems": 1, + "uniqueItems": true } }, "additionalProperties": false, @@ -211,17 +258,15 @@ "properties": { "fn": { "description": "`vcard:fn` (required).", - "type": [ - "string", - "null" - ] + "type": "string", + "maxLength": 255, + "minLength": 1 }, "hasEmail": { "description": "`vcard:hasEmail`, a `mailto:` IRI (required).", - "type": [ - "string", - "null" - ] + "type": "string", + "maxLength": 254, + "pattern": "^mailto:[^@<>\"{}|^`\\\\\\x00-\\x20]+@[^.<>\"{}|^`\\\\\\x00-\\x20]+\\.[^<>\"{}|^`\\\\\\x00-\\x20]+$" }, "hasURL": { "description": "`vcard:hasURL` (optional; recommended sub-field).", @@ -230,21 +275,31 @@ "null" ] } - } + }, + "required": [ + "fn", + "hasEmail" + ] }, "LocalizedText": { - "description": "Either a plain string or a BCP-47 language map (e.g. `{en: \"...\", et: \"...\"}`).\n\nUsed for the localized `title` and `description` fields.", + "description": "Either a plain string or a BCP-47 language map (e.g. `{en: \"...\", et: \"...\"}`).\n\nUsed for the localized `title`, `description` and `provenance` fields.", "anyOf": [ { "description": "Single untagged value.", - "type": "string" + "type": "string", + "maxLength": 10000, + "minLength": 1 }, { "description": "Language-tagged values, keyed by BCP-47 tag.", "type": "object", "additionalProperties": { - "type": "string" - } + "type": "string", + "maxLength": 10000, + "minLength": 1 + }, + "maxProperties": 24, + "minProperties": 1 } ] }, diff --git a/docs/package-format.md b/docs/package-format.md index dcd2b97..fc23986 100644 --- a/docs/package-format.md +++ b/docs/package-format.md @@ -126,13 +126,13 @@ The package's first member. Serialized `camelCase`. Top level: | `applicableLegislation` | IRI array (≥ 1) | yes | EU ELI IRIs. Omitting the EHDS ELI (`http://data.europa.eu/eli/reg/2025/327/oj`) is a warning, not a rejection. | | `license` | IRI string | yes | Reuse license IRI. | | `creator` | `[{name}]` (≥ 1) | yes | Creating agents. | -| `healthCategory` | IRI array (≥ 1) | yes | GDI health-category IRIs. | +| `healthCategory` | IRI array (≥ 1) | yes | GDI health-category IRIs, a closed set (`manifest.schema.json` lists it). Any other value is rejected. | | `contactPoint` | `{fn, hasEmail, hasURL?}` | yes | Who to write to about the dataset: a name and a `mailto:` e-mail, and optionally a web page. | | `keywords` | string array | no | Discovery tags. | | `healthTheme` | IRI array | no | Disease or healthcare areas, from the EU health-theme vocabulary (`https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/…`, 20 codes). Any other value is rejected; genomics is `HEALTH_PRODUCTS`. Recommended: `build` warns without it. | | `numberOfUniqueIndividuals` | uint | no | Distinct sequenced subjects. | | `conformsTo` | IRI array | no | GDI standards-compliance IRIs, a closed set: `ExternallyGoverned`, `1MGCompliant` and `1MGCohort` under `http://data.gdi.eu/core/p2/`. Any other value is rejected. | -| `type` | IRI array | no | Dataset-type IRIs (set only for synthetic data). | +| `type` | IRI array | no | Dataset-type IRIs (set only for synthetic data), a closed set (`manifest.schema.json` lists it). | | `legalBasis` | IRI array | no | DPV legal-basis IRIs. GDI requires one for `NON_PUBLIC` data: without it the tool and the node warn. | | `isReferencedBy` | IRI array | no | Publication DOI IRIs. | | `otherIdentifier` | `[{notation, schemaAgency?, name?}]` | no | Secondary identifiers. | @@ -141,6 +141,9 @@ The package's first member. Serialized `camelCase`. Top level: | `numberOfRecords` | uint | required at ingest | Distinct `(chr, POS, REF, ALT)` count. The node re-counts and rejects a mismatch. | | `populations` | string array | no | The population labels the dataset serves, sorted: the union of each source VCF's emitted set, after the build-time `minAlleleCount` floor. The node re-derives it from the parquet and rejects a mismatch, as for `numberOfRecords`; an absent claim is accepted. Served on the beacon `datasets` entry as `gdiDatasetInfo.populations`. | +The closed lists (`healthCategory`, `healthTheme`, `conformsTo`, `type`) take each value at +most once; a repeat is rejected. + ### `config` Serialized `camelCase` with `deny_unknown_fields`, so an unrecognized key fails ingest From 212cd348b82f76afa7bba3921495a3ce89c298c8 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:34 +0300 Subject: [PATCH 06/16] fix(tool): check contact points and fix-up answers like new packages A remembered contact point is checked before it is offered, so a hand-edited mailto: prefix no longer ends up doubled. The fix-up prompts follow the new-package rules: an e-mail with or without mailto:, the length caps, and an upper-cased org. --- crates/gdi-dataset-tool/src/wizard/author.rs | 171 +++++++++++++++--- crates/gdi-dataset-tool/src/wizard/fields.rs | 16 -- .../gdi-dataset-tool/tests/it/wizard_e2e.rs | 109 +++++++++++ 3 files changed, 255 insertions(+), 41 deletions(-) diff --git a/crates/gdi-dataset-tool/src/wizard/author.rs b/crates/gdi-dataset-tool/src/wizard/author.rs index 9854d85..6ff4fa8 100644 --- a/crates/gdi-dataset-tool/src/wizard/author.rs +++ b/crates/gdi-dataset-tool/src/wizard/author.rs @@ -388,7 +388,8 @@ pub fn replace_markers(yaml: &str) -> Vec { .collect() } -/// The validator for a `REPLACE:` marker, chosen by the YAML key it sits on. +/// The resolver for a `REPLACE:` marker, chosen by the YAML key it sits on: it checks an +/// answer and returns the value to write, with the same rules as the new-package prompts. /// /// Validating every marker as merely non-empty accepts a bad IRI or a bogus assembly at /// its prompt and rejects it in a lump by [`validate_rendered`] after the last question -- @@ -398,18 +399,24 @@ pub fn replace_markers(yaml: &str) -> Vec { /// Key-based only: guessing a type from the placeholder text would be a heuristic over /// prose the operator may have edited. An unrecognised key keeps the non-empty rule, and /// `validate_rendered` still backstops the whole document, so a missing entry here costs a -/// late error, never a bad package. -fn validator_for<'a>(key: &'a str, hint: &'a str) -> impl Fn(&str) -> Result<(), String> + 'a { +/// late error, never a bad package. `name` is the creator's: the template's one `name:` +/// marker. +fn resolver_for<'a>(key: &'a str, hint: &'a str) -> impl Fn(&str) -> Result + 'a { move |s: &str| match key { "accessRights" | "license" | "afSourceReference" | "isReferencedBy" - | "applicableLegislation" => fields::resolve_iri(key, s).map(|_| ()), - "healthCategory" => fields::resolve_health_category(s).map(|_| ()), - "reference" => fields::resolve_assembly(s).map(|_| ()), - "hasEmail" => fields::resolve_email(s).map(|_| ()), - _ => fields::resolve_nonempty(hint, s).map(|_| ()), + | "applicableLegislation" => fields::resolve_iri(key, s), + "healthCategory" => fields::resolve_health_category(s), + "reference" => fields::resolve_assembly(s), + "org" => fields::resolve_org(s), + "title" => fields::resolve_bounded("title", s, MAX_TITLE_LEN), + "description" => fields::resolve_bounded("description", s, MAX_DESCRIPTION_LEN), + "name" => fields::resolve_bounded("creator", s, MAX_CREATOR_NAME_LEN), + "fn" => fields::resolve_bounded("contact name", s, MAX_CONTACT_FN_LEN), + "hasEmail" => fields::resolve_contact_email(s).map(|address| format!("mailto:{address}")), + _ => fields::resolve_nonempty(hint, s), } } @@ -423,7 +430,7 @@ fn validator_for<'a>(key: &'a str, hint: &'a str) -> impl Fn(&str) -> Result<(), /// its first `:`: the marker text carries one too, so a bare list scalar would yield the /// garbage key `"REPLACE`. That drops `healthCategory` and `applicableLegislation` to the /// non-empty fallback, leaving their typed validators (a closed set, an IRI) unrun at the -/// prompt — the late lump-failure [`validator_for`] exists to prevent. +/// prompt — the late lump-failure [`resolver_for`] exists to prevent. fn marker_key<'a>(line: &'a str, parent: &'a str) -> &'a str { let quote = line.find('"').unwrap_or(line.len()); let head = line.get(..quote).unwrap_or(line); @@ -492,12 +499,15 @@ pub fn author_fixup(p: &dyn Prompter, path: &Path) -> Result, ) -> Result<(ProfileContactPoint, Option), ToolError> { - if let Some(cp) = remembered + // A hand-edited profile can hold anything, and a bad value would fail only after every + // Author question, so it is checked before it is offered. + let remembered = match remembered.map(usable_contact_point) { + Some(Ok(cp)) => Some(cp), + Some(Err(e)) => { + crate::output::warn(&format!( + "warning: the profile's contact_point is not usable ({e}); asking for one" + )); + None + } + None => None, + }; + if let Some(cp) = &remembered && p.confirm( &format!( "Contact point: {} <{}>. Use it for this dataset?", @@ -648,6 +670,24 @@ fn prompt_contact_point( Ok((contact.clone(), store.then_some(contact))) } +/// A remembered contact point as the wizard offers it: the name and URL checked, and the +/// e-mail checked and stored without `mailto:`, which the template adds. +/// +/// # Errors +/// +/// Returns the first field's validation message. +fn usable_contact_point(cp: &ProfileContactPoint) -> Result { + Ok(ProfileContactPoint { + name: fields::resolve_bounded("contact name", &cp.name, MAX_CONTACT_FN_LEN)?, + email: fields::resolve_contact_email(&cp.email)?, + url: cp + .url + .as_deref() + .map(|url| fields::resolve_url("contact web page", url)) + .transpose()?, + }) +} + /// Ask for publications that describe the dataset (`isReferencedBy`), until a blank answer. /// /// # Errors @@ -2002,6 +2042,29 @@ mod tests { ); } + /// Genomics data sits under the health-products theme, so that row, and only it, is + /// ticked when the menu opens. + #[test] + fn the_health_theme_menu_opens_with_health_products_ticked() { + use crate::wizard::prompts::ScriptedPrompter; + let p = ScriptedPrompter::new().with_multiselects(vec![vec![]]); + prompt_health_themes(&p).unwrap(); + let (_, ticks) = p.seen_multiselect_defaults().remove(0); + let choices = fields::health_theme_choices(); + let ticked: Vec<&str> = choices + .iter() + .zip(&ticks) + .filter(|&(_, &tick)| tick) + .map(|((_, iri), _)| *iri) + .collect(); + assert_eq!( + ticked, + [ + "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS" + ] + ); + } + #[test] fn keywords_are_deduplicated_and_capped_at_the_prompt() { // A repeated keyword is written once. @@ -2042,19 +2105,34 @@ mod tests { fn fixup_validates_a_typed_field_at_its_own_prompt() { // A typed marker must be rejected at its own prompt, not in a lump by // `validate_rendered` after the last question, which writes nothing and loses every - // answer. The fix-up path therefore wires `resolve_iri`, `resolve_assembly` and - // `resolve_email` rather than validating every marker as merely non-empty. - let iri = validator_for("license", "license IRI"); + // answer. The fix-up path therefore wires the typed resolvers rather than validating + // every marker as merely non-empty. + let iri = resolver_for("license", "license IRI"); assert!(iri("not-an-iri").is_err(), "a license must be an IRI"); assert!(iri("https://example.org/lic").is_ok()); - let asm = validator_for("reference", "GRCh38"); + let asm = resolver_for("reference", "GRCh38"); assert!(asm("hg38").is_err(), "assembly is a closed vocabulary"); assert!(asm("GRCh38").is_ok()); - let email = validator_for("hasEmail", "mailto:x@example.org"); - assert!(email("data@example.org").is_err(), "mailto: is required"); + // The same rules as the new-package prompts: an e-mail with or without `mailto:`, + // and the node's caps on names and text. + let email = resolver_for("hasEmail", "mailto:x@example.org"); + assert!(email("data@").is_err(), "the address is checked"); + assert!(email("data@example.org").is_ok()); assert!(email("mailto:data@example.org").is_ok()); + for (key, max) in [ + ("fn", MAX_CONTACT_FN_LEN), + ("name", MAX_CREATOR_NAME_LEN), + ("title", MAX_TITLE_LEN), + ("description", MAX_DESCRIPTION_LEN), + ] { + let bounded = resolver_for(key, key); + assert!(bounded(&"a".repeat(max + 1)).is_err(), "{key} over its cap"); + assert!(bounded(&"a".repeat(max)).is_ok(), "{key} at its cap"); + } + let org = resolver_for("org", "institute"); + assert!(org("UT-ARTU").is_err(), "an org is letters only"); // Untyped keys keep the non-empty rule: the fallback, not the universal rule. // A bare LIST scalar inherits the mapping key that opened the list, so the @@ -2082,17 +2160,17 @@ mod tests { assert_eq!(block_key(r#" title: "REPLACE: t""#), None); assert_eq!(block_key(" - GRCh38"), None); assert_eq!(block_key(" # a comment:"), None); - let closed = validator_for("healthCategory", "an IRI"); + let closed = resolver_for("healthCategory", "an IRI"); assert!( closed("http://data.gdi.eu/core/p2/HealthCategoryHumanProteomic").is_err(), "a healthCategory outside the vendored closed set must fail AT ITS PROMPT" ); assert!(closed("http://data.gdi.eu/core/p2/HealthCategoryHumanGenomic").is_ok()); - let eli = validator_for("applicableLegislation", "an ELI"); + let eli = resolver_for("applicableLegislation", "an ELI"); assert!(eli("EHDS").is_err(), "legislation entries are IRIs"); assert!(eli("http://data.europa.eu/eli/reg/2025/327/oj").is_ok()); - let free = validator_for("title", "Dataset title"); + let free = resolver_for("title", "Dataset title"); assert!(free(" ").is_err()); assert!(free("A cohort").is_ok()); } @@ -2267,6 +2345,43 @@ mod tests { ); } + #[test] + fn a_remembered_contact_point_is_offered_without_its_mailto_prefix() { + // The profile stores the address without `mailto:`, but a hand-edited one may carry + // it; the template adds the prefix once. + use crate::wizard::prompts::ScriptedPrompter; + let remembered = ProfileContactPoint { + name: "Data access team".into(), + email: "mailto:data@example.org".into(), + url: None, + }; + let p = ScriptedPrompter::new().with_confirms(vec![true]); + let (cp, store) = prompt_contact_point(&p, Some(&remembered)).unwrap(); + assert_eq!(cp.email, "data@example.org"); + assert_eq!(store, None); + } + + #[test] + fn an_invalid_remembered_contact_point_is_not_offered() { + // An unusable remembered value would fail only after every Author question. + use crate::wizard::prompts::ScriptedPrompter; + let remembered = ProfileContactPoint { + name: "Data access team".into(), + email: "not an address".into(), + url: None, + }; + // No "use it?" question: the answers go straight to a new contact point. + let p = ScriptedPrompter::new() + .with_confirms(vec![false]) + .with_inputs(vec!["Helpdesk", "help@example.org", ""]); + let (cp, store) = prompt_contact_point(&p, Some(&remembered)).unwrap(); + assert_eq!( + (cp.name.as_str(), cp.email.as_str()), + ("Helpdesk", "help@example.org") + ); + assert_eq!(store, None); + } + #[test] fn the_profile_contact_point_is_offered_and_a_typed_one_can_be_remembered() { use crate::wizard::prompts::ScriptedPrompter; @@ -2402,7 +2517,7 @@ mod tests { let vcf = test_util::covid_vcf_path(); let answers: Vec<&str> = vec![ "GOE", // 1 prefix - "UTARTU", // 2 org + "utartu", // 2 org "gdi-aggregated", // 3 catalog "Test allele frequencies", // 4 title "A test dataset.", // 5 description @@ -2410,7 +2525,7 @@ mod tests { "https://creativecommons.org/licenses/by/4.0/", // 7 license "Test Institute", // 8 creator name "Data access team", // 9 contact name - "mailto:data@example.org", // 10 contact e-mail + "data@example.org", // 10 contact e-mail "http://data.gdi.eu/core/p2/HealthCategoryHumanGenomic", // 11 healthCategory "GRCh38", // 12 VCF reference vcf.to_str().unwrap(), // 13 VCF path @@ -2424,6 +2539,12 @@ mod tests { let fixed = std::fs::read_to_string(&path).unwrap(); assert!(!fixed.contains("REPLACE:"), "all markers must be filled"); + // Answers are written as resolved, as the new-package path writes them. + assert!( + fixed.contains("hasEmail: \"mailto:data@example.org\""), + "{fixed}" + ); + assert!(fixed.contains("org: \"UTARTU\""), "{fixed}"); assert!( fixed.contains(&yaml_quote(vcf.to_str().unwrap())), "the VCF answer is written resolved, as the source prompt resolved it:\n{fixed}" diff --git a/crates/gdi-dataset-tool/src/wizard/fields.rs b/crates/gdi-dataset-tool/src/wizard/fields.rs index 85a6d93..c75724d 100644 --- a/crates/gdi-dataset-tool/src/wizard/fields.rs +++ b/crates/gdi-dataset-tool/src/wizard/fields.rs @@ -149,16 +149,6 @@ pub fn resolve_assembly(s: &str) -> Result { } } -/// Resolve + validate a `mailto:` email via the core validator. -/// -/// # Errors -/// `Err(message)` when the value is not a valid `mailto:` email. -pub fn resolve_email(s: &str) -> Result { - validate_email("hasEmail", s) - .map(|()| s.to_owned()) - .map_err(|e| e.to_string()) -} - /// Resolve a contact e-mail typed with or without `mailto:` to the bare address, checked /// as the `mailto:` IRI the package carries. /// @@ -445,12 +435,6 @@ mod tests { assert!(resolve_assembly("GRCH38").is_err()); // case-sensitive, like core } - #[test] - fn email_resolver_requires_mailto() { - assert!(resolve_email("mailto:a@b.co").is_ok()); - assert!(resolve_email("a@b.co").is_err()); - } - #[test] fn iri_resolver_rejects_unsafe() { assert!(resolve_iri("license", "https://creativecommons.org/licenses/by/4.0/").is_ok()); diff --git a/crates/gdi-dataset-tool/tests/it/wizard_e2e.rs b/crates/gdi-dataset-tool/tests/it/wizard_e2e.rs index f7e6fbf..454c461 100644 --- a/crates/gdi-dataset-tool/tests/it/wizard_e2e.rs +++ b/crates/gdi-dataset-tool/tests/it/wizard_e2e.rs @@ -1476,3 +1476,112 @@ fn a_typed_org_is_remembered_in_the_profile_when_asked_to() { "storing the org must not disturb the rest of the profile" ); } + +/// Answers for an Author run whose profile already holds the org: no org prompt. +fn scripted_author_with_profile_org(fixture: &str, contact: &[&'static str]) -> ScriptedPrompter { + let mut inputs = vec![ + fixture.to_owned(), + "gdi-aggregated".to_owned(), + "AF test (synthetic data)".to_owned(), + "Synthetic allele-frequency test dataset.".to_owned(), + String::new(), // provenance: skipped + "allele-frequency,genomics".to_owned(), + "2504".to_owned(), + String::new(), // age range: skipped + "Test Institute".to_owned(), + ]; + inputs.extend(contact.iter().map(|s| (*s).to_owned())); + inputs.extend([String::new(), "0".to_owned()]); // publication: none; minAlleleCount + ScriptedPrompter::new() + .with_inputs(inputs.iter().map(String::as_str).collect()) + .with_selects(vec![1, 0, 0, 0, 0]) + .with_multiselects(vec![ + vec![2], // health categories: Human genomic + vec![7], // health themes: Health products + vec![], // conformsTo: nothing ticked + vec![0], // applicable legislation: the EHDS row only + ]) +} + +fn run_author(p: &ScriptedPrompter, pkg: &std::path::Path, config_path: &std::path::Path) { + gdi_dataset_tool::wizard::run( + p, + &WizardArgs { + command: None, + from: Stage::Author, + to: Stage::Author, + output: pkg.to_owned(), + recipient: None, + }, + None, + Some(config_path), + ) + .expect("authoring must succeed"); +} + +/// A typed contact point is stored in the profile when the provider asks for that. +#[test] +fn a_typed_contact_point_is_remembered_in_the_profile_when_asked_to() { + let fixture_path = test_util::covid_vcf_path(); + let dir = tempfile::tempdir().unwrap(); + let config_path = dir.path().join("tool.toml"); + std::fs::write( + &config_path, + "country_code = \"EE\"\n\n[profiles.default]\norg = \"UTARTU\"\n", + ) + .unwrap(); + let p = scripted_author_with_profile_org( + fixture_path.to_str().unwrap(), + &["Data access team", "mailto:data@example.org", ""], + ) + .with_confirms(vec![ + false, // Add another VCF? + true, // keywords + true, // cohort + true, // synthetic + true, // Remember this contact point in the profile? yes + false, // Add another legislation ELI or IRI? + true, // GoE provenance + ]); + run_author(&p, &dir.path().join("package.yaml"), &config_path); + let cfg = gdi_node_standalone_core::config::ToolConfig::load(Some(&config_path)).unwrap(); + let stored = cfg.profiles["default"].contact_point.as_ref(); + assert_eq!(stored.map(|c| c.name.as_str()), Some("Data access team")); + assert_eq!( + stored.map(|c| c.email.as_str()), + Some("data@example.org"), + "stored without mailto:, which the template adds" + ); +} + +/// A remembered contact point is offered, and used without asking for one. +#[test] +fn the_profile_contact_point_is_offered_and_used() { + let fixture_path = test_util::covid_vcf_path(); + let dir = tempfile::tempdir().unwrap(); + let pkg = dir.path().join("package.yaml"); + let config_path = dir.path().join("tool.toml"); + std::fs::write( + &config_path, + "country_code = \"EE\"\n\n[profiles.default]\norg = \"UTARTU\"\n\n\ + [profiles.default.contact_point]\nname = \"Helpdesk\"\nemail = \"help@example.org\"\n", + ) + .unwrap(); + let p = + scripted_author_with_profile_org(fixture_path.to_str().unwrap(), &[]).with_confirms(vec![ + false, // Add another VCF? + true, // keywords + true, // cohort + true, // synthetic + true, // Contact point: Helpdesk . Use it? yes + false, // Add another legislation ELI or IRI? + true, // GoE provenance + ]); + run_author(&p, &pkg, &config_path); + let yaml = std::fs::read_to_string(&pkg).unwrap(); + assert!(yaml.contains("fn: \"Helpdesk\""), "{yaml}"); + assert!( + yaml.contains("hasEmail: \"mailto:help@example.org\""), + "{yaml}" + ); +} From b7a0aabbe93538507bd5092070da1e2ed6be28d7 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:34 +0300 Subject: [PATCH 07/16] fix(tool): say why the recipient fetch failed The error now carries its cause chain, so a refused connection, an unknown host and a bad certificate read differently. A 502, 503 or 504 from an ingress is treated like a node that is still starting. --- crates/gdi-dataset-tool/src/recipient.rs | 50 +++++++++++++++++---- crates/gdi-dataset-tool/src/wizard/setup.rs | 37 ++++++++++++--- 2 files changed, 73 insertions(+), 14 deletions(-) diff --git a/crates/gdi-dataset-tool/src/recipient.rs b/crates/gdi-dataset-tool/src/recipient.rs index e4c8ec9..3e70552 100644 --- a/crates/gdi-dataset-tool/src/recipient.rs +++ b/crates/gdi-dataset-tool/src/recipient.rs @@ -67,6 +67,19 @@ pub(crate) fn answered_status(url: &str, message: &str) -> Option String { + let mut text = e.to_string(); + let mut source = e.source(); + while let Some(cause) = source { + text.push_str(": "); + text.push_str(&cause.to_string()); + source = cause.source(); + } + text +} + /// Fetch + parse the node recipient from a URL. /// /// # Errors @@ -83,11 +96,12 @@ pub async fn fetch_node_recipient(url: &str) -> Result { .timeout(FETCH_TIMEOUT) .build() .map_err(|e| ToolError::user(format!("cannot build HTTP client: {e}")))?; - let mut resp = client - .get(url) - .send() - .await - .map_err(|e| ToolError::user(format!("cannot fetch node recipient {url}: {e}")))?; + let mut resp = client.get(url).send().await.map_err(|e| { + ToolError::user(format!( + "cannot fetch node recipient {url}: {}", + with_causes(&e) + )) + })?; let status = resp.status(); if !status.is_success() { let msg = answered_message(url, status); @@ -860,15 +874,35 @@ mod tests { let base = serve_once(500, b"internal error"); let url = format!("{base}/.well-known/c4gh-recipient"); let err = block_on(fetch_node_recipient(&url)).unwrap_err(); - assert!( - err.message.contains("500"), - "error must mention HTTP 500; got: {}", + // Read back the way the wizard classifies it, so the two cannot drift apart. + assert_eq!( + answered_status(&url, &err.message), + Some(reqwest::StatusCode::INTERNAL_SERVER_ERROR), + "{}", err.message ); // 500 is a generic server error, not auth — stays exit 1. assert_eq!(err.exit_code, crate::EXIT_USER); } + /// The cause is named, not just "error sending request": a refused connection, an + /// unknown host and a rejected certificate need different fixes. + #[test] + fn fetch_node_recipient_names_why_it_could_not_connect() { + let port = std::net::TcpListener::bind("127.0.0.1:0") + .unwrap() + .local_addr() + .unwrap() + .port(); + let url = format!("http://127.0.0.1:{port}/.well-known/c4gh-recipient"); + let err = block_on(fetch_node_recipient(&url)).unwrap_err(); + assert!( + err.message.to_lowercase().contains("refused"), + "{}", + err.message + ); + } + #[test] fn fetch_node_recipient_401_403_classify_as_auth() { for status in [401u16, 403] { diff --git a/crates/gdi-dataset-tool/src/wizard/setup.rs b/crates/gdi-dataset-tool/src/wizard/setup.rs index 55c51a1..8e8e9fd 100644 --- a/crates/gdi-dataset-tool/src/wizard/setup.rs +++ b/crates/gdi-dataset-tool/src/wizard/setup.rs @@ -746,14 +746,20 @@ fn terse_fetch_cause(url: &str, message: &str) -> String { .replace(&format!(" for url ({url})"), "") } -/// The warning for a failed recipient fetch. An error status other than `404` means the -/// node is up and failed; any other failure can be a node that isn't up yet. +/// The warning for a failed recipient fetch. No answer, a `404`, or an ingress's +/// 502/503/504 can be a node that isn't up yet; any other error status means the node is up +/// and failed. fn recipient_fetch_warning(url: &str, message: &str) -> String { + use reqwest::StatusCode; match recipient::answered_status(url, message) { - Some(status) if status != reqwest::StatusCode::NOT_FOUND => { - format!("warning: the node answered {status} at {url}") - } - _ => format!( + Some( + status @ (StatusCode::NOT_FOUND + | StatusCode::BAD_GATEWAY + | StatusCode::SERVICE_UNAVAILABLE + | StatusCode::GATEWAY_TIMEOUT), + ) => format!("warning: the node answered {status} at {url}: expected on a first bring-up"), + Some(status) => format!("warning: the node answered {status} at {url}"), + None => format!( "warning: node not reachable at {url}: expected on a first bring-up ({})", terse_fetch_cause(url, message) ), @@ -1297,6 +1303,25 @@ mod tests { } } + /// An ingress answers 502/503/504 while the node behind it is still starting. + #[test] + fn a_gateway_error_is_still_a_possible_first_bring_up() { + let url = "https://node.example/.well-known/c4gh-recipient"; + for status in [ + reqwest::StatusCode::BAD_GATEWAY, + reqwest::StatusCode::SERVICE_UNAVAILABLE, + reqwest::StatusCode::GATEWAY_TIMEOUT, + ] { + let warning = recipient_fetch_warning(url, &recipient::answered_message(url, status)); + assert_eq!( + warning, + format!( + "warning: the node answered {status} at {url}: expected on a first bring-up" + ) + ); + } + } + #[test] fn profile_complete_needs_some_way_to_reach_a_recipient() { let mut prof = Profile::default(); From 13a0746e6d76a2eaf05d2bfbaef7f80604aa079d Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:34 +0300 Subject: [PATCH 08/16] fix(tool): quote the chmod hint and fix the missing-s3 message The suggested chmod command broke on a path with a space, and the credentials error blamed an empty [profiles..s3] block when the block was missing altogether. --- crates/core/src/config/secrets.rs | 6 +++--- .../src/commands/cmd_doctor.rs | 4 ++-- .../gdi-dataset-tool/src/commands/cmd_keys.rs | 4 ++-- crates/gdi-dataset-tool/src/lib.rs | 21 +++++++++++++++++-- 4 files changed, 26 insertions(+), 9 deletions(-) diff --git a/crates/core/src/config/secrets.rs b/crates/core/src/config/secrets.rs index ce7e419..212e0bc 100644 --- a/crates/core/src/config/secrets.rs +++ b/crates/core/src/config/secrets.rs @@ -247,9 +247,9 @@ impl Secrets { for name in self.profiles.keys() { if config.profiles.get(name).is_some_and(|p| p.s3.is_none()) { return Err(format!( - "{}: credentials for profile '{name}', but [profiles.{name}.s3] in the tool \ - config sets nothing (commented-out keys don't count); add its settings or \ - remove the credentials", + "{}: credentials for profile '{name}', but the tool config has no \ + [profiles.{name}.s3] settings (the block is absent, or holds only \ + commented-out keys); add them or remove the credentials", path.display() )); } diff --git a/crates/gdi-dataset-tool/src/commands/cmd_doctor.rs b/crates/gdi-dataset-tool/src/commands/cmd_doctor.rs index 1ea1e83..0942dd8 100644 --- a/crates/gdi-dataset-tool/src/commands/cmd_doctor.rs +++ b/crates/gdi-dataset-tool/src/commands/cmd_doctor.rs @@ -284,9 +284,9 @@ fn check_provider_identity(config_path: Option<&Path>) -> Check { "provider-identity", format!( "loaded, but {} is readable beyond its owner (mode {mode:o}); file \ - permissions are its only protection; run `chmod 600 {}`", + permissions are its only protection; run `{}`", path.display(), - path.display() + crate::chmod_hint(&path) ), ), None => Check::pass("provider-identity", "loaded; recipient derives"), diff --git a/crates/gdi-dataset-tool/src/commands/cmd_keys.rs b/crates/gdi-dataset-tool/src/commands/cmd_keys.rs index 1e40e1e..7b0fc48 100644 --- a/crates/gdi-dataset-tool/src/commands/cmd_keys.rs +++ b/crates/gdi-dataset-tool/src/commands/cmd_keys.rs @@ -166,9 +166,9 @@ fn loose_permission_warning(path: &Path) -> Option { gdi_node_standalone_core::util::loose_secret_mode(path).map(|mode| { format!( "warning: the provider secret key {} is readable beyond its owner (mode {mode:o}); \ - file permissions are its only protection; run `chmod 600 {}`", + file permissions are its only protection; run `{}`", path.display(), - path.display() + crate::chmod_hint(path) ) }) } diff --git a/crates/gdi-dataset-tool/src/lib.rs b/crates/gdi-dataset-tool/src/lib.rs index c22940e..f0cefbb 100644 --- a/crates/gdi-dataset-tool/src/lib.rs +++ b/crates/gdi-dataset-tool/src/lib.rs @@ -206,6 +206,14 @@ mod vcf_stage_tests { /// package-format name is a core contract the node reads too, so it is defined once. pub use gdi_node_standalone_core::ingest::MANIFEST_FILE as MANIFEST_NAME; +/// The command that limits `path` to its owner, quoted for a POSIX shell. +pub(crate) fn chmod_hint(path: &std::path::Path) -> String { + format!( + "chmod 600 '{}'", + path.display().to_string().replace('\'', r"'\''") + ) +} + /// Run the parsed CLI, returning a [`ToolError`] (with its exit code) on failure. /// /// # Errors @@ -262,9 +270,9 @@ pub fn run(cli: Cli) -> Result<(), ToolError> { { output::warn(&format!( "warning: {} holds S3 credentials and is readable beyond its owner (mode {mode:o}); \ - run `chmod 600 {}`", + run `{}`", path.display(), - path.display() + chmod_hint(&path) )); } match cli.command { @@ -331,4 +339,13 @@ mod tests { assert_ne!(EXIT_TRANSIENT, 2); assert_ne!(EXIT_AUTH, 2); } + + /// The hint is pasted into a shell, so a space or a quote in the path must survive it. + #[test] + fn the_chmod_hint_quotes_its_path() { + assert_eq!( + chmod_hint(std::path::Path::new("/home/a b/it's.toml")), + r"chmod 600 '/home/a b/it'\''s.toml'" + ); + } } From abfdf35ffb248e490411174477c756be811023a0 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:34 +0300 Subject: [PATCH 09/16] test: pass every package field through the conformance check The agreement test dropped the age range, provenance and health theme on the way to the rendered record. It now destructures the package in full, so a new field can't be skipped, and the fixtures carry those fields. --- .../negative/bad-email-pattern.package.yaml | 5 ++ .../bad-enum-access-rights.package.yaml | 5 ++ .../negative/missing-description.package.yaml | 5 ++ ...ing-mandatory-health-category.package.yaml | 5 ++ .../unique-lang-collision.package.yaml | 5 ++ .../fixtures/valid/baseline.package.yaml | 5 ++ .../tests/conformance_agreement.rs | 71 +++++++++++++------ 7 files changed, 78 insertions(+), 23 deletions(-) diff --git a/conformance/fixtures/negative/bad-email-pattern.package.yaml b/conformance/fixtures/negative/bad-email-pattern.package.yaml index b033520..0080bf6 100644 --- a/conformance/fixtures/negative/bad-email-pattern.package.yaml +++ b/conformance/fixtures/negative/bad-email-pattern.package.yaml @@ -29,6 +29,11 @@ metadata: - "http://data.gdi.eu/core/p2/1MGCompliant" legalBasis: - "https://w3id.org/dpv#Consent" + minTypicalAge: 18 + maxTypicalAge: 90 + provenance: "Whole-genome sequencing of the cohort's participants, aggregated per population." + healthTheme: + - "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS" contactPoint: fn: "Data team" hasEmail: "data@example.org" diff --git a/conformance/fixtures/negative/bad-enum-access-rights.package.yaml b/conformance/fixtures/negative/bad-enum-access-rights.package.yaml index 2033b59..0ba789b 100644 --- a/conformance/fixtures/negative/bad-enum-access-rights.package.yaml +++ b/conformance/fixtures/negative/bad-enum-access-rights.package.yaml @@ -27,6 +27,11 @@ metadata: - "http://data.gdi.eu/core/p2/1MGCompliant" legalBasis: - "https://w3id.org/dpv#Consent" + minTypicalAge: 18 + maxTypicalAge: 90 + provenance: "Whole-genome sequencing of the cohort's participants, aggregated per population." + healthTheme: + - "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS" contactPoint: fn: "Data team" hasEmail: "mailto:data@example.org" diff --git a/conformance/fixtures/negative/missing-description.package.yaml b/conformance/fixtures/negative/missing-description.package.yaml index 1b46806..4faf91f 100644 --- a/conformance/fixtures/negative/missing-description.package.yaml +++ b/conformance/fixtures/negative/missing-description.package.yaml @@ -28,6 +28,11 @@ metadata: - "http://data.gdi.eu/core/p2/1MGCompliant" legalBasis: - "https://w3id.org/dpv#Consent" + minTypicalAge: 18 + maxTypicalAge: 90 + provenance: "Whole-genome sequencing of the cohort's participants, aggregated per population." + healthTheme: + - "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS" contactPoint: fn: "Data team" hasEmail: "mailto:data@example.org" diff --git a/conformance/fixtures/negative/missing-mandatory-health-category.package.yaml b/conformance/fixtures/negative/missing-mandatory-health-category.package.yaml index ce44e18..2abd187 100644 --- a/conformance/fixtures/negative/missing-mandatory-health-category.package.yaml +++ b/conformance/fixtures/negative/missing-mandatory-health-category.package.yaml @@ -29,6 +29,11 @@ metadata: - "http://data.gdi.eu/core/p2/1MGCompliant" legalBasis: - "https://w3id.org/dpv#Consent" + minTypicalAge: 18 + maxTypicalAge: 90 + provenance: "Whole-genome sequencing of the cohort's participants, aggregated per population." + healthTheme: + - "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS" contactPoint: fn: "Data team" hasEmail: "mailto:data@example.org" diff --git a/conformance/fixtures/negative/unique-lang-collision.package.yaml b/conformance/fixtures/negative/unique-lang-collision.package.yaml index 95edcae..6028c35 100644 --- a/conformance/fixtures/negative/unique-lang-collision.package.yaml +++ b/conformance/fixtures/negative/unique-lang-collision.package.yaml @@ -28,6 +28,11 @@ metadata: - "http://data.gdi.eu/core/p2/1MGCompliant" legalBasis: - "https://w3id.org/dpv#Consent" + minTypicalAge: 18 + maxTypicalAge: 90 + provenance: "Whole-genome sequencing of the cohort's participants, aggregated per population." + healthTheme: + - "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS" contactPoint: fn: "Data team" hasEmail: "mailto:data@example.org" diff --git a/conformance/fixtures/valid/baseline.package.yaml b/conformance/fixtures/valid/baseline.package.yaml index 7786750..8da8303 100644 --- a/conformance/fixtures/valid/baseline.package.yaml +++ b/conformance/fixtures/valid/baseline.package.yaml @@ -39,6 +39,11 @@ metadata: - "http://data.gdi.eu/core/p2/1MGCompliant" legalBasis: - "https://w3id.org/dpv#Consent" + minTypicalAge: 18 + maxTypicalAge: 90 + provenance: "Whole-genome sequencing of the cohort's participants, aggregated per population." + healthTheme: + - "https://hdeu-dcat.data.health.europa.eu/resource/authority/health-theme/HEALTH_PRODUCTS" contactPoint: fn: "Data team" hasEmail: "mailto:data@example.org" diff --git a/crates/gdi-node-standalone/tests/conformance_agreement.rs b/crates/gdi-node-standalone/tests/conformance_agreement.rs index 876d19d..5a00203 100644 --- a/crates/gdi-node-standalone/tests/conformance_agreement.rs +++ b/crates/gdi-node-standalone/tests/conformance_agreement.rs @@ -34,7 +34,7 @@ use gdi_node_standalone_core::config::{ ContactPointCfg, FairdpAccessUrls, FairdpAgent, FairdpConfig, FairdpDistribution, FairdpPublish, }; use gdi_node_standalone_core::model::{ - Assembly, DatasetMode, ManifestConfig, ManifestMetadata, PackageYaml, + Assembly, DatasetMode, ManifestConfig, ManifestMetadata, PackageMetadata, PackageYaml, }; use gdi_node_standalone_core::state::DatasetState; use gdi_node_standalone_core::validate_pkg::{validate_overlay_result, validate_package}; @@ -147,32 +147,57 @@ impl Fixture { /// sides from drifting: pySHACL only ever sees this side, so a field the YAML carries but /// a hand-written struct omitted would go unexercised. fn metadata(self) -> ManifestMetadata { - let m = self.package_yaml().metadata; - ManifestMetadata { + // Destructured in full: a field added to the package model fails to compile here, and + // one dropped on the way to the record is an unused binding. + let PackageMetadata { // `package.yaml` carries `prefix` and `org`; the node mints `datasetId` at build // time. This test renders rather than ingests, so pin the constant the FDP context // is configured with. + prefix: _, + org: _, + catalog, + title, + description, + access_rights, + applicable_legislation, + license, + creator, + health_category, + keywords, + number_of_unique_individuals, + conforms_to, + type_, + legal_basis, + is_referenced_by, + other_identifier, + contact_point, + min_typical_age, + max_typical_age, + provenance, + health_theme, + } = self.package_yaml().metadata; + ManifestMetadata { dataset_id: DATASET_ID.to_owned(), - catalog: m.catalog, - title: m.title, - description: m.description, - access_rights: m.access_rights, - applicable_legislation: m.applicable_legislation, - license: m.license, - creator: m.creator, - health_category: m.health_category, - keywords: m.keywords, - number_of_unique_individuals: m.number_of_unique_individuals, - conforms_to: m.conforms_to, - type_: m.type_, - legal_basis: m.legal_basis, - is_referenced_by: m.is_referenced_by, - other_identifier: m.other_identifier, - min_typical_age: None, - max_typical_age: None, - provenance: None, - health_theme: None, - contact_point: m.contact_point, + catalog, + title, + description, + access_rights, + applicable_legislation, + license, + creator, + health_category, + keywords, + number_of_unique_individuals, + conforms_to, + type_, + legal_basis, + is_referenced_by, + other_identifier, + min_typical_age, + max_typical_age, + provenance, + health_theme, + contact_point, // Not authored in `package.yaml`: the node computes both during ingest. This test // renders a record instead, so a synthetic count is right (any nonNegativeInteger // validates) and there are no per-population rows to derive. From 3d5be4a7838faa2761252736af505b3d7e09b27d Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:34 +0300 Subject: [PATCH 10/16] test: keep fault keys and shared fixtures honest Two fault tests shared dataset ids with tests that run beside them under cargo test, and the serial-key guard missed arm_panic and arm_io; it now reads the arming functions from faults.rs. The shared stored manifest is valid again, an s3 assertion that could never fail now can, and overlay_change is checked field by field. --- crates/core/src/faults.rs | 43 +++++++++------- crates/core/src/overlay_store.rs | 51 +++++++++++++++++++ crates/core/tests/it/model_roundtrip.rs | 9 ++++ crates/gdi-node-standalone/src/s3.rs | 6 ++- .../tests/it/fault_injection.rs | 6 +-- crates/test-util/src/lib.rs | 24 +++++++-- 6 files changed, 114 insertions(+), 25 deletions(-) diff --git a/crates/core/src/faults.rs b/crates/core/src/faults.rs index 1adc6ac..9df3763 100644 --- a/crates/core/src/faults.rs +++ b/crates/core/src/faults.rs @@ -26,12 +26,11 @@ //! runs tests in parallel, so an unscoped fault would fire on a concurrent test's //! ingest. Every arm therefore carries a key substring: [`guard`] fires only //! when the call-site key (the dataset id, or the durable-write path) contains it, -//! and a non-matching call passes through without consuming a fire. Tests use a -//! unique id so their fault can never contaminate a sibling, and a -//! [`FaultPoint::DurableWrite`] key must be an absolute path into the test's own temp -//! dir: arming refuses a bare name like `.pub`, which every concurrent test has. Arming still -//! serializes (`#[serial(faults)]`) and returns a `FaultGuard` whose `Drop` -//! disarms the point, so a fault never leaks past the test that set it. +//! and a non-matching call passes through without consuming a fire. Key a dataset fault +//! on an id no other test in the binary ingests: `#[serial(faults)]` orders only the tests +//! that arm. Key a [`FaultPoint::DurableWrite`] fault on an absolute path in the test's own +//! temp dir; arming refuses a bare name like `.pub`, which every test has. Arming returns a +//! `FaultGuard` whose `Drop` disarms the point, so a fault never outlives its test. //! //! `FaultGuard` is spelled as plain code rather than an intra-doc link. It lives in the //! `fault-injection`-gated backend, so a link would fail a `-D warnings` doc build of this @@ -304,6 +303,14 @@ mod tests { ); } + /// A `DurableWrite` key that is absolute on Windows too, unlike `/probe`. + fn probe() -> String { + std::env::temp_dir() + .join("probe") + .to_string_lossy() + .into_owned() + } + #[test] #[serial(faults)] fn unarmed_guard_is_ok() { @@ -313,11 +320,11 @@ mod tests { #[test] #[serial(faults)] fn arm_enospc_fires_once_then_clears() { - let _g = arm_enospc(FaultPoint::DurableWrite, "/probe", 1); - let err = guard(FaultPoint::DurableWrite, "/probe").unwrap_err(); + let _g = arm_enospc(FaultPoint::DurableWrite, &probe(), 1); + let err = guard(FaultPoint::DurableWrite, &probe()).unwrap_err(); assert_eq!(err.kind(), ErrorKind::StorageFull); // Fired once (times == 1); the point is now disarmed. - assert!(guard(FaultPoint::DurableWrite, "/probe").is_ok()); + assert!(guard(FaultPoint::DurableWrite, &probe()).is_ok()); } #[test] @@ -337,10 +344,10 @@ mod tests { #[test] #[serial(faults)] fn arm_enospc_fires_the_requested_number_of_times() { - let _g = arm_enospc(FaultPoint::DurableWrite, "/probe", 2); - assert!(guard(FaultPoint::DurableWrite, "/probe").is_err()); - assert!(guard(FaultPoint::DurableWrite, "/probe").is_err()); - assert!(guard(FaultPoint::DurableWrite, "/probe").is_ok()); + let _g = arm_enospc(FaultPoint::DurableWrite, &probe(), 2); + assert!(guard(FaultPoint::DurableWrite, &probe()).is_err()); + assert!(guard(FaultPoint::DurableWrite, &probe()).is_err()); + assert!(guard(FaultPoint::DurableWrite, &probe()).is_ok()); } #[test] @@ -379,10 +386,10 @@ mod tests { #[serial(faults)] fn guard_dropping_disarms() { { - let _g = arm_enospc(FaultPoint::DurableWrite, "/probe", 5); + let _g = arm_enospc(FaultPoint::DurableWrite, &probe(), 5); } // dropped here, well before the 5 fires are consumed assert!( - guard(FaultPoint::DurableWrite, "/probe").is_ok(), + guard(FaultPoint::DurableWrite, &probe()).is_ok(), "dropping the guard disarms the point" ); } @@ -390,11 +397,11 @@ mod tests { #[test] #[serial(faults)] fn faults_are_keyed_by_point() { - let _g = arm_enospc(FaultPoint::DurableWrite, "/probe", 1); + let _g = arm_enospc(FaultPoint::DurableWrite, &probe(), 1); // A different point is unaffected by the arming. - assert!(guard(FaultPoint::IngestStore, "/probe").is_ok()); + assert!(guard(FaultPoint::IngestStore, &probe()).is_ok()); // The armed point still fires. - assert!(guard(FaultPoint::DurableWrite, "/probe").is_err()); + assert!(guard(FaultPoint::DurableWrite, &probe()).is_err()); } #[test] diff --git a/crates/core/src/overlay_store.rs b/crates/core/src/overlay_store.rs index 2692477..e2ddfac 100644 --- a/crates/core/src/overlay_store.rs +++ b/crates/core/src/overlay_store.rs @@ -425,6 +425,57 @@ mod tests { assert_eq!(change.access_rights_after.as_deref(), Some("PUBLIC")); } + /// Each field is named when it alone changes, or its corrections would go unaudited. + #[test] + fn overlay_change_names_every_field_that_moves() { + use serde_json::json; + let iris = json!(["https://example.org/x"]); + for (key, value, name) in [ + ("title", json!("T"), "title"), + ("description", json!("D"), "description"), + ("accessRights", json!("x"), "access_rights"), + ( + "applicableLegislation", + iris.clone(), + "applicable_legislation", + ), + ("license", json!("https://example.org/l"), "license"), + ("creator", json!([{"name": "C"}]), "creator"), + ("healthCategory", iris.clone(), "health_category"), + ("keywords", json!(["k"]), "keywords"), + ( + "numberOfUniqueIndividuals", + json!(1), + "number_of_unique_individuals", + ), + ("conformsTo", iris.clone(), "conforms_to"), + ("type", iris.clone(), "type"), + ("legalBasis", iris.clone(), "legal_basis"), + ("isReferencedBy", iris.clone(), "is_referenced_by"), + ( + "otherIdentifier", + json!([{"notation": "n"}]), + "other_identifier", + ), + ( + "contactPoint", + json!({"fn": "F", "hasEmail": "mailto:a@example.org"}), + "contact_point", + ), + ("minTypicalAge", json!(1), "min_typical_age"), + ("maxTypicalAge", json!(2), "max_typical_age"), + ("provenance", json!("P"), "provenance"), + ("healthTheme", iris, "health_theme"), + ] { + let next: MetadataOverlay = serde_json::from_value(json!({ key: value })).unwrap(); + assert_eq!( + overlay_change(None, &next).changed_fields, + vec![name], + "{key}" + ); + } + } + /// A re-apply of the identical patch must not look like a correction, or the /// idempotent reconcile would flood the audit trail with no-op "changed" lines. #[test] diff --git a/crates/core/tests/it/model_roundtrip.rs b/crates/core/tests/it/model_roundtrip.rs index 2c9fb19..236d0ce 100644 --- a/crates/core/tests/it/model_roundtrip.rs +++ b/crates/core/tests/it/model_roundtrip.rs @@ -283,3 +283,12 @@ fn no_published_schema_description_names_a_rust_item() { } assert!(checked > 0, "no descriptions were examined"); } + +/// The shared test manifest passes the node's validation, not only its parser. +#[test] +fn the_shared_stored_manifest_is_valid() { + let raw = test_util::stored_manifest_json("GDI-EE-TEST-20260101000000000"); + let manifest: Manifest = serde_json::from_str(&raw).expect("the shared manifest parses"); + gdi_node_standalone_core::validate_pkg::validate_overlay_result(&manifest.metadata) + .expect("the shared manifest is valid"); +} diff --git a/crates/gdi-node-standalone/src/s3.rs b/crates/gdi-node-standalone/src/s3.rs index e87f5f6..24acf72 100644 --- a/crates/gdi-node-standalone/src/s3.rs +++ b/crates/gdi-node-standalone/src/s3.rs @@ -2918,7 +2918,11 @@ mod tests { title: gdi_node_standalone_core::model::LocalizedText::Plain( "Baseline title".to_owned(), ), - description: None, + // Valid as a whole, so a wrongly applied sidecar would change the served title + // rather than fail validation. + description: Some(gdi_node_standalone_core::model::LocalizedText::Plain( + "Baseline description".to_owned(), + )), access_rights: "http://publications.europa.eu/resource/authority/access-right/PUBLIC" .to_owned(), diff --git a/crates/gdi-node-standalone/tests/it/fault_injection.rs b/crates/gdi-node-standalone/tests/it/fault_injection.rs index c12c98a..1ec5fdc 100644 --- a/crates/gdi-node-standalone/tests/it/fault_injection.rs +++ b/crates/gdi-node-standalone/tests/it/fault_injection.rs @@ -327,7 +327,7 @@ async fn restart_after_crash_leftovers_rehydrates_published_datasets() { #[tokio::test] #[serial(faults)] async fn crash_after_publish_rename_recovers_on_restart() { - const ID: &str = "GDI-EE-UTARTU-20260706120000010"; + const ID: &str = "GDI-EE-UTARTU-20260706120000110"; let tmp = tempfile::tempdir().unwrap(); let data_dir = tmp.path().join("data"); let inbox = tmp.path().join("inbox"); @@ -400,7 +400,7 @@ async fn crash_after_publish_rename_recovers_on_restart() { #[tokio::test] #[serial(faults)] async fn crash_after_status_purge_completes_erasure_on_restart() { - const ID: &str = "GDI-EE-UTARTU-20260706120000011"; + const ID: &str = "GDI-EE-UTARTU-20260706120000111"; let tmp = tempfile::tempdir().unwrap(); let data_dir = tmp.path().join("data"); let inbox = tmp.path().join("inbox"); @@ -670,7 +670,7 @@ name = "Test Beacon" #[tokio::test] #[serial(faults)] async fn scrub_sweep_skips_a_dataset_whose_erasure_is_in_flight() { - const ID: &str = "GDI-EE-UTARTU-20260706120000012"; + const ID: &str = "GDI-EE-UTARTU-20260706120000112"; let tmp = tempfile::tempdir().unwrap(); let data_dir = tmp.path().join("data"); let inbox = tmp.path().join("inbox"); diff --git a/crates/test-util/src/lib.rs b/crates/test-util/src/lib.rs index d69de46..5a24310 100644 --- a/crates/test-util/src/lib.rs +++ b/crates/test-util/src/lib.rs @@ -525,6 +525,7 @@ pub fn stored_manifest_json(dataset_id: &str) -> String { "license": "https://creativecommons.org/licenses/by/4.0/", "creator": [{{ "name": "test" }}], "healthCategory": ["http://data.gdi.eu/core/p2/HealthCategoryHumanGenomic"], + "contactPoint": {{ "fn": "test", "hasEmail": "mailto:test@example.org" }}, "numberOfRecords": 1 }}, "config": {{ @@ -678,8 +679,24 @@ mod tests { /// `test_util::set_env(` cannot silently drop those sites. const ENV_MUTATORS: [&str; 4] = ["set_env(", "remove_env(", "EnvGuard::", "jail.set_env("]; - /// Every spelling that arms a `core::faults` fault point. - const FAULT_ARMERS: [&str; 4] = ["arm_once(", "arm_delay(", "arm_enospc(", "faults::arm"]; + /// The calls that arm a `core::faults` point: every `pub fn arm_*` in its source, so a + /// new one is covered without a list to update, and the path-qualified `faults::arm`. + fn fault_armers() -> Vec { + let src = std::fs::read_to_string(crates_dir().join("core/src/faults.rs")) + .expect("core/src/faults.rs is readable"); + let mut armers: Vec = src + .lines() + .filter_map(|l| l.trim_start().strip_prefix("pub fn arm_")) + .filter_map(|rest| rest.split_once('(')) + .map(|(name, _)| format!("arm_{name}(")) + .collect(); + assert!( + armers.len() >= 4, + "found only {armers:?} in core/src/faults.rs" + ); + armers.push("faults::arm".to_owned()); + armers + } /// Binding names that mean "I am saving this to put it back". /// @@ -904,6 +921,7 @@ mod tests { let mut files = Vec::new(); rs_files(&crates_dir(), &mut files); assert!(files.len() > 50, "found only {} .rs files", files.len()); + let fault_armers = fault_armers(); let mut offenders = Vec::new(); for path in &files { @@ -943,7 +961,7 @@ mod tests { if ENV_MUTATORS.iter().any(|t| body.contains(t)) { needed.push("env"); } - if FAULT_ARMERS.iter().any(|t| body.contains(t)) { + if fault_armers.iter().any(|t| body.contains(t.as_str())) { needed.push("faults"); } for group in needed { From 0f1bd378c96b8291a55309ec2af61646c164ab63 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:34 +0300 Subject: [PATCH 11/16] test(scripts): fix the source stripper and the wait_log scan The source stripper miscounted braces in '{' and /* } */, and the run-full needle guard missed a wait_log after until, while, elif, {, time or an env prefix. --- scripts/tests/_helpers.py | 30 +++++++++++++++++++++--- scripts/tests/test_run_full_needles.py | 26 +++++++++++++++++--- scripts/tests/test_strip_test_modules.py | 17 ++++++++++++++ 3 files changed, 67 insertions(+), 6 deletions(-) diff --git a/scripts/tests/_helpers.py b/scripts/tests/_helpers.py index 4659899..2a1e1c1 100644 --- a/scripts/tests/_helpers.py +++ b/scripts/tests/_helpers.py @@ -127,9 +127,9 @@ def _starts_raw_string(src: str, i: int) -> bool: def _block_end(src: str, open_brace: int) -> int: """The index just past the `}` that closes the block whose `{` is at `open_brace`, - skipping braces inside string literals, raw strings and `//` comments (a test module is - full of them: `"{}"` format strings, TOML fixtures in `r#"…"#` that hold `https://`, - `// { not a brace`).""" + skipping braces inside string literals, raw strings, char literals and comments (a test + module is full of them: `"{}"` format strings, TOML fixtures in `r#"…"#` that hold + `https://`, `'{'` and `'"'`, `// { not a brace`, `/* } */`).""" depth = 0 i = open_brace n = len(src) @@ -138,6 +138,30 @@ def _block_end(src: str, open_brace: int) -> int: if c == "r" and _starts_raw_string(src, i): i = _RAW_STRING.match(src, i).end() continue + if src.startswith("/*", i): + # Block comments nest in Rust. + nested = 0 + while i < n: + if src.startswith("/*", i): + nested += 1 + i += 2 + elif src.startswith("*/", i): + nested -= 1 + i += 2 + if nested == 0: + break + else: + i += 1 + continue + if c == "'": + # A char literal (`'{'`, `'\''`, `'\u{7d}'`); otherwise a lifetime or label. + if src.startswith("\\", i + 1): + close = src.find("'", i + 3) + i = close + 1 if close >= 0 else n + continue + if i + 2 < n and src[i + 2] == "'": + i += 3 + continue if c == '"': i += 1 while i < n and src[i] != '"': diff --git a/scripts/tests/test_run_full_needles.py b/scripts/tests/test_run_full_needles.py index ccb653f..e1409e2 100755 --- a/scripts/tests/test_run_full_needles.py +++ b/scripts/tests/test_run_full_needles.py @@ -31,9 +31,13 @@ RUN_FULL = SCRIPTS / "e2e" / "run-full.sh" -#: Where a command can start: a line, or after `;`, `&&`, `||`, `!`, `(`, `if`, `then`, -#: `do` or `else`, so a call like `sighup && wait_log …` is read too. -_COMMAND_START = r"(?:^|[;&|!(]|\b(?:if|then|do|else)\b)\s*" +#: Where a command can start: a line, or after `;`, `&&`, `||`, `!`, `(`, `{`, a keyword +#: that takes a command (`if`, `then`, `do`, `else`, `elif`, `until`, `while`, `time`), and +#: any `NAME=value` prefixes, so a call like `sighup && wait_log …` is read too. +_COMMAND_START = ( + r"(?:^|[;&|!({]|\b(?:if|then|do|else|elif|until|while|time)\b)\s*" + r"(?:[A-Za-z_][A-Za-z0-9_]*=\S*\s+)*" +) #: A `wait_log` call and its first argument, a double- or single-quoted word. WAIT_LOG_CALL = re.compile( _COMMAND_START + r"""wait_log\s+("[^"]*"|'[^']*')""", re.MULTILINE @@ -149,6 +153,22 @@ def test_every_wait_log_call_yields_a_needle(self): ) self.assertIn("PME active", self.needles) + def test_a_call_in_any_command_position_is_counted_and_read(self): + script = ( + "until wait_log 'a' x; do :; done\n" + "while wait_log 'b' x; do :; done\n" + "if false; then :; elif wait_log 'c' x; then :; fi\n" + "{ wait_log 'd' x; }\n" + "time wait_log 'e' x\n" + "LOG_FORMAT=json wait_log 'f' x\n" + ) + self.assertEqual(len(ANY_WAIT_LOG_CALL.findall(script)), 6) + self.assertEqual(needles(script), ["a", "b", "c", "d", "e", "f"]) + # Still not a call: the definition, and the words in a trailing comment. + self.assertEqual( + ANY_WAIT_LOG_CALL.findall("wait_log() { # wait_log \n"), [] + ) + def test_every_needle_is_emitted_by_a_tracing_event(self): self.assertGreater( len(self.invocations), diff --git a/scripts/tests/test_strip_test_modules.py b/scripts/tests/test_strip_test_modules.py index 2f539ae..0556e5c 100755 --- a/scripts/tests/test_strip_test_modules.py +++ b/scripts/tests/test_strip_test_modules.py @@ -51,6 +51,23 @@ def test_a_raw_string_holding_a_url_does_not_end_the_module_early(self): self.assertIn("fn tail()", kept) self.assertNotIn("mod tests", kept) + def test_a_brace_or_quote_in_a_char_literal_or_block_comment_does_not_count(self): + src = ( + "fn a() {}\n" + "#[cfg(test)]\n" + "mod tests {\n" + " fn t<'a>(s: &'a str) -> bool { s.starts_with('{') || s.ends_with('\"') }\n" + " /* a } in a block comment /* nested } */ still } */\n" + " const Q: char = '\\'';\n" + " fn u() { let _ = '}'; }\n" + "}\n" + "fn tail() {}\n" + ) + kept = strip_test_modules(src) + self.assertIn("fn tail()", kept) + self.assertNotIn("fn u()", kept) + self.assertNotIn("mod tests", kept) + def test_a_cfg_test_on_a_non_module_item_is_left_alone(self): src = '#[cfg(test)]\nconst FIXTURE: &str = "x";\nfn a() {}\n' self.assertEqual(src, strip_test_modules(src)) From a382749196bb59552d1c6d2c21e5ad6b499a4a86 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:34 +0300 Subject: [PATCH 12/16] fix(harness): find release binaries under CARGO_TARGET_DIR With CARGO_TARGET_DIR set, cargo built elsewhere and the harness ran the stale binary in target/release. --- scripts/lib/release-bins.sh | 7 ++++--- scripts/tests/test_release_bins.py | 9 ++++++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/scripts/lib/release-bins.sh b/scripts/lib/release-bins.sh index 6c2c39a..da49a22 100644 --- a/scripts/lib/release-bins.sh +++ b/scripts/lib/release-bins.sh @@ -4,12 +4,13 @@ # release_bins: set NODE and TOOL, building the release binaries first unless both are # given. Don't skip cargo just because the binaries exist: it is a no-op on a fresh # build, and otherwise an old binary in target/release gets tested instead of the -# current code. +# current code. Look for them where cargo puts them: $CARGO_TARGET_DIR, else $ROOT/target. release_bins() { if [ -z "${NODE:-}" ] || [ -z "${TOOL:-}" ]; then echo "==> building release binaries" cargo build --release --locked --bins -p gdi-node-standalone -p gdi-dataset-tool fi - TOOL="${TOOL:-$ROOT/target/release/gdi-dataset-tool}" - NODE="${NODE:-$ROOT/target/release/gdi-node-standalone}" + local target="${CARGO_TARGET_DIR:-$ROOT/target}" + TOOL="${TOOL:-$target/release/gdi-dataset-tool}" + NODE="${NODE:-$target/release/gdi-node-standalone}" } diff --git a/scripts/tests/test_release_bins.py b/scripts/tests/test_release_bins.py index b3f0764..04a3268 100755 --- a/scripts/tests/test_release_bins.py +++ b/scripts/tests/test_release_bins.py @@ -49,7 +49,7 @@ def run_release_bins(self, **given) -> dict: env = { k: v for k, v in os.environ.items() - if k not in ("NODE", "TOOL", "CARGO_LOG") + if k not in ("NODE", "TOOL", "CARGO_LOG", "CARGO_TARGET_DIR") } env.update(PATH=self.path, CARGO_LOG=str(self.cargo_log), **given) script = ( @@ -90,6 +90,13 @@ def test_one_binary_given_still_builds_the_other(self): self.assertEqual(out["NODE"], "/opt/node") self.assertEqual(out["TOOL"], f"{self.root}/target/release/gdi-dataset-tool") + def test_a_target_dir_from_the_environment_is_where_it_looks(self): + # cargo builds into CARGO_TARGET_DIR; reading $ROOT/target would run a stale binary. + elsewhere = self.root.parent / "elsewhere" + out = self.run_release_bins(CARGO_TARGET_DIR=str(elsewhere)) + self.assertEqual(out["NODE"], f"{elsewhere}/release/gdi-node-standalone") + self.assertEqual(out["TOOL"], f"{elsewhere}/release/gdi-dataset-tool") + def test_every_harness_calls_it(self): for path in HARNESSES: with self.subTest(script=path.name): From 20daaafe8e4e2c0e67d9f868b88492ec3eaef854 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:34 +0300 Subject: [PATCH 13/16] fix(chaos): refuse to start when a host port is taken Like the e2e scripts, the chaos harness now checks the ports its stack publishes before compose up, reading them from the rendered config. --- scripts/chaos/run.sh | 39 +++++++++++++++++++ .../test_compose_host_ports_overridable.py | 15 ++++--- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/scripts/chaos/run.sh b/scripts/chaos/run.sh index 5d920f1..c7a5424 100755 --- a/scripts/chaos/run.sh +++ b/scripts/chaos/run.sh @@ -51,6 +51,45 @@ export VAULT_ADDR="http://toxiproxy:28200" FILES=(-f docker-compose.yml -f docker-compose.chaos.yml) dc() { $COMPOSE -p "$PROJECT" "${FILES[@]}" "$@"; } +# Preflight: refuse to start beside a stack that holds one of our host ports, which +# `compose up` does not check. The ports come from the rendered config, so a remapped +# GDI_HOST_PORT_* is what gets checked. It runs before the cleanup trap, and probes as +# scripts/e2e/run.sh does: one `ss` call, since a `| grep -q` pipeline fails open. +command -v docker >/dev/null 2>&1 || { echo "FAIL: docker not found on PATH" >&2; exit 2; } +command -v ss >/dev/null 2>&1 \ + || { echo "FAIL: ss not found on PATH: install iproute2, needed to check for busy host ports" >&2; exit 2; } +E2E_PORTS=() +while IFS= read -r p; do E2E_PORTS+=("$p"); done < <( + dc config --format json | python3 -c ' +import json, sys +for service in json.load(sys.stdin)["services"].values(): + for port in service.get("ports", []): + if port.get("published"): + print(port["published"])' +) +[ "${#E2E_PORTS[@]}" -gt 0 ] \ + || { echo "FAIL: the rendered compose config publishes no host port, so the preflight would check nothing" >&2; exit 2; } +e2e_port_holder() { # e2e_port_holder -> holding container name(s), or empty + docker ps --format '{{.Names}} {{.Ports}}' 2>/dev/null \ + | awk -v p=":$1->" '$0 ~ p {print $1}' | tr '\n' ' ' +} +listening="$(ss -ltn)" \ + || { echo "FAIL: ss -ltn failed, so whether the chaos ports are free is unknown" >&2; exit 2; } +busy=() +for p in "${E2E_PORTS[@]}"; do + if awk -v p=":${p}" '$4 ~ p"$" {found=1} END {exit !found}' <<<"$listening"; then + busy+=("$p") + fi +done +if [ "${#busy[@]}" -gt 0 ]; then + for p in "${busy[@]}"; do + holder="$(e2e_port_holder "$p")" + echo " port :$p is already bound${holder:+ (holder: $holder)}" + done + echo "FAIL: cannot start: host port(s) ${busy[*]} already bound; stop the stack holding them and re-run" >&2 + exit 1 +fi + # Node logs from the teardown go to a freshly created private file, not a fixed `/tmp` # path: a predictable name in a world-writable directory can be pre-created or symlinked # by any local user, who then captures the node log or redirects the write. `mktemp` diff --git a/scripts/tests/test_compose_host_ports_overridable.py b/scripts/tests/test_compose_host_ports_overridable.py index b0b4a7e..df1fc50 100755 --- a/scripts/tests/test_compose_host_ports_overridable.py +++ b/scripts/tests/test_compose_host_ports_overridable.py @@ -114,12 +114,15 @@ def test_every_ports_entry_derives_its_host_port_from_a_variable(self): "no ports: entries were checked at all, so this test would pass vacuously", ) - def test_every_e2e_script_preflights_the_host_ports_it_is_about_to_bind(self): - # The compose files parameterise their host ports, but each `scripts/e2e/run*.sh` - # is what must refuse to start when one is already held; otherwise a passing e2e - # may be talking to a developer's dev stack. The scripts are enumerated rather - # than listed, so one added later is covered the day it lands. - scripts = sorted((REPO_ROOT / "scripts" / "e2e").glob("run*.sh")) + def test_every_stack_script_preflights_the_host_ports_it_is_about_to_bind(self): + # Each script that brings a stack up (`scripts/e2e/run*.sh`, the chaos harness) must + # refuse to start when one of its ports is held, or a passing run may be talking to + # a developer's dev stack. The e2e scripts are globbed, so a new one is covered the + # day it lands. + scripts = [ + *sorted((REPO_ROOT / "scripts" / "e2e").glob("run*.sh")), + REPO_ROOT / "scripts" / "chaos" / "run.sh", + ] self.assertTrue( scripts, "no scripts/e2e/run*.sh found, so this would pass vacuously" ) From b926f0e8b456c82b8c8dd6ccf4f34795ebaa57f7 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:34 +0300 Subject: [PATCH 14/16] ci: keep the developer's tool config out of every test run GDI_TOOL__* overrides are now dropped along with the config dir, and coverage and mutants run isolated too. The fuzz runs also set a per-input timeout: libFuzzer's default of 1,200 s never fires in a 60 s run. --- .github/workflows/scheduled.yml | 4 +- scripts/ci-local.sh | 25 +++++--- scripts/tests/test_user_config_isolation.py | 71 +++++++++++++++++++++ 3 files changed, 90 insertions(+), 10 deletions(-) create mode 100755 scripts/tests/test_user_config_isolation.py diff --git a/.github/workflows/scheduled.yml b/.github/workflows/scheduled.yml index b9949a3..6d091c0 100644 --- a/.github/workflows/scheduled.yml +++ b/.github/workflows/scheduled.yml @@ -44,6 +44,8 @@ jobs: # # No `-fork`: in fork mode libFuzzer counts an out-of-memory input or a timeout and keeps # going, so the run stays green, and those are the inputs this job exists to find. + # `-timeout` is set because libFuzzer's default (1,200 s per input) never fires in a 60 s + # run; `-rss_limit_mb` states its default. fuzz: runs-on: ubuntu-26.04 # Every target × a 60 s bounded run + `cmin`, plus a from-source cargo-fuzz install and @@ -96,7 +98,7 @@ jobs: echo "fuzzing ${#targets[@]} targets: ${targets[*]}" for target in "${targets[@]}"; do echo "::group::fuzz $target" - cargo +nightly fuzz run "$target" -- -max_total_time=60 + cargo +nightly fuzz run "$target" -- -max_total_time=60 -timeout=20 -rss_limit_mb=2048 # Minimize the (possibly grown) corpus in place so the checked-in seed # set stays small and non-redundant for the next run. cargo +nightly fuzz cmin "$target" diff --git a/scripts/ci-local.sh b/scripts/ci-local.sh index 69686c6..fc3d5d8 100755 --- a/scripts/ci-local.sh +++ b/scripts/ci-local.sh @@ -402,15 +402,19 @@ need() { have_nextest() { cargo nextest --version >/dev/null 2>&1; } # user_config_isolation — sets USER_CONFIG_ISOLATION, an `env` prefix that points the -# default config dir at an unparsable tool.toml. Anything that reads the developer's own -# ~/.config/gdi/tool.toml then fails everywhere, not just on machines that have one. -# Tests that need a config pass a path or set GDI_CONFIG_DIR themselves. +# default config dir at an unparsable tool.toml and drops exported GDI_TOOL__* overrides. +# A test that reads the developer's own config then fails on every machine, not only on +# those that have one. Tests that need a config pass a path or set GDI_CONFIG_DIR themselves. user_config_isolation() { - local dir="$PWD/target/test-user-config" + local dir="$PWD/target/test-user-config" name mkdir -p "$dir/gdi" printf '%s\n' '# Deliberately unparsable: see user_config_isolation in scripts/ci-local.sh.' \ 'not = [toml' >"$dir/gdi/tool.toml" - USER_CONFIG_ISOLATION=(env -u GDI_CONFIG_DIR "XDG_CONFIG_HOME=$dir") + USER_CONFIG_ISOLATION=(env -u GDI_CONFIG_DIR) + for name in $(compgen -e); do + [[ "$name" == GDI_TOOL__* ]] && USER_CONFIG_ISOLATION+=(-u "$name") + done + USER_CONFIG_ISOLATION+=("XDG_CONFIG_HOME=$dir") } # run_tests [extra cargo args...] @@ -1940,7 +1944,8 @@ reuse_lint() { # `crates/core/tests/fixtures/malformed/`, as docs/testing.md describes. # # Needs nightly (cargo-fuzz uses `-Z sanitizer`). `FUZZ_SECONDS` overrides the per-target -# budget for a deeper run. +# budget for a deeper run. Per-input limits as in the scheduled workflow: libFuzzer's default +# timeout (1,200 s) would never fire. fuzz_short() { step "fuzz-short — time-boxed run of every target (${FUZZ_SECONDS:-10}s each)" need cargo-fuzz "cargo install cargo-fuzz --locked" @@ -1952,7 +1957,7 @@ fuzz_short() { local t for t in "${targets[@]}"; do run cargo +nightly fuzz run --fuzz-dir crates/core/fuzz "$t" -- \ - -max_total_time="$secs" -print_final_stats=1 + -max_total_time="$secs" -timeout=20 -rss_limit_mb=2048 -print_final_stats=1 done } @@ -1964,7 +1969,8 @@ mutants() { # No `"$@"`: the dispatcher loops over a target list, so there is no per-target argument # tail any leg could forward — load/soak/chaos/crash-loop/e2e-observability all call their # script bare. Pass flags by invoking ./scripts/mutants-audit.sh directly. - run ./scripts/mutants-audit.sh + user_config_isolation + run "${USER_CONFIG_ISOLATION[@]}" ./scripts/mutants-audit.sh } lint_docker() { dockerfile_check; actionlint; promtool; shellcheck_lint; compose_config; } @@ -2554,7 +2560,8 @@ coverage() { # One instrumented run, two reports. `--no-report` collects the profile data, and the two # `report` invocations below re-render it without re-running the suite. Rendering each # view from scratch would pay the whole instrumented test cost twice for identical data. - run cargo llvm-cov --workspace --features full --locked --no-report -- --test-threads=1 + user_config_isolation + run "${USER_CONFIG_ISOLATION[@]}" cargo llvm-cov --workspace --features full --locked --no-report -- --test-threads=1 run cargo llvm-cov report --summary-only # The one part of coverage that is not advisory. A percentage floor is gameable and diff --git a/scripts/tests/test_user_config_isolation.py b/scripts/tests/test_user_config_isolation.py new file mode 100755 index 0000000..76be1fe --- /dev/null +++ b/scripts/tests/test_user_config_isolation.py @@ -0,0 +1,71 @@ +#!/usr/bin/env python3 +"""Guard: the gate's tests never read the developer's own tool config. + +``ToolConfig::load`` reads the config directory (``GDI_CONFIG_DIR``, else +``$XDG_CONFIG_HOME/gdi``) and merges ``GDI_TOOL__*`` overrides, which the README tells users +to export. ``user_config_isolation`` in ``ci-local.sh`` builds the ``env`` prefix that hides +both from the test legs; this runs the shipped function. +""" + +from __future__ import annotations + +import os +import re +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) +from _helpers import SCRIPTS + +CI_LOCAL = SCRIPTS / "ci-local.sh" +PREFIX = '"${USER_CONFIG_ISOLATION[@]}"' + + +def _extract(name: str) -> str: + """The shipped text of one shell function, from ``ci-local.sh``.""" + src = CI_LOCAL.read_text(encoding="utf-8") + m = re.search(rf"^{re.escape(name)}\(\) \{{.*?^\}}", src, re.MULTILINE | re.DOTALL) + assert m, ( + f"{name} is no longer defined in ci-local.sh in a form this test can extract; " + "it was renamed or reformatted, and this guard is now testing nothing" + ) + return m.group(0) + + +class UserConfigIsolationTest(unittest.TestCase): + def test_the_prefix_drops_the_config_dir_and_every_tool_override(self): + with tempfile.TemporaryDirectory(prefix="gdi-isolation-") as tmp: + env = {k: v for k, v in os.environ.items() if not k.startswith("GDI_")} + env.update( + GDI_CONFIG_DIR="/somewhere/else", + GDI_TOOL__COUNTRY_CODE="FI", + GDI_TOOL__PROFILES__DEFAULT__ORG="EXAMPLE", + GDI_TOOLBOX="kept", + ) + script = ( + f"{_extract('user_config_isolation')}\n" + f"user_config_isolation\n{PREFIX} env\n" + ) + out = subprocess.run( + ["bash", "-c", script], + cwd=tmp, + env=env, + capture_output=True, + text=True, + check=True, + ).stdout + lines = out.splitlines() + names = {line.split("=", 1)[0] for line in lines if "=" in line} + self.assertEqual( + sorted(n for n in names if n.startswith("GDI_TOOL__")), [], out + ) + self.assertNotIn("GDI_CONFIG_DIR", names) + self.assertIn("GDI_TOOLBOX", names, "only the override prefix is dropped") + self.assertIn(f"XDG_CONFIG_HOME={tmp}/target/test-user-config", lines) + + +if __name__ == "__main__": + unittest.main() From 9522e63df7d1eed54b830b7bc5bcc18d996d1b96 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:34 +0300 Subject: [PATCH 15/16] build: pin the silo images by digest The publisher isn't verified, so the tag alone could be re-pushed. --- docker-compose.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 24afc21..ee1f837 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -351,7 +351,8 @@ services: # stopped publishing images, so this runs Silo, a maintained MinIO-compatible fork. # Console on :9001. minio: - image: pgsty/silo:RELEASE.2026-09-16T00-00-00Z + # Pinned by digest: the publisher is unverified and could re-push the tag. + image: pgsty/silo:RELEASE.2026-09-16T00-00-00Z@sha256:635197cb9f36d01bee221d34d1c7d7960f6a95c48b0b6c01d99cd13bdae51a46 profiles: ["minio"] logging: *default-logging command: server /data --console-address ":9001" @@ -373,7 +374,7 @@ services: # --- minio bucket bootstrap (deprecated; `minio` profile) ----------------- minio-setup: - image: pgsty/mc:RELEASE.2026-09-16T00-00-00Z + image: pgsty/mc:RELEASE.2026-09-16T00-00-00Z@sha256:cfc83108c3abb371f8fb84d99c1fdc88f8c237e022409b0081fb7c0a3be634dd profiles: ["minio"] depends_on: minio: From 4f6db7eda0f4c496167b79547e49f5365359e3c9 Mon Sep 17 00:00:00 2001 From: Erik Jaaniso Date: Wed, 7 Oct 2026 12:04:34 +0300 Subject: [PATCH 16/16] docs: fix stale and inaccurate notes The release step's version grep, the fuzz lockfile scan, the README's downloads and wait loop, the harness rebuild note, the localised overlay fields and a few doc comments. --- CONTRIBUTING.md | 13 ++++++------- README.md | 8 ++++---- crates/core/src/config/service.rs | 2 +- docs/operating.md | 15 ++++++++------- docs/testing.md | 2 +- node.example.toml | 4 ++-- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 954e345..7e86e9b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -580,10 +580,8 @@ inside the `release` meta-leg are marked. RSS/open-fd/thread plateau; `scripts/soak/crash-loop.sh` `kill -9`s the node and asserts the store converges on restart. Local: run those scripts directly. - **`advisories`** — re-runs the RustSec scan against the shipped lockfile and the - isolated fuzz lockfile; fails on a freshly-disclosed advisory. Local: `ci-local.sh deny` - covers the workspace lock. The fuzz lock is a local gap: scan it by hand with - `cargo deny --locked --manifest-path crates/core/fuzz/Cargo.toml check advisories` after - refreshing it. + isolated fuzz lockfile; fails on a freshly-disclosed advisory. Local: `ci-local.sh + supply-chain` scans both (`deny` the workspace lock, `deny_fuzz` the fuzz lock). - **`vendored-sync`** — two questions, one tree. `scripts/vendored.sh check` asserts every vendored conformance file is byte-identical to its pinned commit. Because that commit is an immutable SHA it stays green however far upstream moves, so `scripts/vendored.sh @@ -938,9 +936,10 @@ Then: tag. A `v0.x` tag and any suffixed tag publish as pre-releases and do not move `:latest`; only a stable tag does. - Update the version strings in the prose too. `README.md`, `docs/deployment.md`, - `docs/gdi-dataset-tool.md`, `docs/operating.md` and `docs/testing.md` name the current - release, and nothing checks them: `git grep 'v1\.0\.0-rc\.1'` finds the lot. + Update the version strings in the prose too. `README.md` and several docs name the + current release, with and without the `v`, and nothing checks them. With `old` set to + the version step 1 replaced, `git grep -nF "$old" -- ':!*Cargo.lock' ':!THIRD-PARTY-LICENSES.md'` + finds the lot. 5. **What the tag enforces.** `release.yml` gates `publish` and `image` behind `gate` (`ci-local.sh rust supply-chain`), `version-guard`, and the changelog assembly, and `ci.yml` also runs on the tag. If anything is red, nothing publishes: fix and re-tag. diff --git a/README.md b/README.md index c4597d7..b6198b4 100644 --- a/README.md +++ b/README.md @@ -150,10 +150,10 @@ R=https://raw.githubusercontent.com/GenomicDataInfrastructure/gdi-node-standalon mkdir -p ~/gdi-demo && cd ~/gdi-demo N=gdi-node-standalone-$V-x86_64-unknown-linux-gnu T=gdi-dataset-tool-$V-x86_64-unknown-linux-gnu -curl -fsSLO "$U/$N" -O "$U/$T" -O "$U/SHA256SUMS" +curl --fail-early -fsSLO "$U/$N" -O "$U/$T" -O "$U/SHA256SUMS" sha256sum -c SHA256SUMS --ignore-missing # both must print OK mv "$N" gdi-node-standalone && mv "$T" gdi-dataset-tool -curl -fsSLO "$R/covid-package.yaml" -O "$R/COVID.monogneic.aggregate.AFs.GRCh38.vcf" +curl --fail-early -fsSLO "$R/covid-package.yaml" -O "$R/COVID.monogneic.aggregate.AFs.GRCh38.vcf" chmod +x gdi-node-standalone gdi-dataset-tool export PATH="$PWD:$PATH" ``` @@ -227,7 +227,7 @@ gdi-dataset-tool build covid-package.yaml --cc EE -o build # in a checkout: cr ID=$(ls build) # build/ was empty; every build mints a new id gdi-dataset-tool deploy build/$ID --inbox ~/gdi-demo/inbox --wait --management-url http://127.0.0.1:9090 gdi-dataset-tool publish $ID --inbox ~/gdi-demo/inbox # writes {id}.state.json; the node applies it on its next scan -until curl -fsS http://127.0.0.1:9090/datasets/$ID/state | grep -q '"visible"'; do sleep 1; done +for _ in $(seq 60); do curl -fsS http://127.0.0.1:9090/datasets/$ID/state | grep -q '"visible"' && break; sleep 1; done # up to a minute curl -s -X POST http://localhost:8080/aggregated/beacon/v2/g_variants \ -H 'content-type: application/json' \ @@ -242,7 +242,7 @@ build the realistic sample instead and repeat the `deploy` and `publish` lines w ```bash V=v1.0.0-rc.2 S=https://raw.githubusercontent.com/GenomicDataInfrastructure/gdi-node-standalone/$V/crates/test-util/tests/fixtures/sample -curl -fsSLO "$S/gdi-sample.package.yaml" -O "$S/gdi-sample.GRCh38.vcf.gz" +curl --fail-early -fsSLO "$S/gdi-sample.package.yaml" -O "$S/gdi-sample.GRCh38.vcf.gz" gdi-dataset-tool build gdi-sample.package.yaml --cc EE -o build-sample # checkout: crates/test-util/tests/fixtures/sample/ ID=$(ls build-sample) ``` diff --git a/crates/core/src/config/service.rs b/crates/core/src/config/service.rs index a9a849c..5113f3c 100644 --- a/crates/core/src/config/service.rs +++ b/crates/core/src/config/service.rs @@ -3278,7 +3278,7 @@ impl ServiceConfig { /// non-empty, `email` an address, with or without `mailto:`, `url` a valid URL if /// set); /// - at least one `theme` and one `applicable_legislation` entry; these, every - /// `theme_taxonomy` entry, and the publisher's and HDAB's `homepage` and + /// `theme_taxonomy` entry, the publisher's and HDAB's `homepage`, and /// `endpoint_description` are valid IRIs; /// - the distribution has a title and an access URL, and its IRIs are valid; /// - `[fairdp.publish]`: kinds only on datasets and distributions, namespace IRIs; diff --git a/docs/operating.md b/docs/operating.md index f1dd84e..b080172 100644 --- a/docs/operating.md +++ b/docs/operating.md @@ -885,9 +885,10 @@ the read path is unaffected, so this is an alert rather than a probe failure. (default 256 MiB), `max_parquet_file_bytes`, and the whole-package `max_package_bytes` (default 16 GiB). Each data file's footer and page index are also checked before decoding: unexpected or mistyped fields, lists that claim more - than the file holds or over a million elements, a nested schema, and metadata over - the fixed caps (a 16 MiB footer, a 64 MiB page index; not configurable) fail as - `invalid-parquet-schema` (`parquet metadata refused before decoding: …`). The package cap is + than the file holds or over a million elements, a nested schema or one of over 64 + elements, and metadata over the fixed caps (a 16 MiB footer, a 64 MiB page index; not + configurable) fail as `invalid-parquet-schema` (`parquet metadata refused before + decoding: …`). The package cap is enforced on the decrypted archive as it streams to disk, so an over-cap package is refused before extraction. An S3 package whose encrypted `.tar.c4gh` object already exceeds it is refused at @@ -1608,10 +1609,10 @@ A sidecar containing a protected field name or an unrecognised key is invalid an in its entirety; the parser rejects it with `deny_unknown_fields`. Check the logs for the warning, fix the sidecar, and the next reconcile picks up the corrected version. -**Localised fields.** `title` and `description` are localisation maps. When you patch one, -supply all languages at once: the entire map replaces the package baseline. Partial -per-language patching, such as correcting only `"en"` while keeping the package's `"fi"`, -is not supported. +**Localised fields.** `title`, `description` and `provenance` are localisation maps. When +you patch one, supply all languages at once: the entire map replaces the package baseline. +Partial per-language patching, such as correcting only `"en"` while keeping the package's +`"fi"`, is not supported. ### Effect on timestamps diff --git a/docs/testing.md b/docs/testing.md index c189ed2..99946e3 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -41,7 +41,7 @@ Two rules shape where a test goes: | 6 | **Fuzz** | `crates/core/fuzz` | `cargo-fuzz` targets over untrusted-input parsers. | | 7 | **Bench** | `benches/` (criterion) | Advisory hot-path microbenchmarks; gate nothing. | | 8 | **Guard** | `tests/it/` drift-guards + scheduled jobs | Executable invariants about the repo itself. | -| — | *(out-of-process)* | `scripts/e2e`, `scripts/load`, `scripts/soak`, `scripts/chaos` | Compose-stack e2e + `oha` load + leak/crash-loop soak + toxiproxy S3/Vault chaos. `e2e`/`chaos` need Docker; `load`/`soak` boot their own node and build the release binaries if absent. | +| — | *(out-of-process)* | `scripts/e2e`, `scripts/load`, `scripts/soak`, `scripts/chaos` | Compose-stack e2e + `oha` load + leak/crash-loop soak + toxiproxy S3/Vault chaos. `e2e`/`chaos` need Docker; `load`/`soak` boot their own node and rebuild the release binaries on every run, a no-op when they are current. | --- diff --git a/node.example.toml b/node.example.toml index cf6063a..e84776c 100644 --- a/node.example.toml +++ b/node.example.toml @@ -957,8 +957,8 @@ access_url.aggregated = "https://portal.gdi.lu/allele-frequency" # already publishes what HealthDCAT-AP release 8 asks for on top of its own records, in a # form releases 5 and 7 accept too (hasStructuredData, the allele-frequency variables # table, a description of aggregated distributions, and an rdf:type on licence, access -# rights, language, legislation and legal basis values). Those built-in settings are in crates/core/src/config/fairdp-publish.toml, -# written the same way as this block. +# rights, language, legislation and legal basis values). Those built-in settings are in +# crates/core/src/config/fairdp-publish.toml, written the same way as this block. # # When GDI announces a change, add what it asks for here. Your settings go over the # built-in ones key by key, down into nested tables: a key you set replaces only that