From 84865ff90ece9265714c1b5b2f1cec5c39b8b61b Mon Sep 17 00:00:00 2001 From: Antonio Cunei Date: Mon, 18 Mar 2013 14:47:39 +0100 Subject: [PATCH 1/7] Avoid building case classes with nulls If a property is missing, and no default value exists, it should stop and complain rather than using nulls to build a case class instance. --- src/main/scala/jacks/case.scala | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/main/scala/jacks/case.scala b/src/main/scala/jacks/case.scala index 912eabf..6848a1a 100644 --- a/src/main/scala/jacks/case.scala +++ b/src/main/scala/jacks/case.scala @@ -70,7 +70,11 @@ class CaseClassDeserializer(t: JavaType, c: Creator) extends JsonDeserializer[An val params = c.accessors.map { a => values(a.name) match { case Some(v) => v - case None => c.default(a) + case None => { + if (c.hasNoDefault(a)) + throw ctx.mappingException("Required property '"+a.name+"' is missing.") + c.default(a) + } } } @@ -82,6 +86,7 @@ trait Creator { val accessors: Array[Accessor] def apply(args: Seq[AnyRef]): Any def default(a: Accessor): AnyRef + def hasNoDefault(a: Accessor) = a.default == None } class ConstructorCreator(c: Constructor[_], val accessors: Array[Accessor]) extends Creator { From 5d707a0cdb7419b63bf369d8bad903dc66781f4c Mon Sep 17 00:00:00 2001 From: Antonio Cunei Date: Tue, 19 Mar 2013 14:39:29 +0100 Subject: [PATCH 2/7] Added configuration options. --- src/main/scala/jacks/case.scala | 4 ++-- src/main/scala/jacks/jacks.scala | 8 +++++--- src/main/scala/jacks/jacksOptions.scala | 18 ++++++++++++++++++ src/main/scala/jacks/module.scala | 15 ++++++++------- 4 files changed, 33 insertions(+), 12 deletions(-) create mode 100644 src/main/scala/jacks/jacksOptions.scala diff --git a/src/main/scala/jacks/case.scala b/src/main/scala/jacks/case.scala index 6848a1a..ca75e88 100644 --- a/src/main/scala/jacks/case.scala +++ b/src/main/scala/jacks/case.scala @@ -40,7 +40,7 @@ class CaseClassSerializer(t: JavaType, accessors: Array[Accessor]) extends StdSe } } -class CaseClassDeserializer(t: JavaType, c: Creator) extends JsonDeserializer[Any] { +class CaseClassDeserializer(t: JavaType, c: Creator, checkNulls: Boolean) extends JsonDeserializer[Any] { val fields = c.accessors.map(a => a.name -> None).toMap[String, Option[Object]] val types = c.accessors.map(a => a.name -> a.`type`).toMap @@ -71,7 +71,7 @@ class CaseClassDeserializer(t: JavaType, c: Creator) extends JsonDeserializer[An values(a.name) match { case Some(v) => v case None => { - if (c.hasNoDefault(a)) + if (checkNulls && c.hasNoDefault(a)) throw ctx.mappingException("Required property '"+a.name+"' is missing.") c.default(a) } diff --git a/src/main/scala/jacks/jacks.scala b/src/main/scala/jacks/jacks.scala index add6430..113c01e 100644 --- a/src/main/scala/jacks/jacks.scala +++ b/src/main/scala/jacks/jacks.scala @@ -10,9 +10,9 @@ import scala.collection.JavaConversions.JConcurrentMapWrapper import java.io._ import java.util.concurrent.ConcurrentHashMap -trait JacksMapper { +abstract class JacksMapper(options:JacksOptions) { val mapper = new ObjectMapper - mapper.registerModule(new ScalaModule) + mapper.registerModule(new ScalaModule(options)) def readValue[T: Manifest](src: Array[Byte]): T = mapper.readValue(src, resolve) def readValue[T: Manifest](src: InputStream): T = mapper.readValue(src, resolve) @@ -37,4 +37,6 @@ trait JacksMapper { }) } -object JacksMapper extends JacksMapper +object JacksMapper extends JacksMapper(JacksOptions.defaults) { + def withOptions(opts:JacksOption*) = new JacksMapper(JacksOptions(opts:_*)){} +} diff --git a/src/main/scala/jacks/jacksOptions.scala b/src/main/scala/jacks/jacksOptions.scala new file mode 100644 index 0000000..d0a9944 --- /dev/null +++ b/src/main/scala/jacks/jacksOptions.scala @@ -0,0 +1,18 @@ +// Copyright (C) 2011 - Will Glozer. All rights reserved. + +package com.lambdaworks.jacks + +sealed abstract class JacksOption +object JacksOption { + case class CheckCaseClassNulls(enabled:Boolean) extends JacksOption +} + + +private[jacks] class JacksOptions(opts:Seq[JacksOption]=Seq.empty) { + def checkCaseClassNulls=opts contains JacksOption.CheckCaseClassNulls(true) +} +private[jacks] object JacksOptions { + def apply(opts:JacksOption*) = + new JacksOptions(opts.groupBy(_.getClass()).toSeq.map(_._2.last)) + def defaults=new JacksOptions() +} diff --git a/src/main/scala/jacks/module.scala b/src/main/scala/jacks/module.scala index 5bc5ce9..337792a 100644 --- a/src/main/scala/jacks/module.scala +++ b/src/main/scala/jacks/module.scala @@ -21,17 +21,17 @@ import java.lang.reflect.{Constructor, Method} import tools.scalap.scalax.rules.scalasig.ScalaSig -class ScalaModule extends Module { +class ScalaModule(options:JacksOptions) extends Module { def version = new Version(2, 1, 0, null, "com.lambdaworks", "jacks") def getModuleName = "ScalaModule" def setupModule(ctx: Module.SetupContext) { - ctx.addSerializers(new ScalaSerializers) - ctx.addDeserializers(new ScalaDeserializers) + ctx.addSerializers(new ScalaSerializers(options)) + ctx.addDeserializers(new ScalaDeserializers(options)) } } -class ScalaDeserializers extends Deserializers.Base { +class ScalaDeserializers(options:JacksOptions) extends Deserializers.Base { override def findBeanDeserializer(t: JavaType, cfg: DeserializationConfig, bd: BeanDescription): JsonDeserializer[_] = { val cls = t.getRawClass @@ -76,8 +76,9 @@ class ScalaDeserializers extends Deserializers.Base { new TupleDeserializer(t) } else if (classOf[Product].isAssignableFrom(cls)) { ScalaTypeSig(cfg.getTypeFactory, t) match { - case Some(sts) if sts.isCaseClass => new CaseClassDeserializer(t, sts.creator) - case _ => null + case Some(sts) if sts.isCaseClass => + new CaseClassDeserializer(t, sts.creator, options.checkCaseClassNulls) + case _ => null } } else if (classOf[Symbol].isAssignableFrom(cls)) { new SymbolDeserializer @@ -116,7 +117,7 @@ class ScalaDeserializers extends Deserializers.Base { } } -class ScalaSerializers extends Serializers.Base { +class ScalaSerializers(options:JacksOptions) extends Serializers.Base { override def findSerializer(cfg: SerializationConfig, t: JavaType, bd: BeanDescription): JsonSerializer[_] = { val cls = t.getRawClass From f28f4ad2b26cadd3c06f7b083e4716d11a967128 Mon Sep 17 00:00:00 2001 From: Antonio Cunei Date: Thu, 21 Mar 2013 12:36:24 +0100 Subject: [PATCH 3/7] No need for an abstract class. However, the constructor can be made private. --- src/main/scala/jacks/jacks.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/scala/jacks/jacks.scala b/src/main/scala/jacks/jacks.scala index 113c01e..7b5f4c7 100644 --- a/src/main/scala/jacks/jacks.scala +++ b/src/main/scala/jacks/jacks.scala @@ -10,7 +10,7 @@ import scala.collection.JavaConversions.JConcurrentMapWrapper import java.io._ import java.util.concurrent.ConcurrentHashMap -abstract class JacksMapper(options:JacksOptions) { +class JacksMapper private (options:JacksOptions) { val mapper = new ObjectMapper mapper.registerModule(new ScalaModule(options)) @@ -38,5 +38,5 @@ abstract class JacksMapper(options:JacksOptions) { } object JacksMapper extends JacksMapper(JacksOptions.defaults) { - def withOptions(opts:JacksOption*) = new JacksMapper(JacksOptions(opts:_*)){} + def withOptions(opts:JacksOption*) = new JacksMapper(JacksOptions(opts:_*)) } From 165e3aab97215db060e752824ca11dcbc7e817df Mon Sep 17 00:00:00 2001 From: Antonio Cunei Date: Thu, 28 Mar 2013 13:55:24 +0100 Subject: [PATCH 4/7] Rename CheckCaseClassNulls to CaseClassCheckNulls For clarity, as further "CaseClass" options are arriving. --- src/main/scala/jacks/jacksOptions.scala | 4 ++-- src/main/scala/jacks/module.scala | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/scala/jacks/jacksOptions.scala b/src/main/scala/jacks/jacksOptions.scala index d0a9944..8da7acc 100644 --- a/src/main/scala/jacks/jacksOptions.scala +++ b/src/main/scala/jacks/jacksOptions.scala @@ -4,12 +4,12 @@ package com.lambdaworks.jacks sealed abstract class JacksOption object JacksOption { - case class CheckCaseClassNulls(enabled:Boolean) extends JacksOption + case class CaseClassCheckNulls(enabled:Boolean) extends JacksOption } private[jacks] class JacksOptions(opts:Seq[JacksOption]=Seq.empty) { - def checkCaseClassNulls=opts contains JacksOption.CheckCaseClassNulls(true) + def caseClassCheckNulls=opts contains JacksOption.CaseClassCheckNulls(true) } private[jacks] object JacksOptions { def apply(opts:JacksOption*) = diff --git a/src/main/scala/jacks/module.scala b/src/main/scala/jacks/module.scala index 337792a..6e871af 100644 --- a/src/main/scala/jacks/module.scala +++ b/src/main/scala/jacks/module.scala @@ -77,7 +77,7 @@ class ScalaDeserializers(options:JacksOptions) extends Deserializers.Base { } else if (classOf[Product].isAssignableFrom(cls)) { ScalaTypeSig(cfg.getTypeFactory, t) match { case Some(sts) if sts.isCaseClass => - new CaseClassDeserializer(t, sts.creator, options.checkCaseClassNulls) + new CaseClassDeserializer(t, sts.creator, options.caseClassCheckNulls) case _ => null } } else if (classOf[Symbol].isAssignableFrom(cls)) { From 0448e2d17323f7049c036185f3f0bafb017ce885 Mon Sep 17 00:00:00 2001 From: Antonio Cunei Date: Thu, 28 Mar 2013 15:51:02 +0100 Subject: [PATCH 5/7] Deserialize missing Option case class fields as None The version 2.1.4 of Jacks deserializes a missing property corresponding to an Option case class field as "null", rather than None. This leads to situations like: Start with "" Deserialize -> Z(null) Serialize -> """{"s":null}""" Deserialize -> Z(None) Which may be undesirable. When the option CaseClassCheckNulls is in use, nulls should never be generated; therefore, deserialize missing properties that correspond to Option fields as None instead, unless an explicit default is specified. --- src/main/scala/jacks/case.scala | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/src/main/scala/jacks/case.scala b/src/main/scala/jacks/case.scala index ca75e88..41ffcbe 100644 --- a/src/main/scala/jacks/case.scala +++ b/src/main/scala/jacks/case.scala @@ -71,9 +71,26 @@ class CaseClassDeserializer(t: JavaType, c: Creator, checkNulls: Boolean) extend values(a.name) match { case Some(v) => v case None => { - if (checkNulls && c.hasNoDefault(a)) - throw ctx.mappingException("Required property '"+a.name+"' is missing.") - c.default(a) + if (checkNulls) { + // refuse to store nulls into case class fields, unless + // explicitly requested with a default value + // In case of Option, a missing property becomes None + + if (c.hasNoDefault(a)) { + val d = ctx.findContextualValueDeserializer(a.`type`, null) + val e = d.getNullValue + if (e != null) e else + throw ctx.mappingException("Required property '"+a.name+"' is missing.") + } else { + // c hasDefault(a), hence return that + c.default(a) + } + } else { + // pre-existing 2.1.4 behavior will use c.default(). + // default() should use d.getNullValue, but instead + // always returns null (even for Option) + c.default(a) + } } } } From 11ecda93ac89e921e6367cdc950a45e9e68b227d Mon Sep 17 00:00:00 2001 From: Antonio Cunei Date: Thu, 28 Mar 2013 14:20:59 +0100 Subject: [PATCH 6/7] Adding option "CaseClassSkipNulls" Usually the annotation @JsonInclude(Include.NON_NULL) will cause null fields not to be emitted. However, in the case of serialization of an Option[] field containing None, the standard JacksMapper will still write "null": since the field itself is not null, even when the flag is used None is serialized as null, regardless. This behavior cannot be changed because of compatibility reasons. Hence, this option "CaseClassSkipNulls" is introduced, with behavior complementary to CaseClassCheckNulls. When this option is selected, each Option field that contains None will be skipped during serialization, rather than being serialized as null. This option does not override "ALWAYS" or other Jackson flags explicitly set on classes or individual fields: if you do, the flags will still be honored. --- src/main/scala/jacks/case.scala | 27 ++++++++++++++++++------- src/main/scala/jacks/jacksOptions.scala | 2 ++ src/main/scala/jacks/module.scala | 11 +++++----- 3 files changed, 28 insertions(+), 12 deletions(-) diff --git a/src/main/scala/jacks/case.scala b/src/main/scala/jacks/case.scala index 41ffcbe..3fdc29b 100644 --- a/src/main/scala/jacks/case.scala +++ b/src/main/scala/jacks/case.scala @@ -10,7 +10,7 @@ import com.fasterxml.jackson.databind.ser.std.StdSerializer import java.lang.reflect.{Constructor, Method} -class CaseClassSerializer(t: JavaType, accessors: Array[Accessor]) extends StdSerializer[Product](t) { +class CaseClassSerializer(t: JavaType, accessors: Array[Accessor], skipNulls: Boolean) extends StdSerializer[Product](t) { override def serialize(value: Product, g: JsonGenerator, p: SerializerProvider) { g.writeStartObject() @@ -28,16 +28,29 @@ class CaseClassSerializer(t: JavaType, accessors: Array[Accessor]) extends StdSe } @inline final def include(a: Accessor, s: JsonSerializer[AnyRef], v: AnyRef): Boolean = a.include match { - case ALWAYS => true - case NON_DEFAULT => default(a) != v - case NON_EMPTY => !s.isEmpty(v) - case NON_NULL => v != null + case Some(ALWAYS) => true + case Some(NON_DEFAULT) => default(a) != v + case Some(NON_EMPTY) => !s.isEmpty(v) + case Some(NON_NULL) => v != null + case None => { + // unfortunately, while Jackson serializers have an isEmpty() to match the getEmptyValue() of deserializers, + // they lack an isNull() to match the getNullValue() of deserializers. + // We cannot use isEmpty, as otherwise empty iterables would also match. Therefore, the only + // viable solution here is to match Option's None explicitly, unfortunately. + // + // however, null or None should /not/ be skipped it there is an explicit default. It is + // tempting to skip if the value matches the default in any case, but that goes a bit beyond + // what skipNull is for (which is to avoid unrequest nulls to appear in the serialization) + !skipNulls || !hasNoDefault(a) || (v != null && v != None) + } } @inline final def default(a: Accessor) = a.default match { case Some(m) => m.invoke(null) case None => null } + + @inline final def hasNoDefault(a: Accessor) = a.default == None } class CaseClassDeserializer(t: JavaType, c: Creator, checkNulls: Boolean) extends JsonDeserializer[Any] { @@ -82,11 +95,11 @@ class CaseClassDeserializer(t: JavaType, c: Creator, checkNulls: Boolean) extend if (e != null) e else throw ctx.mappingException("Required property '"+a.name+"' is missing.") } else { - // c hasDefault(a), hence return that + // c hasDefault(a), hence return the default c.default(a) } } else { - // pre-existing 2.1.4 behavior will use c.default(). + // Jacks 2.1.4 behavior will use c.default(). // default() should use d.getNullValue, but instead // always returns null (even for Option) c.default(a) diff --git a/src/main/scala/jacks/jacksOptions.scala b/src/main/scala/jacks/jacksOptions.scala index 8da7acc..f390b1c 100644 --- a/src/main/scala/jacks/jacksOptions.scala +++ b/src/main/scala/jacks/jacksOptions.scala @@ -5,11 +5,13 @@ package com.lambdaworks.jacks sealed abstract class JacksOption object JacksOption { case class CaseClassCheckNulls(enabled:Boolean) extends JacksOption + case class CaseClassSkipNulls(enabled:Boolean) extends JacksOption } private[jacks] class JacksOptions(opts:Seq[JacksOption]=Seq.empty) { def caseClassCheckNulls=opts contains JacksOption.CaseClassCheckNulls(true) + def caseClassSkipNulls =opts contains JacksOption.CaseClassSkipNulls(true) } private[jacks] object JacksOptions { def apply(opts:JacksOption*) = diff --git a/src/main/scala/jacks/module.scala b/src/main/scala/jacks/module.scala index 6e871af..9e7e184 100644 --- a/src/main/scala/jacks/module.scala +++ b/src/main/scala/jacks/module.scala @@ -133,8 +133,9 @@ class ScalaSerializers(options:JacksOptions) extends Serializers.Base { new TupleSerializer(t) } else if (classOf[Product].isAssignableFrom(cls)) { ScalaTypeSig(cfg.getTypeFactory, t) match { - case Some(sts) if sts.isCaseClass => new CaseClassSerializer(t, sts.annotatedAccessors) - case _ => null + case Some(sts) if sts.isCaseClass => + new CaseClassSerializer(t, sts.annotatedAccessors, options.caseClassSkipNulls) + case _ => null } } else if (classOf[Symbol].isAssignableFrom(cls)) { new SymbolSerializer(t) @@ -151,7 +152,7 @@ case class Accessor( `type`: JavaType, default: Option[Method], ignored: Boolean = false, - include: Include = ALWAYS + include: Option[Include] = None ) class ScalaTypeSig(val tf: TypeFactory, val `type`: JavaType, val sig: ScalaSig) { @@ -200,7 +201,7 @@ class ScalaTypeSig(val tf: TypeFactory, val `type`: JavaType, val sig: ScalaSig) annotations.foldLeft(accessor) { case (accessor, a:JsonProperty) if a.value != "" => accessor.copy(name = a.value) case (accessor, a:JsonIgnore) => accessor.copy(ignored = a.value) - case (accessor, a:JsonInclude) => accessor.copy(include = a.value) + case (accessor, a:JsonInclude) => accessor.copy(include = Some(a.value)) case (accessor, _) => accessor } }.toArray @@ -212,7 +213,7 @@ class ScalaTypeSig(val tf: TypeFactory, val `type`: JavaType, val sig: ScalaSig) val ignored = ignore.value.toSet accessors.map(a => a.copy(ignored = ignored.contains(a.name))) case (accessors, include: JsonInclude) => - accessors.map(a => a.copy(include = include.value)) + accessors.map(a => a.copy(include = Some(include.value))) case (accessors, _) => accessors } From a80e089717e009f9baf3dfe70b640059bd81de82 Mon Sep 17 00:00:00 2001 From: Antonio Cunei Date: Fri, 29 Mar 2013 15:12:41 +0100 Subject: [PATCH 7/7] Better handling of null and None Do skip null and None even when a default exists, in case the default happens to be null or None --- src/main/scala/jacks/case.scala | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/scala/jacks/case.scala b/src/main/scala/jacks/case.scala index 3fdc29b..5b2083a 100644 --- a/src/main/scala/jacks/case.scala +++ b/src/main/scala/jacks/case.scala @@ -38,10 +38,12 @@ class CaseClassSerializer(t: JavaType, accessors: Array[Accessor], skipNulls: Bo // We cannot use isEmpty, as otherwise empty iterables would also match. Therefore, the only // viable solution here is to match Option's None explicitly, unfortunately. // - // however, null or None should /not/ be skipped it there is an explicit default. It is - // tempting to skip if the value matches the default in any case, but that goes a bit beyond - // what skipNull is for (which is to avoid unrequest nulls to appear in the serialization) - !skipNulls || !hasNoDefault(a) || (v != null && v != None) + // null or None should /not/ be skipped it there is an explicit default, unless the default + // happens to be null or None. But do not skip other values just because they match the default. + !skipNulls || (v != null && v != None) || (!hasNoDefault(a) && { + val df=default(a) + (df != null && df != None) + }) } }