Scala product serializer only works for simple case classes
nvollmar opened this issue · 11 comments
The product serializer relies on product arity to serialize a case class which will fail for case classes with an additional parameter list:
case class SimpleProduct(id: UUID, long: Long) // can be serialized
case class CurriedProduct(id: UUID)(long: Long) // will fail on de-serialization!
Reason:
In both cases getDeclaredConstructors will return a constructor with the same parameters two (UUID, Long). But productArity will return 2 for SimpleProduct but 1 for CurriedProduct.
Proposal:
The ScalaProductSerializer should not produce something that fails on de-serialization.
Either fix it or remove it (currently not configured by default anyway).
To fix it we have several options:
- Extend to handle all kinds of case classes with extended reflection
- Detect and delegate such case classes to another serializer for non-trivial case classes
- Detect and fail already at serialization so a custom serializer can be configured in such cases
should not produce something that fails on de-serialization.
100% agree
And as long as kryo already serializes case classes properly I opt for delete. (@nvollmar except if this serializer provides better performance... have you checked that?)
If we remove the product serializer, will it change the behaviour? E.g. if a case class has a private field (not that it's good idea) that is not marked @transient?
Yes, it would change the behavior. And if anyone actually was using it it would be a breaking change. I’ll change the pull request and deprecate and document it for now. Should be removed with 2.0 then.
@danischroeter speed seems roughly the same, but size is actually considerably bigger. So I don’t see any benefit.
An option is to fall-back to kryo's field serializer if productArity != getDeclaredConstructors.head.getParameters.length
To clarify, the serializer is not used by default, you have to manually register it.
As is, the kryo serializer is used for case classes which seems to work fine/even better.
Fair enough - may be just declare it deprecated and mention the cases it can't handle.
We might even add a log.warn if this serializer is registered so if anyone is using it we can make him aware that this serializer is less efficient and does not work for all cases.
Deprecated and added warning
@danischroeter , I interpret @nvollmar's comment that the product serializer was producing smaller serialized payloads. Adding a log.warn on registration is a good way to way to make deprecation more visible.
@luben product serializer's result size was actually larger
simple example of a case class with a uuid and a long:
kryo: [48,1,1,-7,-59,-126,-92,-126,-65,-35,-10,-36,-97,-56,-51,-14,-70,-3,-20,-19,12,2] (22 bytes)
product: [48,1,2,49,1,-7,-59,-126,-92,-126,-65,-35,-10,-36,-97,-56,-51,-14,-70,-3,-20,-19,12,9,2] (25 bytes)
this is with both classes registered - with fallback to FQCN:
kryo: 113 bytes
product: 131 bytes