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/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/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/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/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/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/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/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/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] diff --git a/crates/core/src/faults.rs b/crates/core/src/faults.rs index 0c0a89a..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 @@ -80,6 +79,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"))] @@ -300,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() { @@ -309,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] @@ -333,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] @@ -375,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" ); } @@ -386,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/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/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/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/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/core/tests/fixtures/malformed/negative_chunk_start.parquet b/crates/core/tests/fixtures/malformed/negative_chunk_start.parquet new file mode 100644 index 0000000..cdada3d Binary files /dev/null and b/crates/core/tests/fixtures/malformed/negative_chunk_start.parquet differ 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/core/tests/it/parquet_probes.rs b/crates/core/tests/it/parquet_probes.rs index 8c2665c..14f198f 100644 --- a/crates/core/tests/it/parquet_probes.rs +++ b/crates/core/tests/it/parquet_probes.rs @@ -727,9 +727,39 @@ fn page_index_residual_shrunk_chunk_is_rejected_before_it_can_diverge() { ); let noindex_err = enforce_page_size_caps(&path, &patch_chunk(md, shrunk, None)) - .expect_err("a shrunk chunk with no index at all must be rejected"); + .expect_err("a shrunk chunk with no offset index must be rejected"); assert!( - noindex_err.to_string().contains("no page (offset) index"), + noindex_err + .to_string() + .contains("row group 0 column 0 has no page (offset) index"), "the Values representation must fail closed on the absent index: {noindex_err}" ); } + +/// With no page index at all, every chunk would be read in `Values` mode, so the gate refuses +/// the file as a whole rather than chunk by chunk. +#[test] +fn a_file_without_any_page_index_is_rejected() { + use gdi_node_standalone_core::parquet_pages::enforce_page_size_caps; + + let tmp = tempfile::tempdir().expect("tempdir"); + let path = tmp.path().join("noindex.parquet"); + write_multi_page(&path, 3, 64, 8); + let file = std::fs::File::open(&path).expect("open"); + let real = ArrowReaderMetadata::load( + &file, + ArrowReaderOptions::new().with_page_index_policy(PageIndexPolicy::Required), + ) + .expect("load metadata with page index"); + let without_index = ParquetMetaDataBuilder::new_from_metadata(real.metadata().as_ref().clone()) + .set_page_index(None) + .build(); + + let err = enforce_page_size_caps(&path, &without_index) + .expect_err("a file with no page index must be rejected"); + assert!( + err.to_string() + .contains("parquet has no page (offset) index"), + "a file without any page index must fail closed: {err}" + ); +} 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/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}"); } 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" 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/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-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'" + ); + } } 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/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/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(); diff --git a/crates/gdi-dataset-tool/tests/it/validate_e2e.rs b/crates/gdi-dataset-tool/tests/it/validate_e2e.rs index 9292a23..c620618 100644 --- a/crates/gdi-dataset-tool/tests/it/validate_e2e.rs +++ b/crates/gdi-dataset-tool/tests/it/validate_e2e.rs @@ -253,6 +253,41 @@ fn validate_subprocess_reports_a_malformed_parquet_without_a_raw_panic() { ); } +#[test] +#[serial(env)] +fn lint_subprocess_reports_a_decode_panic_without_a_raw_panic() { + // `lint` reads rows straight away, with no validation first. `negative_chunk_start` + // makes `parquet` panic as it reads the first column chunk; the read path catches it, + // and the binary's panic hook keeps the raw panic text off stderr. + let tmp = tempfile::tempdir().unwrap(); + let config_dir = tmp.path().join("config"); + let _config_dir = test_util::EnvGuard::set("GDI_CONFIG_DIR", &config_dir); + let staging = build_covid(&tmp.path().join("build")); + let malformed = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../core/tests/fixtures/malformed/negative_chunk_start.parquet"); + fs::copy(&malformed, find_data_parquet(&staging)).unwrap(); + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_gdi-dataset-tool")) + .args(["lint", staging.to_str().unwrap()]) + .env("GDI_CONFIG_DIR", &config_dir) + .output() + .expect("spawn gdi-dataset-tool"); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !output.status.success(), + "a parquet that panics the decoder must fail lint; stderr:\n{stderr}" + ); + assert!( + stderr.contains("parquet decode panicked"), + "expected the panic boundary's error on stderr, got:\n{stderr}" + ); + assert!( + !stderr.contains("panicked at"), + "a raw Rust panic message reached stderr; stderr:\n{stderr}" + ); +} + #[test] #[serial(env)] fn validate_package_file_succeeds() { 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}" + ); +} 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/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. 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/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/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 { 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: 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/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/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 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 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/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/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/_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_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" ) 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): 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)) 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()