diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a0ca6f3..7754c7fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Ambiguous C4 metadata-include lookups now log a WARN when the same kind/name exists across namespaces, listing matching refs and document paths. Selection remains unspecified; use distinct explicit names. Warnings follow logging filters and actual lookups (including search), so cached HTML stays quiet. See [ambiguous include names](docs/metadata.md#ambiguous-include-names). - **Page-local attrs for integrations** — JSON-compatible `attrs` in frontmatter and selected YAML sidecars survive S3 publication and appear as optional `meta.attrs` in NAPI/core `renderPage()` responses. Frontmatter overlays top-level keys; nested values replace whole, null remains data, and attrs never inherit. Empty attrs are omitted. Authored nesting is bounded for manifest/cache transport; Rust JSON byte roundtrips preserve supported finite `f64` values. No built-in HTTP/viewer or rendering semantics change. See [Page Metadata](docs/metadata.md#attrs-page-local-integration-data). ### Changed diff --git a/Cargo.lock b/Cargo.lock index 4eeec70f..8e40e114 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4596,6 +4596,7 @@ dependencies = [ "tempfile", "thiserror", "tracing", + "tracing-subscriber", ] [[package]] diff --git a/crates/rw-site/Cargo.toml b/crates/rw-site/Cargo.toml index 048f3735..55a63327 100644 --- a/crates/rw-site/Cargo.toml +++ b/crates/rw-site/Cargo.toml @@ -32,6 +32,7 @@ rw-storage = { workspace = true, features = ["mock"] } rw-storage-fs = { workspace = true } static_assertions = { workspace = true } tempfile = { workspace = true } +tracing-subscriber = { version = "0.3", features = ["fmt"] } [[bench]] name = "page_rendering" diff --git a/crates/rw-site/src/lib.rs b/crates/rw-site/src/lib.rs index 91654a90..bd1fcc89 100644 --- a/crates/rw-site/src/lib.rs +++ b/crates/rw-site/src/lib.rs @@ -135,3 +135,6 @@ pub use site_state::{NavItem, Navigation, PageEntry, ScopeInfo, SectionEntry}; pub use rw_renderer::TocEntry; pub use path::to_url_path; + +#[cfg(test)] +mod test_support; diff --git a/crates/rw-site/src/page.rs b/crates/rw-site/src/page.rs index 5eeacce4..071f19b9 100644 --- a/crates/rw-site/src/page.rs +++ b/crates/rw-site/src/page.rs @@ -646,12 +646,30 @@ mod tests { message: "kroki said no".to_owned(), transient: false, }), - source => Ok(Resolved { - asset: Asset::Inline(DiagramContent::Svg(format!("{source}"))), - size: None, - digest: "0".to_owned(), - warnings: Vec::new(), - }), + source => { + // Keep metadata resolution real; only replace the remote + // renderer with an SVG echo of its prepared input. + let (source, warnings) = ctx.model.map_or_else( + || (source.to_owned(), Vec::new()), + |model| { + let prepared = rw_plantuml::prepare_diagram_source( + source, + &[], + 192, + Some(model), + ); + (prepared.source, prepared.warnings) + }, + ); + Ok(Resolved { + asset: Asset::Inline(DiagramContent::Svg(format!( + "{source}" + ))), + size: None, + digest: "0".to_owned(), + warnings, + }) + } }) .collect() } @@ -698,6 +716,114 @@ mod tests { (dir, cache) } + #[test] + fn unresolved_metadata_include_warning_reaches_page_response() { + use crate::test_support::snapshot_from_yaml; + + let ctx = RenderContext { + meta_include_source: Some(Arc::new(snapshot_from_yaml(&[]))), + ..Default::default() + }; + let cache: Arc = Arc::new(NullCache); + let (renderer, _) = + stub_renderer(diagram_storage("!include systems/sys_missing.iuml"), &cache); + let page = make_page("Diagram", "diag", true); + let result = renderer.render("diag", &page, vec![], &ctx).unwrap(); + + assert_eq!(result.warnings.len(), 1, "{:?}", result.warnings); + assert!(result.warnings[0].contains("systems/sys_missing.iuml")); + assert!(result.warnings[0].contains("not found")); + } + + #[test] + fn ambiguous_includes_warn_on_render_and_search_but_not_html_cache_hits() { + use crate::test_support::{assert_candidate_order, capture_warnings, snapshot_from_yaml}; + + let snapshot = Arc::new(snapshot_from_yaml(&[ + ( + "a/shared", + "kind: system\nnamespace: billing\ntitle: Shared system", + true, + ), + ( + "b/shared", + "kind: system\nnamespace: shipping\ntitle: Shared system", + true, + ), + ])); + let ctx = RenderContext { + resolution_fingerprint: snapshot.state.resolution_fingerprint(), + meta_include_source: Some(snapshot), + ..Default::default() + }; + let (_dir, cache) = file_cache(); + let storage = diagram_storage("!include systems/sys_shared.iuml") + .with_file("quiet", "Quiet", "# Quiet\n\nUnrelated prose") + .with_mtime("quiet", 1000.0); + let (renderer, _) = stub_renderer(storage, &cache); + let page = make_page("Diagram", "diag", true); + + let (fresh, logs) = + capture_warnings(|| renderer.render("diag", &page, vec![], &ctx).unwrap()); + assert!(!fresh.from_cache); + assert!( + fresh.html.contains("System(sys_shared, \"Shared system\""), + "{}", + fresh.html + ); + assert!( + fresh.warnings.is_empty(), + "logs must not become response warnings" + ); + assert_eq!(logs.lines().count(), 1, "{logs}"); + assert_candidate_order( + &logs, + &[ + ("system:billing/shared", "/a/shared"), + ("system:shipping/shared", "/b/shared"), + ], + ); + + let (cached, logs) = + capture_warnings(|| renderer.render("diag", &page, vec![], &ctx).unwrap()); + assert!(cached.from_cache); + assert_eq!(cached.html, fresh.html); + assert!(cached.warnings.is_empty()); + assert!(logs.is_empty(), "{logs}"); + + let (search, logs) = capture_warnings(|| { + renderer + .render_search_document("diag", &page, &ctx) + .unwrap() + .unwrap() + }); + assert!(search.text.contains("Shared system"), "{}", search.text); + assert!(!search.text.contains("!include"), "{}", search.text); + assert_eq!(logs.lines().count(), 1, "{logs}"); + assert_candidate_order( + &logs, + &[ + ("system:billing/shared", "/a/shared"), + ("system:shipping/shared", "/b/shared"), + ], + ); + + let quiet_page = make_page("Quiet", "quiet", true); + let (quiet, logs) = + capture_warnings(|| renderer.render("quiet", &quiet_page, vec![], &ctx).unwrap()); + assert!(!quiet.from_cache); + assert!(quiet.html.contains("Unrelated prose")); + assert!(logs.is_empty(), "{logs}"); + let (search, logs) = capture_warnings(|| { + renderer + .render_search_document("quiet", &quiet_page, &ctx) + .unwrap() + .unwrap() + }); + assert!(search.text.contains("Unrelated prose")); + assert!(logs.is_empty(), "{logs}"); + } + #[test] fn a_resolved_diagram_is_spliced_where_its_fence_stood() { let storage = MockStorage::new() diff --git a/crates/rw-site/src/site.rs b/crates/rw-site/src/site.rs index 85c57eef..88655d26 100644 --- a/crates/rw-site/src/site.rs +++ b/crates/rw-site/src/site.rs @@ -41,11 +41,27 @@ pub(crate) struct SiteSnapshot { impl SiteModel for SiteSnapshot { fn entity(&self, kind: &str, name: &str) -> Option { - let (section_path, _section) = self - .state - .find_sections_by_name(name) - .into_iter() - .find(|(_, s)| s.kind == kind)?; + let candidates = self.state.find_sections_by_name(name); + let mut matching = candidates.iter().copied().filter(|(_, s)| s.kind == kind); + let (section_path, section) = matching.next()?; + + if matching.any(|(_, s)| s.namespace != section.namespace) { + tracing::warn!( + kind = %kind, + name = %name, + candidates = %{ + // Sort only diagnostic detail, never the index's lookup candidates. + let mut details: Vec<_> = candidates.iter().filter(|(_, s)| s.kind == kind).collect(); + details.sort_unstable_by_key(|(path, _)| *path); + details + .iter() + .map(|(path, s)| format!("{s} (/{path})")) + .collect::>() + .join(", ") + }, + "ambiguous diagram metadata include: lookup does not specify a namespace; use distinct explicit names" + ); + } let document = self.state.get_page(section_path); let has_content = document.is_some_and(|d| d.has_content); @@ -2210,3 +2226,377 @@ mod tests { ); } } + +#[cfg(test)] +mod ambiguity_tests { + use crate::test_support::{assert_candidate_order, capture_warnings, snapshot_from_yaml}; + + #[test] + fn ambiguous_include_warns_with_a_candidate_field_filter() { + use crate::test_support::capture_warnings_matching; + + let snapshot = snapshot_from_yaml(&[ + ("a/shared", "kind: system\nnamespace: billing", true), + ("b/shared", "kind: system\nnamespace: shipping", true), + ]); + let (prepared, logs) = capture_warnings_matching( + |meta| { + *meta.level() == tracing::Level::WARN && meta.fields().field("candidates").is_some() + }, + || { + rw_plantuml::prepare_diagram_source( + "!include systems/sys_shared.iuml", + &[], + 192, + Some(&snapshot), + ) + }, + ); + assert!(prepared.warnings.is_empty()); + assert_eq!(logs.lines().count(), 1, "{logs}"); + assert_candidate_order( + &logs, + &[ + ("system:billing/shared", "/a/shared"), + ("system:shipping/shared", "/b/shared"), + ], + ); + } + + #[test] + fn ambiguous_metadata_include_emits_one_warning() { + let snapshot = snapshot_from_yaml(&[ + ( + "z/shared", + "kind: system\nnamespace: billing\ntitle: Billing", + true, + ), + ( + "a/shared", + "kind: system\nnamespace: shipping\ntitle: Shipping", + true, + ), + ]); + let (prepared, logs) = capture_warnings(|| { + rw_plantuml::prepare_diagram_source( + "!include systems/sys_shared.iuml", + &[], + 192, + Some(&snapshot), + ) + }); + assert!(prepared.warnings.is_empty()); + assert_eq!(logs.lines().count(), 1, "{logs}"); + assert!(logs.contains("kind=system"), "{logs}"); + assert!(logs.contains("name=shared"), "{logs}"); + assert_candidate_order( + &logs, + &[ + ("system:shipping/shared", "/a/shared"), + ("system:billing/shared", "/z/shared"), + ], + ); + assert!(logs.contains("namespace"), "{logs}"); + assert!(logs.contains("distinct explicit names"), "{logs}"); + } + + #[test] + fn all_c4_kinds_and_external_includes_warn_without_changing_selection() { + use rw_diagrams::{Entity, SiteModel}; + + for (kind, prefix, a_ref, z_ref, regular_label) in [ + ( + "domain", + "dmn", + "domain:shipping/shared", + "domain:billing/shared", + "$tags=\"domain\"", + ), + ( + "system", + "sys", + "system:shipping/shared", + "system:billing/shared", + "System(sys_shared,", + ), + ( + "service", + "svc", + "service:shipping/shared", + "service:billing/shared", + "$tags=\"service\"", + ), + ] { + let snapshot = snapshot_from_yaml(&[ + ( + "z/shared", + &format!("kind: {kind}\nnamespace: billing\ntitle: Billing"), + true, + ), + ( + "a/shared", + &format!("kind: {kind}\nnamespace: shipping\ntitle: Shipping"), + true, + ), + ("other/shared", "kind: component\nnamespace: other", true), + ]); + let original = snapshot.state.find_sections_by_name("shared"); + let first = original.iter().find(|(_, s)| s.kind == kind).unwrap().0; + let (title, path) = match first { + "a/shared" => ("Shipping", "/a/shared"), + "z/shared" => ("Billing", "/z/shared"), + other => panic!("unexpected candidate {other}"), + }; + let expected = Entity { + title: if kind == "service" { "shared" } else { title }.to_owned(), + description: None, + url_path: Some(path.to_owned()), + }; + let fingerprint = snapshot.state.resolution_fingerprint(); + let disabled = tracing::subscriber::with_default( + tracing::subscriber::NoSubscriber::default(), + || snapshot.entity(kind, "shared"), + ); + assert_eq!(disabled, Some(expected.clone())); + for external in [false, true] { + let subdir = if external { "ext/" } else { "" }; + let (prepared, logs) = capture_warnings(|| { + rw_plantuml::prepare_diagram_source( + &format!("!include systems/{subdir}{prefix}_shared.iuml"), + &[], + 192, + Some(&snapshot), + ) + }); + assert!(prepared.warnings.is_empty()); + assert_eq!(logs.lines().count(), 1, "{logs}"); + assert_candidate_order(&logs, &[(a_ref, "/a/shared"), (z_ref, "/z/shared")]); + assert!(!logs.contains("component:"), "{logs}"); + assert!( + prepared.source.contains(if external { + "System_Ext(" + } else { + regular_label + }), + "{}", + prepared.source + ); + assert!( + prepared.source.contains(&format!("\"{}\"", expected.title)), + "{}", + prepared.source + ); + assert!( + prepared.source.contains(&format!("$link=\"{path}\"")), + "{}", + prepared.source + ); + } + let (selected, _) = capture_warnings(|| snapshot.entity(kind, "shared")); + assert_eq!(selected, Some(expected)); + assert_eq!(snapshot.state.find_sections_by_name("shared"), original); + assert_eq!(snapshot.state.resolution_fingerprint(), fingerprint); + } + } + + #[test] + fn nonambiguous_lookups_are_quiet_and_full_ref_duplicates_still_warn() { + let (snapshot, construction) = capture_warnings(|| { + snapshot_from_yaml(&[ + ("a/shared", "kind: system\nnamespace: billing", true), + ("b/shared", "kind: system\nnamespace: billing", true), + ("c/shared", "kind: service\nnamespace: shipping", true), + ("unique", "kind: domain", true), + ]) + }); + assert_eq!(construction.lines().count(), 1, "{construction}"); + assert!( + construction.split_whitespace().any(|word| word == "WARN"), + "{construction}" + ); + assert!( + construction.contains("duplicate section identifier"), + "{construction}" + ); + assert!( + construction.contains("system:billing/shared"), + "{construction}" + ); + for (include, resolves) in [ + ("systems/sys_shared.iuml", true), + ("systems/svc_shared.iuml", true), + ("systems/dmn_unique.iuml", true), + ("systems/dmn_shared.iuml", false), + ("systems/sys_missing.iuml", false), + ] { + let (prepared, logs) = capture_warnings(|| { + rw_plantuml::prepare_diagram_source( + &format!("!include {include}"), + &[], + 192, + Some(&snapshot), + ) + }); + assert!(logs.is_empty(), "{include}: {logs}"); + assert_eq!(prepared.warnings.is_empty(), resolves, "{include}"); + } + } + + #[test] + fn mixed_collisions_list_every_path_once_per_actual_lookup() { + let (snapshot, construction) = capture_warnings(|| { + snapshot_from_yaml(&[ + ("z/shared", "kind: system\nnamespace: billing", true), + ( + "a/guide", + "kind: system\nnamespace: billing\nname: shared", + false, + ), + ("m/shared", "kind: system\nnamespace: shipping", true), + ]) + }); + assert_eq!(construction.lines().count(), 1, "{construction}"); + assert!( + construction.contains("duplicate section identifier"), + "{construction}" + ); + let (prepared, logs) = capture_warnings(|| { + rw_plantuml::prepare_diagram_source( + "!include systems/sys_shared.iuml\n!include systems/ext/sys_shared.iuml", + &[], + 192, + Some(&snapshot), + ) + }); + assert!(prepared.warnings.is_empty()); + assert_eq!(logs.lines().count(), 2, "{logs}"); + for event in logs.lines() { + assert_candidate_order( + event, + &[ + ("system:billing/shared", "/a/guide"), + ("system:shipping/shared", "/m/shared"), + ("system:billing/shared", "/z/shared"), + ], + ); + assert_eq!(event.matches("system:").count(), 3, "{event}"); + } + } + + #[test] + fn only_explicitly_named_kind_declaring_roots_join_include_candidates() { + for (root, name, warns) in [ + ("kind: system\nname: shared", "shared", true), + ("kind: system", "root", false), + ("name: shared", "shared", false), + ("", "root", false), + ] { + let snapshot = snapshot_from_yaml(&[ + ( + "", + &format!("namespace: home\ntitle: Homepage\n{root}"), + false, + ), + ( + "guide", + &format!("kind: system\nnamespace: billing\nname: {name}\ntitle: Guide"), + false, + ), + ]); + let (prepared, logs) = capture_warnings(|| { + rw_plantuml::prepare_diagram_source( + &format!("!include systems/sys_{name}.iuml"), + &[], + 192, + Some(&snapshot), + ) + }); + assert!(prepared.warnings.is_empty()); + assert!( + !prepared.source.contains("$link="), + "metadata-only entities have no link" + ); + assert_eq!(logs.lines().count(), usize::from(warns), "{root}: {logs}"); + if warns { + assert_candidate_order( + &logs, + &[ + ("system:home/shared", "/"), + ("system:billing/shared", "/guide"), + ], + ); + } else { + assert!(prepared.source.contains("\"Guide\""), "{}", prepared.source); + } + } + } + + #[test] + fn construction_and_structure_cache_restore_are_quiet_and_keep_the_wire() { + use super::SiteSnapshot; + use crate::site_state::SiteState; + use rw_cache::Cache; + + let temp = tempfile::tempdir().unwrap(); + let cache = rw_cache::FileCache::new(temp.path().join("cache"), "1.0.0"); + let bucket = cache.bucket("site"); + let (snapshot, logs) = capture_warnings(|| { + snapshot_from_yaml(&[ + ("a/shared", "kind: system\nnamespace: billing", true), + ("b/shared", "kind: system\nnamespace: shipping", true), + ]) + }); + assert!(logs.is_empty(), "{logs}"); + snapshot.state.to_cache(bucket.as_ref(), "etag"); + let before = bucket.get("structure", "etag").unwrap(); + let wire: serde_json::Value = serde_json::from_slice(&before).unwrap(); + let mut fields: Vec<_> = wire + .as_object() + .unwrap() + .keys() + .map(String::as_str) + .collect(); + fields.sort_unstable(); + assert_eq!( + fields, + [ + "children", + "pages", + "parents", + "root_namespace", + "roots", + "sections" + ] + ); + let (restored, logs) = capture_warnings(|| SiteSnapshot { + state: SiteState::from_cache(bucket.as_ref(), "etag").unwrap(), + }); + assert!(logs.is_empty(), "{logs}"); + assert_eq!( + restored.state.resolution_fingerprint(), + snapshot.state.resolution_fingerprint() + ); + for model in [&snapshot, &restored] { + let (prepared, logs) = capture_warnings(|| { + rw_plantuml::prepare_diagram_source( + "!include systems/sys_shared.iuml", + &[], + 192, + Some(model), + ) + }); + assert!(prepared.warnings.is_empty()); + assert_eq!(logs.lines().count(), 1, "{logs}"); + assert_candidate_order( + &logs, + &[ + ("system:billing/shared", "/a/shared"), + ("system:shipping/shared", "/b/shared"), + ], + ); + } + // Writing the same snapshot after diagnostics cannot add warning state. + snapshot.state.to_cache(bucket.as_ref(), "etag"); + assert_eq!(bucket.get("structure", "etag").unwrap(), before); + } +} diff --git a/crates/rw-site/src/test_support.rs b/crates/rw-site/src/test_support.rs new file mode 100644 index 00000000..cafcf4b9 --- /dev/null +++ b/crates/rw-site/src/test_support.rs @@ -0,0 +1,90 @@ +//! Provides local tracing capture and real snapshot fixtures for lookup boundary tests. + +use std::io::{self, Write}; +use std::sync::Arc; + +use parking_lot::Mutex; + +use crate::site::SiteSnapshot; +use crate::site_state::SiteStateBuilder; +use rw_storage::Document; + +#[derive(Clone, Default)] +struct LogWriter(Arc>>); + +impl Write for LogWriter { + fn write(&mut self, bytes: &[u8]) -> io::Result { + self.0.lock().extend_from_slice(bytes); + Ok(bytes.len()) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } +} + +pub(crate) fn capture_warnings(f: impl FnOnce() -> T) -> (T, String) { + capture_warnings_matching(|_| true, f) +} + +pub(crate) fn capture_warnings_matching( + filter: impl Fn(&tracing::Metadata<'_>) -> bool + Send + Sync + 'static, + f: impl FnOnce() -> T, +) -> (T, String) { + use tracing_subscriber::prelude::*; + + let writer = LogWriter::default(); + let output = writer.clone(); + let subscriber = tracing_subscriber::registry() + .with(tracing_subscriber::filter::filter_fn(move |meta| { + *meta.level() <= tracing::Level::WARN && filter(meta) + })) + .with( + tracing_subscriber::fmt::layer() + .without_time() + .with_ansi(false) + .with_target(false) + .with_writer(move || writer.clone()), + ); + let result = tracing::subscriber::with_default(subscriber, f); + let logs = String::from_utf8(output.0.lock().clone()).unwrap(); + (result, logs) +} + +pub(crate) fn assert_candidate_order(event: &str, expected: &[(&str, &str)]) { + assert!( + event.split_whitespace().any(|word| word == "WARN"), + "{event}" + ); + // Ignore presentation separators while preserving ref/path pairing and order. + let tokens: Vec<_> = event + .split(|c: char| !(c.is_alphanumeric() || "/:._-".contains(c))) + .filter(|token| !token.is_empty()) + .collect(); + let mut remaining = tokens.as_slice(); + for &(full_ref, path) in expected { + let position = remaining + .iter() + .position(|token| *token == full_ref) + .expect(event); + assert_eq!(remaining.get(position + 1), Some(&path), "{event}"); + remaining = &remaining[position + 2..]; + } +} + +pub(crate) fn snapshot_from_yaml(pages: &[(&str, &str, bool)]) -> SiteSnapshot { + let mut builder = SiteStateBuilder::new(); + for &(path, yaml, has_content) in pages { + builder.add_document(Document { + path: path.to_owned(), + has_content, + meta: Arc::new(rw_meta::Meta::resolve(None, Some(yaml), path)), + origin: None, + is_dir: true, + diagnostics: Vec::new().into(), + }); + } + SiteSnapshot { + state: builder.build(), + } +} diff --git a/docs/metadata.md b/docs/metadata.md index ff77b2e7..71a6bccb 100644 --- a/docs/metadata.md +++ b/docs/metadata.md @@ -371,3 +371,32 @@ underscore back to a hyphen. C4 include users should declare hyphenated names. This feature adds neither a new encoding nor namespace-qualified include syntax. Regular includes generate `System()` macros; external includes generate `System_Ext()` macros. Domain/system labels use the page title; service labels use the effective entity name. Descriptions and links to the actual documentation path are unchanged (metadata-only entities have no page link). + +### Ambiguous include names + +Metadata includes look up **kind and name without a namespace**. For example, +`system:billing/shared` and `system:shipping/shared` both match +`!include systems/sys_shared.iuml`. Selection is unspecified: do not rely on +which entity appears, or expect fresh and cached results to select the same one. +Give these entities **distinct explicit `name` values** and update their includes; +namespace-qualified includes are not supported. + +When a lookup finds matches across namespaces, RW logs one WARN listing every +matching full ref and logical document path (`/` for the homepage), ordered by +path for readability. That order does not determine selection. Declared names, +path-derived names, metadata-only entities and explicitly named, kind-declaring +homepages all follow the eligibility rules above. Duplicates confined to one +namespace keep the existing duplicate-full-ref warning, with no additional +include warning. A group spanning namespaces lists all matching paths, including +any duplicate full refs. + +This warning occurs **at lookup time**, not when constructing or restoring site +structure. Repeated lookups, including repeated includes on one page, may warn +again. A rendered-HTML cache hit skips include resolution and emits no new warning; +unrelated pages do not trigger it. Existing search-text extraction also resolves +metadata includes and may warn. There is no new whole-site validation pass, cache +invalidation or warning field in rendering/API responses. + +Visibility follows the logging filter: use `rw serve --verbose` or +`RUST_LOG=warn` to see WARN events (default verbosity hides them). Cache silence +and logging filters mean absence of a warning is not proof of unique names.