Blackbird: avoid retrying failed bean setters

blackbird/src/main/java/com/fasterxml/jackson/module/blackbird/deser/OptimizedSettableBeanProperty.java

@@ -2,8 +2,6 @@
 
 import java.io.IOException;
 import java.lang.reflect.InvocationTargetException;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 
 import com.fasterxml.jackson.core.*;
 
@@ -26,13 +24,6 @@ abstract class OptimizedSettableBeanProperty<T extends OptimizedSettableBeanProp
      */
     final protected boolean _skipNulls;
 
-    /**
-     * Marker that we set once we have logged an access problem with the generated
-     * mutator, so it's only logged once; the mutator itself is still used, falling
-     * back to the standard one on each such problem.
-     */
-    private volatile boolean loggedAccessProblem = false;
-
     /**
      * Type the generated builder setter casts the instance to: the setter's declaring
      * class, or the builder's own class for an inherited protected setter. A builder
@@ -41,8 +32,8 @@ abstract class OptimizedSettableBeanProperty<T extends OptimizedSettableBeanProp
     protected final Class<?> _receiverType;
 
     /**
-     * Whether this is a property of a builder, whose setter failures are reported
-     * by {@link #_reportProblemAndReturn} from every path, {@link #set} included.
+     * Whether this is a property of a builder; if so, {@link #set} passes instances
+     * the generated setter can't accept (see {@link #_receiverType}) to the delegate.
      */
     protected final boolean _forBuilder;
 
@@ -116,71 +107,24 @@ public abstract Object deserializeSetAndReturn(JsonParser jp,
      */
 
     /**
-     * Helper method called when an exception is throw from mutator, to figure
-     * out what to do. The parser is only used for a builder's error location, and
-     * is null when called from {@link #set}.
-     *
-     * @since 2.9
-     */
-    protected void _reportProblem(JsonParser p, Object bean, Object value, Throwable e)
-        throws IOException
-    {
-        // databind calls `set()` on builders too (for properties buffered before a
-        // property-based creator, or external type ids), which must behave like
-        // `setAndReturn`, regardless of the order of properties in the JSON
-        if (_forBuilder) {
-            _reportProblemAndReturn(p, e);
-            return;
-        }
-        if ((e instanceof IllegalAccessError)
-                || (e instanceof SecurityException)) {
-            _logAccessProblem(bean, e);
-            delegate.set(bean, value);
-            return;
-        }
-        if (e instanceof IOException) {
-            throw (IOException) e;
-        }
-        if (e instanceof Error) {
-            throw (Error) e;
-        }
-        if (e instanceof RuntimeException) {
-            throw (RuntimeException) e;
-        }
-        // how could this ever happen?
-        throw new RuntimeException(e);
-    }
-
-    /**
-     * Like {@link #_reportProblem}, for builders: anything the generated
-     * setter throws is reported the way {@code MethodProperty} reports it, and never
-     * retried. Telling a failure of the generated accessor itself from one thrown by
-     * the setter would mean guessing from the stack trace, and a retry could run the
-     * setter twice; setup instead leaves the setters it can't call to reflection.
+     * Helper method called when an exception is thrown from mutator, to be thrown
+     * by the caller (which keeps the compiler aware it never returns): anything the
+     * generated setter throws is reported the way {@code MethodProperty} reports it,
+     * and never retried. Telling a failure of the generated accessor itself from one
+     * thrown by the setter would mean guessing from the stack trace, and a retry could
+     * run the setter twice; setup instead leaves the setters it can't call to reflection.
      * The parser is only used for the error location, and is null outside of
      * {@link #deserializeSetAndReturn} and {@link #deserializeAndSet}, as it is
      * for {@code MethodProperty}.
+     *
+     * @since 2.9
      */
-    protected Object _reportProblemAndReturn(JsonParser p, Throwable e)
+    protected IOException _reportProblem(JsonParser p, Throwable e)
         throws IOException
     {
         // `Method.invoke` wraps whatever the setter throws, and `MethodProperty`
         // translates that wrapper; doing the same keeps type, message, cause and path
-        throw _throwAsIOE(p, new InvocationTargetException(e));
-    }
-
-    private void _logAccessProblem(Object bean, Throwable e) {
-        synchronized (this) {
-            // yes, double-locking, so not guaranteed; but all we want here is to reduce likelihood
-            // of multiple logging of same underlying problem. Not guaranteed, just improved.
-            if (!loggedAccessProblem) {
-                loggedAccessProblem = true;
-                String msg = String.format("Blackbird deserialization for %s (field %s; mutator %s) falling back to reflection, due to access error (type %s, message=%s); further such errors are not logged%n",
-                        bean.getClass(), _propName, getClass().getName(),
-                        e.getClass().getName(), e.getMessage());
-                Logger.getLogger(getClass().getName()).log(Level.WARNING, msg, e);
-            }
-        }
+        return _throwAsIOE(p, new InvocationTargetException(e));
     }
 
     /*

blackbird/src/main/java/com/fasterxml/jackson/module/blackbird/deser/SettableBooleanProperty.java

@@ -48,7 +48,7 @@ public void deserializeAndSet(JsonParser p, DeserializationContext ctxt, Object
         try {
             _optimizedSetter.accept(bean, b);
         } catch (Throwable e) {
-            _reportProblem(p, bean, b, e);
+            throw _reportProblem(p, e);
         }
     }
 
@@ -82,7 +82,7 @@ public void set(Object bean, Object value) throws IOException {
         try {
             _optimizedSetter.accept(bean, b);
         } catch (Throwable e) {
-            _reportProblem(null, bean, b, e);
+            throw _reportProblem(null, e);
         }
     }
 
@@ -104,7 +104,7 @@ private Object _setAndReturn(JsonParser p, Object instance, boolean b) throws IO
             Object result = _optimizedSetterAndReturn.apply(instance, b);
             return (result == null) ? instance : result;
         } catch (Throwable e) {
-            return _reportProblemAndReturn(p, e);
+            throw _reportProblem(p, e);
         }
     }
 }

blackbird/src/main/java/com/fasterxml/jackson/module/blackbird/deser/SettableIntProperty.java

@@ -46,7 +46,7 @@ public void deserializeAndSet(JsonParser p, DeserializationContext ctxt,
         try {
             _optimizedSetter.accept(bean, v);
         } catch (Throwable e) {
-            _reportProblem(p, bean, v, e);
+            throw _reportProblem(p, e);
         }
     }
 
@@ -76,7 +76,7 @@ public void set(Object bean, Object value) throws IOException {
         try {
             _optimizedSetter.accept(bean, v);
         } catch (Throwable e) {
-            _reportProblem(null, bean, v, e);
+            throw _reportProblem(null, e);
         }
     }
 
@@ -98,7 +98,7 @@ private Object _setAndReturn(JsonParser p, Object instance, int v) throws IOExce
             Object result = _optimizedSetterAndReturn.apply(instance, v);
             return (result == null) ? instance : result;
         } catch (Throwable e) {
-            return _reportProblemAndReturn(p, e);
+            throw _reportProblem(p, e);
         }
     }
 }

blackbird/src/main/java/com/fasterxml/jackson/module/blackbird/deser/SettableLongProperty.java

@@ -46,7 +46,7 @@ public void deserializeAndSet(JsonParser p, DeserializationContext ctxt,
         try {
            _optimizedSetter.accept(bean, v);
         } catch (Throwable e) {
-            _reportProblem(p, bean, v, e);
+            throw _reportProblem(p, e);
         }
     }
 
@@ -75,7 +75,7 @@ public void set(Object bean, Object value) throws IOException {
         try {
             _optimizedSetter.accept(bean, v);
         } catch (Throwable e) {
-            _reportProblem(null, bean, v, e);
+            throw _reportProblem(null, e);
         }
     }
 
@@ -97,7 +97,7 @@ private Object _setAndReturn(JsonParser p, Object instance, long v) throws IOExc
             Object result = _optimizedSetterAndReturn.apply(instance, v);
             return (result == null) ? instance : result;
         } catch (Throwable e) {
-            return _reportProblemAndReturn(p, e);
+            throw _reportProblem(p, e);
         }
     }
 }

blackbird/src/main/java/com/fasterxml/jackson/module/blackbird/deser/SettableObjectProperty.java

@@ -61,6 +61,8 @@ public void deserializeAndSet(JsonParser p, DeserializationContext ctxt,
         } else {
             value = _valueDeserializer.deserializeWithType(p, ctxt, _valueTypeDeserializer);
         }
+        // Do not call set(): MethodProperty passes nulls returned by polymorphic
+        // deserialization to the setter, even with Nulls.SKIP.
         _set(p, bean, value);
     }
 
@@ -118,7 +120,7 @@ private void _set(JsonParser p, Object bean, Object v) throws IOException {
         try {
             _optimizedSetter.accept(bean, v);
         } catch (Throwable e) {
-            _reportProblem(p, bean, v, e);
+            throw _reportProblem(p, e);
         }
     }
 
@@ -143,7 +145,7 @@ private Object _setAndReturn(JsonParser p, Object instance, Object value) throws
             Object result = _optimizedSetterAndReturn.apply(instance, value);
             return (result == null) ? instance : result;
         } catch (Throwable e) {
-            return _reportProblemAndReturn(p, e);
+            throw _reportProblem(p, e);
         }
     }
 

blackbird/src/main/java/com/fasterxml/jackson/module/blackbird/deser/SettableStringProperty.java

@@ -38,11 +38,14 @@ protected SettableBeanProperty withDelegate(SettableBeanProperty del) {
     @Override
     public void deserializeAndSet(JsonParser p, DeserializationContext ctxt, Object bean) throws IOException
     {
-        if (!p.hasToken(JsonToken.VALUE_STRING)) {
+        String text = p.hasToken(JsonToken.VALUE_STRING) ? p.getText() : null;
+        // some parsers give null text (e.g. for a `TextNode(null)`), which needs the
+        // delegate's null handling
+        if (text == null) {
             delegate.deserializeAndSet(p, ctxt, bean);
             return;
         }
-        _set(p, bean, p.getText());
+        _set(p, bean, text);
     }
 
     @Override
@@ -80,7 +83,7 @@ private void _set(JsonParser p, Object bean, Object value) throws IOException {
         try {
             _optimizedSetter.accept(bean, (String) value);
         } catch (Throwable e) {
-            _reportProblem(p, bean, value, e);
+            throw _reportProblem(p, e);
         }
     }
 
@@ -105,7 +108,7 @@ private Object _setAndReturn(JsonParser p, Object instance, Object value) throws
             Object result = _optimizedSetterAndReturn.apply(instance, value);
             return (result == null) ? instance : result;
         } catch (Throwable e) {
-            return _reportProblemAndReturn(p, e);
+            throw _reportProblem(p, e);
         }
     }
 }

blackbird/src/test/java/com/fasterxml/jackson/module/blackbird/TestAccessFallback.java

@@ -17,36 +17,36 @@ public BogusTestError(String msg) {
         }
     }
 
+    // Getter fails when called through the generated accessor, and works when
+    // called through reflection (the fallback writer)
     static class MyBean
     {
         private String e;
 
+        int optimizedCalls, reflectiveCalls;
+
         public MyBean() { }
 
         MyBean(String e)
         {
-            setE(e);
+            this.e = e;
         }
 
         public void setE(String e)
         {
-            /* 07-Mar-2015, tatu: This is bit tricky, as the exact stack trace varies
-             *   depending on how code is generated. So right now we must get the call
-             *   immediately from constructed class.
-             */
-
-            StackTraceElement[] elems = new Throwable().getStackTrace();
-            StackTraceElement prev = elems[1];
-            if (prev.getClassName().contains("Access4JacksonDeserializer")) {
-                throw new BogusTestError("boom!");
-            }
             this.e = e;
         }
 
         public String getE()
         {
+            // nearest of `Method.invoke` or a Blackbird frame tells which path called
             for (StackTraceElement elem : new Throwable().getStackTrace()) {
-                if (elem.getClassName().contains("Access4JacksonSerializer")) {
+                if (elem.getClassName().equals("java.lang.reflect.Method")) {
+                    ++reflectiveCalls;
+                    return e;
+                }
+                if (elem.getClassName().startsWith("com.fasterxml.jackson.module.blackbird.ser.")) {
+                    ++optimizedCalls;
                     throw new BogusTestError("boom!");
                 }
             }
@@ -60,22 +60,17 @@ public String getE()
     public void testSerializeAccess() throws Exception
     {
         ObjectMapper abMapper = newObjectMapper();
-        assertEquals(BEAN_JSON, abMapper.writeValueAsString(new MyBean("a")));
-
-        // actually try again, to ensure handling works reliably
-        assertEquals(BEAN_JSON, abMapper.writeValueAsString(new MyBean("a")));
-    }
-
-    @Test
-    public void testDeserializeAccess() throws Exception
-    {
-        ObjectMapper abMapper = newObjectMapper();
-        MyBean bean = abMapper.readValue(BEAN_JSON, MyBean.class);
-        assertEquals("a", bean.getE());
-
-        // actually try again, to ensure handling works reliably
-        MyBean bean2 = abMapper.readValue(BEAN_JSON, MyBean.class);
-        assertEquals("a", bean2.getE());
+        MyBean bean = new MyBean("a");
+        assertEquals(BEAN_JSON, abMapper.writeValueAsString(bean));
+        // the generated getter failed, and the value was then written through reflection
+        assertEquals(1, bean.optimizedCalls);
+        assertEquals(1, bean.reflectiveCalls);
+
+        // after that, the writer stays on reflection
+        MyBean bean2 = new MyBean("a");
+        assertEquals(BEAN_JSON, abMapper.writeValueAsString(bean2));
+        assertEquals(0, bean2.optimizedCalls);
+        assertEquals(1, bean2.reflectiveCalls);
     }
 
     @Test