Merge pull request #139 from geirolz/hotfix-regression-mutable-state

Hotfix safe xml duplication

core/example/modifying.sc

@@ -16,4 +16,4 @@ val data = xml"""
        </wrapper>"""
 
 val result: Modifier.Result[XmlNode] =
-  data.modify(_.root.foo.bar.baz.value.modifyIfNode(_.withText(2)))
+  data.modify(_.root.foo.bar.baz.value.modifyNode(_.withText(2)))

core/src/main/scala/cats/xml/NodeContent.scala

@@ -32,6 +32,16 @@ sealed trait NodeContent {
     case NodeContent.Children(children) => children.toList
     case _                              => Nil
   }
+
+  final def duplicate: NodeContent =
+    this match {
+      case NodeContent.Empty =>
+        NodeContent.Empty
+      case NodeContent.Text(data) =>
+        NodeContent.Text(data)
+      case NodeContent.Children(children: NonEmptyList[XmlNode]) =>
+        NodeContent.Children(children.map(_.duplicate))
+    }
 }
 object NodeContent extends NodeContentInstances {
 

core/src/main/scala/cats/xml/Xml.scala

@@ -40,6 +40,13 @@ trait Xml {
     case _                    => None
   }
 
+  /** Create a new immutable instance with the same values of the current one
+    *
+    * @return
+    *   A new instance with the same values of the current one
+    */
+  def duplicate: Xml
+
   override def equals(obj: Any): Boolean =
     obj match {
       case obj: Xml => Xml.eqXml.eqv(this, obj)

core/src/main/scala/cats/xml/cursor/AttrCursor.scala

@@ -25,7 +25,7 @@ final class AttrCursor(protected val vCursor: NodeCursor, op: AttrCursor.Op)
       $this.focus(node) match {
         case Right(attr: XmlAttribute) =>
           vCursor
-            .modifyIfNode(_.updateAttr(attr.key)(modifier))
+            .modifyNode(_.updateAttr(attr.key)(modifier))
             .apply(node)
         case Left(failure) =>
           ModifierFailure.CursorFailed(NonEmptyList.one(failure)).asLeft
@@ -39,7 +39,7 @@ final class AttrCursor(protected val vCursor: NodeCursor, op: AttrCursor.Op)
           attr.mapDecode(f) match {
             case Valid(newAttrValue) =>
               vCursor
-                .modifyIfNode(_.updateAttr(attr.key)(_ => newAttrValue))
+                .modifyNode(_.updateAttr(attr.key)(_ => newAttrValue))
                 .apply(node)
             case Invalid(failures: NonEmptyList[DecoderFailure]) =>
               ModifierFailure.DecoderFailed(failures).asLeft

core/src/main/scala/cats/xml/cursor/NodeCursor.scala

@@ -36,10 +36,10 @@ sealed trait NodeCursor
     }
 
   // modify
-  def modifyIfNode(modifier: Endo[XmlNode.Node]): Modifier[XmlNode] =
+  def modifyNode(modifier: Endo[XmlNode.Node]): Modifier[XmlNode] =
     modify(_.fold(ifNode = modifier, ifGroup = identity))
 
-  def modifyIfGroup(modifier: Endo[XmlNode.Group]): Modifier[XmlNode] =
+  def modifyGroup(modifier: Endo[XmlNode.Group]): Modifier[XmlNode] =
     modify(_.fold(ifNode = identity, ifGroup = modifier))
 
   override def modify(modifier: Endo[XmlNode]): Modifier[XmlNode] =

core/src/main/scala/cats/xml/cursor/TextCursor.scala

@@ -25,7 +25,7 @@ final class TextCursor(protected[xml] val lastCursor: NodeCursor)
     Modifier(node =>
       $this.as[T].focus(node) match {
         case Validated.Valid(textValue) =>
-          lastCursor.modifyIfNode(_.withText(f(textValue)))(node)
+          lastCursor.modifyNode(_.withText(f(textValue)))(node)
         case Validated.Invalid(failures) =>
           ModifierFailure.CursorFailed(failures).asLeft
       }

core/src/main/scala/cats/xml/xmlAttribute.scala

@@ -31,6 +31,9 @@ final case class XmlAttribute(key: String, value: XmlData) extends Xml with Seri
     dummyImplicit: DummyImplicit
   ): Boolean =
     exists(_ == key, (data: XmlData) => valuep(data.asString))
+
+  def duplicate: XmlAttribute =
+    XmlAttribute(key, value.duplicate)
 }
 object XmlAttribute extends XmlAttributeSyntax with XmlAttributeInstances {
 

core/src/main/scala/cats/xml/xmlData.scala

@@ -1,5 +1,6 @@
 package cats.xml
 
+import cats.xml.XmlData.*
 import cats.xml.codec.Decoder
 import cats.{Eq, Order, Show}
 
@@ -23,9 +24,23 @@ sealed trait XmlData extends Xml with Serializable {
     case XmlData.XmlArray(value)  => value.isEmpty
     case _                        => false
   }
+
+  override def duplicate: XmlData =
+    this match {
+      case XmlNull              => XmlNull
+      case value: XmlString     => value.copy()
+      case value: XmlChar       => value.copy()
+      case value: XmlBool       => value.copy()
+      case value: XmlArray[_]   => value.copy()
+      case value: XmlLong       => value.copy()
+      case value: XmlFloat      => value.copy()
+      case value: XmlDouble     => value.copy()
+      case value: XmlBigDecimal => value.copy()
+    }
 }
 case object XmlNull extends Xml with XmlNode.Null with XmlData {
-  override final def isEmpty: Boolean = true
+  override final def isEmpty: Boolean        = true
+  override final def duplicate: XmlNull.type = this
   // To avoid stackoverflow since the XmlPrinter used by the Eq instance uses patter matching.
   override final def equals(obj: Any): Boolean = obj.isInstanceOf[XmlNull.type]
 }

core/src/main/scala/cats/xml/xmlNode.scala

@@ -136,7 +136,7 @@ sealed trait XmlNode extends Xml {
     * @return
     *   A new instance with the same values of the current one
     */
-  def duplicate: Self
+  def duplicate: XmlNode
 
   /** Convert the node to a group. If this instance already is a group it will be returned the same
     * instance.
@@ -206,11 +206,11 @@ object XmlNode extends XmlNodeInstances with XmlNodeSyntax {
 
   private[xml] trait Null extends XmlNode {
     override type Self = Null
-    override def label: String                                          = ""
-    override def attributes: List[XmlAttribute]                         = Nil
-    override def content: NodeContent                                   = NodeContent.empty
-    override def duplicate: Self                                        = this
-    override private[xml] def updateContent(f: Endo[NodeContent]): Self = this
+    override final def label: String                                          = ""
+    override final def attributes: List[XmlAttribute]                         = Nil
+    override final def content: NodeContent                                   = NodeContent.empty
+    override def duplicate: Self                                              = this
+    override final private[xml] def updateContent(f: Endo[NodeContent]): Self = this
   }
 
   lazy val emptyGroup: XmlNode.Group = new Group(NodeContent.empty)
@@ -311,7 +311,11 @@ object XmlNode extends XmlNodeInstances with XmlNodeSyntax {
 
     override def content: NodeContent = mContent
 
-    override def duplicate: XmlNode.Node = safeCopy()
+    override def duplicate: XmlNode.Node = safeCopy(
+      label      = label,
+      attributes = attributes.map(_.duplicate),
+      content    = content.duplicate
+    )
 
     @impure
     private[xml] def updateLabel(f: Endo[String]): XmlNode.Node =
@@ -377,7 +381,9 @@ object XmlNode extends XmlNodeInstances with XmlNodeSyntax {
 
     override def content: NodeContent = mContent
 
-    override def duplicate: XmlNode.Group = safeCopy()
+    override def duplicate: XmlNode.Group = safeCopy(
+      content = content.duplicate
+    )
 
     /** Convert current instance to a [[XmlNode.Node]].
       *
@@ -416,9 +422,8 @@ object XmlNode extends XmlNodeInstances with XmlNodeSyntax {
     @impure
     private[xml] def safeCopy(
       content: NodeContent = this.mContent
-    ): XmlNode.Group = new Group(
-      unsafeRequireNotNull(content)
-    )
+    ): XmlNode.Group =
+      new Group(unsafeRequireNotNull(content))
 
     // -----------------------------//
     /* ################################################ */

core/src/test/scala/cats/xml/XmlPrinterSuite.scala

@@ -1,6 +1,6 @@
 package cats.xml
 
-import cats.xml.testing.{DataSize, Ops, XmlNodeGen}
+import cats.xml.testing.{DataSize, Ops, XmlGen}
 import org.scalacheck.Arbitrary
 import org.scalacheck.Prop.forAll
 
@@ -233,7 +233,7 @@ class XmlPrinterPerformanceSuite extends munit.ScalaCheckSuite {
   property("XmlPrinter.default.prettyString with XL document") {
 
     implicit val arbXmlNode: Arbitrary[XmlNode] = Arbitrary(
-      XmlNodeGen.genXmlNode(DataSize.L)
+      XmlGen.genXmlNode(DataSize.L)
     )
 
     forAll { (value: XmlNode) =>

core/src/test/scala/cats/xml/codec/decoderSuite.scala

@@ -210,7 +210,7 @@ class DecoderSuite extends munit.FunSuite {
 
 class DecoderInstancesSuite extends munit.DisciplineSuite {
 
-  import cats.xml.testing.arbitrary.XmlDataArbitrary.*
+  import cats.xml.testing.arbitrary.XmlArbitrary.*
 
   test("Decoder.decodeUnit") {
     assertEquals(

core/src/test/scala/cats/xml/codec/encoderSuite.scala

@@ -42,7 +42,7 @@ class EncoderSuite extends munit.FunSuite {
 class EncoderInstancesSuite extends munit.ScalaCheckSuite {
 
   import cats.xml.testing.arbitrary.CommonArbitrary.*
-  import cats.xml.testing.arbitrary.XmlDataArbitrary.*
+  import cats.xml.testing.arbitrary.XmlArbitrary.*
 
   property(s"Encoder.encodeOption") {
     forAll { (value: Option[Int]) =>

core/src/test/scala/cats/xml/modifier/ModifierSuite.scala

@@ -1,7 +1,48 @@
 package cats.xml.modifier
 
+import cats.data.Validated.Valid
+import cats.xml.XmlNode
+import cats.xml.syntax.XmlAttrStringOps
+
 class ModifierSuite extends munit.FunSuite {
 
+  test("Modifier works as expected") {
+    val node: XmlNode =
+      XmlNode("wrapper")
+        .withAttributes("a" := "1", "b" := "2")
+        .withChildren(
+          XmlNode("root")
+            .withAttributes("a" := "1", "b" := "2")
+            .withChildren(
+              XmlNode("foo")
+                .withChildren(
+                  XmlNode("baz")
+                    .withAttributes("a" := "1", "b" := "2")
+                    .withChildren(
+                      XmlNode("bar")
+                        .withChildren(
+                          XmlNode("value")
+                            .withText(1)
+                        )
+                    )
+                )
+            )
+        )
+
+    val result: Modifier.Result[XmlNode] =
+      node.modify(_.root.foo.baz.bar.value.modifyNode(_.withText(2)))
+
+    assertEquals(
+      obtained = node.focus(_.root.foo.baz.bar.value.text.as[Int]),
+      expected = Valid(1)
+    )
+
+    assertEquals(
+      obtained = result.map(_.focus(_.root.foo.baz.bar.value.text.as[Int])),
+      expected = Right(Valid(2))
+    )
+  }
+
   test("Modifier.id") {
     assertEquals(
       obtained = Modifier.id[String]("FOO"),