diff --git a/README.md b/README.md index 8c03cb2..bf899ed 100644 --- a/README.md +++ b/README.md @@ -38,13 +38,21 @@ Or download a prebuilt binary for your platform from the | Dart / Flutter | `pubspec.yaml` | pub.dev | `pubspec.lock` | ๐Ÿงช Experimental | | C# / .NET | `*.csproj`, `Directory.Packages.props` | NuGet | โ€” | ๐Ÿงช Experimental | | Elixir | `mix.exs` | Hex | `mix.lock` | ๐Ÿงช Experimental | -| Kotlin / Java | `gradle/libs.versions.toml` | Maven Central | โ€” | ๐Ÿงช Experimental | - -Kotlin / Java coverage is the **declarative** half of a Gradle build: the version -catalog. A build script (`build.gradle`, `build.gradle.kts`) is a program, and reading -one means running your build โ€” so a build script found without a catalog beside it is -reported as unread rather than silently skipped, and a handful of catalog entries never -gets presented as a whole dependency list. +| Kotlin / Java | `gradle/libs.versions.toml`, `pom.xml` | Maven Central | โ€” | ๐Ÿงช Experimental | + +Kotlin / Java coverage is the **declarative** half of a JVM build. For Gradle that is +the version catalog: a build script (`build.gradle`, `build.gradle.kts`) is a program, +and reading one means running your build โ€” so a build script found without a catalog +beside it is reported as unread rather than silently skipped, and a handful of catalog +entries never gets presented as a whole dependency list. + +A Maven `pom.xml` is data throughout, so its `` are read directly, with +`${property}` resolved against the `` of the same file. What a POM defers +to its ``, to ``, or to an imported BOM is **not** +resolved โ€” doing so correctly can mean fetching the parent POM from a registry, which +is a resolution engine rather than a parser. Those dependencies are still listed, with +no version and nothing claimed about them, so a POM that inherits some of its versions +is never presented as depending on only the rest. ### Lockfiles diff --git a/crates/dependable-core/src/item.rs b/crates/dependable-core/src/item.rs index 89dafc6..4f3bbd5 100644 --- a/crates/dependable-core/src/item.rs +++ b/crates/dependable-core/src/item.rs @@ -40,9 +40,13 @@ impl Item { /// sources are skipped. /// /// An [`Inherited`](PackageSource::Inherited) item is checkable only once - /// [`resolve_workspace_inheritance`](crate::resolve_workspace_inheritance) has - /// supplied the workspace root's constraint. Unresolved, the manifest states no - /// version at all, and there is nothing to ask a registry for. + /// something has supplied the constraint declared elsewhere โ€” the workspace root + /// via [`resolve_workspace_inheritance`](crate::resolve_workspace_inheritance) for + /// Cargo, the `[versions]` table for a Gradle catalog, the `` table + /// for a POM. Without one the manifest states no version at all, and there is + /// nothing to ask a registry for; a check reports such an item as + /// [`Undetermined`](crate::result::DependencyStatus::Undetermined) rather than + /// claiming it has no registry. #[must_use] pub fn is_checkable(&self) -> bool { match self.source { @@ -60,7 +64,9 @@ impl Item { /// parser that declines to record a span also gives the item a source nothing would /// fetch, so [`is_checkable`](Self::is_checkable) covers all of them but one: a /// resolved [`Inherited`](PackageSource::Inherited) item is worth checking and still - /// has no home here, because its version string is in the workspace root. + /// has no home here, because the version string it was resolved from belongs to + /// another entry โ€” a workspace root's table, a catalog `[versions]` alias, a shared + /// POM `` value. #[must_use] pub fn has_position(&self) -> bool { self.is_checkable() && self.source != PackageSource::Inherited @@ -153,15 +159,56 @@ pub enum PackageSource { Local, /// A git dependency โ€” skipped for version checks. Git, - /// A Cargo `dep.workspace = true`: the manifest opts into a version declared in - /// the workspace root's `[workspace.dependencies]` and states none of its own. + /// The dependency's version is declared somewhere other than this entry, so + /// there is no version string here to check against or to rewrite. /// - /// Reading `workspace = true` needs no filesystem, so the IO-free parser records it - /// โ€” which is what keeps it distinct from a [`Local`](Self::Local) `path` entry that - /// happens to share a name with a root declaration. Resolving it against the root - /// does need IO, and is [`resolve_workspace_inheritance`](crate::resolve_workspace_inheritance) - /// applied by the caller that has the root in hand. + /// Three parsers emit it, for the same reason and with the same consequences: + /// + /// - Cargo's `dep.workspace = true` โ€” the version is in the workspace root's + /// `[workspace.dependencies]`. Reading `workspace = true` needs no + /// filesystem, so the IO-free parser records the fact; *resolving* it does + /// need IO, and is + /// [`resolve_workspace_inheritance`](crate::resolve_workspace_inheritance) + /// applied by the caller that has the root in hand. + /// - A Gradle version catalog entry whose `version.ref` names a `[versions]` + /// alias several entries share. + /// - A Maven POM entry whose version comes from a `` value several + /// dependencies share, or from a `` / `` / + /// undeclared property this file does not state. + /// + /// What keeps it distinct from [`Local`](Self::Local) is that the package is a + /// real registry package โ€” an entry that merely shares a name with a root + /// `path` declaration is `Local`, and a POM `system` jar is + /// `Local`, because neither has a registry at all. + /// + /// The constraint tells the two halves apart. Filled in, the version was found + /// elsewhere and the item is checkable โ€” never rewritable, since the string it + /// would rewrite is not this dependency's own. Empty, no version was found at + /// all, and a check reports + /// [`DependencyStatus::Undetermined`](crate::result::DependencyStatus::Undetermined). Inherited, + /// The entry names a package this manifest cannot identify, whatever version it + /// states beside it. + /// + /// A Maven POM is the case that needs it: `${project.groupId}` + /// names a built-in this file does not state, and a `` may omit + /// `` altogether and inherit it from a ``. Either way the + /// coordinate is not a name any registry could answer for, so the entry is never + /// fetched โ€” and a check reports it + /// [`Undetermined`](crate::result::DependencyStatus::Undetermined), because what + /// went unread is which package this is. + /// + /// Distinct from [`Inherited`](Self::Inherited), whose name *is* known and whose + /// missing half is the version: the two get different explanations, and an entry + /// whose version is stated right there in the file must never be told it takes + /// that version from somewhere else. Distinct from [`Local`](Self::Local), which + /// asserts there is no registry behind the package rather than that this file + /// could not say which package it is. + /// + /// The version, when the entry states one, is still recorded in + /// [`version_constraint`](Item::version_constraint) โ€” it is written in this file, + /// and dropping it would report a manifest as stating less than it does. + Unidentified, } #[cfg(test)] diff --git a/crates/dependable-core/src/lib.rs b/crates/dependable-core/src/lib.rs index a31f285..450c96c 100644 --- a/crates/dependable-core/src/lib.rs +++ b/crates/dependable-core/src/lib.rs @@ -37,9 +37,10 @@ pub use parsers::{ AutoTargets, CargoPackageManifest, CargoTarget, CargoTargetKind, CargoTomlParser, CfgDependencyTable, ComposerJsonParser, CsprojParser, DenoJsonParser, DependencySection, GoModParser, GradleCatalogParser, MixExsParser, PackageField, PackageJsonParser, Parser, - PnpmWorkspaceParser, ProjectMeta, ProjectRole, PubspecYamlParser, PyprojectTomlParser, - RequirementsTxtParser, WorkspaceDecl, parse, parse_cargo_config, parse_package_manifest, - parse_package_name, parse_project, parse_workspace, resolve_workspace_inheritance, + PnpmWorkspaceParser, PomXmlParser, ProjectMeta, ProjectRole, PubspecYamlParser, + PyprojectTomlParser, RequirementsTxtParser, WorkspaceDecl, parse, parse_cargo_config, + parse_package_manifest, parse_package_name, parse_project, parse_workspace, + resolve_workspace_inheritance, }; pub use result::{CheckResult, DependencyStatus}; pub use semver::{Evaluation, UnstableFilter, check_version, is_prerelease, to_semver_constraint}; diff --git a/crates/dependable-core/src/manifest.rs b/crates/dependable-core/src/manifest.rs index a93a35d..c8c438d 100644 --- a/crates/dependable-core/src/manifest.rs +++ b/crates/dependable-core/src/manifest.rs @@ -16,6 +16,18 @@ pub struct ParsedManifest { pub items: Vec, /// Alternate registry declarations (Rust `[registries.*]`). pub alternate_registries: Vec, + /// What the parser saw and deliberately did not read, in the parser's own + /// words, ready to print. + /// + /// Not errors and not warnings *about* the dependencies in + /// [`items`](Self::items): each one names a construct this parser declines to + /// interpret, so that a list which reads as complete and is not says so. + /// A Maven `` block holding dependencies is the motivating case โ€” + /// excluding conditional dependencies is defensible, printing + /// `(0 dependencies)` for a POM that declares twelve of them is not. + /// + /// Empty for every parser that has nothing to declare, which is most of them. + pub notices: Vec, } /// A declared alternate registry (Rust only). @@ -48,6 +60,7 @@ pub enum ManifestKind { MixExs, Csproj, GradleVersionCatalog, + PomXml, } impl ManifestKind { @@ -65,7 +78,7 @@ impl ManifestKind { ManifestKind::PubspecYaml => Ecosystem::Dart, ManifestKind::MixExs => Ecosystem::Elixir, ManifestKind::Csproj => Ecosystem::CSharp, - ManifestKind::GradleVersionCatalog => Ecosystem::Jvm, + ManifestKind::GradleVersionCatalog | ManifestKind::PomXml => Ecosystem::Jvm, } } @@ -179,6 +192,7 @@ impl ManifestKind { "pubspec.yaml" => ManifestKind::PubspecYaml, "mix.exs" => ManifestKind::MixExs, "Directory.Packages.props" => ManifestKind::Csproj, + "pom.xml" => ManifestKind::PomXml, // Gradle reads every `*.versions.toml` under `gradle/` as a catalog; // `libs` is only the conventional name of the default one. _ if name.ends_with(".versions.toml") => ManifestKind::GradleVersionCatalog, @@ -429,6 +443,7 @@ mod tests { "gradle/deps.versions.toml", ManifestKind::GradleVersionCatalog, ), + ("services/api/pom.xml", ManifestKind::PomXml), ]; for (path, expected) in cases { assert_eq!( @@ -491,6 +506,7 @@ mod tests { ManifestKind::MixExs, ManifestKind::Csproj, ManifestKind::GradleVersionCatalog, + ManifestKind::PomXml, ] { assert!(kind.workspace_roots().is_none(), "{kind:?}"); assert!( @@ -540,6 +556,9 @@ mod tests { ); } assert!(ManifestKind::CargoToml.unreadable_manifests().is_empty()); + // A `pom.xml` is data and reads fine; what it cannot resolve is reported + // entry by entry, so there is nothing here to declare unreadable. + assert!(ManifestKind::PomXml.unreadable_manifests().is_empty()); } #[test] diff --git a/crates/dependable-core/src/parsers/cargo_toml.rs b/crates/dependable-core/src/parsers/cargo_toml.rs index 338c9e6..bbdd540 100644 --- a/crates/dependable-core/src/parsers/cargo_toml.rs +++ b/crates/dependable-core/src/parsers/cargo_toml.rs @@ -70,6 +70,7 @@ impl Parser for CargoTomlParser { kind: ManifestKind::CargoToml, items, alternate_registries, + notices: Vec::new(), }) } } diff --git a/crates/dependable-core/src/parsers/composer_json.rs b/crates/dependable-core/src/parsers/composer_json.rs index a747435..288160b 100644 --- a/crates/dependable-core/src/parsers/composer_json.rs +++ b/crates/dependable-core/src/parsers/composer_json.rs @@ -47,6 +47,7 @@ impl Parser for ComposerJsonParser { kind: ManifestKind::ComposerJson, items, alternate_registries: Vec::new(), + notices: Vec::new(), }) } } diff --git a/crates/dependable-core/src/parsers/csproj.rs b/crates/dependable-core/src/parsers/csproj.rs index 711b669..c8ddfd6 100644 --- a/crates/dependable-core/src/parsers/csproj.rs +++ b/crates/dependable-core/src/parsers/csproj.rs @@ -67,6 +67,7 @@ impl Parser for CsprojParser { kind: ManifestKind::Csproj, items, alternate_registries: Vec::new(), + notices: Vec::new(), }) } } diff --git a/crates/dependable-core/src/parsers/deno_json.rs b/crates/dependable-core/src/parsers/deno_json.rs index 71ab1df..93898ef 100644 --- a/crates/dependable-core/src/parsers/deno_json.rs +++ b/crates/dependable-core/src/parsers/deno_json.rs @@ -29,6 +29,7 @@ impl Parser for DenoJsonParser { kind: ManifestKind::DenoJson, items, alternate_registries: Vec::new(), + notices: Vec::new(), }) } } diff --git a/crates/dependable-core/src/parsers/go_mod.rs b/crates/dependable-core/src/parsers/go_mod.rs index dddebb6..9932387 100644 --- a/crates/dependable-core/src/parsers/go_mod.rs +++ b/crates/dependable-core/src/parsers/go_mod.rs @@ -52,6 +52,7 @@ impl Parser for GoModParser { kind: ManifestKind::GoMod, items, alternate_registries: Vec::new(), + notices: Vec::new(), }) } } diff --git a/crates/dependable-core/src/parsers/gradle_catalog.rs b/crates/dependable-core/src/parsers/gradle_catalog.rs index c4d930c..3d600d6 100644 --- a/crates/dependable-core/src/parsers/gradle_catalog.rs +++ b/crates/dependable-core/src/parsers/gradle_catalog.rs @@ -121,6 +121,7 @@ impl Parser for GradleCatalogParser { kind: ManifestKind::GradleVersionCatalog, items, alternate_registries: Vec::new(), + notices: Vec::new(), }) } } diff --git a/crates/dependable-core/src/parsers/mix_exs.rs b/crates/dependable-core/src/parsers/mix_exs.rs index 45bb1f4..8e46124 100644 --- a/crates/dependable-core/src/parsers/mix_exs.rs +++ b/crates/dependable-core/src/parsers/mix_exs.rs @@ -56,6 +56,7 @@ impl Parser for MixExsParser { kind: ManifestKind::MixExs, items, alternate_registries: Vec::new(), + notices: Vec::new(), }) } } diff --git a/crates/dependable-core/src/parsers/mod.rs b/crates/dependable-core/src/parsers/mod.rs index 19b2937..41fd4e4 100644 --- a/crates/dependable-core/src/parsers/mod.rs +++ b/crates/dependable-core/src/parsers/mod.rs @@ -20,6 +20,7 @@ pub mod json_scan; pub mod mix_exs; pub mod package_json; pub mod pnpm_workspace; +pub mod pom_xml; pub mod position; pub mod project; pub mod pubspec_yaml; @@ -42,6 +43,7 @@ pub use gradle_catalog::GradleCatalogParser; pub use mix_exs::MixExsParser; pub use package_json::PackageJsonParser; pub use pnpm_workspace::PnpmWorkspaceParser; +pub use pom_xml::PomXmlParser; pub use project::{ProjectMeta, ProjectRole, parse_project}; pub use pubspec_yaml::PubspecYamlParser; pub use pyproject_toml::PyprojectTomlParser; @@ -68,5 +70,6 @@ pub fn parse(kind: ManifestKind, content: &str) -> Result CsprojParser.parse(content), ManifestKind::MixExs => MixExsParser.parse(content), ManifestKind::GradleVersionCatalog => GradleCatalogParser.parse(content), + ManifestKind::PomXml => PomXmlParser.parse(content), } } diff --git a/crates/dependable-core/src/parsers/package_json.rs b/crates/dependable-core/src/parsers/package_json.rs index b6dc778..51cb23b 100644 --- a/crates/dependable-core/src/parsers/package_json.rs +++ b/crates/dependable-core/src/parsers/package_json.rs @@ -36,6 +36,7 @@ impl Parser for PackageJsonParser { kind: ManifestKind::PackageJson, items, alternate_registries: Vec::new(), + notices: Vec::new(), }) } } diff --git a/crates/dependable-core/src/parsers/pnpm_workspace.rs b/crates/dependable-core/src/parsers/pnpm_workspace.rs index c233200..9d6ec63 100644 --- a/crates/dependable-core/src/parsers/pnpm_workspace.rs +++ b/crates/dependable-core/src/parsers/pnpm_workspace.rs @@ -64,6 +64,7 @@ impl Parser for PnpmWorkspaceParser { kind: ManifestKind::PnpmWorkspaceYaml, items, alternate_registries: Vec::new(), + notices: Vec::new(), }) } } diff --git a/crates/dependable-core/src/parsers/pom_xml.rs b/crates/dependable-core/src/parsers/pom_xml.rs new file mode 100644 index 0000000..d19dfed --- /dev/null +++ b/crates/dependable-core/src/parsers/pom_xml.rs @@ -0,0 +1,1352 @@ +//! Parser for a Maven POM (`pom.xml`). +//! +//! The declarative half of a Maven build. A read-only DOM walk (`roxmltree`) over +//! the `` element directly under ``, taking the coordinate +//! from ``/`` and the constraint from ``. The exact +//! byte range of the version *text* is recorded for `--fix` โ€” the one difference +//! from [`csproj`](super::csproj), where a version is an attribute and the span +//! comes from `Attribute::range_value`. +//! +//! # `${property}` +//! +//! A version may name a property instead of stating one: +//! +//! ```xml +//! +//! 4.12.0 +//! +//! +//! +//! com.squareup.okhttp3 +//! okhttp +//! ${okhttp.version} +//! +//! +//! ``` +//! +//! That is the same shape as a Gradle catalog's `version.ref`, and it is resolved +//! the same way and for the same reason: the literal is in **this** file, so the +//! resolution needs no IO and the span of the `` line is a real place +//! to rewrite. A property used by exactly one dependency carries that span; a +//! property several dependencies share carries none, because one line cannot be +//! rewritten to two different versions. Such a dependency is +//! [`PackageSource::Inherited`]: checked and scanned for advisories, never written +//! to. Only a version that is *entirely* one `${โ€ฆ}` is resolved โ€” a composed value +//! like `1.${minor}` has no span that could be replaced with a version. +//! +//! # What is deliberately not resolved +//! +//! `` inheritance, ``, and BOM imports are out of +//! scope: resolving any of them correctly can require fetching the parent POM from +//! a registry, which makes this a resolution engine rather than a parser. Maven's +//! built-in properties (`${project.version}`, `${revision}`, โ€ฆ) are not +//! `` entries and are likewise not resolved. +//! +//! The same holds for a coordinate: `${project.groupId}` โ€” the +//! standard idiom for a sibling module in a multi-module build โ€” names a built-in +//! this file does not state, and the dependency is reported under that literal +//! rather than dropped. Such an entry is +//! [`PackageSource::Unidentified`]: nothing is fetched under a name this file never +//! spelled. Its `` is read and reported all the same, because the two are +//! independent โ€” a POM stating `2.0.13` beside +//! `${project.groupId}` states that version whatever else it leaves to Maven, and +//! blanking it would then have to be explained by a `` that is not involved. +//! +//! A dependency whose version those rules leave unknown is **reported anyway**, with +//! no constraint and no span โ€” the shape a Cargo member's unresolved +//! `dep.workspace = true` already has, which the CLI renders as `(unresolved)` and +//! never fetches, fixes, or claims a status for. Dropping it instead, as the +//! `csproj` parser drops an MSBuild `$(โ€ฆ)` version, would report a POM that +//! inherits half its versions as depending on only the other half. The +//! [`unreadable_manifests`](crate::manifest::ManifestKind::unreadable_manifests) +//! notice is the wrong surface for this: it is keyed on a file *name*, and a +//! `pom.xml` is perfectly readable โ€” it is individual entries within one that are +//! not. + +use std::collections::HashMap; +use std::ops::Range; + +use super::Parser; +use super::position::{line_starts, offset_to_line_col}; +use crate::error::ParseError; +use crate::item::{DependencyKind, Item, PackageSource}; +use crate::manifest::{ManifestKind, ParsedManifest}; + +/// How many `${a}` โ†’ `${b}` **hops** a property chain may take before it is given +/// up on, which also bounds a property that (illegally) refers to itself. +/// +/// A hop is a step from one property to the next, so the longest chain that still +/// resolves is `MAX_PROPERTY_HOPS + 1` properties long: eight hops from `${p0}` +/// reach `p8`, and `p8` is read. [`terminal`] loops one more time than this number +/// for exactly that reason โ€” the first read is not a hop. +const MAX_PROPERTY_HOPS: usize = 8; + +/// Parses `pom.xml`. +pub struct PomXmlParser; + +/// A version literal and where it is written, before it is known whether the +/// dependency that uses it may rewrite it. +pub(super) struct Located { + pub(super) value: String, + span: Option>, +} + +/// Where one dependency's version comes from. +enum Source { + /// Stated on the dependency itself. + Literal(Located), + /// Deferred to a `` entry, named here by the property the chain + /// ends at โ€” the one whose line a rewrite would have to touch. + Property(String), + /// Not knowable from this file alone: absent, a built-in property, a composed + /// value, or a property this POM does not declare. + Unknown, +} + +/// One half of a `groupId:artifactId` coordinate: the text to report it under, and +/// whether that text is the resolved value or the literal this file could not +/// resolve. +struct Half { + text: String, + resolved: bool, +} + +/// One ``, read but not yet resolved. +struct Declared { + name: String, + version: Source, + source: PackageSource, + kind: DependencyKind, +} + +impl Parser for PomXmlParser { + fn parse(&self, content: &str) -> Result { + let doc = roxmltree::Document::parse(content) + .map_err(|e| ParseError::Structural(e.to_string()))?; + let starts = line_starts(content); + let project = doc.root_element(); + + let properties = read_properties(project); + let declared = read_dependencies(project, &properties); + + // A property used by exactly one thing in this document is that + // dependency's own line to fix; one anything else also reads belongs to + // none of them โ€” see `count_property_refs`. + let mut uses: HashMap = HashMap::new(); + count_property_refs(project, &properties, &mut uses); + + let items = declared + .iter() + .map(|entry| { + // The version this entry reports, and whether the line it is written + // on belongs to this entry alone. A `` value several + // dependencies read is nobody's line to rewrite; a literal beside the + // dependency is its own. Read for every entry, including one whose + // coordinate did not resolve: a version stated in this file is + // reported whether or not anything can be fetched under the name + // beside it, because it *is* the version, and reporting nothing there + // would then be explained by a `` that has nothing to do + // with it. + let (located, sole) = match &entry.version { + Source::Literal(version) => (Some(version), true), + Source::Property(name) => ( + Some(&properties[name.as_str()]), + uses.get(name.as_str()).copied() == Some(1), + ), + Source::Unknown => (None, false), + }; + let Some(located) = located else { + return unresolved(entry); + }; + if sole { + item(entry, located, &starts) + } else { + inherited(entry, &located.value) + } + }) + .collect(); + + Ok(ParsedManifest { + kind: ManifestKind::PomXml, + items, + alternate_registries: Vec::new(), + notices: profile_notice(project) + .into_iter() + .chain(unnameable_notice(&declared)) + .collect(), + }) + } +} + +/// Read `` into property name โ†’ stated value. +/// +/// The value is kept exactly as written, `${โ€ฆ}` and all: whether it is a literal or +/// another reference is [`terminal`]'s question, not this one's. +fn read_properties<'a>(project: roxmltree::Node<'a, 'a>) -> HashMap { + let mut out = HashMap::new(); + let Some(table) = child(project, "properties") else { + return out; + }; + for entry in table.children().filter(roxmltree::Node::is_element) { + if let Some(located) = text_of(entry) { + out.insert(entry.tag_name().name().to_owned(), located); + } + } + out +} + +/// Read the `` directly under ``, in source order. +/// +/// Only that one: `` states versions for dependencies +/// declared elsewhere, `` describes the build rather than the +/// artifact, and `` applies conditionally. None of the three is a +/// dependency of this project as written. +fn read_dependencies<'a>( + project: roxmltree::Node<'a, 'a>, + properties: &HashMap, +) -> Vec { + let mut out = Vec::new(); + let Some(list) = child(project, "dependencies") else { + return out; + }; + for node in list.children().filter(roxmltree::Node::is_element) { + if node.tag_name().name() != "dependency" { + continue; + } + // A `${โ€ฆ}` half is interpolated, since `${project.groupId}` aside, a group is + // often a property. A half this file cannot resolve is reported under the + // literal it states rather than dropped โ€” see [`coordinate`]. + let group = coordinate(node, "groupId", properties); + let artifact = coordinate(node, "artifactId", properties); + // A `` naming neither half states nothing at all: there is no + // text to report it under, so there is nothing to report. + if group.is_none() && artifact.is_none() { + continue; + } + let known = group.as_ref().is_some_and(|half| half.resolved) + && artifact.as_ref().is_some_and(|half| half.resolved); + let name = format!( + "{}:{}", + group.map(|half| half.text).unwrap_or_default(), + artifact.map(|half| half.text).unwrap_or_default(), + ); + let scope = child(node, "scope") + .and_then(text_of) + .map(|located| located.value) + .unwrap_or_default(); + out.push(Declared { + name, + // Read whatever the entry states, whether or not the coordinate beside it + // resolved. The two are independent facts: forcing the version to + // `Unknown` here discarded a `2.0.13` the reader can + // see in their own file, and left its absence to be explained by + // `` inheritance that was never involved. + version: version_source(node, properties), + // A `system` dependency is a jar at a path on this machine, not + // something a registry has ever heard of. A coordinate this file cannot + // state is not a name any registry could answer for either โ€” a different + // fact, and one with its own token, because `Local` would claim the + // package has no registry rather than that this file could not say which + // package it is. + source: match (scope.as_str(), known) { + ("system", _) => PackageSource::Local, + (_, false) => PackageSource::Unidentified, + _ => PackageSource::Registry, + }, + kind: dependency_kind(node, &scope), + }); + } + out +} + +/// Which section a `` belongs to, read from `` and +/// `` rather than guessed. +/// +/// `test` is the only scope that names a section [`DependencyKind`] has: `provided` +/// and `runtime` both describe a runtime dependency whose *provider* differs, which +/// is not the distinction this records. +fn dependency_kind(node: roxmltree::Node<'_, '_>, scope: &str) -> DependencyKind { + if scope == "test" { + return DependencyKind::Dev; + } + let optional = child(node, "optional") + .and_then(text_of) + .is_some_and(|located| located.value == "true"); + if optional { + DependencyKind::Optional + } else { + DependencyKind::Normal + } +} + +/// Where a ``'s version comes from, as far as this file can say. +fn version_source(node: roxmltree::Node<'_, '_>, properties: &HashMap) -> Source { + // No `` at all: supplied by `` or a ``, + // neither of which is read here. + let Some(located) = child(node, "version").and_then(text_of) else { + return Source::Unknown; + }; + let Some(reference) = interpolation(&located.value) else { + // A composed value (`1.${minor}`) states no version this file can rewrite. + return if located.value.contains('$') { + Source::Unknown + } else { + Source::Literal(located) + }; + }; + match terminal(reference, properties) { + Some(name) => Source::Property(name.to_owned()), + None => Source::Unknown, + } +} + +/// The name of the property a `${โ€ฆ}` chain ends at โ€” the one that states a literal. +/// +/// `None` when the chain leaves this file, states nothing, or does not terminate. +fn terminal<'a>(start: &str, properties: &'a HashMap) -> Option<&'a str> { + let mut name = start; + // Inclusive: `MAX_PROPERTY_HOPS` hops means one more property read than hops + // taken, since arriving at the first property costs no hop. + for _ in 0..=MAX_PROPERTY_HOPS { + let (key, located) = properties.get_key_value(name)?; + match interpolation(&located.value) { + Some(next) => name = next, + None if located.value.contains('$') => return None, + None => return Some(key.as_str()), + } + } + None +} + +/// The inner name of a value that is *entirely* one `${โ€ฆ}` reference. +fn interpolation(value: &str) -> Option<&str> { + let inner = value.strip_prefix("${")?.strip_suffix('}')?; + if inner.is_empty() || inner.contains(['$', '{', '}']) { + return None; + } + Some(inner) +} + +/// One half of a coordinate, with `${โ€ฆ}` resolved against `properties` where it can +/// be โ€” and reported as written where it cannot. +/// +/// `None` only when the element is absent or states nothing. An *unresolvable* half +/// (`${project.groupId}`, the standard idiom for a sibling module, or a property +/// declared in a ``) still yields the literal text, because dropping the +/// dependency would report a POM as depending on fewer things than it declares โ€” +/// the silent omission this parser exists to avoid, and an asymmetry with +/// ``, which is reported unresolved rather than deleted. +fn coordinate( + node: roxmltree::Node<'_, '_>, + tag: &str, + properties: &HashMap, +) -> Option { + let located = child(node, tag).and_then(text_of)?; + let resolved = match interpolation(&located.value) { + Some(reference) => terminal(reference, properties) + .map(|name| properties[name].value.clone()) + .map(|text| Half { + text, + resolved: true, + }), + None if located.value.contains('$') => None, + None => Some(Half { + text: located.value.clone(), + resolved: true, + }), + }; + Some(resolved.unwrap_or(Half { + text: located.value, + resolved: false, + })) +} + +/// Count every `${โ€ฆ}` reference in the document, by the property its chain ends at. +/// +/// The whole document, because sole ownership of a `` line is a fact +/// about that line and not about ``. A POM that states +/// `32.1.3-jre` and reads it from both the top-level +/// `guava` and a ``-only `guava-gwt` has one line and two readers; +/// counting only the top-level list makes `guava` the sole reader, so `--fix` +/// rewrites the `` line and silently moves `guava-gwt` with it โ€” a +/// different artifact, never fetched, never validated, never named in the fix +/// record. `` and a ``'s `` are the same +/// story. This is the `count_version_refs` rule of +/// [`gradle_catalog`](super::gradle_catalog), applied to the same defect. +/// +/// A `` value that is *entirely* one `${โ€ฆ}` is a link in a chain +/// rather than a reader of it, and is skipped: the reference is counted against +/// the property the chain ends at when whoever started the chain is counted. +/// A composed value (`${core.version}-jre`) **is** a reader, and is counted. +fn count_property_refs( + project: roxmltree::Node<'_, '_>, + properties: &HashMap, + uses: &mut HashMap, +) { + let table = child(project, "properties"); + for node in project.descendants().filter(roxmltree::Node::is_text) { + let Some(text) = node.text() else { continue }; + // A top-level `` entry's own value: a pure `${โ€ฆ}` is a chain + // link, not a use of the property it names. + let chain_link = table + .is_some_and(|table| node.parent().and_then(|entry| entry.parent()) == Some(table)) + && interpolation(text.trim()).is_some(); + if chain_link { + continue; + } + for reference in references(text) { + if let Some(name) = terminal(reference, properties) { + *uses.entry(name.to_owned()).or_default() += 1; + } + } + } +} + +/// Every `${โ€ฆ}` reference in a string, in order, by the name each names. +/// +/// A version may compose one (`1.${minor}`) and a plugin configuration may hold +/// several, so this scans rather than matching the whole value the way +/// [`interpolation`] does. +fn references(value: &str) -> impl Iterator { + let mut rest = value; + std::iter::from_fn(move || { + loop { + let start = rest.find("${")?; + let after = &rest[start + 2..]; + let Some(end) = after.find('}') else { + rest = ""; + return None; + }; + let inner = &after[..end]; + rest = &after[end + 1..]; + if !inner.is_empty() && !inner.contains(['$', '{']) { + return Some(inner); + } + } + }) +} + +/// Say that a `` block was seen and its dependencies were not read. +/// +/// A profile applies conditionally โ€” on a JDK version, an activated property, an +/// operating system โ€” so its dependencies are not this project's as written, and +/// parsing them would state as fact something that holds only under a condition +/// this file does not evaluate. Staying out is the decision; staying *silent* +/// about it is not, because a POM that declares every one of its dependencies +/// inside a profile then lists as `(0 dependencies)`, which reads as complete and +/// is not. +fn profile_notice(project: roxmltree::Node<'_, '_>) -> Option { + let profiles = child(project, "profiles")?; + let count = profiles + .descendants() + .filter(|node| { + node.is_element() + && node.tag_name().name() == "dependency" + && node + .parent() + .is_some_and(|list| list.tag_name().name() == "dependencies") + }) + .count(); + if count == 0 { + return None; + } + Some(format!( + "{count} {} declared inside {} not listed: a profile applies conditionally, so its dependencies are not this project's as written", + if count == 1 { + "dependency" + } else { + "dependencies" + }, + if count == 1 { "is" } else { "are" }, + )) +} + +/// Say that some entries name a coordinate this file cannot state, so nothing was +/// fetched for them however clearly they state a version. +/// +/// The sibling of the `` notice above, and for the same reason: an entry +/// that reports no status reads as an oversight unless the run says why. It is kept +/// apart from the check's own "this version comes from a ``" notice because +/// the two are different failures โ€” here the *name* went unread, and the version may +/// be stated right there in the file, so borrowing the version story would tell the +/// reader two false things at once and point them at a parent POM that does not +/// exist. +fn unnameable_notice(declared: &[Declared]) -> Option { + let mut names: Vec<&str> = declared + .iter() + .filter(|entry| entry.source == PackageSource::Unidentified) + .map(|entry| entry.name.as_str()) + .collect(); + names.sort_unstable(); + names.dedup(); + if names.is_empty() { + return None; + } + let (subject, verb) = if names.len() == 1 { + ("dependency names a coordinate", "does") + } else { + ("dependencies name coordinates", "do") + }; + Some(format!( + "{} {subject} this file {verb} not state โ€” a `${{project.*}}` built-in, a \ + `` left to a ``, or a property declared elsewhere โ€” so \ + nothing was fetched for {}: {}", + names.len(), + if names.len() == 1 { "it" } else { "them" }, + names.join(", ") + )) +} + +/// The first direct child element named `tag`. +fn child<'a>(node: roxmltree::Node<'a, 'a>, tag: &str) -> Option> { + node.children() + .find(|child| child.is_element() && child.tag_name().name() == tag) +} + +/// An element's text content and the byte span of the text itself, trimmed of the +/// whitespace a pretty-printed POM puts around it. +/// +/// **Every** text node is concatenated, because that is the value Maven reads. +/// `1.0.0` then reads as `1.0.0` in one place and `1.0` in the +/// other. +pub(super) fn text_of(node: roxmltree::Node<'_, '_>) -> Option { + let mut texts = node.children().filter(roxmltree::Node::is_text); + let first = texts.next()?; + let raw = first.text()?; + let mut joined = raw.to_owned(); + let mut single = true; + for rest in texts { + single = false; + joined.push_str(rest.text().unwrap_or_default()); + } + let value = joined.trim(); + if value.is_empty() { + return None; + } + // Concatenating across a comment splices whatever a pretty-printer put around + // it into the middle of the value: a `` broken over three lines joins + // to `1.0\n \n .0`, which is no version anyone wrote and which + // Maven itself would reject. Trimming the ends does not reach it, and stitching + // the halves together would state a version the file does not. Refusing leaves + // the dependency unresolved, which is what it is. + if !single && value.chars().any(char::is_whitespace) { + return None; + } + let range = first.range(); + let faithful = single && range.len() == raw.len(); + let start = range.start + (raw.len() - raw.trim_start().len()); + let len = value.len(); + Some(Located { + value: value.to_owned(), + span: faithful.then(|| start..start + len), + }) +} + +/// An item pointing at the version literal that governs it, wherever in this file +/// that literal is written. +fn item(entry: &Declared, version: &Located, starts: &[usize]) -> Item { + let Some(span) = &version.span else { + return inherited(entry, &version.value); + }; + let (line, col_start) = offset_to_line_col(starts, span.start); + Item { + name: entry.name.clone(), + version_constraint: version.value.clone(), + source: entry.source, + version_line: line, + version_col_start: col_start, + version_col_end: col_start + span.len(), + registry: None, + locked_version: None, + kind: entry.kind, + } +} + +/// An item whose version is known but whose line is not this dependency's to +/// rewrite โ€” a `` entry several dependencies share. +/// +/// The entry's own source survives when it says something +/// [`Inherited`](PackageSource::Inherited) would contradict: a `system` +/// jar is [`Local`](PackageSource::Local) โ€” there is no registry that has heard of +/// it โ€” whether or not its version happened to need reconstructing. Only a registry +/// entry becomes `Inherited`, which is the case the variant describes. +fn inherited(entry: &Declared, version: &str) -> Item { + Item { + name: entry.name.clone(), + version_constraint: version.to_owned(), + source: match entry.source { + PackageSource::Registry => PackageSource::Inherited, + other => other, + }, + version_line: 0, + version_col_start: 0, + version_col_end: 0, + registry: None, + locked_version: None, + kind: entry.kind, + } +} + +/// An item this file states no version for: it is reported, and nothing is claimed +/// about it. +fn unresolved(entry: &Declared) -> Item { + inherited(entry, "") +} + +#[cfg(test)] +mod tests { + use super::*; + + fn parse(content: &str) -> ParsedManifest { + PomXmlParser.parse(content).unwrap() + } + + fn sliced<'a>(content: &'a str, item: &Item) -> &'a str { + let line = content.lines().nth(item.version_line).unwrap(); + &line[item.version_col_start..item.version_col_end] + } + + fn find<'a>(manifest: &'a ParsedManifest, name: &str) -> &'a Item { + manifest + .items + .iter() + .find(|item| item.name == name) + .unwrap_or_else(|| panic!("no item {name}")) + } + + fn pom(body: &str) -> String { + format!( + "\n\ + {body}\n" + ) + } + + #[test] + fn parses_literal_versions_with_positions() { + let content = pom(" \n\ + \x20 \n\ + \x20 com.google.guava\n\ + \x20 guava\n\ + \x20 32.1.3-jre\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + assert_eq!(m.kind, ManifestKind::PomXml); + let guava = find(&m, "com.google.guava:guava"); + assert_eq!(guava.version_constraint, "32.1.3-jre"); + assert_eq!(sliced(&content, guava), "32.1.3-jre"); + assert_eq!(guava.source, PackageSource::Registry); + assert!(guava.is_rewritable()); + } + + /// A property used once carries the `` line that governs it, so + /// `--fix` rewrites where the version is actually written. + #[test] + fn a_property_used_once_carries_the_line_that_states_it() { + let content = pom(" \n\ + \x20 4.12.0\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 com.squareup.okhttp3\n\ + \x20 okhttp\n\ + \x20 ${okhttp.version}\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + let okhttp = find(&m, "com.squareup.okhttp3:okhttp"); + assert_eq!(okhttp.version_constraint, "4.12.0"); + assert_eq!(sliced(&content, okhttp), "4.12.0"); + assert!(okhttp.is_rewritable()); + } + + /// One line cannot be rewritten to two different versions, so a shared property + /// is resolved and checked but never written to. + #[test] + fn a_shared_property_is_resolved_but_never_rewritten() { + let content = pom(" \n\ + \x20 2.17.0\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 com.fasterxml.jackson.core\n\ + \x20 jackson-core\n\ + \x20 ${jackson.version}\n\ + \x20 \n\ + \x20 \n\ + \x20 com.fasterxml.jackson.core\n\ + \x20 jackson-databind\n\ + \x20 ${jackson.version}\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + for artifact in ["jackson-core", "jackson-databind"] { + let item = find(&m, &format!("com.fasterxml.jackson.core:{artifact}")); + assert_eq!(item.version_constraint, "2.17.0", "{artifact}"); + assert_eq!(item.source, PackageSource::Inherited, "{artifact}"); + assert!(item.is_checkable(), "{artifact}"); + assert!(!item.is_rewritable(), "{artifact}"); + } + } + + /// The required behaviour: a version this file cannot resolve is reported with + /// no constraint rather than dropped, so the dependency list is never quietly + /// short. + #[test] + fn an_unresolvable_version_is_reported_without_a_constraint() { + let content = pom(" \n\ + \x20 org.springframework.boot\n\ + \x20 spring-boot-starter-parent\n\ + \x20 3.2.5\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 org.springframework.boot\n\ + \x20 spring-boot-starter-web\n\ + \x20 \n\ + \x20 \n\ + \x20 org.example\n\ + \x20 sibling\n\ + \x20 ${project.version}\n\ + \x20 \n\ + \x20 \n\ + \x20 org.example\n\ + \x20 composed\n\ + \x20 1.${minor}\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + // The `` is not a dependency, and its version is not read. + let names: Vec<&str> = m.items.iter().map(|i| i.name.as_str()).collect(); + assert_eq!( + names, + vec![ + "org.springframework.boot:spring-boot-starter-web", + "org.example:sibling", + "org.example:composed", + ] + ); + for item in &m.items { + assert!(item.version_constraint.is_empty(), "{item:?}"); + assert_eq!(item.source, PackageSource::Inherited, "{item:?}"); + assert!(!item.is_checkable(), "{item:?}"); + assert!(!item.has_position(), "{item:?}"); + } + } + + /// Versions stated for dependencies declared elsewhere, for the build, or under + /// a condition are not this project's dependencies. + #[test] + fn only_the_projects_own_dependencies_are_read() { + let content = pom(" \n\ + \x20 \n\ + \x20 \n\ + \x20 managed\n\ + \x20 managed\n\ + \x20 1.0.0\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 plugin\n\ + \x20 plugin\n\ + \x20 2.0.0\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 real\n\ + \x20 real\n\ + \x20 3.0.0\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + let names: Vec<&str> = m.items.iter().map(|i| i.name.as_str()).collect(); + assert_eq!(names, vec!["real:real"]); + } + + /// `` and `` are stated in the manifest, so they are read + /// rather than guessed at from a file name. + #[test] + fn scope_and_optional_name_the_section() { + let content = pom(" \n\ + \x20 \n\ + \x20 org.junit.jupiter\n\ + \x20 junit-jupiter\n\ + \x20 5.10.2\n\ + \x20 test\n\ + \x20 \n\ + \x20 \n\ + \x20 org.example\n\ + \x20 extra\n\ + \x20 1.0.0\n\ + \x20 true\n\ + \x20 \n\ + \x20 \n\ + \x20 org.example\n\ + \x20 vendored\n\ + \x20 1.0.0\n\ + \x20 system\n\ + \x20 /opt/vendored.jar\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + assert_eq!( + find(&m, "org.junit.jupiter:junit-jupiter").kind, + DependencyKind::Dev + ); + assert_eq!(find(&m, "org.example:extra").kind, DependencyKind::Optional); + + let vendored = find(&m, "org.example:vendored"); + assert_eq!(vendored.source, PackageSource::Local); + assert!(!vendored.is_checkable(), "a system jar has no registry"); + } + + /// A property may name another; the span that governs is the one that finally + /// states a literal. + #[test] + fn a_property_chain_resolves_to_the_line_that_states_a_literal() { + let content = pom(" \n\ + \x20 real.version\n\ + \x20 9.9.9\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 g\n\ + \x20 a\n\ + \x20 ${alias}\n\ + \x20 \n\ + \x20 \n"); + // `` states a literal of its own, so it is the terminal property. + let m = parse(&content); + assert_eq!(find(&m, "g:a").version_constraint, "real.version"); + + let chained = content.replace( + "real.version", + "${real.version}", + ); + let m = parse(&chained); + let item = find(&m, "g:a"); + assert_eq!(item.version_constraint, "9.9.9"); + assert_eq!(sliced(&chained, item), "9.9.9"); + } + + /// A cycle must terminate rather than spin, and states no version. + #[test] + fn a_property_cycle_resolves_to_nothing() { + let content = pom(" \n\ + \x20 ${b}\n\ + \x20 ${a}\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 g\n\ + \x20 a\n\ + \x20 ${a}\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + assert_eq!(find(&m, "g:a").version_constraint, ""); + } + + /// A group stated as a property still names a package. One that is not + /// resolvable names nothing that could be looked up โ€” but the dependency is + /// still declared, so it is reported under the literal it states rather than + /// deleted from the manifest's own list. + #[test] + fn a_coordinate_may_be_stated_by_property() { + let content = pom(" \n\ + \x20 org.example\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 ${group}\n\ + \x20 named\n\ + \x20 1.0.0\n\ + \x20 \n\ + \x20 \n\ + \x20 ${project.groupId}\n\ + \x20 unnameable\n\ + \x20 1.0.0\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + let names: Vec<&str> = m.items.iter().map(|i| i.name.as_str()).collect(); + assert_eq!( + names, + vec!["org.example:named", "${project.groupId}:unnameable"] + ); + } + + /// `${project.groupId}` is *the* idiom for a sibling module in a multi-module + /// build, and a POM using it for two of its three dependencies must not list as + /// depending on one. Reported under the literal it states, as + /// [`PackageSource::Unidentified`] โ€” never fetched, because no registry has heard + /// of a package by that name โ€” rather than dropped in silence. + #[test] + fn a_group_this_file_cannot_resolve_is_reported_not_dropped() { + let content = pom(" \n\ + \x20 \n\ + \x20 ${project.groupId}\n\ + \x20 app-core\n\ + \x20 ${project.version}\n\ + \x20 \n\ + \x20 \n\ + \x20 org.slf4j\n\ + \x20 slf4j-api\n\ + \x20 2.0.13\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + assert_eq!(m.items.len(), 2, "both dependencies are declared"); + let sibling = find(&m, "${project.groupId}:app-core"); + // `${project.version}` is a built-in too, so this entry states no version + // either โ€” the coordinate and the version are unread independently. + assert_eq!(sibling.version_constraint, ""); + assert_eq!(sibling.source, PackageSource::Unidentified); + assert!( + !sibling.is_checkable(), + "no coordinate, so nothing to fetch" + ); + assert!(!sibling.is_rewritable()); + assert_eq!(find(&m, "org.slf4j:slf4j-api").version_constraint, "2.0.13"); + assert_eq!( + m.notices.len(), + 1, + "the reader is told why one of the two reports nothing: {:?}", + m.notices + ); + assert!( + m.notices[0].contains("${project.groupId}:app-core"), + "{:?}", + m.notices + ); + } + + /// A coordinate this file cannot state and a version it can are independent + /// facts, and the standard multi-module idiom states both at once. Blanking the + /// version because the name did not resolve dropped a `2.0.13` written in plain + /// sight, and the check then explained its absence with `` inheritance + /// that is not involved. + #[test] + fn an_unnameable_coordinate_still_reports_the_version_it_states() { + let content = pom(" \n\ + \x20 \n\ + \x20 ${project.groupId}\n\ + \x20 app-core\n\ + \x20 2.0.13\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + let sibling = find(&m, "${project.groupId}:app-core"); + assert_eq!( + sibling.version_constraint, "2.0.13", + "the version is written in this file and is reported" + ); + assert_eq!(sibling.source, PackageSource::Unidentified); + assert!( + !sibling.is_checkable(), + "no registry has heard of a package named by an unresolved `${{โ€ฆ}}`" + ); + assert!(!sibling.is_rewritable()); + assert_eq!(m.notices.len(), 1, "{:?}", m.notices); + assert!( + m.notices[0].contains("app-core") && m.notices[0].contains("coordinate"), + "the unread coordinate is the story, not a version borrowed elsewhere: {:?}", + m.notices + ); + } + + /// A `` this file never states is the same omission by a shorter + /// route: the artifact is named, so the entry is reported under what there is โ€” + /// and so is the version beside it, which the file states outright. + /// A `` naming neither half states nothing at all and is skipped. + #[test] + fn a_missing_coordinate_half_is_still_reported_under_the_other() { + let content = pom(" \n\ + \x20 \n\ + \x20 orphan\n\ + \x20 1.0.0\n\ + \x20 \n\ + \x20 \n\ + \x20 2.0.0\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + let names: Vec<&str> = m.items.iter().map(|i| i.name.as_str()).collect(); + assert_eq!(names, vec![":orphan"]); + assert_eq!( + m.items[0].version_constraint, "1.0.0", + "the version is stated here; only the coordinate is not" + ); + assert_eq!(m.items[0].source, PackageSource::Unidentified); + assert!(!m.items[0].is_checkable()); + assert!(!m.items[0].is_rewritable()); + } + + /// A `system` jar is a file on this machine whatever shape its version takes. + /// Reconstructing the version โ€” here around a comment โ€” must not relabel the + /// entry as one a registry could answer for: `Inherited` is checkable, and the + /// run would go and ask crates of a jar no registry has ever published. + #[test] + fn a_system_scoped_jar_stays_local_when_its_version_is_reconstructed() { + let content = pom(" \n\ + \x20 \n\ + \x20 g\n\ + \x20 cmtsys\n\ + \x20 1.0.0\n\ + \x20 system\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + let item = find(&m, "g:cmtsys"); + assert_eq!(item.version_constraint, "1.0.0"); + assert_eq!(item.source, PackageSource::Local); + assert!(!item.is_checkable(), "a system jar has no registry to ask"); + } + + /// The same trick spread over lines splices the pretty-printer's indentation + /// into the middle of the version. `1.0\n \n .0` is no version + /// anyone wrote; stating it as the constraint would send it to the registry as + /// if it were. + #[test] + fn a_version_split_across_lines_states_no_version() { + let content = pom(" \n\ + \x20 \n\ + \x20 g\n\ + \x20 a\n\ + \x20 \n\ + \x20 1.0\n\ + \x20 \n\ + \x20 .0\n\ + \x20 \n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + let item = find(&m, "g:a"); + assert_eq!(item.version_constraint, ""); + assert!(!item.is_checkable()); + } + + /// Whitespace around a version is formatting, not part of it, and the span has + /// to exclude it or `--fix` would rewrite the indentation too. + #[test] + fn a_span_excludes_the_whitespace_around_the_text() { + let content = pom(" \n\ + \x20 \n\ + \x20 g\n\ + \x20 a\n\ + \x20 \n\ + \x20 1.2.3\n\ + \x20 \n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + let item = find(&m, "g:a"); + assert_eq!(item.version_constraint, "1.2.3"); + assert_eq!(sliced(&content, item), "1.2.3"); + } + + /// An `` block names packages that must **not** be pulled in. Its + /// coordinates are nested, and reading only direct children is what keeps them + /// from being mistaken for the dependency's own. + #[test] + fn an_exclusion_is_not_a_dependency() { + let content = pom(" \n\ + \x20 \n\ + \x20 org.example\n\ + \x20 app\n\ + \x20 1.0.0\n\ + \x20 \n\ + \x20 \n\ + \x20 commons-logging\n\ + \x20 commons-logging\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + let names: Vec<&str> = m.items.iter().map(|i| i.name.as_str()).collect(); + assert_eq!(names, vec!["org.example:app"]); + assert_eq!(sliced(&content, &m.items[0]), "1.0.0"); + } + + #[test] + fn malformed_xml_is_a_structural_error() { + assert!(PomXmlParser.parse("").is_err()); + } + + #[test] + fn a_pom_without_dependencies_yields_none() { + let m = parse(&pom(" solo\n")); + assert!(m.items.is_empty()); + assert!(m.notices.is_empty()); + } + + /// A comment splits the version into two text nodes. Reading only the first + /// states `1.0` โ€” a version this file never declares, which would then be + /// fetched and evaluated as if it were the real constraint. + #[test] + fn a_version_interrupted_by_a_comment_is_read_whole() { + let content = pom(" \n\ + \x20 \n\ + \x20 g\n\ + \x20 a\n\ + \x20 1.0.0\n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + let item = find(&m, "g:a"); + assert_eq!( + item.version_constraint, "1.0.0", + "every text node is the value Maven reads" + ); + assert!( + !item.is_rewritable(), + "the source bytes are not the value's bytes, so there is nothing to rewrite" + ); + } + + /// The sibling cases: an escaped character and a `CDATA` section both state the + /// whole version, and neither offers bytes a rewrite could replace. + #[test] + fn an_escaped_or_wrapped_version_is_read_whole_and_never_rewritten() { + for spelling in ["1.0.0", ""] { + let content = pom(&format!( + " \n\ + \x20 \n\ + \x20 g\n\ + \x20 a\n\ + \x20 {spelling}\n\ + \x20 \n\ + \x20 \n" + )); + let m = parse(&content); + let item = find(&m, "g:a"); + assert_eq!(item.version_constraint, "1.0.0", "{spelling}"); + assert!(!item.is_rewritable(), "{spelling}"); + } + } + + /// A property the top-level list reads once but a `` block reads too + /// has two readers, not one. Rewriting its line would move an artifact that was + /// never fetched, never validated, and never named in the fix record. + #[test] + fn a_property_a_profile_also_reads_is_not_one_dependencys_to_rewrite() { + let content = pom(" \n\ + \x20 32.1.3-jre\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 com.google.guava\n\ + \x20 guava\n\ + \x20 ${lib.version}\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 gwt\n\ + \x20 \n\ + \x20 \n\ + \x20 com.google.guava\n\ + \x20 guava-gwt\n\ + \x20 ${lib.version}\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + let guava = find(&m, "com.google.guava:guava"); + assert_eq!(guava.version_constraint, "32.1.3-jre"); + assert!( + guava.is_checkable(), + "the version is known and worth checking" + ); + assert!( + !guava.is_rewritable(), + "the profile reads the same line, and would move with it" + ); + } + + /// The same rule for the other two readers a POM has, so no single reader is + /// privileged: `` and a build plugin's own version. + #[test] + fn a_property_read_elsewhere_in_the_document_is_never_rewritable() { + let elsewhere = [ + " \n\ + \x20 \n\ + \x20 \n\ + \x20 g\n\ + \x20 managed\n\ + \x20 ${lib.version}\n\ + \x20 \n\ + \x20 \n\ + \x20 \n", + " \n\ + \x20 \n\ + \x20 \n\ + \x20 g\n\ + \x20 plug\n\ + \x20 ${lib.version}\n\ + \x20 \n\ + \x20 \n\ + \x20 \n", + ]; + for other in elsewhere { + let content = pom(&format!( + " \n\ + \x20 1.2.3\n\ + \x20 \n\ + {other}\ + \x20 \n\ + \x20 \n\ + \x20 g\n\ + \x20 a\n\ + \x20 ${{lib.version}}\n\ + \x20 \n\ + \x20 \n" + )); + let m = parse(&content); + let item = find(&m, "g:a"); + assert_eq!(item.version_constraint, "1.2.3", "{other}"); + assert!(!item.is_rewritable(), "{other}"); + } + } + + /// A composed `` value reads the property it names, so the line it + /// names is shared; a value that is *only* a reference is a link in a chain and + /// is not itself a reader, or no chained property could ever be rewritten. + #[test] + fn a_chain_link_is_not_a_reader_but_a_composed_value_is() { + let chained = pom(" \n\ + \x20 ${real.version}\n\ + \x20 9.9.9\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 g\n\ + \x20 a\n\ + \x20 ${alias}\n\ + \x20 \n\ + \x20 \n"); + assert!( + find(&parse(&chained), "g:a").is_rewritable(), + "one dependency, one chain, one line to rewrite" + ); + + // A second property composing the same line is a reader of it, so the line + // is no longer any one dependency's to rewrite. + let composed = chained + .replace( + "${alias}", + "${real.version}", + ) + .replace( + "${real.version}", + "${real.version}-jre", + ); + let m = parse(&composed); + let item = find(&m, "g:a"); + assert_eq!(item.version_constraint, "9.9.9"); + assert!( + !item.is_rewritable(), + "`` composes the same line into a second value" + ); + } + + /// Eight hops is what the constant says, so eight hops has to resolve. + #[test] + fn a_chain_resolves_up_to_the_documented_number_of_hops() { + let chain = |hops: usize| { + let mut properties = String::from(" \n"); + for hop in 0..hops { + properties.push_str(&format!(" ${{p{}}}\n", hop + 1)); + } + properties.push_str(&format!(" 9.9.9\n \n")); + let content = pom(&format!( + "{properties} \n\ + \x20 \n\ + \x20 g\n\ + \x20 a\n\ + \x20 ${{p0}}\n\ + \x20 \n\ + \x20 \n" + )); + parse(&content).items[0].version_constraint.clone() + }; + assert_eq!(chain(MAX_PROPERTY_HOPS), "9.9.9", "the documented limit"); + assert_eq!( + chain(MAX_PROPERTY_HOPS + 1), + "", + "one hop past it states nothing, rather than spinning" + ); + } + + /// Not parsing conditional dependencies is the decision; not *saying so* would + /// leave a POM that declares all of them in a profile listing as empty. + #[test] + fn a_profiles_block_holding_dependencies_is_announced() { + let content = pom(" \n\ + \x20 \n\ + \x20 native\n\ + \x20 \n\ + \x20 \n\ + \x20 g\n\ + \x20 a\n\ + \x20 1.0.0\n\ + \x20 \n\ + \x20 \n\ + \x20 g\n\ + \x20 b\n\ + \x20 2.0.0\n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \x20 \n"); + let m = parse(&content); + assert!(m.items.is_empty(), "a profile dependency is conditional"); + assert_eq!(m.notices.len(), 1, "{:?}", m.notices); + assert!( + m.notices[0].contains("2 dependencies declared inside "), + "{:?}", + m.notices + ); + } + + /// A profile that declares no dependency of its own has nothing to announce. + #[test] + fn a_profile_without_dependencies_says_nothing() { + let content = pom(" \n\ + \x20 \n\ + \x20 release\n\ + \x20 \n\ + \x20 true\n\ + \x20 \n\ + \x20 \n\ + \x20 \n"); + assert!(parse(&content).notices.is_empty()); + } +} diff --git a/crates/dependable-core/src/parsers/project.rs b/crates/dependable-core/src/parsers/project.rs index 9b9a35a..abf158a 100644 --- a/crates/dependable-core/src/parsers/project.rs +++ b/crates/dependable-core/src/parsers/project.rs @@ -74,6 +74,7 @@ pub fn parse_project(kind: ManifestKind, content: &str) -> ProjectMeta { ManifestKind::GoMod => go_module(content), ManifestKind::PubspecYaml => pubspec(content), ManifestKind::MixExs => mix(content), + ManifestKind::PomXml => pom(content), // A `pnpm-workspace.yaml` exists to hold catalogs; `Directory.Packages.props` // exists to hold central versions. A Gradle version catalog is the same shape // again โ€” the project it serves is described by a build script. None names a @@ -215,6 +216,46 @@ fn mix(content: &str) -> ProjectMeta { } } +/// `pom.xml`: the ``, ``, and `` directly under +/// ``. +/// +/// The coordinate is the identity, and it is spelled `groupId:artifactId` โ€” the same +/// way the POM parser names the dependencies it reads, so a project and a dependency +/// on it are the same string. A POM that states no `` of its own inherits it +/// from its ``, which is out of reach here (see +/// [`pom_xml`](super::pom_xml)); the bare `artifactId` is reported rather than a +/// coordinate that would be half guessed. A `` that is a property +/// (`${revision}`) is likewise not a literal this file states. +/// +/// The element's text is read by [`pom_xml::text_of`](super::pom_xml::text_of) and +/// not by `Node::text`, so this reader and the dependency reader agree on what an +/// element says: `Node::text` returns the *first* text node only, which makes +/// `1.0.0` read `1.0` here and `1.0.0` there โ€” +/// one file, one element, two answers, and one of them a version nobody wrote. +fn pom(content: &str) -> ProjectMeta { + let Ok(doc) = roxmltree::Document::parse(content) else { + return unnamed(); + }; + let project = doc.root_element(); + let field = |tag: &str| { + project + .children() + .find(|child| child.is_element() && child.tag_name().name() == tag) + .and_then(super::pom_xml::text_of) + .map(|located| located.value) + .filter(|text| !text.contains('$')) + }; + let name = field("artifactId").map(|artifact| match field("groupId") { + Some(group) => format!("{group}:{artifact}"), + None => artifact, + }); + ProjectMeta { + role: role_for(name.as_deref()), + name, + version: field("version").map(PackageField::Literal), + } +} + /// A manifest that declares dependency versions for others but no package of its own. fn workspace_meta() -> ProjectMeta { ProjectMeta { @@ -392,6 +433,84 @@ mod tests { ); } + /// A POM names itself by coordinate โ€” the same string a dependency on it would + /// use. Anything it leaves to its `` is left unstated rather than guessed. + #[test] + fn a_pom_is_named_by_its_coordinate() { + let meta = parse_project( + ManifestKind::PomXml, + "\n org.example\n \ + demo\n 1.4.0\n\n", + ); + assert_eq!(meta.name.as_deref(), Some("org.example:demo")); + assert_eq!(meta.literal_version(), Some("1.4.0")); + assert_eq!(meta.role, ProjectRole::Package); + + // Group and version both inherited from a parent, and a `` of its own + // whose fields must not be mistaken for the project's. + let inheriting = parse_project( + ManifestKind::PomXml, + "\n \n org.parent\n \ + parent\n 9.9.9\n \ + \n child\n\n", + ); + assert_eq!(inheriting.name.as_deref(), Some("child")); + assert_eq!(inheriting.literal_version(), None); + + // `${revision}` is CI-friendly versioning, not a version. + let templated = parse_project( + ManifestKind::PomXml, + "\n demo\n \ + ${revision}\n\n", + ); + assert_eq!(templated.literal_version(), None); + + assert_eq!( + parse_project(ManifestKind::PomXml, "` a comment or a character reference splits in two used to read as + /// its first half here while the dependency reader read it whole โ€” the project + /// then claimed version `1.0` of itself while a sibling POM depending on it read + /// `1.0.0`, and a monorepo could not match the two. + #[test] + fn a_split_element_reads_the_same_here_as_in_the_dependency_reader() { + let split = parse_project( + ManifestKind::PomXml, + "\n org.example\n \ + demo\n \ + 1.0.0\n\n", + ); + assert_eq!(split.name.as_deref(), Some("org.example:demo")); + assert_eq!(split.literal_version(), Some("1.0.0")); + + // And the dependency reader, over the same shape, agrees. + let items = crate::parsers::parse( + ManifestKind::PomXml, + "\n \n \n \ + org.example\n \ + demo\n \ + 1.0.0\n \ + \n \n\n", + ) + .expect("the POM parses") + .items; + assert_eq!(items.len(), 1); + assert_eq!(items[0].name, "org.example:demo"); + assert_eq!(items[0].version_constraint, "1.0.0"); + + // A character reference is read whole by both too. + let entity = parse_project( + ManifestKind::PomXml, + "\n demo\n \ + 1.0.0\n\n", + ); + assert_eq!(entity.literal_version(), Some("1.0.0")); + } + #[test] fn manifests_named_by_their_file_are_unnamed() { assert_eq!( diff --git a/crates/dependable-core/src/parsers/pubspec_yaml.rs b/crates/dependable-core/src/parsers/pubspec_yaml.rs index 6de88f2..fde581e 100644 --- a/crates/dependable-core/src/parsers/pubspec_yaml.rs +++ b/crates/dependable-core/src/parsers/pubspec_yaml.rs @@ -70,6 +70,7 @@ impl Parser for PubspecYamlParser { kind: ManifestKind::PubspecYaml, items, alternate_registries: Vec::new(), + notices: Vec::new(), }) } } diff --git a/crates/dependable-core/src/parsers/pyproject_toml.rs b/crates/dependable-core/src/parsers/pyproject_toml.rs index 27e2826..8de269b 100644 --- a/crates/dependable-core/src/parsers/pyproject_toml.rs +++ b/crates/dependable-core/src/parsers/pyproject_toml.rs @@ -89,6 +89,7 @@ impl Parser for PyprojectTomlParser { kind: ManifestKind::PyprojectToml, items, alternate_registries: Vec::new(), + notices: Vec::new(), }) } } diff --git a/crates/dependable-core/src/parsers/requirements_txt.rs b/crates/dependable-core/src/parsers/requirements_txt.rs index b1c5cd5..31c0d76 100644 --- a/crates/dependable-core/src/parsers/requirements_txt.rs +++ b/crates/dependable-core/src/parsers/requirements_txt.rs @@ -26,6 +26,7 @@ impl Parser for RequirementsTxtParser { kind: ManifestKind::RequirementsTxt, items, alternate_registries: Vec::new(), + notices: Vec::new(), }) } } diff --git a/crates/dependable-core/src/result.rs b/crates/dependable-core/src/result.rs index 732032a..b607e11 100644 --- a/crates/dependable-core/src/result.rs +++ b/crates/dependable-core/src/result.rs @@ -114,14 +114,48 @@ impl CheckResult { #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub enum DependencyStatus { + /// The best available version is already the one in use. UpToDate, + /// A newer patch release exists within the declared constraint. PatchAvailable, + /// A newer release exists within the declared constraint. UpdateAvailable, + /// A newer release exists outside the declared constraint. Outdated, + /// A known advisory affects the version in use. Vulnerable, + /// The registry was asked and the request failed; the text is what it said. Error(String), + /// There is no registry behind this dependency: a `path` entry, a Maven + /// `system` jar. Nothing was looked up because there is + /// nowhere to look. Local, + /// A git dependency, tracked by revision rather than by version. Git, + /// Whether this dependency is current **could not be determined**, and no + /// claim is made either way. + /// + /// Distinct from all three of its neighbours, and the distinction is the + /// point: + /// + /// - [`Local`](Self::Local) says *there is no registry for this*. Applied to + /// a package that is on one, it is a false statement. + /// - [`Error`](Self::Error) says *the registry was asked and it failed*. + /// Nothing was asked here. + /// - [`UpToDate`](Self::UpToDate) says *this is current*, which is precisely + /// what was not established. + /// + /// Two situations produce it. The manifest names a real package but states no + /// version this tool can resolve โ€” a Maven POM deferring to ``, + /// ``, or a property it does not declare, or a Cargo + /// member inheriting a name its workspace root never declares. Or the + /// ecosystem publishes no registry to compare a version against at all, so + /// currency is not merely unread but unknowable. + /// + /// A run is expected to say *why* alongside it: the check that produces one + /// emits a manifest-level warning naming the dependencies involved, because a + /// status word on its own does not tell a reader what to fix. + Undetermined, } impl DependencyStatus { @@ -137,6 +171,7 @@ impl DependencyStatus { DependencyStatus::Error(_) => "error", DependencyStatus::Local => "local", DependencyStatus::Git => "git", + DependencyStatus::Undetermined => "undetermined", } } @@ -152,6 +187,7 @@ impl DependencyStatus { DependencyStatus::Error(_) => "ERROR", DependencyStatus::Local => "LOCAL", DependencyStatus::Git => "GIT", + DependencyStatus::Undetermined => "UNDETERMINED", } } } @@ -798,6 +834,29 @@ mod tests { assert_eq!(Advisory::unrated_count(&[]), 0); } + /// The tokens are what a CI consumer matches on, so they are pinned here. + /// `UNDETERMINED` in particular must never collapse into `LOCAL`: one says + /// there is no registry for this package, the other says nothing was read + /// about a package that has one. + #[test] + fn status_labels_and_tokens_are_stable_and_distinct() { + let cases = [ + (DependencyStatus::UpToDate, "up to date", "OK"), + (DependencyStatus::Local, "local", "LOCAL"), + (DependencyStatus::Git, "git", "GIT"), + ( + DependencyStatus::Undetermined, + "undetermined", + "UNDETERMINED", + ), + ]; + for (status, label, token) in &cases { + assert_eq!(status.label(), *label); + assert_eq!(status.token(), *token); + } + assert_ne!(DependencyStatus::Undetermined, DependencyStatus::Local); + } + #[test] fn a_fresh_result_carries_no_advisories() { let bare = CheckResult::new(item("serde"), DependencyStatus::Local); diff --git a/crates/dependable-fetch/src/check.rs b/crates/dependable-fetch/src/check.rs index 8446710..7bda56f 100644 --- a/crates/dependable-fetch/src/check.rs +++ b/crates/dependable-fetch/src/check.rs @@ -189,6 +189,36 @@ struct FetchTask { cache_key: String, } +/// What the walk for a governing workspace root found โ€” or that no walk ran. +/// +/// `Option` could not say both, and the difference is load-bearing: +/// [`Checker::check_manifest`] takes content with no file behind it and therefore +/// never walks anything, while [`Checker::check_path`] walks and may legitimately +/// come back empty. [`detached_inheritance`] asserts the second โ€” "no root exists +/// above this file" โ€” which is false of the first, whose caller may be an IDE +/// holding an open buffer of a member deep inside an ordinary workspace. +enum WorkspaceContext { + /// No search was performed: there is no path to walk up from. Says nothing at + /// all about whether a root exists on disk. + Unsearched, + /// The walk ran and reached the top of the tree without finding a root. + NotFound, + /// The walk found this root, and these are the dependencies it declares. + Found(PathBuf, Arc>), +} + +impl WorkspaceContext { + /// The root the walk found, for [`ManifestCheck::workspace_root`] โ€” `None` + /// both when the walk found nothing and when no walk ran, because the field + /// reports a located root and neither case located one. + fn into_root(self) -> Option { + match self { + Self::Found(root, _) => Some(root), + Self::Unsearched | Self::NotFound => None, + } + } +} + /// The result of one fetch task: `(name, cache_key, versions-or-error)`. type FetchOutcome = (String, String, Result, String>); @@ -230,10 +260,12 @@ impl Checker { // Content with no file behind it: the manifest's first lockfile is the // only thing it can be attributed to. let lockfile = lockfile.and_then(|lock| Some((*kind.lockfiles().first()?, lock))); - // No file, so no tree above it: a `dep.workspace = true` here stays unresolved - // and reports as it always has. [`Checker::check_path`] is the entry point that - // can answer the question. - self.check_inner(kind, manifest, lockfile, None).await + // No file, so nothing to walk upwards from: a `dep.workspace = true` here stays + // unresolved and reports as it always has. `Unsearched` and not `NotFound` โ€” + // this call never looked, so it must not say what a look would have found. + // [`Checker::check_path`] is the entry point that can answer the question. + self.check_inner(kind, manifest, lockfile, WorkspaceContext::Unsearched) + .await } /// Check a manifest on disk: detect its kind, read it (and, when @@ -494,7 +526,12 @@ impl Checker { // A member's `dep.workspace = true` states no version of its own; the constraint // is in the root above it. Only a path can find that root, which is why this // resolves here and not in `check_manifest`. - let workspace = self.workspace_source(path, kind, &manifest).await; + let workspace = match self.workspace_source(path, kind, &manifest).await { + Some((root, declarations)) => WorkspaceContext::Found(root, declarations), + // The walk ran and reached the top with nothing: that is a fact about the + // tree, and the one `detached_inheritance` is allowed to report. + None => WorkspaceContext::NotFound, + }; self.check_inner(kind, &manifest, lockfile, workspace).await } @@ -548,7 +585,7 @@ impl Checker { kind: ManifestKind, manifest: &str, lockfile: Option<(LockfileKind, &str)>, - workspace: Option<(PathBuf, Arc>)>, + workspace: WorkspaceContext, ) -> Result { let ecosystem = kind.ecosystem(); let fetcher = self @@ -565,10 +602,21 @@ impl Checker { // `PackageSource::Inherited`, which is what keeps `--fix` off a span that means // nothing in this file. let mut warnings = Vec::new(); - if let Some((root, declarations)) = &workspace { - // The resolved names are the caller's business; the annotated items are ours. - let _ = resolve_workspace_inheritance(&mut parsed.items, declarations); - warnings.extend(undeclared_inheritance(&parsed.items, root)); + warnings.extend(std::mem::take(&mut parsed.notices)); + match &workspace { + WorkspaceContext::Found(root, declarations) => { + // The resolved names are the caller's business; the annotated items are ours. + let _ = resolve_workspace_inheritance(&mut parsed.items, declarations); + warnings.extend(undeclared_inheritance(&parsed.items, root)); + } + WorkspaceContext::NotFound => { + warnings.extend(detached_inheritance(&parsed.items, kind)); + } + // Nothing was walked, so nothing is known about what sits above this + // content โ€” least of all that there is nothing. The entry still reports + // `Undetermined`; what it does not do is explain that with a search that + // never ran. + WorkspaceContext::Unsearched => {} } // Apply the lockfile to annotate locked versions, dispatching on the file @@ -582,6 +630,10 @@ impl Checker { apply_lockfile(&mut parsed.items, &data); } + if let Some(warning) = deferred_versions(&parsed.items, kind) { + warnings.push(warning); + } + // Build the fetch task list, routing each checkable item to a fetcher: // JSR-sourced items (Deno `jsr:` deps) to the JSR fetcher, items naming a // resolved alternate Rust registry to that registry, and everything else @@ -628,7 +680,7 @@ impl Checker { ecosystem, results, warnings, - workspace_root: workspace.map(|(root, _)| root), + workspace_root: workspace.into_root(), }; // Enrichment is a post-pass over the finished results, so it can equally @@ -789,6 +841,79 @@ fn undeclared_inheritance(items: &[Item], root: &Path) -> Vec { .collect() } +/// Name every entry that says it inherits when no governing root was found at all. +/// +/// The sibling of [`undeclared_inheritance`], for the case where the walk upwards ran +/// out before it found a root rather than finding one that declares the wrong things. +/// A detached member โ€” a `Cargo.toml` with `serde = { workspace = true }` and nothing +/// above it โ€” reports every such entry as [`DependencyStatus::Undetermined`], which +/// `--fail-on any` treats as a failure; without this the run exits non-zero with +/// nothing on stderr to say why. +/// +/// Only Cargo, because `workspace = true` is the only spelling that promises a root: +/// a POM deferring to its `` has no root to be missing and is +/// [`deferred_versions`]' story to tell. +fn detached_inheritance(items: &[Item], kind: ManifestKind) -> Vec { + if kind != ManifestKind::CargoToml { + return Vec::new(); + } + let mut seen = HashSet::new(); + items + .iter() + .filter(|item| { + item.source == PackageSource::Inherited && item.version_constraint.is_empty() + }) + .filter(|item| seen.insert(item.name.as_str())) + .map(|item| { + format!( + "`{}` is declared `workspace = true`, but no workspace root was found above this manifest, so no version was read for it and nothing was checked", + item.name, + ) + }) + .collect() +} + +/// Say, once per manifest, that some of its entries state no version this file can +/// resolve โ€” and therefore that nothing was checked for them. +/// +/// Without it the run is silent: those entries report as +/// [`DependencyStatus::Undetermined`] in a table a reader may not be reading, and +/// stderr says nothing at all. `undeclared_inheritance` above is the Cargo +/// equivalent and stays separate, because a Cargo member inheriting a name its root +/// never declared is a *broken* manifest, while a POM deferring to its `` is +/// an ordinary, valid, extremely common one โ€” the same status, two different things +/// to tell the reader. +fn deferred_versions(items: &[Item], kind: ManifestKind) -> Option { + let source = match kind { + ManifestKind::PomXml => { + "a ``, ``, or a property this file does not declare" + } + _ => return None, + }; + let mut names: Vec<&str> = items + .iter() + .filter(|item| { + item.source == PackageSource::Inherited && item.version_constraint.is_empty() + }) + .map(|item| item.name.as_str()) + .collect(); + names.sort_unstable(); + names.dedup(); + if names.is_empty() { + return None; + } + let (subject, verb, object) = if names.len() == 1 { + ("dependency", "takes its version", "it") + } else { + ("dependencies", "take their version", "them") + }; + Some(format!( + "{} {subject} {verb} from {source}, so no version was read for {object} and nothing was checked: {}", + names.len(), + names.join(", ") + )) +} + /// Evaluate one parsed item against the fetched version lists, applying the /// configured pre-release filter before classification. fn evaluate_item( @@ -800,6 +925,18 @@ fn evaluate_item( if !item.is_checkable() { let status = match item.source { PackageSource::Git => DependencyStatus::Git, + // An entry that defers its version elsewhere and found nothing there is + // a real package on a real registry whose version this run never read. + // `Local` would say the opposite โ€” that there is no registry for it โ€” + // which of `spring-boot-starter-web` is simply false, and is the wrong + // token for a CI consumer to read. + PackageSource::Inherited => DependencyStatus::Undetermined, + // A coordinate this manifest could not state is the same shape of + // ignorance reached through the name instead of the version: nothing was + // asked, so nothing is known. `Local` would again say the wrong thing โ€” + // that there is no registry behind the entry, rather than that this file + // never said which package it is. + PackageSource::Unidentified => DependencyStatus::Undetermined, _ => DependencyStatus::Local, }; return CheckResult::new(item.clone(), status); diff --git a/crates/dependable-fetch/tests/checker.rs b/crates/dependable-fetch/tests/checker.rs index 07e9041..baaba07 100644 --- a/crates/dependable-fetch/tests/checker.rs +++ b/crates/dependable-fetch/tests/checker.rs @@ -1045,9 +1045,27 @@ async fn a_member_is_checked_against_the_workspace_roots_constraint() { .iter() .find(|r| r.item.name == "serde") .expect("serde is declared"); - assert_eq!(serde.status, DependencyStatus::Local); + assert_eq!( + serde.status, + DependencyStatus::Undetermined, + "no root was found, so no version was read โ€” not `Local`, which would say \ + serde has no registry" + ); assert!(serde.item.version_constraint.is_empty()); assert!(detached.workspace_root.is_none()); + // And it says nothing about a root, because it never went looking for one. The + // same buffer on disk (above) resolves against a root one directory up, so a + // content-only check claiming "no workspace root was found above this manifest" + // would contradict `check_path` about the very same file โ€” the shape an IDE + // checking an open buffer hits every keystroke. + assert!( + detached + .warnings + .iter() + .all(|w| !w.contains("no workspace root was found")), + "no search ran, so nothing may be reported about what one would have found: {:?}", + detached.warnings + ); } /// A root declaring a crate by `path` lends the member a path dependency, not a registry @@ -1147,6 +1165,6 @@ async fn an_inherited_name_the_root_never_declared_is_reported() { "both declarations are still reported" ); for result in &check.results { - assert_eq!(result.status, DependencyStatus::Local); + assert_eq!(result.status, DependencyStatus::Undetermined); } } diff --git a/crates/dependable-report/src/html/model.rs b/crates/dependable-report/src/html/model.rs index c503d61..e840fa3 100644 --- a/crates/dependable-report/src/html/model.rs +++ b/crates/dependable-report/src/html/model.rs @@ -191,6 +191,7 @@ pub(crate) struct SummaryView { pub error: usize, pub local: usize, pub git: usize, + pub undetermined: usize, pub advisory_instances: usize, pub distinct_advisories: usize, pub withdrawn_advisories: usize, @@ -381,6 +382,7 @@ fn summary_view(summary: &Summary) -> SummaryView { error: summary.error, local: summary.local, git: summary.git, + undetermined: summary.undetermined, advisory_instances: summary.advisory_instances, distinct_advisories: summary.distinct_advisories, withdrawn_advisories: summary.withdrawn_advisories, diff --git a/crates/dependable-report/src/html/templates/styles.css b/crates/dependable-report/src/html/templates/styles.css index 304b63e..5271e76 100644 --- a/crates/dependable-report/src/html/templates/styles.css +++ b/crates/dependable-report/src/html/templates/styles.css @@ -88,6 +88,9 @@ code, .ver, .vector { font-family: ui-monospace, SFMono-Regular, "SF Mono", Menl .st-OK { border-color: var(--ok); color: var(--ok); } .st-ERROR { border-color: var(--bad); color: var(--bad); } .sev-UNRATED, .sev-NONE, .st-LOCAL, .st-GIT { color: var(--muted); } +/* Not muted with the skipped statuses: nothing is known about this row, which is + a gap in the report rather than a row there was nothing to say about. */ +.st-UNDETERMINED { border-color: var(--warn); color: var(--warn); } .withdrawn { color: var(--muted); text-transform: lowercase; } .score { color: var(--muted); font-size: .8rem; } diff --git a/crates/dependable-report/src/html/templates/summary.html b/crates/dependable-report/src/html/templates/summary.html index e104067..69c6970 100644 --- a/crates/dependable-report/src/html/templates/summary.html +++ b/crates/dependable-report/src/html/templates/summary.html @@ -39,6 +39,7 @@

1. Executive summary

Errored{{ summary.error }} Path (local){{ summary.local }} Git{{ summary.git }} +Undetermined{{ summary.undetermined }}

diff --git a/crates/dependable-report/src/sarif.rs b/crates/dependable-report/src/sarif.rs index 65e8de8..caf3eb3 100644 --- a/crates/dependable-report/src/sarif.rs +++ b/crates/dependable-report/src/sarif.rs @@ -184,7 +184,11 @@ fn findings(report: &Report) -> Vec { // Security tab drowning in notes. // `UpToDate`, `Local` and `Git` have nothing to report, and // `Error` is a tool failure rather than a finding about the code - // โ€” the CLI already puts it on stderr. + // โ€” the CLI already puts it on stderr. `Undetermined` is the same + // shape as `Error`: nothing was learned, so there is no finding to + // pin to a line โ€” and it has no line to pin one to, since a + // dependency deferring its version elsewhere records no span. The + // CLI's manifest-level warning is where that belongs. _ => {} } } diff --git a/crates/dependable-report/src/summary.rs b/crates/dependable-report/src/summary.rs index e635b05..acd86b0 100644 --- a/crates/dependable-report/src/summary.rs +++ b/crates/dependable-report/src/summary.rs @@ -23,9 +23,13 @@ pub struct Summary { pub manifests: usize, /// Every declared dependency across every manifest. pub total: usize, - /// Dependencies that could actually be checked against a registry: - /// [`Self::total`] minus the path and git dependencies. The only honest - /// denominator for an "up to date" percentage. + /// Dependencies whose currency this run actually established: + /// [`Self::total`] minus the path and git dependencies, and minus the ones + /// whose version could not be read at all. The only honest denominator for an + /// "up to date" percentage โ€” an + /// [`Undetermined`](DependencyStatus::Undetermined) dependency is neither up to + /// date nor behind, so counting it below the line would quietly depress the + /// percentage on every parent-inheriting POM. pub checkable: usize, /// [`DependencyStatus::UpToDate`] count. pub up_to_date: usize, @@ -43,6 +47,10 @@ pub struct Summary { pub local: usize, /// [`DependencyStatus::Git`] count. pub git: usize, + /// [`DependencyStatus::Undetermined`] count: dependencies this run could not + /// establish anything about. Excluded from [`Self::checkable`], and kept apart + /// from [`Self::local`] and [`Self::git`], which were skipped deliberately. + pub undetermined: usize, /// Distinct `(dependency, advisory ID)` pairs โ€” one per row a vulnerability /// table would print. The same advisory affecting three packages counts three /// times here and once in [`Self::distinct_advisories`]. @@ -157,9 +165,17 @@ impl Summary { /// /// `None` when nothing is checkable, so a renderer can print "n/a" rather /// than dividing by zero and emitting `NaN%`. + /// + /// `None` too when anything went [`Undetermined`](DependencyStatus::Undetermined). + /// Such a dependency is rightly outside the denominator โ€” an unread version is no + /// evidence of currency โ€” but a headline "100% up to date" over a run that read + /// half the manifest is a stronger claim than the run can make, and it is the one + /// figure a reader takes away. A run that could not read everything has no single + /// number for how current it is; the status counts beside it still say exactly + /// what was and was not read. #[must_use] pub fn up_to_date_percent(&self) -> Option { - if self.checkable == 0 { + if self.checkable == 0 || self.undetermined > 0 { return None; } #[allow(clippy::cast_precision_loss)] @@ -209,6 +225,7 @@ impl Report { DependencyStatus::Error(_) => summary.error += 1, DependencyStatus::Local => summary.local += 1, DependencyStatus::Git => summary.git += 1, + DependencyStatus::Undetermined => summary.undetermined += 1, // `DependencyStatus` is `#[non_exhaustive]`; an unrecognized // status still counts toward the total and nothing else. _ => {} @@ -239,7 +256,7 @@ impl Report { } } - summary.checkable = summary.total - summary.local - summary.git; + summary.checkable = summary.total - summary.local - summary.git - summary.undetermined; summary.distinct_advisories = seen_advisories.len(); summary.withdrawn_advisories = withdrawn.len(); // The same key the HTML pie chart sorts its slices by, so the chart and @@ -300,13 +317,14 @@ mod tests { ("broken", DependencyStatus::Error("502".into())), ("mine", DependencyStatus::Local), ("forked", DependencyStatus::Git), + ("unread", DependencyStatus::Undetermined), ]), )]); let summary = report.summary(); assert_eq!(summary.manifests, 1); - assert_eq!(summary.total, 8); + assert_eq!(summary.total, 9); assert_eq!(summary.up_to_date, 1); assert_eq!(summary.patch_available, 1); assert_eq!(summary.update_available, 1); @@ -315,11 +333,42 @@ mod tests { assert_eq!(summary.error, 1); assert_eq!(summary.local, 1); assert_eq!(summary.git, 1); - // Path and git dependencies have no registry verdict, so they are not - // part of the denominator. + assert_eq!(summary.undetermined, 1); + // Path and git dependencies have no registry verdict, and an undetermined + // one produced none, so none of the three is part of the denominator โ€” an + // unread version is neither up to date nor behind, and counting it below + // the line would depress the percentage on every parent-inheriting POM. assert_eq!(summary.checkable, 6); } + /// A run that read half the manifest is not a run that is 100% up to date. + /// + /// The undetermined dependency stays out of the denominator โ€” an unread version + /// is not evidence of currency โ€” and the percentage is withheld rather than + /// rendered, because "100%" of a run that never read one of two dependencies is + /// the wrong headline for the one figure a reader remembers. + #[test] + fn an_undetermined_dependency_is_outside_the_up_to_date_denominator() { + let report = report(vec![ManifestResults::new( + PathBuf::from("pom.xml"), + Ecosystem::Jvm, + results(&[ + ("serde", DependencyStatus::UpToDate), + ("tokio", DependencyStatus::Undetermined), + ]), + )]); + + let summary = report.summary(); + + assert_eq!(summary.checkable, 1); + assert_eq!(summary.undetermined, 1); + assert_eq!( + summary.up_to_date_percent(), + None, + "no single number describes a run that could not read everything" + ); + } + #[test] fn up_to_date_percent_is_none_when_nothing_is_checkable() { let report = report(vec![ManifestResults::new( diff --git a/crates/dependable-report/tests/golden/empty.html b/crates/dependable-report/tests/golden/empty.html index 1dbc845..ef7ac3a 100644 --- a/crates/dependable-report/tests/golden/empty.html +++ b/crates/dependable-report/tests/golden/empty.html @@ -87,6 +87,9 @@ .st-OK { border-color: var(--ok); color: var(--ok); } .st-ERROR { border-color: var(--bad); color: var(--bad); } .sev-UNRATED, .sev-NONE, .st-LOCAL, .st-GIT { color: var(--muted); } +/* Not muted with the skipped statuses: nothing is known about this row, which is + a gap in the report rather than a row there was nothing to say about. */ +.st-UNDETERMINED { border-color: var(--warn); color: var(--warn); } .withdrawn { color: var(--muted); text-transform: lowercase; } .score { color: var(--muted); font-size: .8rem; } @@ -182,6 +185,7 @@

1. Executive summary

Errored0 Path (local)0 Git0 +Undetermined0

diff --git a/crates/dependable-report/tests/golden/full.html b/crates/dependable-report/tests/golden/full.html index 696aa0c..b7c5314 100644 --- a/crates/dependable-report/tests/golden/full.html +++ b/crates/dependable-report/tests/golden/full.html @@ -87,6 +87,9 @@ .st-OK { border-color: var(--ok); color: var(--ok); } .st-ERROR { border-color: var(--bad); color: var(--bad); } .sev-UNRATED, .sev-NONE, .st-LOCAL, .st-GIT { color: var(--muted); } +/* Not muted with the skipped statuses: nothing is known about this row, which is + a gap in the report rather than a row there was nothing to say about. */ +.st-UNDETERMINED { border-color: var(--warn); color: var(--warn); } .withdrawn { color: var(--muted); text-transform: lowercase; } .score { color: var(--muted); font-size: .8rem; } @@ -161,9 +164,9 @@

1. Executive summary

    -
  • 7dependencies
    • +
  • 8dependencies
  • 2manifests
    • -
  • 16.7%of 6 checkable up to date
    • +
  • โ€”of 6 checkable up to date
  • 2vulnerable
  • 1critical advisories
  • 9.8highest CVSS
    • @@ -188,6 +191,7 @@

    1. Executive summary

    Errored1 Path (local)1 Git0 +Undetermined1

    @@ -281,7 +285,7 @@

    2. Vulnerability detail

    3. Dependency status

    -Cargo.toml โ€” Rust (5 dependencies) +Cargo.toml โ€” Rust (6 dependencies) @@ -346,6 +350,16 @@

    3. Dependency status

    502 Bad Gateway from the index
    + + + + + + + +
    helperโ€”โ€” +undetermined +
    @@ -450,26 +464,26 @@

    5. Ecosystem breakdown

    Dependencies by ecosystem -A pie chart of 7 dependencies split across 2 ecosystem(s). The same figures are in the table beneath it. - - +A pie chart of 8 dependencies split across 2 ecosystem(s). The same figures are in the table beneath it. + +
    • Rust -5 (71.4%) - -up to date: 1 -outdated: 1 -vulnerable: 1 -not checkable: 2 +6 (75.0%) + +up to date: 1 +outdated: 1 +vulnerable: 1 +not checkable: 3
    • npm -2 (28.6%) +2 (25.0%) outdated: 1 vulnerable: 1 @@ -493,17 +507,17 @@

      5. Ecosystem breakdown

      Rust -5 -71.4% +6 +75.0% 1 1 1 -2 +3 npm 2 -28.6% +25.0% 0 1 1 diff --git a/crates/dependable-report/tests/golden/single_ecosystem.html b/crates/dependable-report/tests/golden/single_ecosystem.html index 642aaad..e103c63 100644 --- a/crates/dependable-report/tests/golden/single_ecosystem.html +++ b/crates/dependable-report/tests/golden/single_ecosystem.html @@ -87,6 +87,9 @@ .st-OK { border-color: var(--ok); color: var(--ok); } .st-ERROR { border-color: var(--bad); color: var(--bad); } .sev-UNRATED, .sev-NONE, .st-LOCAL, .st-GIT { color: var(--muted); } +/* Not muted with the skipped statuses: nothing is known about this row, which is + a gap in the report rather than a row there was nothing to say about. */ +.st-UNDETERMINED { border-color: var(--warn); color: var(--warn); } .withdrawn { color: var(--muted); text-transform: lowercase; } .score { color: var(--muted); font-size: .8rem; } @@ -182,6 +185,7 @@

      1. Executive summary

      Errored0 Path (local)0 Git0 +Undetermined0

      diff --git a/crates/dependable-report/tests/html_golden.rs b/crates/dependable-report/tests/html_golden.rs index 32a35ce..0d068b5 100644 --- a/crates/dependable-report/tests/html_golden.rs +++ b/crates/dependable-report/tests/html_golden.rs @@ -84,7 +84,8 @@ fn full_report() -> Report { serde = \"1.0.228\"\n\ regex = \"1.5\"\n\ dependable-core = { path = \"../dependable-core\" }\n\ - brokenpkg = \"2\"\n", + brokenpkg = \"2\"\n\ + helper = { workspace = true }\n", ); let mut rust_results: Vec = Vec::new(); @@ -159,6 +160,13 @@ fn full_report() -> Report { rust[4].clone(), DependencyStatus::Error("502 Bad Gateway from the index".to_owned()), )); + // An entry whose version was never read: on a registry, but nothing was fetched + // for it. It is in `summary.total`, so a status table that omits it accounts for + // fewer dependencies than the run reports. + rust_results.push(CheckResult::new( + rust[5].clone(), + DependencyStatus::Undetermined, + )); let npm = items( ManifestKind::PackageJson, @@ -215,6 +223,36 @@ fn full_report_matches_the_golden() { // Sanity checks the golden alone would not make obvious. assert!(html.contains(" 0, + "the fixture exercises the new row" + ); + // The count, not just the label: an undeclared template variable renders as the + // empty string rather than failing, so a row that reads nothing still "contains" + // its own heading. + assert!( + html.contains(&format!( + "Undetermined{}", + summary.undetermined + )), + "{html}" + ); assert_golden("full", &html); } diff --git a/crates/dependable-tui/src/ui/detail.rs b/crates/dependable-tui/src/ui/detail.rs index 33b07db..40fcc77 100644 --- a/crates/dependable-tui/src/ui/detail.rs +++ b/crates/dependable-tui/src/ui/detail.rs @@ -416,6 +416,10 @@ fn status_style(status: &DependencyStatus) -> Style { } DependencyStatus::Outdated => theme::fg(Token::Critical), DependencyStatus::Vulnerable => theme::bold(Token::Critical), + // Nothing was established about this dependency, which is not the same as + // there being nothing to establish โ€” it reads as a gap, not as a skip. + DependencyStatus::Undetermined => theme::fg(Token::Warn), + // `Local`, `Git`, `Error`, and any status a later release adds. _ => theme::fg(Token::Muted), } } diff --git a/crates/dependable/src/cli.rs b/crates/dependable/src/cli.rs index bb2b6cb..c9e3082 100644 --- a/crates/dependable/src/cli.rs +++ b/crates/dependable/src/cli.rs @@ -305,9 +305,15 @@ pub enum TreeFormat { #[derive(Copy, Clone, Debug, PartialEq, Eq, ValueEnum, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum FailOn { + /// Never fail: report and exit 0. None, + /// Fail on a dependency behind its latest release, or vulnerable. Outdated, + /// Fail only on a known advisory. Vulnerable, + /// Fail unless every dependency was checked and is current. A dependency + /// whose version could not be read counts against this: nothing was + /// established about it, so exiting 0 would claim more than the run knows. Any, } diff --git a/crates/dependable/src/fix.rs b/crates/dependable/src/fix.rs index ddc0c5f..05ad50a 100644 --- a/crates/dependable/src/fix.rs +++ b/crates/dependable/src/fix.rs @@ -120,7 +120,7 @@ fn plan_fixes(content: &str, results: &[CheckResult], all: bool) -> (String, Vec /// (npm/pubspec `>=1.0.0 <2.0.0`), a `||` alternation (`^1 || ^2`), a dist-tag /// (`latest`), a wildcard (`*`, `1.x`, `1.*`), or anything carrying an `@` /// (a Composer stability flag such as `@dev` or `^1.0@beta`, an npm alias such -/// as `npm:pkg@1.0.0`). +/// as `npm:pkg@1.0.0`), or a Maven interval (`[1.0]`, `(,2.0)`). fn rewrite_constraint(original: &str, new_version: &str) -> Option { let trimmed = original.trim(); if trimmed.contains(',') { @@ -153,6 +153,16 @@ fn rewrite_constraint(original: &str, new_version: &str) -> Option { if rest.starts_with(|c: char| c.is_ascii_alphabetic()) { return None; } + // A Maven interval โ€” `[1.0]`, `[1.0,2.0)`, `(,1.0]` โ€” states its bounds in + // brackets rather than with an operator, and `[1.0]` in particular is Maven's + // one way to say "exactly this, and defeat nearest-wins mediation". Rewriting it + // to a bare `1.0` reverts it to a *soft* requirement any transitive declaration + // may outvote, and the fix record reads "from [1.0] to 1.0", which does not read + // as the semantics change it is. A range with a comma is already declined above; + // the single-version form has no comma to catch it. + if rest.starts_with(['[', '(']) { + return None; + } // A wildcard (`*`, `1.x`, `1.*`, Gradle's `1.+`) is a range the author chose, // not a version โ€” substituting a concrete release narrows it to a pin (#87). // The decline is deliberately blanket rather than per-ecosystem: substituting @@ -282,6 +292,25 @@ mod tests { assert_eq!(rewrite_constraint("*", "2.0.0"), None); } + /// A Maven interval states its bounds in brackets, and `[1.0]` is Maven's only + /// way to say "exactly 1.0, and do not let nearest-wins mediation substitute + /// anything else". Rewriting it to a bare `1.0` turns a hard requirement into a + /// soft one that any transitive declaration may outvote, and the fix record reads + /// "from [1.0] to 1.0" โ€” a semantics change that does not look like one. Every + /// other guard misses it: no comma, no space or `|` after the empty operator + /// prefix, no `@`, it starts with `[` rather than a letter, and `is_wildcard` + /// splits it into `["[1", "0]"]`, neither of which is `x`/`X` nor starts with + /// `*`/`+`. + #[test] + fn rewrite_never_softens_a_maven_interval() { + assert_eq!(rewrite_constraint("[1.0]", "2.0.0"), None); + assert_eq!(rewrite_constraint("[1.0,2.0)", "2.0.0"), None); + assert_eq!(rewrite_constraint("(,1.0]", "2.0.0"), None); + assert_eq!(rewrite_constraint("[1.0,)", "2.0.0"), None); + // Whitespace before the bracket is still a bracket. + assert_eq!(rewrite_constraint(" [1.0] ", "2.0.0"), None); + } + /// A wildcard segment is not always the whole dot-segment. Composer allows a /// stability flag after the constraint, so `"symfony/symfony": "2.8.*@dev"` /// splits into `["2", "8", "*@dev"]` โ€” no segment *equals* a wildcard, and a diff --git a/crates/dependable/src/output/json.rs b/crates/dependable/src/output/json.rs index eaf10d4..47f3696 100644 --- a/crates/dependable/src/output/json.rs +++ b/crates/dependable/src/output/json.rs @@ -27,6 +27,10 @@ struct SummaryDto { outdated: usize, vulnerable: usize, error: usize, + /// Declarations whose currency could not be established โ€” a POM deferring to + /// its ``, an unresolved workspace inheritance. Additive, and + /// deliberately not folded into `error`: nothing failed, nothing was asked. + undetermined: usize, } #[derive(Serialize)] @@ -94,6 +98,7 @@ pub fn render(reports: &[ManifestReport]) -> anyhow::Result<()> { outdated: summary.outdated, vulnerable: summary.vulnerable, error: summary.error, + undetermined: summary.undetermined, }, results, }; diff --git a/crates/dependable/src/output/list.rs b/crates/dependable/src/output/list.rs index 04fe9e9..e910c10 100644 --- a/crates/dependable/src/output/list.rs +++ b/crates/dependable/src/output/list.rs @@ -175,7 +175,8 @@ fn json(reports: &[ProjectReport], root: &Path) -> anyhow::Result<()> { source: source_token(item.source), locked: item.locked_version.as_deref(), registry: item.registry.as_deref(), - inherited: report.inherited.contains(&item.name), + inherited: item.source == PackageSource::Inherited + || report.inherited.contains(&item.name), features: report.features.get(&item.name).map(Vec::as_slice), license: report.licenses.get(&item.name).map(String::as_str), }) @@ -235,7 +236,14 @@ struct DependencyDto<'a> { source: &'static str, locked: Option<&'a str>, registry: Option<&'a str>, - /// Whether the constraint came from the workspace root rather than this manifest. + /// Whether this dependency's version is declared somewhere other than its own + /// entry: a Cargo `workspace = true` resolved against the root, a Gradle + /// `[versions]` alias, a shared Maven `` value. + /// + /// True for every entry whose `source` is `inherited`, so the two fields can no + /// longer contradict each other on the same object. Also true where the root's + /// declaration supplied a `path` or `git` source, which replaces `source` + /// outright and would otherwise lose the fact that it was inherited at all. inherited: bool, #[serde(skip_serializing_if = "Option::is_none")] features: Option<&'a [String]>, @@ -270,6 +278,7 @@ fn source_token(source: PackageSource) -> &'static str { PackageSource::Local => "local", PackageSource::Git => "git", PackageSource::Inherited => "inherited", + PackageSource::Unidentified => "unidentified", _ => "unknown", } } @@ -297,6 +306,9 @@ fn annotation(item: &Item) -> &'static str { // would otherwise render as a bare `โ€”` that reads like a parse failure. A // resolved one falls through to its section, so a `dev` dep still says so. PackageSource::Inherited if item.version_constraint.is_empty() => " (unresolved)", + // The version beside it may be perfectly clear; what this file never stated is + // which package it belongs to, so nothing was fetched under that name. + PackageSource::Unidentified => " (unidentified)", _ => match item.kind { DependencyKind::Dev => " (dev)", DependencyKind::Build => " (build)", diff --git a/crates/dependable/src/output/mod.rs b/crates/dependable/src/output/mod.rs index 5f20b25..e556afb 100644 --- a/crates/dependable/src/output/mod.rs +++ b/crates/dependable/src/output/mod.rs @@ -55,6 +55,11 @@ pub struct Summary { pub error: usize, pub local: usize, pub git: usize, + /// [`DependencyStatus::Undetermined`] count: declarations whose currency this + /// run could not establish. Kept apart from [`local`](Self::local) and + /// [`git`](Self::git), which are deliberately skipped and therefore clean, + /// because these were not skipped on purpose โ€” nothing was learned about them. + pub undetermined: usize, } impl Summary { @@ -78,6 +83,7 @@ impl Summary { DependencyStatus::Error(_) => s.error += 1, DependencyStatus::Local => s.local += 1, DependencyStatus::Git => s.git += 1, + DependencyStatus::Undetermined => s.undetermined += 1, _ => {} } } diff --git a/crates/dependable/src/output/table.rs b/crates/dependable/src/output/table.rs index d23b753..23d8da3 100644 --- a/crates/dependable/src/output/table.rs +++ b/crates/dependable/src/output/table.rs @@ -109,6 +109,10 @@ fn status_cell(result: &CheckResult) -> String { DependencyStatus::Outdated | DependencyStatus::Error(_) => Style::new().red(), DependencyStatus::Vulnerable => Style::new().red().bold(), DependencyStatus::Local | DependencyStatus::Git => Style::new().dimmed(), + // Not dimmed with the deliberately-skipped rows beside it: this one is a + // gap in the report rather than a row there was nothing to say about, and + // `--fail-on any` fails the build over it. + DependencyStatus::Undetermined => Style::new().yellow(), _ => Style::new(), }; format!( @@ -148,6 +152,11 @@ fn print_totals(summary: &Summary) { if skipped > 0 { parts.push(format!("{skipped} skipped")); } + // Counted separately from `skipped`: a path dependency was passed over on + // purpose, an undetermined one is a version this run failed to read. + if summary.undetermined > 0 { + parts.push(format!("{} undetermined", summary.undetermined)); + } if parts.is_empty() { parts.push("nothing to check".to_string()); } diff --git a/crates/dependable/src/runner.rs b/crates/dependable/src/runner.rs index 73ba766..736e92c 100644 --- a/crates/dependable/src/runner.rs +++ b/crates/dependable/src/runner.rs @@ -622,6 +622,14 @@ pub async fn run_list(args: ListArgs) -> anyhow::Result { } }; + // What the parser saw and declined to read. On stderr rather than in the + // listing, so the same words reach a reader whichever `--format` they + // chose, and no machine-readable document changes shape โ€” the same place + // `check` puts a manifest-level warning. + for notice in &parsed.notices { + eprintln!("warning: {} โ€” {notice}", manifest.display()); + } + // A member writing `dep.workspace = true` states no version of its own; the // constraint lives in the workspace root. Same resolution `check` and `fix` get, // so an inventory and a check never disagree about what a member depends on. @@ -1315,6 +1323,29 @@ fn expand_env(content: &str) -> String { out } +/// The exit code for a finished run under the configured gate. +/// +/// # Where [`DependencyStatus::Undetermined`] sits +/// +/// It is **clean** under `--fail-on vulnerable` and `--fail-on outdated`, and +/// **not clean** under `--fail-on any`. +/// +/// Those two named gates ask a specific question โ€” is anything vulnerable, is +/// anything behind โ€” and an unread version answers neither. Failing a build that +/// asked about vulnerabilities because a POM defers to its `` would make +/// the flag mean something other than what it says. +/// +/// `--fail-on any` asks the general one: is every dependency checked and current. +/// A dependency whose version was never read is not current โ€” it is unestablished, +/// and exiting `0` asserts something this run never determined. That is precisely +/// how a parent-inheriting POM used to pass green while dependable had read +/// nothing at all. It is grouped with the failures rather than with +/// [`DependencyStatus::Local`] and [`DependencyStatus::Git`], which are clean +/// because they were skipped *on purpose*: there is no registry behind them, so +/// there is nothing a stricter run could ever learn. +/// +/// The status never travels alone: `check` emits a manifest-level warning on +/// stderr naming the dependencies involved, so a failing job says what to fix. fn exit_code(reports: &[ManifestReport], fail_on: FailOn) -> ExitCode { let triggered = reports .iter() @@ -1328,6 +1359,8 @@ fn exit_code(reports: &[ManifestReport], fail_on: FailOn) -> ExitCode { | DependencyStatus::UpdateAvailable | DependencyStatus::Vulnerable ), + // `Undetermined` is absent from this clean list on purpose โ€” see the + // doc comment above. FailOn::Any => !matches!( result.status, DependencyStatus::UpToDate | DependencyStatus::Local | DependencyStatus::Git diff --git a/crates/dependable/tests/cli_workspace.rs b/crates/dependable/tests/cli_workspace.rs index 406497e..76650f2 100644 --- a/crates/dependable/tests/cli_workspace.rs +++ b/crates/dependable/tests/cli_workspace.rs @@ -182,7 +182,15 @@ fn a_relative_path_never_adopts_the_current_directorys_workspace() { dependency["constraint"], "9.9.9", "took the constraint from a workspace that is not an ancestor: {dependency}" ); - assert_eq!(dependency["inherited"], false, "{dependency}"); + assert!( + dependency["constraint"].is_null(), + "no constraint was adopted, so none is reported: {dependency}" + ); + // `inherited` says the entry defers its version elsewhere, which this one does + // โ€” it says nothing about whether anything was found there. It agrees with + // `source` by construction, so the two can never contradict each other. + assert_eq!(dependency["source"], "inherited", "{dependency}"); + assert_eq!(dependency["inherited"], true, "{dependency}"); } /// `workspace_root` names the manifest that *governs* this one, whether or not anything @@ -202,9 +210,65 @@ fn a_constraint_the_root_never_declared_is_attributed_to_nobody() { let doc = check_json(&dir, &["check", "--manifest", member.to_str().unwrap()]); let tokio = result(&doc, "crates/app/Cargo.toml", "tokio"); - assert_eq!(tokio["status"], "LOCAL", "nothing to check: {tokio}"); + assert_eq!( + tokio["status"], "UNDETERMINED", + "the root declares no version, so nothing is known โ€” `tokio` is on crates.io, \ + and calling it LOCAL would say it is not: {tokio}" + ); assert!( tokio["inherited_from"].is_null(), "the root declares no tokio: {tokio}" ); } + +/// A package that inherits from a workspace root there is none of. +/// +/// The status is `UNDETERMINED`, which `--fail-on any` fails the run for โ€” so the +/// exit code alone tells a reader nothing, and stderr saying nothing at all leaves a +/// CI job red with no explanation anywhere. Reporting an unread version as +/// undetermined rather than local is the right call; doing it silently is not. +#[test] +fn a_package_inheriting_from_no_workspace_at_all_says_so() { + let dir = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join("workspace_detached_member"); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(dir.join(".git")).unwrap(); + // A package, not a workspace: nothing above it declares `serde`, and the `.git` + // marker stops the walk before it reaches this repository's own root. + fs::write( + dir.join("Cargo.toml"), + "[package]\nname = \"detached\"\nversion = \"0.1.0\"\n\n[dependencies]\nserde.workspace = true\n", + ) + .unwrap(); + fs::write(dir.join("dependable.toml"), CONFIG).unwrap(); + + let output = Command::new(env!("CARGO_BIN_EXE_dependable")) + .args([ + "check", + dir.to_str().unwrap(), + "--config", + dir.join("dependable.toml").to_str().unwrap(), + "--fail-on", + "any", + "--format", + "json", + "--no-vuln", + "--no-cache", + ]) + .output() + .expect("run dependable"); + + let doc: Value = serde_json::from_slice(&output.stdout).expect("valid JSON"); + let serde = result(&doc, "Cargo.toml", "serde"); + assert_eq!(serde["status"], "UNDETERMINED", "{serde}"); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert_eq!( + output.status.code(), + Some(1), + "`--fail-on any` fails on an undetermined dependency" + ); + assert!( + stderr.contains("serde") && stderr.contains("no workspace root was found"), + "an undetermined dependency that fails the run has to say why: {stderr:?}" + ); +} diff --git a/crates/dependable/tests/fixture_maven.rs b/crates/dependable/tests/fixture_maven.rs new file mode 100644 index 0000000..db7ccc6 --- /dev/null +++ b/crates/dependable/tests/fixture_maven.rs @@ -0,0 +1,629 @@ +//! Offline parse of the Maven fixture: a POM's coordinates, `${property}` +//! resolution, version-span round-tripping, and what a version this file cannot +//! resolve is reported as. +//! +//! The `check`-level tests are hermetic the same way `cli_workspace` is: the JVM +//! registry points at `http://127.0.0.1:1`, where the connection is refused at +//! once. Nothing here depends on what a registry would have said โ€” what is under +//! test is how a dependency this file states **no version for** is classified, +//! which is decided before any request is made. + +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +use dependable_fetch::core::{DependencyKind, Item, PackageSource, parse, parse_project}; +use dependable_fetch::{Ecosystem, ManifestKind}; +use serde_json::Value; + +fn fixture(rel: &str) -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .join("tests/fixtures") + .join(rel) +} + +fn slice<'a>(content: &'a str, item: &Item) -> &'a str { + let line = content.lines().nth(item.version_line).unwrap(); + &line[item.version_col_start..item.version_col_end] +} + +fn find<'a>(items: &'a [Item], name: &str) -> &'a Item { + items + .iter() + .find(|item| item.name == name) + .unwrap_or_else(|| panic!("no item {name}")) +} + +#[test] +fn parses_a_maven_pom() { + let path = fixture("sample-maven/pom.xml"); + let kind = ManifestKind::detect(&path).expect("recognised by name"); + assert_eq!(kind, ManifestKind::PomXml); + assert_eq!(kind.ecosystem(), Ecosystem::Jvm); + assert_eq!(kind.ecosystem().osv_name(), "Maven"); + + let manifest = std::fs::read_to_string(&path).unwrap(); + let parsed = parse(kind, &manifest).unwrap(); + + // Only `` under ``: the ``, the + // `` entry, and the plugin's own dependency are not + // dependencies of this artifact. + let names: Vec<&str> = parsed.items.iter().map(|i| i.name.as_str()).collect(); + assert_eq!( + names, + vec![ + "com.google.guava:guava", + "com.squareup.okhttp3:okhttp", + "com.fasterxml.jackson.core:jackson-core", + "com.fasterxml.jackson.core:jackson-databind", + "org.springframework.boot:spring-boot-starter-web", + "com.example:sample-shared", + "org.junit.jupiter:junit-jupiter", + ] + ); + + // A version stated on the dependency is rewritable where it is written. + let guava = find(&parsed.items, "com.google.guava:guava"); + assert_eq!(guava.version_constraint, "32.1.3-jre"); + assert_eq!(slice(&manifest, guava), "32.1.3-jre"); + assert_eq!(guava.source, PackageSource::Registry); + assert!(guava.is_rewritable()); + + // A property used once points at the `` line that governs it, so + // `--fix` rewrites the version where Maven actually reads it. + let okhttp = find(&parsed.items, "com.squareup.okhttp3:okhttp"); + assert_eq!(okhttp.version_constraint, "4.12.0"); + assert_eq!(slice(&manifest, okhttp), "4.12.0"); + assert!(okhttp.is_rewritable()); + assert!( + manifest + .lines() + .nth(okhttp.version_line) + .unwrap() + .contains("okhttp.version"), + "the span belongs to the entry, not to the " + ); + + // A property two dependencies share is resolved but never rewritten: one line + // cannot be rewritten to two different versions. + for artifact in ["jackson-core", "jackson-databind"] { + let item = find( + &parsed.items, + &format!("com.fasterxml.jackson.core:{artifact}"), + ); + assert_eq!(item.version_constraint, "2.17.0", "{artifact}"); + assert_eq!(item.source, PackageSource::Inherited, "{artifact}"); + assert!(item.is_checkable(), "{artifact}"); + assert!(!item.is_rewritable(), "{artifact}"); + } + + // `` is stated in the manifest, so the section is read rather than guessed. + let junit = find(&parsed.items, "org.junit.jupiter:junit-jupiter"); + assert_eq!(junit.kind, DependencyKind::Dev); + assert_eq!(junit.version_constraint, "5.10.2"); +} + +/// The required behaviour. A version supplied by a `` and one written as a +/// Maven built-in are both out of a parser's reach, and both are **reported** with no +/// constraint rather than dropped. Dropping them, the way the `csproj` parser drops an +/// MSBuild `$(โ€ฆ)` version, would present a POM that inherits some of its versions as +/// depending on only the rest โ€” a short list that looks complete. +#[test] +fn a_version_this_file_cannot_resolve_is_reported_rather_than_dropped() { + let path = fixture("sample-maven/pom.xml"); + let manifest = std::fs::read_to_string(&path).unwrap(); + let parsed = parse(ManifestKind::PomXml, &manifest).unwrap(); + + for name in [ + "org.springframework.boot:spring-boot-starter-web", + "com.example:sample-shared", + ] { + let item = find(&parsed.items, name); + assert!(item.version_constraint.is_empty(), "{name}"); + assert_eq!(item.source, PackageSource::Inherited, "{name}"); + // Nothing is claimed about it: it is not fetched, not positioned, not fixed. + assert!(!item.is_checkable(), "{name}"); + assert!(!item.has_position(), "{name}"); + assert!(!item.is_rewritable(), "{name}"); + } +} + +/// A POM names itself by coordinate, and the ``'s coordinate is not it. +#[test] +fn a_pom_reports_its_own_coordinate() { + let manifest = std::fs::read_to_string(fixture("sample-maven/pom.xml")).unwrap(); + let meta = parse_project(ManifestKind::PomXml, &manifest); + assert_eq!(meta.name.as_deref(), Some("com.example:sample-maven")); + assert_eq!(meta.literal_version(), Some("1.4.0")); +} + +/// The other half of the requirement, at the surface a user sees: `list` is offline, +/// so it reports exactly what the parser produced. An unresolvable dependency is +/// present with a null constraint and an `inherited` source โ€” visible, and not +/// mistaken for one that was checked. +#[test] +fn the_cli_lists_an_unresolvable_dependency_instead_of_omitting_it() { + let output = Command::new(env!("CARGO_BIN_EXE_dependable")) + .args([ + "list", + fixture("sample-maven").to_str().unwrap(), + "--format", + "json", + ]) + .output() + .expect("run dependable"); + assert!( + output.status.success(), + "list failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let doc: Value = serde_json::from_slice(&output.stdout).expect("valid JSON"); + + let project = doc["projects"] + .as_array() + .expect("projects array") + .iter() + .find(|p| p["name"] == "com.example:sample-maven") + .unwrap_or_else(|| panic!("no Maven project in {}", doc["projects"])); + assert_eq!(project["ecosystem"], "JVM"); + + let dependencies = project["dependencies"].as_array().expect("dependencies"); + let dependency = |name: &str| { + dependencies + .iter() + .find(|d| d["name"] == name) + .unwrap_or_else(|| panic!("no dependency {name}")) + }; + + let unresolved = dependency("org.springframework.boot:spring-boot-starter-web"); + assert!(unresolved["constraint"].is_null(), "{unresolved}"); + assert_eq!(unresolved["source"], "inherited"); + + let guava = dependency("com.google.guava:guava"); + assert_eq!(guava["constraint"], "32.1.3-jre"); + assert_eq!(guava["source"], "registry"); + assert_eq!(guava["inherited"], false, "its version is its own: {guava}"); + + // `source` and `inherited` describe the same fact, so they can never disagree + // on one object. They used to: the boolean was filled in only by Cargo + // workspace resolution, so every non-Cargo `"source": "inherited"` arrived + // beside `"inherited": false`, and a consumer reading both got a + // contradiction. + for name in [ + "com.fasterxml.jackson.core:jackson-core", + "com.fasterxml.jackson.core:jackson-databind", + "org.springframework.boot:spring-boot-starter-web", + ] { + let entry = dependency(name); + assert_eq!(entry["source"], "inherited", "{entry}"); + assert_eq!(entry["inherited"], true, "{entry}"); + } +} + +/// A registry nothing is listening on: `connect` fails at once. +const OFFLINE: &str = "[jvm]\nregistry = \"http://127.0.0.1:1\"\n"; + +/// A temp directory holding one `pom.xml`, walled off from this repository. +fn pom_dir(name: &str, body: &str) -> PathBuf { + let dir = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(name); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + // A repository boundary, so the discovery walk stays inside the temp dir. + fs::create_dir_all(dir.join(".git")).unwrap(); + fs::write(dir.join("dependable.toml"), OFFLINE).unwrap(); + fs::write( + dir.join("pom.xml"), + format!( + "\n \ + com.example\n \ + app\n \ + 1.0.0\n{body}\n" + ), + ) + .unwrap(); + dir +} + +/// Run the CLI in `dir`. `--config` and the hermetic flags belong to the +/// subcommand, not to the binary, and `list` takes no config at all โ€” it reads no +/// registry, so it needs none. +fn run(dir: &Path, args: &[&str]) -> Output { + let config = dir.join("dependable.toml"); + let mut all: Vec = args.iter().map(|a| (*a).to_string()).collect(); + if args[0] != "list" { + all.push("--config".to_string()); + all.push(config.to_string_lossy().into_owned()); + } + if args[0] == "check" { + all.push("--no-vuln".to_string()); + all.push("--no-cache".to_string()); + } + Command::new(env!("CARGO_BIN_EXE_dependable")) + .current_dir(dir) + .args(all) + .output() + .expect("run dependable") +} + +fn check_json(dir: &Path, extra: &[&str]) -> Value { + let mut args: Vec<&str> = vec!["check", ".", "--format", "json"]; + args.extend_from_slice(extra); + let output = run(dir, &args); + serde_json::from_slice(&output.stdout).unwrap_or_else(|e| { + panic!( + "invalid JSON ({e}): {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ) + }) +} + +fn status_of<'a>(doc: &'a Value, name: &str) -> &'a Value { + doc["results"] + .as_array() + .expect("results array") + .iter() + .find(|r| r["name"] == name) + .unwrap_or_else(|| panic!("no result {name}: {}", doc["results"])) +} + +/// The dominant real-world POM: a `` supplies every version, so this file +/// states none. +const PARENT_ONLY: &str = " \n \ + org.springframework.boot\n \ + spring-boot-starter-parent\n \ + 3.2.5\n \ + \n \ + \n \ + \n \ + org.springframework.boot\n \ + spring-boot-starter-web\n \ + \n \ + \n \ + org.springframework.boot\n \ + spring-boot-starter-test\n \ + test\n \ + \n \ + \n"; + +/// `check` must not call a dependency it merely failed to read a **local** one. +/// +/// `local` is what this tool prints for a Cargo `path = "../x"` and a Maven +/// `system` jar, and it means one thing: there is no registry +/// behind this. Said of `spring-boot-starter-web` โ€” which is on Maven Central โ€” +/// it is a plain false statement, and `LOCAL` is the wrong token for a CI job to +/// read. It also contradicted `list`, which calls the same entry `(unresolved)`. +#[test] +fn a_version_supplied_by_a_parent_is_undetermined_and_never_called_local() { + let dir = pom_dir("maven_parent_only", PARENT_ONLY); + let doc = check_json(&dir, &[]); + + for artifact in ["spring-boot-starter-web", "spring-boot-starter-test"] { + let name = format!("org.springframework.boot:{artifact}"); + let result = status_of(&doc, &name); + assert_eq!( + result["status"], "UNDETERMINED", + "the artifact is on Maven Central; only its version went unread: {result}" + ); + assert_ne!(result["status"], "LOCAL", "{result}"); + } + assert_eq!(doc["summary"]["undetermined"], 2, "{}", doc["summary"]); + assert_eq!( + doc["summary"]["error"], 0, + "nothing was asked, so nothing failed" + ); +} + +/// The same fact in the table and in `--format text`, since those are what a +/// person and a shell pipeline actually read. +#[test] +fn the_table_and_the_text_format_both_say_undetermined() { + let dir = pom_dir("maven_parent_only_surfaces", PARENT_ONLY); + + let table = run(&dir, &["check", "."]); + let stdout = String::from_utf8_lossy(&table.stdout); + assert!( + stdout.contains("undetermined"), + "the table calls it undetermined: {stdout}" + ); + assert!(!stdout.contains("local"), "and never local: {stdout}"); + assert!( + stdout.contains("2 undetermined"), + "the totals count it apart from the deliberately skipped: {stdout}" + ); + + let text = run(&dir, &["check", ".", "--format", "text"]); + let stdout = String::from_utf8_lossy(&text.stdout); + assert_eq!( + stdout.matches("UNDETERMINED").count(), + 2, + "one stable token per unread dependency: {stdout}" + ); +} + +/// A CI job that reads nothing must not go green. `--fail-on any` asks whether +/// every dependency is checked and current; an unread version answers neither. +/// The two narrower gates keep meaning what they say. +#[test] +fn fail_on_any_does_not_pass_a_pom_whose_versions_were_never_read() { + let dir = pom_dir("maven_parent_only_gate", PARENT_ONLY); + for (gate, expected) in [("any", false), ("outdated", true), ("vulnerable", true)] { + let output = run(&dir, &["check", ".", "--fail-on", gate]); + assert_eq!( + output.status.success(), + expected, + "--fail-on {gate}: {}", + String::from_utf8_lossy(&output.stdout) + ); + } +} + +/// Nothing anywhere used to say that a POM's versions had not been read: stderr +/// was empty, and a reader looking at a table of dashes had to infer it. +#[test] +fn a_pom_whose_versions_were_not_read_says_so_on_stderr() { + let dir = pom_dir("maven_parent_only_warning", PARENT_ONLY); + let output = run(&dir, &["check", ".", "--format", "json"]); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!(stderr.contains("2 dependencies"), "{stderr}"); + assert!(stderr.contains(""), "{stderr}"); + assert!(stderr.contains(""), "{stderr}"); + assert!( + stderr.contains("org.springframework.boot:spring-boot-starter-web"), + "the warning names them, so the reader knows what to fix: {stderr}" + ); +} + +/// The same-file `` case, which needs no `` at all +/// and is just as common. +#[test] +fn a_version_supplied_by_the_files_own_dependency_management_is_undetermined() { + let dir = pom_dir( + "maven_managed_only", + " \n \ + \n \ + \n \ + com.google.guava\n \ + guava\n \ + 32.1.3-jre\n \ + \n \ + \n \ + \n \ + \n \ + \n \ + com.google.guava\n \ + guava\n \ + \n \ + \n", + ); + let doc = check_json(&dir, &[]); + let guava = status_of(&doc, "com.google.guava:guava"); + assert_eq!(guava["status"], "UNDETERMINED", "{guava}"); +} + +/// A POM that says nothing about its versions is not the same as one whose +/// dependencies genuinely have no registry. A `system` jar is +/// still `LOCAL`, and must stay that way, or the new status means nothing. +#[test] +fn a_system_scoped_jar_is_still_local() { + let dir = pom_dir( + "maven_system_scope", + " \n \ + \n \ + org.example\n \ + vendored\n \ + 1.0.0\n \ + system\n \ + /opt/vendored.jar\n \ + \n \ + \n", + ); + let doc = check_json(&dir, &[]); + let vendored = status_of(&doc, "org.example:vendored"); + assert_eq!( + vendored["status"], "LOCAL", + "there is no registry: {vendored}" + ); + assert_eq!(doc["summary"]["undetermined"], 0, "{}", doc["summary"]); + + // And `--fail-on any` stays green over it: it was skipped on purpose. + let output = run(&dir, &["check", ".", "--fail-on", "any"]); + assert!(output.status.success()); +} + +/// A POM whose dependencies all live in `` used to list as +/// `(0 dependencies)` โ€” a list that reads as complete and is not. Excluding +/// conditional dependencies stays the decision; being silent about it does not. +#[test] +fn a_pom_whose_dependencies_are_all_in_profiles_says_so() { + let dir = pom_dir( + "maven_profiles_only", + " \n \ + \n \ + native\n \ + \n \ + \n \ + com.google.guava\n \ + guava\n \ + 32.1.3-jre\n \ + \n \ + \n \ + \n \ + \n", + ); + + let listed = run(&dir, &["list", "."]); + let stdout = String::from_utf8_lossy(&listed.stdout); + let stderr = String::from_utf8_lossy(&listed.stderr); + assert!( + stdout.contains("(0 dependencies)"), + "a profile dependency is still not listed: {stdout}" + ); + assert!( + stderr.contains("1 dependency declared inside "), + "but the reader is told why the list is empty: {stderr}" + ); + + let checked = run(&dir, &["check", "."]); + assert!( + String::from_utf8_lossy(&checked.stderr).contains(""), + "and `check` says it too: {}", + String::from_utf8_lossy(&checked.stderr) + ); +} + +/// `${project.groupId}` is the standard way a multi-module build names a sibling +/// module, and this file cannot resolve it โ€” Maven's built-ins are not +/// `` entries. Dropping the entry made `list` report a POM declaring +/// three dependencies as having one, with nothing on stderr: the same silent +/// omission a parent-supplied `` used to cause, reached through the +/// coordinate instead of the version. +#[test] +fn a_group_this_file_cannot_resolve_is_still_listed() { + let dir = pom_dir( + "maven_unresolvable_group", + " \n \ + \n \ + ${project.groupId}\n \ + app-core\n \ + ${project.version}\n \ + \n \ + \n \ + ${project.groupId}\n \ + app-web\n \ + ${project.version}\n \ + \n \ + \n \ + org.slf4j\n \ + slf4j-api\n \ + 2.0.13\n \ + \n \ + \n", + ); + + let listed = run(&dir, &["list", "."]); + let stdout = String::from_utf8_lossy(&listed.stdout); + assert!(stdout.contains("3 dependencies"), "{stdout}"); + for artifact in ["app-core", "app-web", "slf4j-api"] { + assert!(stdout.contains(artifact), "{artifact} is missing: {stdout}"); + } + + // And `check` classifies the two as undetermined rather than omitting them. + let doc = check_json(&dir, &[]); + assert_eq!(doc["summary"]["total"], 3, "{}", doc["summary"]); + for name in ["${project.groupId}:app-core", "${project.groupId}:app-web"] { + assert_eq!(status_of(&doc, name)["status"], "UNDETERMINED", "{name}"); + } + assert_eq!(doc["summary"]["undetermined"], 2, "{}", doc["summary"]); +} + +/// The multi-module idiom states a version *and* a coordinate this file cannot +/// spell, and the two are read independently. +/// +/// Forcing the version to unknown because the coordinate did not resolve threw away +/// a `2.0.13` written in plain sight: `list` showed the entry with no version at +/// all, and the run then explained the blank with a `` notice โ€” false twice, +/// because the version was stated and no parent is involved. Under `--fail-on any` +/// that pointed the reader at a parent POM that does not exist. +#[test] +fn a_version_beside_an_unnameable_coordinate_is_read_and_reported() { + let dir = pom_dir( + "maven_unnameable_with_version", + " \n \ + \n \ + ${project.groupId}\n \ + app-core\n \ + 2.0.13\n \ + \n \ + \n \ + org.springframework.boot\n \ + spring-boot-starter-web\n \ + \n \ + \n", + ); + + let listed = run(&dir, &["list", "."]); + let stdout = String::from_utf8_lossy(&listed.stdout); + assert!( + stdout.contains("2.0.13"), + "the version is written in the file and must appear: {stdout}" + ); + + let stderr = String::from_utf8_lossy(&listed.stderr); + assert!( + stderr.contains("coordinate") && stderr.contains("${project.groupId}:app-core"), + "the unread coordinate gets its own notice: {stderr}" + ); + + // `check` still declines to claim a status for it โ€” nothing was fetched under a + // name no registry has heard of โ€” but reports the version it read. + let doc = check_json(&dir, &[]); + let core = status_of(&doc, "${project.groupId}:app-core"); + assert_eq!(core["status"], "UNDETERMINED", "{core}"); + assert_eq!(core["current"], "2.0.13", "{core}"); + // The entry whose version really does come from the `` keeps that story, + // and keeps it to itself. + let checked = run(&dir, &["check", "."]); + let stderr = String::from_utf8_lossy(&checked.stderr); + assert!( + parent_story(&stderr).contains("spring-boot-starter-web"), + "{stderr}" + ); + assert!(!parent_story(&stderr).contains("app-core"), "{stderr}"); +} + +/// The one stderr line that blames a `` / `` / +/// undeclared property, or the empty string when the run emitted none. +fn parent_story(stderr: &str) -> String { + stderr + .lines() + .find(|line| line.contains("takes its version from") || line.contains("take their version")) + .unwrap_or_default() + .to_owned() +} + +/// The `system` scope survives a version this parser had to reconstruct. +/// +/// A comment inside `` costs the entry its byte-faithful span, and the +/// span-less path used to overwrite the source with `Inherited` โ€” which is +/// checkable, so a jar with no registry at all was fetched from one and reported +/// `ERROR`, failing `--fail-on any`. Same POM, same scope, two answers. +#[test] +fn a_system_scoped_jar_stays_local_when_its_version_is_reconstructed() { + let dir = pom_dir( + "maven_system_scope_reconstructed", + " \n \ + \n \ + org.example\n \ + plainsys\n \ + 1.0.0\n \ + system\n \ + /opt/plain.jar\n \ + \n \ + \n \ + org.example\n \ + cmtsys\n \ + 1.0.0\n \ + system\n \ + /opt/cmt.jar\n \ + \n \ + \n", + ); + + let doc = check_json(&dir, &[]); + for artifact in ["plainsys", "cmtsys"] { + let result = status_of(&doc, &format!("org.example:{artifact}")); + assert_eq!( + result["status"], "LOCAL", + "a system jar has no registry however its version is spelled: {result}" + ); + } + + let output = run(&dir, &["check", ".", "--fail-on", "any"]); + assert!( + output.status.success(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); +} diff --git a/crates/dependable/tests/fixtures/sample-maven/pom.xml b/crates/dependable/tests/fixtures/sample-maven/pom.xml new file mode 100644 index 0000000..c2b5a3d --- /dev/null +++ b/crates/dependable/tests/fixtures/sample-maven/pom.xml @@ -0,0 +1,101 @@ + + + 4.0.0 + + + + org.springframework.boot + spring-boot-starter-parent + 3.2.5 + + + com.example + sample-maven + 1.4.0 + + + 4.12.0 + 2.17.0 + + + + + + + org.apache.commons + commons-text + 1.11.0 + + + + + + + + com.google.guava + guava + 32.1.3-jre + + + + + com.squareup.okhttp3 + okhttp + ${okhttp.version} + + + + + com.fasterxml.jackson.core + jackson-core + ${jackson.version} + + + com.fasterxml.jackson.core + jackson-databind + ${jackson.version} + + + + + org.springframework.boot + spring-boot-starter-web + + + + + com.example + sample-shared + ${revision} + + + + org.junit.jupiter + junit-jupiter + 5.10.2 + test + + + + + + + + org.apache.maven.plugins + maven-surefire-plugin + 3.2.5 + + + org.junit.platform + junit-platform-launcher + 1.10.2 + + + + + +