XRENDERING-802: Prepared macro content leads to duplicated IDs when inserted

xwiki-rendering-api/pom.xml

@@ -32,7 +32,7 @@
   <packaging>jar</packaging>
   <description>XWiki Rendering - Api</description>
   <properties>
-    <xwiki.jacoco.instructionRatio>0.62</xwiki.jacoco.instructionRatio>
+    <xwiki.jacoco.instructionRatio>0.64</xwiki.jacoco.instructionRatio>
     <!-- Skipping revapi since xwiki-rendering-legacy-api wraps this module and runs checks on it -->
     <xwiki.revapi.skip>true</xwiki.revapi.skip>
   </properties>

xwiki-rendering-api/src/main/java/org/xwiki/rendering/block/XDOM.java

@@ -25,6 +25,7 @@
 import org.xwiki.rendering.listener.Listener;
 import org.xwiki.rendering.listener.MetaData;
 import org.xwiki.rendering.util.IdGenerator;
+import org.xwiki.stability.Unstable;
 
 /**
  * Contains the full tree of {@link Block} that represent a XWiki Document's content.
@@ -101,7 +102,54 @@ public IdGenerator getIdGenerator()
      */
     public void setIdGenerator(IdGenerator idGenerator)
     {
+        setIdGenerator(idGenerator, false);
+    }
+
+    /**
+     * Sets a new id generator for this document and optionally adapts the existing ids to make them unique in the scope
+     * of the new id generator. Adapting the existing ids is needed if you plan to insert this document in a larger one,
+     * in which case you will have to reuse the id generator of the larger document for this document. On the other
+     * hand, if this document is a clone of another document, and you plan to use it alone then you don't need to adapt
+     * the existing ids. In this case, even if the id generator is different, it was created as a copy of the original
+     * id generator, so the existing ids are already unique.
+     *
+     * @param idGenerator a stateful id generator for the whole document
+     * @param adaptExistingIds whether to adapt the existing ids to make them unique in the scope of the new id
+     *            generator; pass true if the new id generator is from a another document where you plan to insert this
+     *            document; pass false if this document is a clone and the new id generator is a copy of the original id
+     *            generator
+     * @since 17.10.6
+     * @since 18.3.0RC1
+     */
+    @Unstable
+    public void setIdGenerator(IdGenerator idGenerator, boolean adaptExistingIds)
+    {
+        boolean changed = this.idGenerator != idGenerator;
         this.idGenerator = idGenerator;
+        if (this.idGenerator != null && changed && adaptExistingIds) {
+            // Make sure the existing ids are unique in the scope of the new id generator.
+            makeIdsUnique();
+        }
+    }
+
+    /**
+     * Make sure heading and image blocks have unique ids in the scope of the provided id generator. We target only
+     * heading and image blocks because these are currently the only blocks that can have generated ids. The ids of
+     * macro blocks are not generated.
+     */
+    private void makeIdsUnique()
+    {
+        // Traverse the XDOM and adapt all image and heading blocks.
+        this.getBlocks(block -> {
+            // Would be nice to have an interface that marks blocks with generated ids, but for now we just check the
+            // known block types.
+            if (block instanceof ImageBlock imageBlock) {
+                imageBlock.setId(this.idGenerator.adaptId(imageBlock.getId()));
+            } else if (block instanceof HeaderBlock headerBlock) {
+                headerBlock.setId(this.idGenerator.adaptId(headerBlock.getId()));
+            }
+            return false;
+        }, Block.Axes.DESCENDANT);
     }
 
     @Override

xwiki-rendering-api/src/main/java/org/xwiki/rendering/util/IdGenerator.java

@@ -24,6 +24,7 @@
 import java.util.concurrent.ConcurrentHashMap;
 
 import org.apache.commons.lang3.StringUtils;
+import org.xwiki.stability.Unstable;
 
 /**
  * Stateful generator of id attributes. It's stateful since it remembers the generated ids. Thus a new instance of it
@@ -139,6 +140,26 @@ public String generateUniqueId(String prefix, String text)
         return id;
     }
 
+    /**
+     * Adapts the given id to make it unique in the scope of this id generator. Use this method to make existing ids
+     * unique after setting a new id generator.
+     *
+     * @param id the id to adapt to make it unique
+     * @return a unique id, in the scope of this id generator; it returns the same id if it's already unique
+     * @since 17.10.6
+     * @since 18.3.0RC1
+     */
+    @Unstable
+    public String adaptId(String id)
+    {
+        if (StringUtils.isNotBlank(id)) {
+            String prefix = id.substring(0, 1);
+            String suffix = id.substring(1);
+            return generateUniqueId(prefix, suffix);
+        }
+        return id;
+    }
+
     /**
      * Normalize passed string into valid string.
      * <ul>

xwiki-rendering-api/src/test/java/org/xwiki/rendering/block/XDOMTest.java

@@ -0,0 +1,145 @@
+/*
+ * See the NOTICE file distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+package org.xwiki.rendering.block;
+
+import java.util.List;
+
+import org.junit.jupiter.api.Test;
+import org.xwiki.rendering.listener.HeaderLevel;
+import org.xwiki.rendering.util.IdGenerator;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+/**
+ * Unit tests for {@link XDOM}.
+ *
+ * @version $Id$
+ */
+class XDOMTest
+{
+    private IdGenerator idGenerator = new IdGenerator();
+
+    @Test
+    void setIdGenerator()
+    {
+        HeaderBlock heading1 = new HeaderBlock(List.of(new WordBlock("Heading 1")), HeaderLevel.LEVEL1);
+        heading1.setId("Hheading");
+
+        ImageBlock image1 = new ImageBlock(null, true);
+        image1.setId("Ilogo");
+        ParagraphBlock paragraph1 = new ParagraphBlock(List.of(image1));
+
+        HeaderBlock heading2 = new HeaderBlock(List.of(new WordBlock("Heading 2")), HeaderLevel.LEVEL2);
+        heading2.setId("Hheading-2");
+
+        ImageBlock image2 = new ImageBlock(null, true);
+        image2.setId("Ilogo-2");
+        ParagraphBlock paragraph2 = new ParagraphBlock(List.of(image2));
+
+        HeaderBlock heading3 = new HeaderBlock(List.of(new WordBlock("Heading 3")), HeaderLevel.LEVEL3);
+        heading3.setId("Hheading");
+
+        ImageBlock image3 = new ImageBlock(null, true);
+        image3.setId("Ilogo");
+        ParagraphBlock paragraph3 = new ParagraphBlock(List.of(image3));
+
+        HeaderBlock heading4 = new HeaderBlock(List.of(new WordBlock("Heading 4")), HeaderLevel.LEVEL1);
+        ImageBlock image4 = new ImageBlock(null, true);
+        ParagraphBlock paragraph4 = new ParagraphBlock(List.of(image4));
+
+        XDOM xdom =
+            new XDOM(
+                List.of(
+                    new SectionBlock(List.of(heading1, paragraph1,
+                        new SectionBlock(
+                            List.of(heading2, paragraph2, new SectionBlock(List.of(heading3, paragraph3)))))),
+                    new SectionBlock(List.of(heading4, paragraph4))));
+
+        // Suppose the id generated has already been used to generated some ids.
+        idGenerator.generateUniqueId("H", "heading");
+        idGenerator.generateUniqueId("logo");
+
+        // Set the id generator without adapting the existing ids.
+        xdom.setIdGenerator(idGenerator);
+
+        assertEquals("Hheading", heading1.getId());
+        assertEquals("Hheading-2", heading2.getId());
+        assertEquals("Hheading", heading3.getId());
+        assertNull(heading4.getId());
+
+        assertEquals("Ilogo", image1.getId());
+        assertEquals("Ilogo-2", image2.getId());
+        assertEquals("Ilogo", image3.getId());
+        assertNull(image4.getId());
+
+        // Set the same id generator again. The existing ids are not adapted because it's the same id generator.
+        xdom.setIdGenerator(idGenerator, true);
+
+        assertEquals("Hheading", heading1.getId());
+        assertEquals("Hheading-2", heading2.getId());
+        assertEquals("Hheading", heading3.getId());
+        assertNull(heading4.getId());
+
+        assertEquals("Ilogo", image1.getId());
+        assertEquals("Ilogo-2", image2.getId());
+        assertEquals("Ilogo", image3.getId());
+        assertNull(image4.getId());
+
+        // Set a new id generator and adapt the existing ids.
+        xdom.setIdGenerator(new IdGenerator(idGenerator), true);
+
+        assertEquals("Hheading-1", heading1.getId());
+        assertEquals("Hheading-2", heading2.getId());
+        assertEquals("Hheading-3", heading3.getId());
+        assertNull(heading4.getId());
+
+        assertEquals("Ilogo-1", image1.getId());
+        assertEquals("Ilogo-2", image2.getId());
+        assertEquals("Ilogo-3", image3.getId());
+        assertNull(image4.getId());
+
+        // Verify that setting a null id generator doesn't change the existing ids.
+        xdom.setIdGenerator(null, true);
+
+        assertEquals("Hheading-1", heading1.getId());
+        assertEquals("Hheading-2", heading2.getId());
+        assertEquals("Hheading-3", heading3.getId());
+        assertNull(heading4.getId());
+
+        assertEquals("Ilogo-1", image1.getId());
+        assertEquals("Ilogo-2", image2.getId());
+        assertEquals("Ilogo-3", image3.getId());
+        assertNull(image4.getId());
+
+        // Set again a new id generator without adapting the existing ids.
+        xdom.setIdGenerator(idGenerator, false);
+
+        assertEquals("Hheading-1", heading1.getId());
+        assertEquals("Hheading-2", heading2.getId());
+        assertEquals("Hheading-3", heading3.getId());
+        assertNull(heading4.getId());
+
+        assertEquals("Ilogo-1", image1.getId());
+        assertEquals("Ilogo-2", image2.getId());
+        assertEquals("Ilogo-3", image3.getId());
+        assertNull(image4.getId());
+    }
+}

xwiki-rendering-api/src/test/java/org/xwiki/rendering/util/IdGeneratorTest.java

@@ -19,43 +19,37 @@
  */
 package org.xwiki.rendering.util;
 
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 /**
  * Validate {@link IdGenerator}.
  *
  * @version $Id$
  */
-public class IdGeneratorTest
+class IdGeneratorTest
 {
-    private IdGenerator idGenerator;
-
-    @BeforeEach
-    public void setUp()
-    {
-        this.idGenerator = new IdGenerator();
-    }
+    private IdGenerator idGenerator = new IdGenerator();
 
     @Test
-    public void generateUniqueId()
+    void generateUniqueId()
     {
         assertEquals("Itext", this.idGenerator.generateUniqueId("text"));
         assertEquals("Itext-1", this.idGenerator.generateUniqueId("te xt"));
     }
 
     @Test
-    public void generateUniqueIdWithPrefix()
+    void generateUniqueIdWithPrefix()
     {
         assertEquals("prefixtext", this.idGenerator.generateUniqueId("prefix", "text"));
         assertEquals("prefixtext-1", this.idGenerator.generateUniqueId("prefix", "te xt"));
     }
 
     @Test
-    public void generateUniqueIdFromNonAlphaNum()
+    void generateUniqueIdFromNonAlphaNum()
     {
         assertEquals("I:_.-", this.idGenerator.generateUniqueId(":_.-"));
         assertEquals("Iwithspace", this.idGenerator.generateUniqueId("with space"));
@@ -65,7 +59,7 @@ public void generateUniqueIdFromNonAlphaNum()
     }
 
     @Test
-    public void generateUniqueIdWhenInvalidEmptyPrefix()
+    void generateUniqueIdWhenInvalidEmptyPrefix()
     {
         Throwable exception = assertThrows(IllegalArgumentException.class, () -> {
             this.idGenerator.generateUniqueId("", "whatever");
@@ -75,12 +69,32 @@ public void generateUniqueIdWhenInvalidEmptyPrefix()
     }
 
     @Test
-    public void generateUniqueIdWhenInvalidNonAlphaPrefix()
+    void generateUniqueIdWhenInvalidNonAlphaPrefix()
     {
         Throwable exception = assertThrows(IllegalArgumentException.class, () -> {
             this.idGenerator.generateUniqueId("a-b", "whatever");
         });
         assertEquals("The prefix [a-b] should only contain alphanumerical characters and not be empty.",
             exception.getMessage());
     }
+
+    @Test
+    void adaptId()
+    {
+        // Blank id.
+        assertNull(this.idGenerator.adaptId(null));
+        assertEquals("", this.idGenerator.adaptId(""));
+        assertEquals("", this.idGenerator.adaptId(""));
+        assertEquals("   ", this.idGenerator.adaptId("   "));
+        assertEquals("   ", this.idGenerator.adaptId("   "));
+
+        // Id that is already unique.
+        assertEquals("test", this.idGenerator.adaptId("test"));
+        assertEquals("t", this.idGenerator.adaptId("t"));
+
+        // Id that is not unique.
+        assertEquals("test-1", this.idGenerator.adaptId("test"));
+        assertEquals("test-2", this.idGenerator.adaptId("test"));
+        assertEquals("t-1", this.idGenerator.adaptId("t"));
+    }
 }