Fix #1351

Author: cowtowncoder

release-notes/CREDITS

@@ -530,6 +530,10 @@ Andrew Snare (asnare@github)
   * Reported #1315: Binding numeric values can BigDecimal lose precision
    (2.8.2)
 
+Gili Tzabari (cowwoc@github)
+  * Reported #1351: `@JsonInclude(NON_DEFAULT)` doesn't omit null fields
+   (2.8.3)
+
 Connor Kuhn (ckuhn@github)
   * Contributed #1341: FAIL_ON_MISSING_EXTERNAL_TYPE_ID_PROPERTY
    (2.9.0)

release-notes/VERSION

@@ -11,6 +11,8 @@ Project: jackson-databind
 
 2.8.3 (not yet released)
 
+#1351: `@JsonInclude(NON_DEFAULT)` doesn't omit null fields
+ (reported by Gili T)
 #1353: Improve error-handling for `java.net.URL` deserialization
 #1361: Change `TokenBuffer` to use new `writeEmbeddedObject()` if possible
 

src/main/java/com/fasterxml/jackson/databind/BeanDescription.java

@@ -195,6 +195,12 @@ protected BeanDescription(JavaType type) {
      */
 
     /**
+     * Method for finding annotation-indicated inclusion definition (if any);
+     * possibly overriding given default value.
+     *<p>
+     * NOTE: does NOT use global inclusion default settings as the base, unless
+     * passed as `defValue`.
+     *
      * @since 2.7
      */
     public abstract JsonInclude.Value findPropertyInclusion(JsonInclude.Value defValue);

src/main/java/com/fasterxml/jackson/databind/introspect/BasicBeanDescription.java

@@ -391,8 +391,7 @@ public JsonInclude.Value findPropertyInclusion(JsonInclude.Value defValue) {
         if (_annotationIntrospector != null) {
             JsonInclude.Value incl = _annotationIntrospector.findPropertyInclusion(_classInfo);
             if (incl != null) {
-                return (defValue == null) ? incl
-                        : defValue.withOverrides(incl);
+                return (defValue == null) ? incl : defValue.withOverrides(incl);
             }
         }
         return defValue;

src/main/java/com/fasterxml/jackson/databind/ser/PropertyBuilder.java

@@ -16,17 +16,10 @@ public class PropertyBuilder
 {
     // @since 2.7
     private final static Object NO_DEFAULT_MARKER = Boolean.FALSE;
-    
+
     final protected SerializationConfig _config;
     final protected BeanDescription _beanDesc;
 
-    /**
-     * Default inclusion mode for properties of the POJO for which
-     * properties are collected; possibly overridden on
-     * per-property basis.
-     */
-    final protected JsonInclude.Value _defaultInclusion;
-
     final protected AnnotationIntrospector _annotationIntrospector;
 
     /**
@@ -40,14 +33,41 @@ public class PropertyBuilder
      */
     protected Object _defaultBean;
 
+    /**
+     * Default inclusion mode for properties of the POJO for which
+     * properties are collected; possibly overridden on
+     * per-property basis. Combines global inclusion defaults and
+     * per-type (annotation and type-override) inclusion overrides.
+     */
+    final protected JsonInclude.Value _defaultInclusion;
+
+    /**
+     * Marker flag used to indicate that "real" default values are to be used
+     * for properties, as per per-type value inclusion of type <code>NON_DEFAULT</code>
+     *
+     * @since 2.8
+     */
+    final protected boolean _useRealPropertyDefaults;
+    
     public PropertyBuilder(SerializationConfig config, BeanDescription beanDesc)
     {
         _config = config;
         _beanDesc = beanDesc;
-        // NOTE: this includes global defaults and defaults of POJO that contains property,
-        // but not defaults for types of properties referenced
-        _defaultInclusion = beanDesc.findPropertyInclusion(
-                config.getDefaultPropertyInclusion(beanDesc.getBeanClass()));
+        // 08-Sep-2016, tatu: This gets tricky, with 3 levels of definitions:
+        //  (a) global default inclusion
+        //  (b) per-type default inclusion (from annotation or config overrides;
+        //     latter having precedence
+        //  Cc) per-property override
+        //
+        //  and not only requiring merging, but also considering special handling
+        //  for NON_DEFAULT in case of (b) (vs (a) or (c))
+        JsonInclude.Value inclPerType = JsonInclude.Value.merge(
+                beanDesc.findPropertyInclusion(JsonInclude.Value.empty()),
+                config.getDefaultPropertyInclusion(beanDesc.getBeanClass(),
+                        JsonInclude.Value.empty()));
+        _defaultInclusion = JsonInclude.Value.merge(config.getDefaultPropertyInclusion(),
+                inclPerType);
+        _useRealPropertyDefaults = inclPerType.getValueInclusion() == JsonInclude.Include.NON_DEFAULT;
         _annotationIntrospector = _config.getAnnotationIntrospector();
     }
 
@@ -123,15 +143,16 @@ protected BeanPropertyWriter buildWriter(SerializerProvider prov,
             //    so that if enclosing class has this, we may need to access values of property,
             //    whereas for global defaults OR per-property overrides, we have more
             //    static definition. Sigh.
-            // First: case of class specifying it; try to find POJO property defaults
-            if (_defaultInclusion.getValueInclusion() == JsonInclude.Include.NON_DEFAULT) {
+            // First: case of class/type specifying it; try to find POJO property defaults
+            if (_useRealPropertyDefaults) {
                 // 07-Sep-2016, tatu: may also need to front-load access forcing now
                 if (prov.isEnabled(MapperFeature.CAN_OVERRIDE_ACCESS_MODIFIERS)) {
                     am.fixAccess(_config.isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS));
                 }
                 valueToSuppress = getPropertyDefaultValue(propDef.getName(), am, actualType);
             } else {
                 valueToSuppress = getDefaultValue(actualType);
+                suppressNulls = true;
             }
             if (valueToSuppress == null) {
                 suppressNulls = true;
@@ -140,7 +161,6 @@ protected BeanPropertyWriter buildWriter(SerializerProvider prov,
                     valueToSuppress = ArrayBuilders.getArrayComparator(valueToSuppress);
                 }
             }
-
             break;
         case NON_ABSENT: // new with 2.6, to support Guava/JDK8 Optionals
             // always suppress nulls

src/test/java/com/fasterxml/jackson/databind/filter/JsonIncludeTest.java

@@ -261,4 +261,23 @@ public void testPropConfigOverridesForInclude() throws IOException
         assertEquals(aposToQuotes("{'map':{}}"),
                 mapper.writeValueAsString(empty));
     }
+
+    static class Issue1351Bean
+    {
+        public final String first;
+        public final double second;
+
+        public Issue1351Bean(String first, double second) {
+            this.first = first;
+            this.second = second;
+        }
+    }
+    
+    public void testIssue1351() throws Exception
+    {
+        ObjectMapper mapper = new ObjectMapper();
+        mapper.setSerializationInclusion(JsonInclude.Include.NON_DEFAULT);
+        assertEquals(aposToQuotes("{}"),
+                mapper.writeValueAsString(new Issue1351Bean(null, (double) 0)));
+    }
 }