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

[Circe] Add a way to skip dispatch over intermediate abstract classes/traits #88

Open
vpavkin opened this issue Apr 10, 2018 · 5 comments · Fixed by #98
Open

[Circe] Add a way to skip dispatch over intermediate abstract classes/traits #88

vpavkin opened this issue Apr 10, 2018 · 5 comments · Fixed by #98
Assignees

Comments

@vpavkin
Copy link
Contributor

vpavkin commented Apr 10, 2018

Reproduction: https://github.com/vpavkin/magnolia/blob/e278256e713ddf3b1aa3365898e930eff168274b/tests/src/main/scala/IntermediateTraitsTest.scala

Shapeless generic skips intermediate traits/abstract classes and forms a coproduct of only leaf types.
This makes perfect sense for many scenarios, JSON included.

Magnolia, on the other hand, dispatches through all intermediate types.
For encoding it's not that bad, but for decoding it's a showstopper.

Given

  sealed trait T
  case class A(a: Int) extends T
  case class B(b: String) extends T
  sealed trait C extends T
  case class C1(c1: Int) extends C
  case class C2(c2: String) extends C

Encoding an instance of C1 as T will produce this (which is not nice, but manageable probably):

{
   "C":{
      "C1":{
         "c1":2
      }
   }
}

But problem is that to properly decode an instance of C1 we have to scope it as C, which is very unfriendly to JSON API users. And the more simple and intuitive form won't decode:

{
   "C1":{
      "c1":2
   }
}
@propensive
Copy link
Collaborator

Thanks for the report and reproduction. I think it shouldn't be too difficult to fix. We currently use knownDirectSubclasses whereas we really want all subclasses, not just the direct ones. I think it should be possible to call knownDirectSubclasses recursively. I'll take a look.

@propensive propensive self-assigned this Jun 5, 2018
propensive added a commit that referenced this issue Jun 5, 2018
@propensive
Copy link
Collaborator

Would you mind checking whether this branch fixes the issue, @vpavkin? I haven't had a chance to test it...

https://github.com/propensive/magnolia/tree/known-direct-and-indirect-subclasses

@vpavkin
Copy link
Contributor Author

vpavkin commented Jun 5, 2018

@propensive thanks! Will test the branch asap, hopefully by the end of the day will have some results

@vpavkin
Copy link
Contributor Author

vpavkin commented Jun 5, 2018

@propensive it helped! I rebased #86 onto #98 and now these two pass:

tests/runMain CirceRecursiveTypeTest
tests/runMain IntermediateTraitsTest

@propensive
Copy link
Collaborator

Great, that's progress! It's only a partial fix, so I've reopened after it auto-closed.

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.

2 participants