Fix Maven ${property} groupId/artifactId (#683)

crates/socket-patch-core/src/patch/redirect/maven_index.rs

@@ -119,9 +119,14 @@ fn authored_element(text: &str, at: usize, name: &str) -> Element {
 }
 
 impl PomIndex {
-    /// Fails (fail-closed) when the pom is not readable as one.
-    pub(super) fn build(pom: &str) -> Result<PomIndex, String> {
-        let scope = PomScope::new(pom)?;
+    /// Fails (fail-closed) when the pom is not readable as one. `user` is
+    /// the build's `.mvn/maven.config` user properties (see
+    /// [`PomScope::with_user_properties`]).
+    pub(super) fn build(
+        pom: &str,
+        user: &std::collections::BTreeMap<String, String>,
+    ) -> Result<PomIndex, String> {
+        let scope = PomScope::with_user_properties(pom, user)?;
         let masked = &scope.masked;
         let deps = scope
             .dependencies()?
@@ -171,6 +176,31 @@ impl PomIndex {
             .collect()
     }
 
+    /// A versioned declaration (inside `<profiles>` exactly when
+    /// `in_profile`) that is not `group:artifact` as written but may be
+    /// its main jar through a coordinate property this pom does not define
+    /// (see [`may_shadow`](crate::formats::maven::may_shadow)).
+    pub(super) fn placeholder_shadow(
+        &self,
+        group: &str,
+        artifact: &str,
+        in_profile: bool,
+    ) -> Option<&IndexedDep> {
+        self.deps.iter().find(|d| {
+            let (Some(g), Some(a)) = (d.group.as_deref(), d.artifact.as_deref()) else {
+                return false;
+            };
+            d.in_profile == in_profile
+                && d.version_text.is_some()
+                && crate::formats::maven::may_shadow(
+                    (g, a),
+                    d.type_text.as_deref(),
+                    d.classifier.as_deref(),
+                    (group, artifact),
+                )
+        })
+    }
+
     pub(super) fn dep(&self, i: usize) -> &IndexedDep {
         &self.deps[i]
     }
@@ -419,7 +449,7 @@ mod tests {
     /// After any sequence of the rewriter's edits, the kept index is the
     /// index a fresh build of the edited pom reads.
     fn assert_in_step(index: &PomIndex, pom: &str) {
-        let fresh = PomIndex::build(pom).expect("the edited pom reads");
+        let fresh = PomIndex::build(pom, &Default::default()).expect("the edited pom reads");
         let key = |i: &PomIndex| {
             let mut deps: Vec<_> = i
                 .deps
@@ -462,7 +492,7 @@ mod tests {
             "<project>\n  <dependencyManagement>\n  </dependencyManagement>\n  <dependencies>\n    <dependency><groupId>g</groupId><artifactId>a</artifactId><version>1</version></dependency>\n  </dependencies>\n</project>\n",
         ] {
             let mut pom = pom.to_string();
-            let mut index = PomIndex::build(&pom).unwrap();
+            let mut index = PomIndex::build(&pom, &Default::default()).unwrap();
             let [i] = index.matches("g", "a")[..] else {
                 panic!("one match");
             };
@@ -492,7 +522,7 @@ mod tests {
                 let mut pom = format!(
                     "<project>\n  <dependencies>\n    <dependency><groupId>g</groupId><artifactId>a</artifactId></dependency>\n  </dependencies>\n  {tail}</project>\n"
                 );
-                let mut index = PomIndex::build(&pom).unwrap();
+                let mut index = PomIndex::build(&pom, &Default::default()).unwrap();
                 if dm_first {
                     index.insert_dependency_management(&mut pom, "g", "a", "1-socket.1");
                     assert_in_step(&index, &pom);

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

@@ -8590,8 +8590,17 @@ fn rewrite_maven_pom(
     // unchanged (a no-op rescan then never re-scans the pom per dep).
     // The hosted rewriter's view of the pom, read once (through `PomScope`)
     // and kept in step with every edit below.
-    let mut index: Option<Result<maven_index::PomIndex, String>> =
-        pom.as_deref().map(maven_index::PomIndex::build);
+    // `.mvn/maven.config`'s `-D` user properties win over the pom's own
+    // when Maven interpolates a coordinate.
+    let user_properties = crate::formats::maven::maven_config_properties(
+        files
+            .get(MVN_CONFIG)
+            .map(String::as_str)
+            .unwrap_or_default(),
+    );
+    let mut index: Option<Result<maven_index::PomIndex, String>> = pom
+        .as_deref()
+        .map(|pom| maven_index::PomIndex::build(pom, &user_properties));
     let original_repos: Vec<(String, Option<String>)> = match (&index, pom.as_deref()) {
         (Some(Ok(index)), Some(pom)) => index.repository_ids_and_urls(pom),
         _ => Vec::new(),
@@ -8740,6 +8749,53 @@ fn rewrite_maven_pom(
                 continue;
             }
         }
+        // A declaration whose groupId / artifactId is a property this pom
+        // does not define (a parent's) may be this GA, and then its own
+        // `<version>` beats any pin written here (#683): nothing is pinned.
+        // One inside `<profiles>` beats it only while that profile is
+        // active, and is reported like a literal profile declaration.
+        let shadow_text = |d: &maven_index::IndexedDep| {
+            format!(
+                "{}:{} <version>{}</version>",
+                d.group.as_deref().unwrap_or_default(),
+                d.artifact.as_deref().unwrap_or_default(),
+                d.version_text.as_deref().unwrap_or_default(),
+            )
+        };
+        if let Some(shadow) = idx.placeholder_shadow(&group_id, &artifact_id, false) {
+            result.warnings.push(RewriteWarning {
+                code: "redirect_maven_coordinate_unresolved".into(),
+                detail: format!(
+                    "{} names a property pom.xml does not define, so it may be \
+                     {group_id}:{artifact_id} and its own version would win over a pin; pom.xml \
+                     was left as it is and the dep is not counted as redirected",
+                    shadow_text(shadow),
+                ),
+            });
+            continue;
+        }
+        if profile_version.is_none() {
+            if let Some(shadow) = idx.placeholder_shadow(&group_id, &artifact_id, true) {
+                result.warnings.push(RewriteWarning {
+                    code: "redirect_maven_profile_dependency_unpatched".into(),
+                    detail: format!(
+                        "{} inside <profiles> names a property pom.xml does not define, so it \
+                         may be {group_id}:{artifact_id}; it is not edited, and while that \
+                         profile is active it may resolve that version, not the patched \
+                         artifact{}",
+                        shadow_text(shadow),
+                        if matches.is_empty() {
+                            " (no declaration outside <profiles>, so nothing is pinned)"
+                        } else {
+                            ""
+                        }
+                    ),
+                });
+                if matches.is_empty() {
+                    continue;
+                }
+            }
+        }
         let has_live_repo = idx.has_repository(&repo_id);
 
         // LEGACY same-GAV fallback: no suffixed version means the patched jar is
@@ -10099,6 +10155,199 @@ mod tests {
         assert!(!warning_codes(&r).contains(&"redirect_maven_multimodule_unsupported"));
     }
 
+    /// A direct declaration whose groupId / artifactId is a property of
+    /// the pom's own `<properties>` is the GA (#683): its literal is
+    /// rewritten, not shadowed by an added `<dependencyManagement>` pin
+    /// Maven would never apply. `<exclusions>` before the coordinates do
+    /// not hide it either.
+    #[test]
+    fn maven_pom_property_coordinates_are_the_direct_declaration() {
+        for (case, props, coords) in [
+            (
+                "property groupId",
+                "<g>org.slf4j</g>",
+                "<groupId>${g}</groupId><artifactId>slf4j-api</artifactId>",
+            ),
+            (
+                "property artifactId",
+                "<a>slf4j-api</a>",
+                "<groupId>org.slf4j</groupId><artifactId>${a}</artifactId>",
+            ),
+            (
+                "interpolated groupId",
+                "<vendor>slf4j</vendor>",
+                "<groupId>org.${vendor}</groupId><artifactId>slf4j-api</artifactId>",
+            ),
+            (
+                "exclusions first",
+                "",
+                "<exclusions><exclusion><groupId>org.example</groupId><artifactId>none</artifactId></exclusion></exclusions><groupId>org.slf4j</groupId><artifactId>slf4j-api</artifactId>",
+            ),
+        ] {
+            let mut files = BTreeMap::new();
+            files.insert(
+                "pom.xml".to_string(),
+                format!(
+                    "<project>\n  <properties>{props}</properties>\n  <dependencies>\n    <dependency>{coords}<version>1.7.36</version></dependency>\n  </dependencies>\n</project>\n"
+                ),
+            );
+            let r = rewrite_registry_redirect(&files, &[maven_override()]);
+            let out = r.files.get("pom.xml").expect("pom rewritten");
+            assert!(
+                out.contains(&format!("{coords}<version>{MAVEN_SUFFIXED}</version>")),
+                "{case}: the direct literal is pinned: {out}"
+            );
+            assert!(
+                !out.contains("<dependencyManagement>"),
+                "{case}: no shadowed managed pin: {out}"
+            );
+            assert!(
+                !warning_codes(&r).contains(&"redirect_maven_dep_management_added"),
+                "{case}: {:?}",
+                r.warnings
+            );
+        }
+    }
+
+    /// `.mvn/maven.config`'s `-D` user properties override the pom's own
+    /// when Maven interpolates a coordinate: one naming another group is
+    /// not this GA (its literal is left alone), and one defining a property
+    /// the pom lacks makes the declaration this GA.
+    #[test]
+    fn maven_pom_coordinate_properties_read_maven_config_overrides() {
+        let pom = |props: &str| {
+            format!(
+                "<project>\n  <properties>{props}</properties>\n  <dependencies>\n    <dependency><groupId>${{g}}</groupId><artifactId>slf4j-api</artifactId><version>1.7.36</version></dependency>\n  </dependencies>\n</project>\n"
+            )
+        };
+        let mut files = BTreeMap::new();
+        files.insert("pom.xml".to_string(), pom("<g>org.slf4j</g>"));
+        files.insert(MVN_CONFIG.to_string(), "-Dg=com.acme.fork\n".to_string());
+        let r = rewrite_registry_redirect(&files, &[maven_override()]);
+        let out = r.files.get("pom.xml").expect("pom rewritten");
+        assert!(
+            out.contains("<groupId>${g}</groupId><artifactId>slf4j-api</artifactId><version>1.7.36</version>"),
+            "the com.acme.fork declaration keeps its version: {out}"
+        );
+
+        let mut files = BTreeMap::new();
+        files.insert("pom.xml".to_string(), pom(""));
+        files.insert(MVN_CONFIG.to_string(), "-Dg=org.slf4j\n".to_string());
+        let r = rewrite_registry_redirect(&files, &[maven_override()]);
+        let out = r.files.get("pom.xml").expect("pom rewritten");
+        assert!(
+            out.contains(&format!(
+                "<groupId>${{g}}</groupId><artifactId>slf4j-api</artifactId><version>{MAVEN_SUFFIXED}</version>"
+            )) && !out.contains("<dependencyManagement>"),
+            "the maven.config-defined GA is pinned directly: {out}"
+        );
+        assert!(
+            !warning_codes(&r).contains(&"redirect_maven_coordinate_unresolved"),
+            "{:?}",
+            r.warnings
+        );
+    }
+
+    /// A versioned declaration whose groupId / artifactId is a property
+    /// the pom does not define (a parent's) may be the GA, and its own
+    /// version would then beat any pin (#683): the dep is refused with
+    /// nothing written, not pinned through `<dependencyManagement>`.
+    #[test]
+    fn maven_pom_undefined_coordinate_property_refuses_the_dep() {
+        for coords in [
+            "<groupId>${parent.g}</groupId><artifactId>slf4j-api</artifactId>",
+            "<groupId>org.slf4j</groupId><artifactId>${parent.a}</artifactId>",
+            "<groupId>${g}</groupId><artifactId>${a}</artifactId>",
+        ] {
+            let mut files = BTreeMap::new();
+            files.insert(
+                "pom.xml".to_string(),
+                format!(
+                    "<project>\n  <dependencies>\n    <dependency>{coords}<version>1.7.36</version></dependency>\n  </dependencies>\n</project>\n"
+                ),
+            );
+            let r = rewrite_registry_redirect(&files, &[maven_override()]);
+            assert!(
+                r.files.is_empty() && r.edits.is_empty(),
+                "{coords}: nothing written: {:?}",
+                r.files
+            );
+            assert_eq!(
+                warning_codes(&r),
+                vec!["redirect_maven_coordinate_unresolved"],
+                "{coords}"
+            );
+        }
+        // A placeholder that cannot be the GA (the other coordinate, or the
+        // literal text around the reference, differs), or a versionless
+        // one (it takes the managed pin), does not block the pin.
+        for other in [
+            "<groupId>${parent.g}</groupId><artifactId>logback-classic</artifactId><version>1.4.14</version>",
+            "<groupId>${parent.g}</groupId><artifactId>slf4j-api</artifactId>",
+            "<groupId>${project.groupId}</groupId><artifactId>${project.artifactId}-core</artifactId><version>${project.version}</version>",
+            "<groupId>${parent.g}</groupId><artifactId>${bom}</artifactId><version>3</version><type>pom</type><scope>import</scope>",
+        ] {
+            let mut files = BTreeMap::new();
+            files.insert(
+                "pom.xml".to_string(),
+                format!(
+                    "<project>\n  <dependencies>\n    <dependency>{other}</dependency>\n  </dependencies>\n</project>\n"
+                ),
+            );
+            let r = rewrite_registry_redirect(&files, &[maven_override()]);
+            assert!(
+                !warning_codes(&r).contains(&"redirect_maven_coordinate_unresolved"),
+                "{other}: {:?}",
+                r.warnings
+            );
+            assert!(
+                r.files
+                    .get("pom.xml")
+                    .is_some_and(|p| p.contains(MAVEN_SUFFIXED)),
+                "{other}: pinned: {:?}",
+                r.warnings
+            );
+        }
+    }
+
+    /// A versioned `<profiles>` declaration through an undefined coordinate
+    /// property may be the GA while its profile is active: reported like a
+    /// literal profile declaration (#683), pinned beside a top-level
+    /// literal and skipped when it is the only declaration.
+    #[test]
+    fn maven_pom_undefined_coordinate_property_in_a_profile_is_reported() {
+        let profile = "<profiles><profile><id>p</id><dependencies><dependency>\
+                       <groupId>${parent.g}</groupId><artifactId>slf4j-api</artifactId>\
+                       <version>1.7.36</version></dependency></dependencies></profile></profiles>";
+        for (top, pinned) in [
+            (
+                "<dependencies><dependency><groupId>org.slf4j</groupId><artifactId>slf4j-api</artifactId><version>1.7.36</version></dependency></dependencies>",
+                true,
+            ),
+            ("", false),
+        ] {
+            let mut files = BTreeMap::new();
+            files.insert(
+                "pom.xml".to_string(),
+                format!("<project>\n  {top}\n  {profile}\n</project>\n"),
+            );
+            let r = rewrite_registry_redirect(&files, &[maven_override()]);
+            assert!(
+                warning_codes(&r).contains(&"redirect_maven_profile_dependency_unpatched"),
+                "{pinned}: {:?}",
+                r.warnings
+            );
+            assert_eq!(
+                r.files
+                    .get("pom.xml")
+                    .is_some_and(|p| p.contains(MAVEN_SUFFIXED)),
+                pinned,
+                "{:?}",
+                r.files
+            );
+        }
+    }
+
     /// Fail-closed transitive-only (no matching dependency): a
     /// `<dependencyManagement>` pin for the suffixed version is authored (with
     /// the informational note, NOT the legacy dep_not_found warning).