Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to modify nested case class #418

Open
masonedmison opened this issue Oct 24, 2022 · 9 comments · Fixed by #419
Open

Unable to modify nested case class #418

masonedmison opened this issue Oct 24, 2022 · 9 comments · Fixed by #419

Comments

@masonedmison
Copy link

masonedmison commented Oct 24, 2022

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
Copy link
Collaborator

Does diffx support modification of nested case class parameters?

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

@ghostbuster91
Copy link
Collaborator

Released as 0.8.1

@ghostbuster91
Copy link
Collaborator

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
Copy link
Member

adamw commented Oct 25, 2022

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

@masonedmison
Copy link
Author

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
Copy link
Collaborator

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

@ghostbuster91 ghostbuster91 reopened this Oct 25, 2022
@masonedmison
Copy link
Author

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

@ghostbuster91
Copy link
Collaborator

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants