EnumSerializer fails when when multiple enums are used

[aboisvert]

lift-json-ext's EnumSerializer and EnumNameSerializer do not work properly when multiple enums are used to serialize/deserialize data.

The root cause appears to be the use of,

  val EnumerationClass = classOf[E#Value]

which returns the same class for all Enumeration-deriving classes, and therefore the subsequent match:

    PartialFunction[(TypeInfo, JValue), E#Value] = {
      case (TypeInfo(EnumerationClass, _), json) => json match {

matches against any possible enums, leading to -- at best -- a runtime failure because the enum value cannot be found, or -- at worst -- a sneaky failure since a different enum value may be returned.

I'm attaching a test that demonstrates the issue with EnumNameSerializer.

[github-importer]

[[file:cLt16Q5v0r4j4ZacwqjQYw]]

[github-importer]

I ran into the same problem. I've created a Serializer named EnumerationSerializer (attached) that stores the enum type along with the value so that it can make sure it deserializes to the correct type. You can either use the enum class name as the key or specify one (the optional second argument). If you specify one, you need to make sure that it's unique among all the configured EnumerationSerializers.

Specifying a different serialization key allows you to shorten the serialized form as well as decouple it from the implementation.

[iron9light]

Me too

[github-importer]

Imported from Assembla: http://www.assembla.com/spaces/liftweb/tickets/1080

[hiltym]

In the following thread, it was suggested that this might be fixed with the 2.10 reflection API:
http://comments.gmane.org/gmane.comp.web.lift/57096

Are there any plans to fix this soon now that this reflection API exists?

[jonifreeman]

Scala reflection API is still experimental. I have no plans to use it until it is finished, that's something I don't know when it will happen. But patches are welcomed :)

[hiltym]

Ok, thanks for the update! Then I'll hope that this reflection API will be stable soon :-)

[holograph]

Until Scala 2.10 is finalized and becomes popular, another workaround is to chain multiple enums into a single serializer and lookup the relevant value in each one on deserialization. You still need to be careful with collisions though. See the gist here: https://gist.github.com/4328894

[Shadowfiend]

Another possible solution is to avoid using regular enums and instead use sealed traits with singleton object extensions. You do need a special singleton serializer at that point, however; @farmdawgnation , any chance you can provide an example for folks who wander through here?

[farmdawgnation]

I think I posted an example to the list at some point. Let me see if I can dig it up.

[farmdawgnation]

Ok, here we go.

The solution to this, as @Shadowfiend mentioned, is to not use Enums. The fact that these don't serialize and deserialize correctly is more a byproduct of how Scala implements them, and isn't truly a defect in our implementation of JSON serialization.

To avoid that, you can use sealed traits and singletons as enumerations. For example:

sealed trait Food
case object Bacon
case object Sausage

Then you can do match statements, and you'll get compiler warnings for any occasions where you're doing a match and you haven't hit a clause for every possible option. To facilitate this, you can use a singleton serializer I wrote available here.

To use it with the above example, you'd declare something like

object FoodSerializer extends SingletonSerializer[Food]

Then just ensure that FoodSerializer is in whatever formats object you have in scope at the time of serialization/deserialization and things will Just Work™.

Since this is more-or-less a WONTFIX from Lift's perspective, I'm closing off this ticket. Feel free to reach out on the Mailing List if you have any questions.

[andreak]

FWIW I totally disagree with this decision. I think the recommendation not to use Enums is very unprofessional. Enums are part Scala and it just seems weird not to support them. Stating that Scala's reflection-api is experimental and use that as an excuse not to use it seems weird to from a user's perspective as many production frameworks use it a lot.
I don't have resources to fix this my self so I know I'm not in a position to complain too much but these are my thoughts anyway.

[Shadowfiend]

I think “unprofessional” may have a different meaning for you and me. I have found most people I've talked to consider enums in Scala to be broken for an assortment of reasons. Supporting a broken part of the language is wasted effort, and suggesting an alternative that provides many of the same advantages with relatively few disadvantages is exactly the “professional” thing to do.

Now, you can argue that it isn't a WONTFIX because at some point we will move to the reflection API—that's fair. But we don't have the resources of some other frameworks, and relying on something marked experimental right now is just inviting maintenance pain. The Scala folks have a track record of maintaining bad compatibility even with supposedly stable parts of the language; if they mark it experimental, all bets are off. You may argue that they've done a better job recently—that's also fair, but they've still got a long time before I for one will be willing to trust my spare time to them. If working on Lift was something I did full-time, supporting an experimental API wouldn't be as big a deal. As it is, optimizing my effort means I want to minimize maintenance pain, and the reflection API's experimental status means I'm not willing to try supporting it.

If a committer steps forward who's willing to maintain lift-json based on the reflection API as it changes from version to version, and that fixes the Enum issues, rockin'.

All of that said, the long 2.12 release cycle does have me reconsidering my stance on the matter for the purposes of Lift 3.

[farmdawgnation]

Now, you can argue that it isn't a WONTFIX because at some point we will move to the reflection API—that's fair.

I do stand by considering this a WONTFIX because it's filed as a defect and is specifically related to Enums. The move to the new reflection API will be an enhancement and be far more wide-reaching across multiple modules when it happens. In the interest of keeping the task list on the GitHub project full of actionable items for the committers I think closing this is the right move.

That said, I'm with Antonio. If a committer wants to implement and maintain json based on the new API I'm all for it.

[fbettag]

Hey there, this can be solved with existing tools like mentioned in this blogpost here.
By having a single Enum to match this against, this apparently works.

/*
*  Serializer and Deserializers for Enumeration
*/
class EnumerationSerializer(enumList: List[Enumeration]) extends net.liftweb.json.Serializer[Enumeration#Value] {
  import JsonDSL._
  val EnumerationClass = classOf[Enumeration#Value]
  val formats = Serialization.formats(NoTypeHints)

 //  Deserializer Function for Enumeration
  def deserialize(implicit format: Formats): PartialFunction[(TypeInfo, JValue), Enumeration#Value] = { 
    case (TypeInfo(EnumerationClass, _), json) => json match {
      case JObject(List(JField(name, JString(value)))) => fetchEnumValue(enumList, value)  // Here we can perform the
      case JString(value) => fetchEnumValue(enumList, value)                               // desired function
      case value => throw new MappingException("Can't convert " +
        value + " to " + EnumerationClass)
    }
  }

  def serialize(implicit format: Formats): PartialFunction[Any, JValue] = {
    case i: Enumeration#Value => i.toString
  }

  private def fetchEnumValue(enumList: List[Enumeration], value: String): Enumeration#Value = {
    var defaultEnumValue: Enumeration#Value = Year.withName("FinalYear")
    for (enumItem <- enumList) {
      for (enumValue <- enumItem.values) {
        enumValue.toString == value match {
          case true => {
            defaultEnumValue = enumItem.withName(value)
          }
          case _ => None
        }
      }
    }
    defaultEnumValue
  }

}

[Shadowfiend]

Yes, I believe that's very similar to the solution holograph posted above, with the same caveat that you need to make sure your enums don't share any same-named values.

[andreak]

Are we really satisfied with this?
I don't think this is satisfactory at all. We use lots of Enumerations, like this:

trait ValueWithDescription[T]  {
    def description: String
    def name: String
    def wrapped: T
}

abstract class EnumWithDescriptionAndObject[T] extends Enumeration {

    abstract class ExtendedValue(id: Int) extends Val(id) with ValueWithDescription[T]

    def Value(inDescription: String, inWrapped: T) = new ExtendedValue(nextId) {
        def description = inDescription
        def name = toString()
        def wrapped = inWrapped
    }

    def Value(inWrapped: T): ExtendedValue = Value("", inWrapped)

    def getValues = {
        super.values.map(v => v.asInstanceOf[ExtendedValue]).asInstanceOf[Set[ExtendedValue]].toSeq
    }

    def valueOf(name: String) = try{Some(withName(name).asInstanceOf[ExtendedValue])} catch {case _: Throwable => None}

    def unapply(value: String) = getValues.find(_.toString == value)

}

object InvoiceStatus extends EnumWithDescriptionAndObject[(String, Int)] {
    val DRAFT = Value("Draft" -> 1)
    val IN_PROGRESS = Value("In progress" -> 2)
}

With the same-name restriction we won't get to serialize another Enum with value DRAFT, which we have several others of.

Enums we use must hold the following properties:

• Must be able to contain "extra info", hence the [T]. This way we can create Enums holding arbitrary info (like i18n strings etc).
• Must be able to query the values for an Enum
• Must be able to create one from its String-representation

Other well established, Scala based, JSON frameworks use macros and/or reflection now, maybe it's time to reconsider?

[Shadowfiend]

I actually think we've been vindicated in our decision—scala.reflect will never leave experimental mode as I've understood it, as it's meant to be superseded by scala.meta. When 2.12 comes out it may be worth looking at building on top of scala.meta, since it seems that there are longer-term plans for its support—though time will tell.

We largely stopped using Scala Enumerations altogether in work projects a long while ago, opting instead to deal with our own as Farmer outlined above (singleton objects that extend sealed traits, with accompanying serializer). We haven't felt much if any pain from it.

That said, if you have a good serializer for the example you gave above, sharing it would be cool. I'm not morally opposed to improving the lift-json enum serializer with the implementation Franz and holograph posted, mind you, given that it's “less limited”, though still limited.

[andreak]

Scala.meta is (will be) nice, but we need solutions which work today, with today's tools, ie. scala-2.11.
I don't understand how people working with Enums don't have the 3 requirements we have.

[farmdawgnation]

What requirements are not met by the sealed trait / case object pattern above?

[farmdawgnation]

Also we're probably at the point where this discussion should be happening on the ML.

[andreak]

1. How do you hold "extra info" like a [T]?
2. Create an enum-instance from its String-representation
3. Query all values of an Enum

With my abstract EnumWithDescriptionAndObject creating an Enum by extending it is easy:

object InvoiceStatus extends EnumWithDescriptionAndObject[(String, Int)] {
    val DRAFT = Value("Draft" -> 1)
    val IN_PROGRESS = Value("In progress" -> 2)
}

It meats the requirements:

1. Hold "extra info"
   The InvoiceStatus enum holds "extra info" of type (String, Int), which is passed to Value()
2. Query all values of an Enum
   InvoiceStatus.getValues
3. Create an instance form its String-representation:
   InvoiceStatus.valueOf("DRAFT")

How do you do this with the sealed trait approach?

[farmdawgnation]

Ah, so I see what you mean by extra info now.

So, in all honesty you can use case classes with sealed traits as well. Those could hold more information in places where you need it. Likewise, creating a case object from it's string like representation is what the SingletonSerializer does. W.r.t. querying all values, you would need to maintain an all collection on your own, which is a little bit of boilerplate but doable. Might also be some options with Java reflection to accomplish that if you must.

[andreak]

I'm sure its doable, but when you have many Enums with those requirements then constructing them by extending EnumWithDescriptionAndObject looks much easier then having lots of traits/case classes.

[farmdawgnation]

I think we've reached a difference in opinion, or perhaps one in design. We've been using the trait/case object pattern for ~ 2 years now and never run into a situation where it looked difficult enough to be annoying. There's some boilerplate, but we avoided the technical debt involved in trying to figure out custom Enum support based on a library that (it looks like) will be thrown away.

There could be some discrete requirements that you have which we don't that makes that difficult for you. YMMV. But I think the right answer from the POV of the framework is to wait on meta.

Just my $0.02.

[andreak]

We're using JPA and writing converters for our kind of Enums is very easy, the converter looks like this:

// The abstract converter for our Enums
abstract class EnumConverter[T, D](et: Enumeration) extends AttributeConverter[T, String] {
    def convertToDatabaseColumn(attribute: T): String = {
        if (attribute == null) {
            null
        } else {
            attribute.toString
        }
    }

    def convertToEntityAttribute(dbData: String): T = {
        if (dbData == null) {
            null.asInstanceOf[T]
        } else {
            et.withName(dbData).asInstanceOf[T]
        }
    }

}

@Converter
class InvoiceStatusConverter extends EnumConverter[(String, Int), String](InvoiceStatus)

That's one line for the developer for each Enum-type. I'm not sure the converters are so easy to write using the sealed trait approach.

I think its strange that there are so many objections to provide a good solution for this serialization-problem for Scala-2.11, given that's the version 3.0 is aiming for. Isn't waiting on meta the same as saying we're aiming for 2.12? That certainly isn't the case.

Why is using Scala-reflection any worse then today's solution using java reflection and scalap/paranamer? If the argument was lack of resources to implement this I'd be OK with it but that doesn't seem to be the case reading this thread.

[farmdawgnation]

Let's move this to the ML, where everyone can see and participate instead of having the conversation in the silo of a GitHub issue, if we want to continue the discussion.

[Shadowfiend]

The existing code is already written and battle-tested. If a committer wants to write new code based on scala-reflect and commit to maintaining it until scala-meta, that's fine. I'm not, and Joni seems largely occupied with other things, so it doesn't look like he is either. This isn't the first time we've had this discussion, and the only new information that's come to light since then is the certainty that scala-reflect is going to disappear from under us, which isn't exactly a vote in its favor.

Agreed that continuing on the ML is the right way to go if we want to continue.

[andreak]

The lack of maintainer for lift-json is, IMO, a serious concern. Even if the API is stable, if it's not meeting the needs of developers developing modern apps then other tools are needed, and developers are going to look for them. I'll try to write some "not so negative" mail on the ML and move the discussion there.