softwaremill / diffx

Pretty diffs for scala case classes
Apache License 2.0
342 stars 30 forks source link

Unable to `modify` nested case class #418

Open masonedmison opened 2 years ago

masonedmison commented 2 years ago

Given the following simple example:

package diffxsandbox
import com.softwaremill.diffx.generic.auto._
import com.softwaremill.diffx.Diff

case class Address(house: Int, street: String)
case class Person(name: String, address: Address) {
  def diff(other: Person) = {
    val add = Diff.summon[Address]

    val d = 
      Diff.summon[Person]
        .modify(_.address)
        .setTo(add)

    d.apply(this, other)
  }
}

object diffxdemo extends App {
  val a1 = Address(123, "Robin St.")
  val a2 = Address(456, "Robin St.")
  val p1 = Person("Mason", a1)
  val p2 = Person("Mason", a2)

  println {
    p1.diff(p2).isIdentical
  }
}

we are receiving the following stack trace

error] java.lang.ClassCastException: class java.lang.Integer cannot be cast to class scala.Product (java.lang.Integer is in module java.base of loader 'bootstrap'; scala.Product is in unnamed module of loader sbt.internal.ScalaLibraryClassLoader @616527ba)
[error]         at magnolia1.ReadOnlyParam$$anon$3.dereference(interface.scala:213)
[error]         at com.softwaremill.diffx.generic.DiffMagnoliaDerivation.$anonfun$join$3(DiffMagnoliaDerivation.scala:19)
[error]         at scala.collection.immutable.ArraySeq.map(ArraySeq.scala:75)
[error]         at scala.collection.immutable.ArraySeq.map(ArraySeq.scala:35)
[error]         at com.softwaremill.diffx.generic.DiffMagnoliaDerivation.$anonfun$join$2(DiffMagnoliaDerivation.scala:14)
[error]         at com.softwaremill.diffx.DiffxSupport.nullGuard(DiffxSupport.scala:20)
[error]         at com.softwaremill.diffx.DiffxSupport.nullGuard$(DiffxSupport.scala:14)
<elided...>

Does diffx support modification of nested case class parameters? If I instead provide Diff.useEquals[Address] instead of the automically derived instance, the example runs successfully.

ghostbuster91 commented 2 years ago

Does diffx support modification of nested case class parameters?

It should. This is definitely a bug. Thanks for reporting.

ghostbuster91 commented 2 years ago

Released as 0.8.1

ghostbuster91 commented 2 years ago

There is some problem with sonatype:

[189/243] Execution failed: [404: Not Found] Request failed: {"errors":[{"id":"*","msg":"No activity for repository: comsoftwaremill-1133"}]}

The release will probably fail. I will retry it later in the evening.

adamw commented 2 years ago

It's also possible that the release succeeded, but the job failed. Better to check on central if it's there :)

masonedmison commented 2 years ago

Good news, it does look like the release succeeded, I am able to pull it from maven central in my build.sbt. Not so good news, though the error is no longer thrown, it still doesn't look like things work as expected.

For example (substituting the diff function from the example above),

  def diff(other: Person) = {
    val add = 
      Diff.summon[Address]
        .ignore(_.house)

    val d = 
      Diff.summon[Person]
        .modify(_.address)
        .setTo(add)

    d.apply(this, other)
  }

and running the example

  val a1 = Address(123, "Robin St.")
  val a2 = Address(456, "Robin St.")
  val p1 = Person("Mason", a1)
  val p2 = Person("Mason", a2)

  println {
    p1.diff(p2).isIdentical
  }

prints false when I would expect this to print true.

ghostbuster91 commented 2 years ago

We clearly miss some test cases for those. Thanks I will look into that.

masonedmison commented 2 years ago

All good, let me know if there is anything I can do to help :)

ghostbuster91 commented 2 years ago

I found a fix for your case but I also dig a little bit more into that issue and I found countless other edge-cases that would not work even we this new fix applied. This is a design problem and I need to think about that more...

ghostbuster91 commented 2 years ago

I released another fix as 0.8.2 It should solve your problem but that is not a complete solution to that issue as there are still some edge cases where it will not work. I have an idea how to fix it completely but I need to rethink how to approach it.