Skip to content

Commit 37d9fd9

Browse files
mikolalysenkoclaude
andcommitted
Read profile declarations' coordinates under their own profile
/code-review high on #2026: - A <profiles> declaration's ${...} groupId / artifactId was read with no profile active, its own profile's reading only an alternative. A coordinate only its own <properties> define stayed "${g}", which may_name reads as any text, so VEX withheld and hosted scan refused a valid pin beside a profile dependency its own profile names as another group (a regression from #2020). Coordinates read the declaration's own profile again, as its version does; other profiles stay alternatives. - The vendored reactor's warn_property_coordinates skipped a ${...} <type>, which discovery and the hosted index read as possibly a jar: it is weighed now, so vendored mode warns instead of pinning a root VEX then never attests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 237307e commit 37d9fd9

4 files changed

Lines changed: 100 additions & 47 deletions

File tree

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

Lines changed: 22 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -440,10 +440,8 @@ impl PomScope {
440440
/// artifactId (keeping an unresolved reference,
441441
/// [`interpolate_keeping`]) and version ([`interpolate_strict`]) read
442442
/// together ([`Self::resolve_all_in_build`]), every reading whose
443-
/// coordinates differ from what it reads with no profile active
444-
/// ([`ScopedDep::group`] / [`ScopedDep::artifact`]). A profile
445-
/// declaration's own profile is always active with it, so what it reads
446-
/// there is one too.
443+
/// coordinates differ from what it reads with no profile active but
444+
/// the one holding it ([`ScopedDep::group`] / [`ScopedDep::artifact`]).
447445
pub(crate) fn coordinate_alternatives(&self, dep: &ScopedDep) -> Vec<CoordinateAlternative> {
448446
let (Some(group_text), Some(artifact_text), Some(group), Some(artifact)) = (
449447
dep.group_text.as_deref(),
@@ -463,22 +461,9 @@ impl PomScope {
463461
if let Some(version) = dep.version_text.as_deref() {
464462
texts.push((version, interpolate_strict));
465463
}
466-
let pos = dep.element.start;
467-
let (own_base, readings) = self.resolve_all_in_build(&texts, pos);
468-
let own: Vec<String> = self
469-
.own_profile(pos)
470-
.into_iter()
471-
.map(|i| self.profile_properties[i].id.clone())
472-
.collect();
473-
let own_reading = (!own.is_empty()).then(|| {
474-
let mut values = own_base;
475-
if let Some(v) = values.get_mut(2) {
476-
*v = v.take().or_else(|| dep.version_text.clone());
477-
}
478-
(values, MovedBy::Profiles(own))
479-
});
464+
let (_, readings) = self.resolve_all_in_build(&texts, dep.element.start);
480465
let mut out: Vec<CoordinateAlternative> = Vec::new();
481-
for (values, by) in own_reading.into_iter().chain(readings) {
466+
for (values, by) in readings {
482467
let mut values = values.into_iter();
483468
let (Some(Some(g)), Some(Some(a))) = (values.next(), values.next()) else {
484469
continue;
@@ -503,13 +488,16 @@ impl PomScope {
503488
out
504489
}
505490

506-
/// `text` (a coordinate) with every `${name}` the build defines with
507-
/// no profile active interpolated, an undefined one kept as written
508-
/// ([`interpolate_lenient`]). What any profile (a declaration's own
509-
/// included) makes of it is [`ScopedDep::coordinate_alternatives`]'
510-
/// job, which every coordinate check weighs.
511-
fn interpolate_coordinate(&self, text: &str) -> String {
512-
interpolate_lenient(text, &|name| self.lookup(name, &[], true))
491+
/// `text` (a coordinate of the declaration at `pos`) with every
492+
/// `${name}` the build defines interpolated, reading the declaration's
493+
/// own profile `<properties>` first when it sits in a profile (as its
494+
/// version is: that profile is always active with it), an undefined
495+
/// one kept as written ([`interpolate_lenient`]). What other profiles
496+
/// make of it is [`Self::coordinate_alternatives`]' job, which every
497+
/// coordinate check weighs.
498+
fn interpolate_coordinate(&self, text: &str, pos: usize) -> String {
499+
let own = self.own_profile(pos);
500+
interpolate_lenient(text, &|name| self.lookup(name, &own, true))
513501
}
514502

515503
/// Inside a plugin / deploy section (never project resolution).
@@ -556,12 +544,11 @@ fn inside(pos: usize, set: &[Element]) -> bool {
556544
pub(crate) struct ScopedDep {
557545
pub(crate) element: Element,
558546
/// `<groupId>` / `<artifactId>` with every `${prop}` the build defines
559-
/// with no profile active (user properties, the project model, the
560-
/// root `<properties>`) interpolated
561-
/// ([`PomScope::interpolate_coordinate`]); an undefined one stays as
562-
/// written, so [`may_name`] can tell it apart. What a profile (a
563-
/// `<profiles>` declaration's own included) makes of them is
564-
/// [`PomScope::coordinate_alternatives`]' job.
547+
/// with no profile active but, for a `<profiles>` declaration, its own
548+
/// (user properties, the project model, the root `<properties>`)
549+
/// interpolated ([`PomScope::interpolate_coordinate`]); an undefined
550+
/// one stays as written, so [`may_name`] can tell it apart. What other
551+
/// profiles make of them is [`PomScope::coordinate_alternatives`]' job.
565552
pub(crate) group: Option<String>,
566553
pub(crate) artifact: Option<String>,
567554
/// The trimmed `<groupId>` / `<artifactId>` text as written.
@@ -591,10 +578,10 @@ impl PomScope {
591578
element: dep,
592579
group: group_text
593580
.as_deref()
594-
.map(|g| self.interpolate_coordinate(g)),
581+
.map(|g| self.interpolate_coordinate(g, dep.start)),
595582
artifact: artifact_text
596583
.as_deref()
597-
.map(|a| self.interpolate_coordinate(a)),
584+
.map(|a| self.interpolate_coordinate(a, dep.start)),
598585
group_text,
599586
artifact_text,
600587
version_inner: version.map(|v| {
@@ -1658,7 +1645,7 @@ mod tests {
16581645
deps,
16591646
[
16601647
(false, "org.a".to_string(), vec![pair("org.b", "x")]),
1661-
(true, "org.a".to_string(), vec![pair("org.b", "x")]),
1648+
(true, "org.b".to_string(), vec![]),
16621649
]
16631650
);
16641651
}

‎crates/socket-patch-core/src/patch/redirect/mod.rs‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11085,6 +11085,22 @@ mod tests {
1108511085
let out = r.files.get("pom.xml").expect("pom rewritten");
1108611086
assert!(out.contains(MAVEN_SUFFIXED), "{deps}: pinned: {out}");
1108711087
}
11088+
// A profile declaration whose groupId only its own profile's
11089+
// `<properties>` define, as another group: it is read under its own
11090+
// profile, so it never names the GA, and the pin lands.
11091+
let own = format!(
11092+
"<project>\n <dependencies>\n {pinned}\n </dependencies>\n <profiles>\
11093+
<profile><id>p</id><properties><g>org.other</g></properties><dependencies>{}\
11094+
</dependencies></profile></profiles>\n</project>\n",
11095+
decl("${g}", "slf4j-api", "2.0")
11096+
);
11097+
let r = rewrite_registry_redirect(
11098+
&BTreeMap::from([("pom.xml".to_string(), own)]),
11099+
&[maven_override()],
11100+
);
11101+
assert!(warning_codes(&r).is_empty(), "{:?}", r.warnings);
11102+
let out = r.files.get("pom.xml").expect("pom rewritten");
11103+
assert!(out.contains(MAVEN_SUFFIXED), "pinned: {out}");
1108811104
}
1108911105

1109011106
/// A versioned `<profiles>` declaration through an undefined coordinate

‎crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2274,7 +2274,7 @@ impl Reactor {
22742274
/// coordinate, which no pin reaches: an executable one is warned about
22752275
/// as `classifier_declared`, as `rewrite_declarations` does (`sources` /
22762276
/// `javadoc` never run; `test-jar` is the `tests` copy), and any other
2277-
/// `<type>` but `jar` is not the g:a's jar. Text that can never
2277+
/// literal `<type>` but `jar` is not the g:a's jar (a `${…}` one may be). Text that can never
22782278
/// read the g:a, whatever its properties say ([`may_interpolate_to`]),
22792279
/// is ruled out first, before any inheritor or profile is read.
22802280
fn warn_property_coordinates(
@@ -2312,9 +2312,10 @@ impl Reactor {
23122312
.is_some_and(|c| !is_executable_classifier(c))
23132313
|| !may_interpolate_to(&g, patch.group_id)
23142314
|| !may_interpolate_to(&a, patch.artifact_id)
2315+
// A `${…}` type may be `jar`, as discovery reads it.
23152316
|| type_text
23162317
.as_deref()
2317-
.is_some_and(|t| t != "jar" && t != "test-jar")
2318+
.is_some_and(|t| t != "jar" && t != "test-jar" && !t.contains("${"))
23182319
{
23192320
continue;
23202321
}
@@ -6394,6 +6395,32 @@ mod tests {
63946395
}
63956396
}
63966397

6398+
// A `${…}` type may be `jar`, as discovery reads it: weighed too.
6399+
let typed = module(
6400+
"a",
6401+
&format!(
6402+
"{}{}",
6403+
dep("1.10.0"),
6404+
other("<version>1.9</version><type>${t}</type>")
6405+
),
6406+
);
6407+
let files = fs(&[
6408+
(
6409+
"pom.xml",
6410+
&root_with(props, "<lib.group>org.apache.commons</lib.group>"),
6411+
),
6412+
("a/pom.xml", &typed),
6413+
("b/pom.xml", &module("b", "")),
6414+
]);
6415+
let plan = run(&files).unwrap();
6416+
assert_eq!(
6417+
reasons(&plan),
6418+
["coordinates_overridden"],
6419+
"{:?}",
6420+
plan.warnings
6421+
);
6422+
assert!(!root_pinned(&plan, &files));
6423+
63976424
// No version of its own: the pin applies, so the root stays pinned.
63986425
let files = fs(&[
63996426
(

‎crates/socket-patch-core/src/vex/discover/maven.rs‎

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2645,16 +2645,6 @@ mod tests {
26452645
vec![to_slf4j.clone()],
26462646
None,
26472647
),
2648-
// A profile declaration its own profile's properties retarget.
2649-
case(
2650-
h,
2651-
with(""),
2652-
vec![format!(
2653-
"{to_slf4j}<dependencies>\n{}</dependencies>\n",
2654-
other(Some("1.7.36"))
2655-
)],
2656-
None,
2657-
),
26582648
// Through a user property lower in the chain.
26592649
case(
26602650
"<h>${corp}</h>",
@@ -2666,6 +2656,26 @@ mod tests {
26662656
for c in &withheld {
26672657
assert_coordinate_case(c, false).await;
26682658
}
2659+
// A profile declaration its own profile's properties retarget: it
2660+
// is read under its own profile, so it is a profile declaration of
2661+
// the GA itself.
2662+
let own = property_project(
2663+
h,
2664+
&with(""),
2665+
&[&format!(
2666+
"{to_slf4j}<dependencies>\n{}</dependencies>\n",
2667+
other(Some("1.7.36"))
2668+
)],
2669+
);
2670+
let out = run(&own).await;
2671+
assert_refs(&out, &[(FX_PURL, UUID_A, WiringMode::Hosted)]);
2672+
let kinds: Vec<_> = out.unattested.iter().map(|u| u.kind).collect();
2673+
assert_eq!(
2674+
kinds,
2675+
[UnattestedKind::MavenProfileUnpatched],
2676+
"{:?}",
2677+
out.unattested
2678+
);
26692679
let attested = [
26702680
// No version of its own: the live pin applies.
26712681
case(h, with(&other(None)), vec![to_slf4j.clone()], None),
@@ -2771,6 +2781,19 @@ mod tests {
27712781
vec![in_props("<x>org.other</x>")],
27722782
none.clone(),
27732783
),
2784+
// A profile declaration whose groupId only its own profile's
2785+
// `<properties>` define, as another group: whenever it is
2786+
// read, it names that group.
2787+
(
2788+
String::new(),
2789+
with(""),
2790+
vec![format!(
2791+
"{}<dependencies>\n{}</dependencies>\n",
2792+
in_props("<g>org.other</g>"),
2793+
dep("${g}", "slf4j-api", Some("2.0"))
2794+
)],
2795+
none.clone(),
2796+
),
27742797
] {
27752798
assert_coordinate_case(&case, true).await;
27762799
}

0 commit comments

Comments
 (0)