fix: guard toolchain dedup and preserve existing root attributes

Address reviewer feedback on the Maven toolchains dedup logic:

- Treat jdk toolchains without a string `provides.id` as non-deduplicatable
  and use optional access when comparing ids, so partially-formed or
  nonstandard toolchains.xml files no longer crash setup.
- Preserve the existing `<toolchains>` root attributes (xmlns,
  schemaLocation, …) when present, falling back to the 1.1.0 defaults only
  for attributes the existing file is missing. This avoids silently
  rewriting user-managed metadata or changing the effective namespace.

Adds tests covering custom root attributes and id-less jdk toolchains, and
rebuilds dist/.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

__tests__/toolchains.test.ts

@@ -650,6 +650,170 @@ describe('toolchains tests', () => {
     ).toEqual(result);
   }, 100000);
 
+  it('preserves custom root attributes on existing toolchains.xml', async () => {
+    const jdkInfo = {
+      version: '17',
+      vendor: 'Eclipse Temurin',
+      id: 'temurin_17',
+      jdkHome: '/opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64'
+    };
+
+    const originalFile = `<toolchains xmlns="http://maven.apache.org/TOOLCHAINS/1.0.0"
+  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xsi:schemaLocation="http://maven.apache.org/TOOLCHAINS/1.0.0 http://maven.apache.org/xsd/toolchains-1.0.0.xsd">
+        <toolchain>
+          <type>jdk</type>
+          <provides>
+            <version>1.6</version>
+            <vendor>Sun</vendor>
+            <id>sun_1.6</id>
+          </provides>
+          <configuration>
+            <jdkHome>/opt/jdk/sun/1.6</jdkHome>
+          </configuration>
+        </toolchain>
+      </toolchains>`;
+    const result = `<?xml version="1.0"?>
+<toolchains xmlns="http://maven.apache.org/TOOLCHAINS/1.0.0"
+  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xsi:schemaLocation="http://maven.apache.org/TOOLCHAINS/1.0.0 http://maven.apache.org/xsd/toolchains-1.0.0.xsd">
+  <toolchain>
+    <type>jdk</type>
+    <provides>
+      <version>17</version>
+      <vendor>Eclipse Temurin</vendor>
+      <id>temurin_17</id>
+    </provides>
+    <configuration>
+      <jdkHome>/opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64</jdkHome>
+    </configuration>
+  </toolchain>
+  <toolchain>
+    <type>jdk</type>
+    <provides>
+      <version>1.6</version>
+      <vendor>Sun</vendor>
+      <id>sun_1.6</id>
+    </provides>
+    <configuration>
+      <jdkHome>/opt/jdk/sun/1.6</jdkHome>
+    </configuration>
+  </toolchain>
+</toolchains>`;
+
+    fs.mkdirSync(m2Dir, {recursive: true});
+    fs.writeFileSync(toolchainsFile, originalFile);
+    expect(fs.existsSync(m2Dir)).toBe(true);
+    expect(fs.existsSync(toolchainsFile)).toBe(true);
+
+    await toolchains.createToolchainsSettings({
+      jdkInfo,
+      settingsDirectory: m2Dir,
+      overwriteSettings: true
+    });
+
+    expect(fs.existsSync(m2Dir)).toBe(true);
+    expect(fs.existsSync(toolchainsFile)).toBe(true);
+    expect(fs.readFileSync(toolchainsFile, 'utf-8')).toEqual(
+      toolchains.generateToolchainDefinition(
+        originalFile,
+        jdkInfo.version,
+        jdkInfo.vendor,
+        jdkInfo.id,
+        jdkInfo.jdkHome
+      )
+    );
+    expect(
+      toolchains.generateToolchainDefinition(
+        originalFile,
+        jdkInfo.version,
+        jdkInfo.vendor,
+        jdkInfo.id,
+        jdkInfo.jdkHome
+      )
+    ).toEqual(result);
+  }, 100000);
+
+  it('keeps partially-formed jdk toolchains without an id instead of crashing', async () => {
+    const jdkInfo = {
+      version: '17',
+      vendor: 'Eclipse Temurin',
+      id: 'temurin_17',
+      jdkHome: '/opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64'
+    };
+
+    const originalFile = `<toolchains>
+        <toolchain>
+          <type>jdk</type>
+          <provides>
+            <version>1.6</version>
+            <vendor>Sun</vendor>
+          </provides>
+          <configuration>
+            <jdkHome>/opt/jdk/sun/1.6</jdkHome>
+          </configuration>
+        </toolchain>
+      </toolchains>`;
+    const result = `<?xml version="1.0"?>
+<toolchains xmlns="http://maven.apache.org/TOOLCHAINS/1.1.0"
+  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xsi:schemaLocation="http://maven.apache.org/TOOLCHAINS/1.1.0 https://maven.apache.org/xsd/toolchains-1.1.0.xsd">
+  <toolchain>
+    <type>jdk</type>
+    <provides>
+      <version>17</version>
+      <vendor>Eclipse Temurin</vendor>
+      <id>temurin_17</id>
+    </provides>
+    <configuration>
+      <jdkHome>/opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64</jdkHome>
+    </configuration>
+  </toolchain>
+  <toolchain>
+    <type>jdk</type>
+    <provides>
+      <version>1.6</version>
+      <vendor>Sun</vendor>
+    </provides>
+    <configuration>
+      <jdkHome>/opt/jdk/sun/1.6</jdkHome>
+    </configuration>
+  </toolchain>
+</toolchains>`;
+
+    fs.mkdirSync(m2Dir, {recursive: true});
+    fs.writeFileSync(toolchainsFile, originalFile);
+    expect(fs.existsSync(m2Dir)).toBe(true);
+    expect(fs.existsSync(toolchainsFile)).toBe(true);
+
+    await toolchains.createToolchainsSettings({
+      jdkInfo,
+      settingsDirectory: m2Dir,
+      overwriteSettings: true
+    });
+
+    expect(fs.existsSync(m2Dir)).toBe(true);
+    expect(fs.existsSync(toolchainsFile)).toBe(true);
+    expect(fs.readFileSync(toolchainsFile, 'utf-8')).toEqual(
+      toolchains.generateToolchainDefinition(
+        originalFile,
+        jdkInfo.version,
+        jdkInfo.vendor,
+        jdkInfo.id,
+        jdkInfo.jdkHome
+      )
+    );
+    expect(
+      toolchains.generateToolchainDefinition(
+        originalFile,
+        jdkInfo.version,
+        jdkInfo.vendor,
+        jdkInfo.id,
+        jdkInfo.jdkHome
+      )
+    ).toEqual(result);
+  }, 100000);
+
   it('does not overwrite existing toolchains.xml files', async () => {
     const jdkInfo = {
       version: '17',

dist/setup/index.js

@@ -81237,6 +81237,12 @@ function generateToolchainDefinition(original, version, vendor, id, jdkHome) {
             }
         }
     ];
+    // default root attributes, used when the existing file does not declare its own
+    let rootAttributes = {
+        '@xmlns': 'http://maven.apache.org/TOOLCHAINS/1.1.0',
+        '@xmlns:xsi': 'http://www.w3.org/2001/XMLSchema-instance',
+        '@xsi:schemaLocation': 'http://maven.apache.org/TOOLCHAINS/1.1.0 https://maven.apache.org/xsd/toolchains-1.1.0.xsd'
+    };
     if (original === null || original === void 0 ? void 0 : original.length) {
         // convert existing toolchains into TS native objects for better handling
         // xmlbuilder2 will convert the document into a `{toolchains: { toolchain: [] | {} }}` structure
@@ -81244,7 +81250,14 @@ function generateToolchainDefinition(original, version, vendor, id, jdkHome) {
         const jsObj = (0, xmlbuilder2_1.create)(original)
             .root()
             .toObject();
-        if (jsObj.toolchains && jsObj.toolchains.toolchain) {
+        if (jsObj.toolchains) {
+            // preserve the existing root attributes (xmlns, schemaLocation, …) so we don't
+            // silently rewrite user-managed metadata or change the effective XML namespace;
+            // xmlbuilder2 exposes attributes as `@`-prefixed keys on the element object
+            const existingAttributes = Object.fromEntries(Object.entries(jsObj.toolchains).filter(([key]) => key.startsWith('@')));
+            // fall back to the defaults only for attributes the existing file is missing
+            rootAttributes = Object.assign(Object.assign({}, rootAttributes), existingAttributes);
+            if (jsObj.toolchains.toolchain) {
                 // in case only a single child exists xmlbuilder2 will not create an array and using verbose = true equally doesn't work here
                 // See https://oozcitak.github.io/xmlbuilder2/serialization.html#js-object-and-map-serializers for details
                 if (Array.isArray(jsObj.toolchains.toolchain)) {
@@ -81254,22 +81267,22 @@ function generateToolchainDefinition(original, version, vendor, id, jdkHome) {
                     jsToolchains.push(jsObj.toolchains.toolchain);
                 }
             }
+        }
         // remove potential duplicates based on type & id (which should be a unique combination);
         // self.findIndex will only return the first occurrence, ensuring duplicates are skipped
-        jsToolchains = jsToolchains.filter((value, index, self) => 
+        jsToolchains = jsToolchains.filter((value, index, self) => {
+            var _a;
             // ensure non-jdk toolchains are kept in the results, we must not touch them because they belong to the user
-        value.type !== 'jdk' ||
+            return value.type !== 'jdk' ||
+                // keep toolchains that lack a usable string id (e.g. partially-formed user files);
+                // we cannot safely deduplicate them and must not crash while reading them
+                typeof ((_a = value.provides) === null || _a === void 0 ? void 0 : _a.id) !== 'string' ||
                 index ===
-                self.findIndex(t => t.type === value.type && t.provides.id === value.provides.id));
+                    self.findIndex(t => { var _a, _b; return t.type === value.type && ((_a = t.provides) === null || _a === void 0 ? void 0 : _a.id) === ((_b = value.provides) === null || _b === void 0 ? void 0 : _b.id); });
+        });
     }
-    // TODO: technically bad because we shouldn't re-create the toolchains root node (with possibly different schema values) if it already exists, however, just overriding the toolchain array with xmlbuilder2 is … uh non-trivial
     return (0, xmlbuilder2_1.create)({
-        toolchains: {
+        toolchains: Object.assign(Object.assign({}, rootAttributes), { toolchain: jsToolchains })
-            '@xmlns': 'http://maven.apache.org/TOOLCHAINS/1.1.0',
-            '@xmlns:xsi': 'http://www.w3.org/2001/XMLSchema-instance',
-            '@xsi:schemaLocation': 'http://maven.apache.org/TOOLCHAINS/1.1.0 https://maven.apache.org/xsd/toolchains-1.1.0.xsd',
-            toolchain: jsToolchains
-        }
     }).end({
         format: 'xml',
         wellFormed: false,

src/toolchains.ts

@@ -96,6 +96,13 @@ export function generateToolchainDefinition(
       }
     }
   ];
+  // default root attributes, used when the existing file does not declare its own
+  let rootAttributes: Record<string, string> = {
+    '@xmlns': 'http://maven.apache.org/TOOLCHAINS/1.1.0',
+    '@xmlns:xsi': 'http://www.w3.org/2001/XMLSchema-instance',
+    '@xsi:schemaLocation':
+      'http://maven.apache.org/TOOLCHAINS/1.1.0 https://maven.apache.org/xsd/toolchains-1.1.0.xsd'
+  };
   if (original?.length) {
     // convert existing toolchains into TS native objects for better handling
     // xmlbuilder2 will convert the document into a `{toolchains: { toolchain: [] | {} }}` structure
@@ -103,7 +110,17 @@ export function generateToolchainDefinition(
     const jsObj = xmlCreate(original)
       .root()
       .toObject() as unknown as ExtractedToolchains;
-    if (jsObj.toolchains && jsObj.toolchains.toolchain) {
+    if (jsObj.toolchains) {
+      // preserve the existing root attributes (xmlns, schemaLocation, …) so we don't
+      // silently rewrite user-managed metadata or change the effective XML namespace;
+      // xmlbuilder2 exposes attributes as `@`-prefixed keys on the element object
+      const existingAttributes = Object.fromEntries(
+        Object.entries(jsObj.toolchains).filter(([key]) => key.startsWith('@'))
+      ) as Record<string, string>;
+      // fall back to the defaults only for attributes the existing file is missing
+      rootAttributes = {...rootAttributes, ...existingAttributes};
+
+      if (jsObj.toolchains.toolchain) {
         // in case only a single child exists xmlbuilder2 will not create an array and using verbose = true equally doesn't work here
         // See https://oozcitak.github.io/xmlbuilder2/serialization.html#js-object-and-map-serializers for details
         if (Array.isArray(jsObj.toolchains.toolchain)) {
@@ -112,6 +129,7 @@ export function generateToolchainDefinition(
           jsToolchains.push(jsObj.toolchains.toolchain);
         }
       }
+    }
 
     // remove potential duplicates based on type & id (which should be a unique combination);
     // self.findIndex will only return the first occurrence, ensuring duplicates are skipped
@@ -119,20 +137,19 @@ export function generateToolchainDefinition(
       (value, index, self) =>
         // ensure non-jdk toolchains are kept in the results, we must not touch them because they belong to the user
         value.type !== 'jdk' ||
+        // keep toolchains that lack a usable string id (e.g. partially-formed user files);
+        // we cannot safely deduplicate them and must not crash while reading them
+        typeof value.provides?.id !== 'string' ||
         index ===
           self.findIndex(
-            t => t.type === value.type && t.provides.id === value.provides.id
+            t => t.type === value.type && t.provides?.id === value.provides?.id
           )
     );
   }
 
-  // TODO: technically bad because we shouldn't re-create the toolchains root node (with possibly different schema values) if it already exists, however, just overriding the toolchain array with xmlbuilder2 is … uh non-trivial
   return xmlCreate({
     toolchains: {
-      '@xmlns': 'http://maven.apache.org/TOOLCHAINS/1.1.0',
+      ...rootAttributes,
-      '@xmlns:xsi': 'http://www.w3.org/2001/XMLSchema-instance',
-      '@xsi:schemaLocation':
-        'http://maven.apache.org/TOOLCHAINS/1.1.0 https://maven.apache.org/xsd/toolchains-1.1.0.xsd',
       toolchain: jsToolchains
     }
   }).end({
@@ -181,7 +198,9 @@ async function writeToolchainsFileToDisk(
 
 interface ExtractedToolchains {
   toolchains: {
-    toolchain: Toolchain[] | Toolchain;
+    // root attributes such as xmlns / schemaLocation are exposed as `@`-prefixed keys
+    [attribute: `@${string}`]: string;
+    toolchain?: Toolchain[] | Toolchain;
   };
 }