Skip to content

Commit e458517

Browse files
mikolalysenkoclaude
andcommitted
Withhold Maven pins whose coordinates a profile redefines
Closes the last Maven property false-attestation gap in VEX discovery after #2020: a <groupId> / <artifactId> written as ${prop} that a profile's <properties> redefine. Coordinates were read only with no profile active, so discovery attested two shapes the build may not resolve: (a) The declaration carrying the pin names another package whenever a profile redefining its coordinate property is active; the pin then no longer applies to the patched GA, which resolves from its other declarations or transitively, unpinned. (b) Another declaration (live, or inside a profile whose own properties retarget it) becomes the patched GA under a profile, at its own version off the pin. Both readers now resolve coordinates through #2020's shared helpers (formats::maven::properties): the groupId and artifactId are joined with COORDINATE_SEPARATOR (U+0001, which XML 1.0 forbids) so one profile_combinations walk and one interpolation per combination read both under the same active profiles, through interpolate_keeping (the keep-unresolved interpolate_lenient as an Interpolate), and split_coordinates splits the result. formats/maven: PomScope::coordinate_alternatives runs the joined text through resolve_in_build; PomDep carries group_text / artifact_text and coordinate_alternatives (CoordinateAlternative: group, artifact, MovedBy), every pair some combination gives other than the no-profile reading (ScopedDep.group / artifact, unchanged), plus a profile declaration's own-profile reading. More than 8 relevant profiles or an unreadable profile fail closed (the text as written, which may_name reads as any text). vex/discover/maven: bypasses() runs coordinate_override after profile_override, reported as MavenProfileUnpatched (detail names the profiles), for (a) the live main-jar declaration carrying the pin (build_version == pin) with any alternative, and (b) any declaration with an alternative that may be the GA's main jar (literal or may_shadow; pom / sources / javadoc types skipped) at a version other than the pin among its resolutions(), or literally an executable classifier copy off the pin or with no version of its own. As in profile_override, a managed declaration counts only where management governs the pin, and a versionless main jar takes the live pin. CLI note and CLI_CONTRACT.md updated. vendor/jvm/maven_reactor: the v5 planner had the same gap. interpolate_with and profile_override take the Interpolate to use (interpolate_strict for versions, as before), and coordinates_elsewhere reads the joined coordinates in each inheritor and under each profile combination on the chain through them. (a) The existing coordinates_overridden check now weighs profiles, not only inheritors. (b) New warn_moved_coordinates: a declaration that names another artifact in its pom but may be the GA in an inheritor or under a profile combination is warned (coordinates_overridden) and, when versioned, unpins its root and is uncertain, so attestable_checked no longer attests it. This also covers the inheritor form of (b). Tests (each fails before this change): - vex::discover::maven::tests::profile_properties_moving_the_pinned_coordinates_are_not_attested - vex::discover::maven::tests::profile_properties_turning_a_declaration_into_the_pinned_ga_are_not_attested - vendor::jvm::maven_reactor::tests::a_profile_moving_the_declared_coordinates_is_left_alone - vendor::jvm::maven_reactor::tests::a_profile_turning_a_declaration_into_the_patched_artifact_is_weighed - formats::maven::tests::profile_properties_give_coordinate_alternatives Guard: vex::discover::maven::tests::unrelated_coordinate_properties_do_not_withhold. User-property variants (a chain through -D, -D beating the profile) are in each. The golden corpus is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 854c3f6 commit e458517

6 files changed

Lines changed: 1019 additions & 33 deletions

File tree

‎crates/socket-patch-cli/CLI_CONTRACT.md‎

Lines changed: 2 additions & 2 deletions
Large diffs are not rendered by default.

‎crates/socket-patch-cli/src/commands/vex_sources.rs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -280,9 +280,11 @@ fn unattested_note(kind: UnattestedKind) -> (&'static str, &'static str) {
280280
UnattestedKind::MavenProfileUnpatched => (
281281
NOTE_MAVEN_PROFILE_UNPATCHED,
282282
"not attested while a profile declares it at, or its <properties> move it to, \
283-
another version; the wiring itself is intact, so re-running `scan` does not change \
284-
this (drop that profile <version> or property or set it to the pin, or re-patch \
285-
with `scan --mode vendored`)",
283+
another version, or its <properties> redefine a groupId / artifactId property so \
284+
the pin names another package or another declaration names this one; the wiring \
285+
itself is intact, so re-running `scan` does not change this (drop that profile \
286+
<version> or property or set it to the pin, or re-patch with \
287+
`scan --mode vendored`)",
286288
),
287289
UnattestedKind::MavenConfigOverride => (
288290
NOTE_MAVEN_CONFIG_OVERRIDE,

‎crates/socket-patch-core/src/formats/maven/mod.rs‎

Lines changed: 233 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,12 @@ mod properties;
66

77
use std::collections::BTreeMap;
88

9-
use properties::{interpolate_lenient, read_properties, Interpolate};
109
pub(crate) use properties::{
11-
interpolate_strict, is_model_reference, lookup_property, profile_combinations, Definitions,
12-
PomLayer, MAX_COMBINED_PROFILES, MAX_INTERPOLATION_DEPTH,
10+
interpolate_keeping, interpolate_strict, is_model_reference, lookup_property,
11+
profile_combinations, split_coordinates, Definitions, Interpolate, PomLayer,
12+
COORDINATE_SEPARATOR, MAX_COMBINED_PROFILES, MAX_INTERPOLATION_DEPTH,
1313
};
14+
use properties::{interpolate_lenient, read_properties};
1415

1516
use super::xml::{blank_non_markup, child_text, elements, open_tags, Element};
1617

@@ -47,8 +48,20 @@ pub(crate) struct PomRepo {
4748

4849
#[derive(Debug)]
4950
pub(crate) struct PomDep {
51+
/// `<groupId>` / `<artifactId>` as [`ScopedDep::group`] reads them:
52+
/// with no profile active, an undefined `${…}` kept as written.
5053
pub(crate) group: String,
5154
pub(crate) artifact: String,
55+
/// The trimmed `<groupId>` / `<artifactId>` text as written.
56+
pub(crate) group_text: String,
57+
pub(crate) artifact_text: String,
58+
/// The other `(groupId, artifactId)` pairs the declaration names when
59+
/// some combination of profiles is active whose `<properties>` redefine
60+
/// a property its coordinates name, or, for one inside a profile, while
61+
/// its own profile is ([`PomScope::coordinate_alternatives`]); a
62+
/// reference left unresolved stays as written ([`may_name`] reads it as
63+
/// any text). Empty when no profile can move it.
64+
pub(crate) coordinate_alternatives: Vec<CoordinateAlternative>,
5265
/// Trimmed version, every `${prop}` interpolated
5366
/// ([`interpolate_strict`]) from the project model (`project.*` /
5467
/// `pom.*`), the `<properties>` of the profile holding the declaration
@@ -91,6 +104,15 @@ pub(crate) struct Alternative {
91104
pub(crate) by: MovedBy,
92105
}
93106

107+
/// A `(groupId, artifactId)` some combination of active profiles gives a
108+
/// declaration other than [`PomDep::group`] / [`PomDep::artifact`], and why.
109+
#[derive(Debug, Clone, PartialEq, Eq)]
110+
pub(crate) struct CoordinateAlternative {
111+
pub(crate) group: String,
112+
pub(crate) artifact: String,
113+
pub(crate) by: MovedBy,
114+
}
115+
94116
/// What gives an [`Alternative`].
95117
#[derive(Debug, Clone, PartialEq, Eq)]
96118
pub(crate) enum MovedBy {
@@ -350,6 +372,54 @@ impl PomScope {
350372
(base, alternatives)
351373
}
352374

375+
/// [`PomDep::coordinate_alternatives`] of the declaration at `pos`
376+
/// whose coordinates are written `text` and read `base` with no profile
377+
/// active ([`Self::interpolate_coordinate`]): both resolved together
378+
/// ([`COORDINATE_SEPARATOR`]) through [`Self::resolve_in_build`],
379+
/// keeping an unresolved reference ([`interpolate_keeping`]). A
380+
/// profile declaration's own profile is always active with it, so what
381+
/// it reads there is an alternative too.
382+
fn coordinate_alternatives(
383+
&self,
384+
text: (&str, &str),
385+
base: (&str, &str),
386+
pos: usize,
387+
) -> Vec<CoordinateAlternative> {
388+
if !text.0.contains("${") && !text.1.contains("${") {
389+
return Vec::new();
390+
}
391+
let joined = format!("{}{COORDINATE_SEPARATOR}{}", text.0, text.1);
392+
let (own_base, alternatives) = self.resolve_in_build(&joined, pos, interpolate_keeping);
393+
let own_ids = || {
394+
self.own_profile(pos)
395+
.into_iter()
396+
.map(|i| self.profile_properties[i].id.clone())
397+
.collect()
398+
};
399+
let mut out: Vec<CoordinateAlternative> = Vec::new();
400+
let candidates = own_base
401+
.map(|v| (v, MovedBy::Profiles(own_ids())))
402+
.into_iter()
403+
.chain(alternatives.into_iter().map(|a| (a.value, a.by)));
404+
for (value, by) in candidates {
405+
let Some((group, artifact)) = split_coordinates(&value) else {
406+
continue;
407+
};
408+
if (group.as_str(), artifact.as_str()) != base
409+
&& out
410+
.iter()
411+
.all(|o| (&o.group, &o.artifact) != (&group, &artifact))
412+
{
413+
out.push(CoordinateAlternative {
414+
group,
415+
artifact,
416+
by,
417+
});
418+
}
419+
}
420+
out
421+
}
422+
353423
/// `text` (a coordinate) with every `${name}` the build defines with
354424
/// no profile active interpolated, an undefined one kept as written
355425
/// ([`interpolate_lenient`]).
@@ -407,6 +477,9 @@ pub(crate) struct ScopedDep {
407477
/// written, so [`may_name`] can tell it apart.
408478
pub(crate) group: Option<String>,
409479
pub(crate) artifact: Option<String>,
480+
/// The trimmed `<groupId>` / `<artifactId>` text as written.
481+
pub(crate) group_text: Option<String>,
482+
pub(crate) artifact_text: Option<String>,
410483
/// Inner-text byte range of the literal `<version>`, when present.
411484
pub(crate) version_inner: Option<(usize, usize)>,
412485
/// Trimmed `<version>` text as written (no property resolution).
@@ -425,10 +498,18 @@ impl PomScope {
425498
for dep in self.elements("dependency")? {
426499
let body = blank_elements(dep.inner(&self.masked), "exclusions")?;
427500
let version = elements(&body, "version")?.into_iter().next();
501+
let group_text = child_text(&body, "groupId")?;
502+
let artifact_text = child_text(&body, "artifactId")?;
428503
out.push(ScopedDep {
429504
element: dep,
430-
group: child_text(&body, "groupId")?.map(|g| self.interpolate_coordinate(&g)),
431-
artifact: child_text(&body, "artifactId")?.map(|a| self.interpolate_coordinate(&a)),
505+
group: group_text
506+
.as_deref()
507+
.map(|g| self.interpolate_coordinate(g)),
508+
artifact: artifact_text
509+
.as_deref()
510+
.map(|a| self.interpolate_coordinate(a)),
511+
group_text,
512+
artifact_text,
432513
version_inner: version.map(|v| {
433514
(
434515
dep.inner_start + v.inner_start,
@@ -473,9 +554,16 @@ pub(crate) fn parse_pom_with(raw: &str, user: &BTreeMap<String, String>) -> Resu
473554
}
474555

475556
for dep in scope.dependencies()? {
476-
let (Some(group), Some(artifact)) = (dep.group, dep.artifact) else {
557+
let (Some(group), Some(artifact), Some(group_text), Some(artifact_text)) =
558+
(dep.group, dep.artifact, dep.group_text, dep.artifact_text)
559+
else {
477560
continue;
478561
};
562+
let coordinate_alternatives = scope.coordinate_alternatives(
563+
(&group_text, &artifact_text),
564+
(&group, &artifact),
565+
dep.element.start,
566+
);
479567
let (version, build_version, version_alternatives) = match &dep.version_text {
480568
Some(text) => {
481569
let (spelled, build, alternatives) = scope.resolve_version(text, dep.element.start);
@@ -486,6 +574,9 @@ pub(crate) fn parse_pom_with(raw: &str, user: &BTreeMap<String, String>) -> Resu
486574
pom.deps.push(PomDep {
487575
group,
488576
artifact,
577+
group_text,
578+
artifact_text,
579+
coordinate_alternatives,
489580
version,
490581
build_version,
491582
version_text: dep.version_text,
@@ -1245,6 +1336,142 @@ mod tests {
12451336
);
12461337
}
12471338

1339+
/// The `(groupId, artifactId)` pairs a profile's `<properties>` may move
1340+
/// a `${property}` coordinate to: every combination of profiles counts,
1341+
/// both coordinates read under the same one, a declaration inside a
1342+
/// profile reads its own first, a value left unresolved stays as
1343+
/// written, and user properties and model references no profile can
1344+
/// redefine.
1345+
#[test]
1346+
fn profile_properties_give_coordinate_alternatives() {
1347+
let pairs = |alternatives: Vec<super::CoordinateAlternative>| {
1348+
alternatives
1349+
.into_iter()
1350+
.map(|c| (c.group, c.artifact))
1351+
.collect::<Vec<_>>()
1352+
};
1353+
let alternatives = |props: &str, profiles: &[&str], user: &[(&str, &str)], deps: &str| {
1354+
let profiles: String = profiles
1355+
.iter()
1356+
.map(|p| format!("<profile><properties>{p}</properties></profile>"))
1357+
.collect();
1358+
let pom = format!(
1359+
"<project><groupId>org.self</groupId><properties>{props}</properties>\
1360+
<dependencies>{deps}</dependencies><profiles>{profiles}</profiles></project>"
1361+
);
1362+
let user = user
1363+
.iter()
1364+
.map(|(k, v)| (k.to_string(), v.to_string()))
1365+
.collect();
1366+
super::parse_pom_with(&pom, &user)
1367+
.unwrap()
1368+
.deps
1369+
.into_iter()
1370+
.map(|d| ((d.group, d.artifact), pairs(d.coordinate_alternatives)))
1371+
.collect::<Vec<_>>()
1372+
};
1373+
let dep = |g: &str, a: &str| {
1374+
format!(
1375+
"<dependency><groupId>{g}</groupId><artifactId>{a}</artifactId>\
1376+
<version>1</version></dependency>"
1377+
)
1378+
};
1379+
let pair = |g: &str, a: &str| (g.to_string(), a.to_string());
1380+
let one = |props, profiles: &[&str], user: &[(&str, &str)], g: &str, a: &str| {
1381+
alternatives(props, profiles, user, &dep(g, a)).remove(0)
1382+
};
1383+
assert_eq!(
1384+
one("<g>org.a</g>", &["<g>org.b</g>"], &[], "${g}", "x"),
1385+
(pair("org.a", "x"), vec![pair("org.b", "x")])
1386+
);
1387+
assert_eq!(
1388+
one("<g>org.a</g>", &["<g>org.a</g>"], &[], "${g}", "x"),
1389+
(pair("org.a", "x"), vec![])
1390+
);
1391+
assert_eq!(
1392+
one("<g>org.a</g>", &["<y>org.b</y>"], &[], "${g}", "x"),
1393+
(pair("org.a", "x"), vec![])
1394+
);
1395+
// Both coordinates, read under the same profiles.
1396+
assert_eq!(
1397+
one("<v>a</v>", &["<v>b</v>"], &[], "org.${v}", "${v}-api").1,
1398+
[pair("org.b", "b-api")]
1399+
);
1400+
// Nested, and two profiles together.
1401+
assert_eq!(
1402+
one(
1403+
"<g>${w}</g><w>org.a</w>",
1404+
&["<w>org.b</w>", "<g>${w}.c</g>"],
1405+
&[],
1406+
"${g}",
1407+
"x"
1408+
)
1409+
.1,
1410+
[
1411+
pair("org.b", "x"),
1412+
pair("org.a.c", "x"),
1413+
pair("org.b.c", "x")
1414+
]
1415+
);
1416+
// Unresolved under a profile, or a cycle: kept as written.
1417+
assert_eq!(
1418+
one("<g>org.a</g>", &["<g>${nope}</g>"], &[], "${g}", "x").1,
1419+
[pair("${nope}", "x")]
1420+
);
1421+
assert_eq!(
1422+
one("<g>org.a</g>", &["<g>${g}</g>"], &[], "${g}", "x").1,
1423+
[pair("${g}", "x")]
1424+
);
1425+
// Undefined with no profile involved: no alternative.
1426+
assert_eq!(
1427+
one("<g>org.a</g>", &[], &[], "${g}.${nope}", "x"),
1428+
(pair("org.a.${nope}", "x"), vec![])
1429+
);
1430+
// User properties and model references win over every profile's.
1431+
assert_eq!(
1432+
one(
1433+
"<g>org.a</g>",
1434+
&["<g>org.b</g>"],
1435+
&[("g", "org.c")],
1436+
"${g}",
1437+
"x"
1438+
),
1439+
(pair("org.c", "x"), vec![])
1440+
);
1441+
assert_eq!(
1442+
one(
1443+
"",
1444+
&["<project.groupId>org.b</project.groupId>"],
1445+
&[],
1446+
"${project.groupId}",
1447+
"x"
1448+
),
1449+
(pair("org.self", "x"), vec![])
1450+
);
1451+
1452+
// A profile's own declaration reads its own properties first.
1453+
let pom = format!(
1454+
"<project><properties><g>org.a</g></properties><dependencies>{}</dependencies>\
1455+
<profiles><profile><id>p</id><properties><g>org.b</g></properties>\
1456+
<dependencies>{}</dependencies></profile></profiles></project>",
1457+
dep("${g}", "x"),
1458+
dep("${g}", "x")
1459+
);
1460+
let deps: Vec<_> = super::parse_pom(&pom)
1461+
.unwrap()
1462+
.deps
1463+
.into_iter()
1464+
.map(|d| (d.in_profile, d.group, pairs(d.coordinate_alternatives)))
1465+
.collect();
1466+
assert_eq!(
1467+
deps,
1468+
[
1469+
(false, "org.a".to_string(), vec![pair("org.b", "x")]),
1470+
(true, "org.a".to_string(), vec![pair("org.b", "x")]),
1471+
]
1472+
);
1473+
}
1474+
12481475
#[test]
12491476
fn suffix_grammar_is_exact() {
12501477
assert_eq!(

‎crates/socket-patch-core/src/formats/maven/properties.rs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,27 @@ pub(crate) fn interpolate_lenient(value: &str, lookup: &dyn Fn(&str) -> Option<S
109109
expand(value, lookup, &mut Vec::new(), &lookups, true).unwrap_or_else(|| value.to_string())
110110
}
111111

112+
/// [`interpolate_lenient`] as an [`Interpolate`] (it always gives a text).
113+
pub(crate) fn interpolate_keeping(
114+
value: &str,
115+
lookup: &dyn Fn(&str) -> Option<String>,
116+
) -> Option<String> {
117+
Some(interpolate_lenient(value, lookup))
118+
}
119+
120+
/// Joins a `<groupId>` and an `<artifactId>` into one text, so that one
121+
/// interpolation (and one [`profile_combinations`] walk) reads both under
122+
/// the same active profiles; a control character neither may hold (XML
123+
/// 1.0 forbids it in a document). [`split_coordinates`] undoes it.
124+
pub(crate) const COORDINATE_SEPARATOR: char = '\u{1}';
125+
126+
/// The `(groupId, artifactId)` a [`COORDINATE_SEPARATOR`]-joined text
127+
/// holds.
128+
pub(crate) fn split_coordinates(joined: &str) -> Option<(String, String)> {
129+
let (g, a) = joined.split_once(COORDINATE_SEPARATOR)?;
130+
Some((g.to_string(), a.to_string()))
131+
}
132+
112133
fn expand(
113134
value: &str,
114135
lookup: &dyn Fn(&str) -> Option<String>,

0 commit comments

Comments
 (0)