scalalandio / chimney

Scala library for boilerplate-free, type-safe data transformations
https://chimney.readthedocs.io
Apache License 2.0
1.18k stars 95 forks source link

Protobuf import doesn't deal with Empty case #425

Closed ghostdogpr closed 1 year ago

ghostdogpr commented 1 year ago

Checklist

Describe the bug Apologies if I misunderstood something in the docs.

In the oneof part of the docs (https://chimney.readthedocs.io/en/stable/cookbook/#oneof-fields), it says we can handle Empty manually like this:

.withCoproductInstancePartial[pb.addressbook.AddressBookType.Value.Empty.type](
  _ => partial.Result.fromEmpty
)

Or using a simple import:

import io.scalaland.chimney.protobufs._ // includes support for empty scalapb.GeneratedMessage

While the first one works well, the second doesn't.

It looks like the import provides partialTransformerFromEmptyOneOfInstance for a GeneratedOneof with ValueType = Nothing, but the protobuf generated code doesn't look like that. It's just a simple case object Empty.

Reproduction An easy way to reproduce is to modify the existing unit test ProtobufOneOfSpec.

Before:

pbStatus
  .intoPartial[order.CustomerStatus]
  .withCoproductInstancePartial[pb.order.CustomerStatus.Empty.type](_ => partial.Result.fromEmpty)
  .withCoproductInstance[pb.order.CustomerStatus.NonEmpty](_.transformInto[order.CustomerStatus])
  .transform
  .asOption ==> Some(domainStatus)

After:

import io.scalaland.chimney.protobufs.*
pbStatus
 .intoPartial[order.CustomerStatus]
//  .withCoproductInstancePartial[pb.order.CustomerStatus.Empty.type](_ => partial.Result.fromEmpty)
  .withCoproductInstance[pb.order.CustomerStatus.NonEmpty](_.transformInto[order.CustomerStatus])
  .transform
  .asOption ==> Some(domainStatus)

Expected behavior According to the docs, it should compile.

Actual behavior

[error]    |      pbStatus
[error]    |      ^
[error]    |Chimney can't derive transformation from io.scalaland.chimney.examples.pb.order.CustomerStatus to io.scalaland.chimney.fixtures.order.CustomerStatus
[error]    |
[error]    |io.scalaland.chimney.fixtures.order.CustomerStatus
[error]    |  derivation from empty: io.scalaland.chimney.examples.pb.order.CustomerStatus.Empty to io.scalaland.chimney.fixtures.order.CustomerStatus is not supported in Chimney!
[error]    |
[error]    |
[error]    |io.scalaland.chimney.fixtures.order.CustomerStatus
[error]    |  can't transform coproduct instance io.scalaland.chimney.examples.pb.order.CustomerStatus.Empty to io.scalaland.chimney.fixtures.order.CustomerStatus

Which Chimney version do you use 0.8.0

Which platform do you use

If you checked JVM 17

Additional context Looking at the generated code from scalapb, Empty is defined like this:

case object Empty extends io.scalaland.chimney.examples.pb.order.CustomerStatus

There is also another Empty which is used internally:

  sealed trait SealedValue extends _root_.scalapb.GeneratedOneof {
    ...
  }
  object SealedValue {
    @SerialVersionUID(0L)
    case object Empty extends io.scalaland.chimney.examples.pb.order.CustomerStatusMessage.SealedValue {
      type ValueType = _root_.scala.Nothing

But it's not the one used when we convert types with Chimney so the import doesn't help.

MateuszKubuszok commented 1 year ago

It is rather clear to me that your case is described here: https://chimney.readthedocs.io/en/stable/cookbook/#sealed_value-oneof-fields.

ScalaPB can generate oneOf in 3 different ways:

So no, it is not a bug. Your protobuf definition explicitly opted-in with the only representation of 3 possible, that Chimney cannot automatically handle and it is described in the documentation. (I say explicitly because you have to name oneOf sealed_value, and I don't think someone would come up with such name accidentally). You can map things manually or, if you have the ability, change sealed_value name in your protobufs to sealed_value_optional. If you look at the ProtobufOneOfSpec you mentioned, you'll see that there are 3 different tests for 3 different representations. We are generating these protobufs from sources, and update dependencies regularly so any (unlikely) change in behavior would be discovered.

ghostdogpr commented 1 year ago

Thanks for the detailed explanation! I wrongly assumed the import was relevant to the second case. This is all clear now 🙏

MateuszKubuszok commented 1 year ago

NP, I can add some warning to the docs to make it clearer.