arangodb / java-velocypack-module-scala

Apache License 2.0
6 stars 3 forks source link

Option deserialization does not operate according to expectations #2

Closed sheinbergon closed 7 years ago

sheinbergon commented 7 years ago

According to VPackScalaDeserializers :

  val OPTION = new VPackDeserializerParameterizedType[Option[Any]] {
    ...
    ...
    ...
    def deserialize(parent: VPackSlice, vpack: VPackSlice, context: VPackDeserializationContext, t: ParameterizedType): Option[Any] = {
      val value = context.deserialize(vpack, t.getActualTypeArguments()(0).asInstanceOf[Class[Any]])
      value match {
        case null => None
        case _    => Some(value)
      }
    }
  }

However, it seems null are ignored during deserialization, so this method never executes, resulting in null where None was expected

sheinbergon commented 7 years ago

To make things even worse, per reflected field deserialization does take place, so using None for field default value in Scala also doesn't work

sheinbergon commented 7 years ago

The same behavior is observed when using the Jdk8 Optional Deserialization

mvollmary commented 7 years ago

Your right that this method never should return None because value can't be null, the match is useless.

When I implemented the custom deserializer mechanism my opinion was that you don't want to take care of null values when implementing a VPackDeserializer. To reduce potential exceptions in the users code I decided to only call deserializers on values that aren't null.

When implementing deserializers for Optional and Option I decided to leave this mechanism untouched because I thought the most developer use Optional/Option as same as me and initialize them always in the constructor of my class with None/Option.empty(). The idea behind Optional/Option from my understanding is that they should never be null, so you shouldn't care of missing values or null values in your VelocyPack/Json.

What do you think?

sheinbergon commented 7 years ago

Your understanding of Option/Optional is correct, that is why the current behavior is a bug and not an opinion/approach.

See what I wrote 2 comments above - using Scala default values doesn't work ( and java doesn't even have those ). So... the current behavior breaks the very point of Option/Optional by explicitly setting the field as null in case of missing fields. The correct behavior would be your intended one - deserialize as None/Empty Optional in case of null/non-existent values.

mvollmary commented 7 years ago

Thanks. Now I got it. I added a method to register a deserializer which can handle null values by itself and fixed scala and jdk8 modules.

My plan is to release a new driver version this week.

sheinbergon commented 7 years ago

Bear in mind that these deserializers are incorporated into the very modules you created yourself. So these modules should use your new method when needed (meaning you should apply the fixes there as well )

mvollmary commented 7 years ago

Already done this, thanks :-)

sheinbergon commented 7 years ago

Thank you :)

sheinbergon commented 7 years ago

Was this fixed as part of 4.2.x driver ? If so, could you please release a new version of this jar so we can use it straight out of maven?

mvollmary commented 7 years ago

I released version 1.0.1 right now