From 2b0669c2ae1bee3a40fda30ee638618d55361f65 Mon Sep 17 00:00:00 2001
From: rob bygrave <robin.bygrave@gmail.com>
Date: Wed, 9 Jan 2019 20:03:26 +1300
Subject: [PATCH] #1594 - Support loading of beans with invalid JSON content

Adjust to be per query via query.setAllowLoadErrors() rather than a
global config via ServerConfig.
---
 src/main/java/io/ebean/Query.java             | 21 +++++++++
 .../java/io/ebean/config/ServerConfig.java    | 31 -------------
 .../java/io/ebeaninternal/api/SpiQuery.java   |  4 +-
 .../server/deploy/DbReadContext.java          |  2 +-
 .../DynamicPropertyAggregationFormula.java    |  3 +-
 .../io/ebeaninternal/server/query/CQuery.java |  5 +--
 .../server/query/SqlBeanLoad.java             |  2 +-
 .../server/querydefn/DefaultOrmQuery.java     | 36 ++++++++++-----
 .../java/org/tests/json/TestDbJson_List.java  | 45 ++++++++++++++-----
 src/test/resources/ebean.properties           |  1 -
 10 files changed, 86 insertions(+), 64 deletions(-)

diff --git a/src/main/java/io/ebean/Query.java b/src/main/java/io/ebean/Query.java
index 8541a7090..bd8d59e43 100644
--- a/src/main/java/io/ebean/Query.java
+++ b/src/main/java/io/ebean/Query.java
@@ -372,6 +372,27 @@ public interface Query<T> {
    */
   Query<T> setAutoTune(boolean autoTune);
 
+  /**
+   * Execute the query allowing properties with invalid JSON to be collected and not fail the query.
+   * <pre>{@code
+   *
+   *   // fetch a bean with JSON content
+   *   EBasicJsonList bean= Ebean.find(EBasicJsonList.class)
+   *       .setId(42)
+   *       .setAllowLoadErrors()  // collect errors into bean state if we have invalid JSON
+   *       .findOne();
+   *
+   *
+   *   // get the invalid JSON errors from the bean state
+   *   Map<String, Exception> errors = server().getBeanState(bean).getLoadErrors();
+   *
+   *   // If this map is not empty tell we have invalid JSON
+   *   // and should try and fix the JSON content or inform the user
+   *
+   * }</pre>
+   */
+  Query<T> setAllowLoadErrors();
+
   /**
    * Set the default lazy loading batch size to use.
    * <p>
diff --git a/src/main/java/io/ebean/config/ServerConfig.java b/src/main/java/io/ebean/config/ServerConfig.java
index b2e29d281..9979057d0 100644
--- a/src/main/java/io/ebean/config/ServerConfig.java
+++ b/src/main/java/io/ebean/config/ServerConfig.java
@@ -30,15 +30,9 @@ import io.ebean.event.readaudit.ReadAuditLogger;
 import io.ebean.event.readaudit.ReadAuditPrepare;
 import io.ebean.meta.MetaInfoManager;
 import io.ebean.migration.MigrationRunner;
-import io.ebean.plugin.LoadErrorHandler;
 import io.ebean.util.StringHelper;
 
-import javax.persistence.PersistenceException;
 import javax.sql.DataSource;
-
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import java.time.Clock;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -527,10 +521,6 @@ public class ServerConfig {
    */
   private boolean idGeneratorAutomatic = true;
 
-  private LoadErrorHandler loadErrorHandler = (bean, prop, fullName, e) -> {
-    throw new PersistenceException("Error loading on " + fullName, e);
-  };
-
   /**
    * Construct a Server Configuration for programmatically creating an EbeanServer.
    */
@@ -3016,13 +3006,6 @@ public class ServerConfig {
 
     String mappingsProp = p.get("mappingLocations", null);
     mappingLocations = getSearchList(mappingsProp, mappingLocations);
-
-    if (!p.getBoolean("failOnLoadError", true)) {
-      Logger logger = LoggerFactory.getLogger("io.ebean.SQL");
-      loadErrorHandler = (bean, prop, fullName, e) -> {
-        logger.error("Error loading on {}", fullName, e);
-      };
-    }
   }
 
   private NamingConvention createNamingConvention(PropertiesWrapper properties, NamingConvention namingConvention) {
@@ -3282,20 +3265,6 @@ public class ServerConfig {
     this.idGeneratorAutomatic = idGeneratorAutomatic;
   }
 
-  /**
-   * Returns the load error handler.
-   */
-  public LoadErrorHandler getLoadErrorHandler() {
-    return loadErrorHandler;
-  }
-
-  /**
-   * Sets the loadErrorHandler.
-   */
-  public void setLoadErrorHandler(LoadErrorHandler loadErrorHandler) {
-    this.loadErrorHandler = loadErrorHandler;
-  }
-
   /**
    * Return true if query plan capture is enabled.
    */
diff --git a/src/main/java/io/ebeaninternal/api/SpiQuery.java b/src/main/java/io/ebeaninternal/api/SpiQuery.java
index fc990f6d0..fe431b268 100644
--- a/src/main/java/io/ebeaninternal/api/SpiQuery.java
+++ b/src/main/java/io/ebeaninternal/api/SpiQuery.java
@@ -8,7 +8,6 @@ import io.ebean.PersistenceContextScope;
 import io.ebean.ProfileLocation;
 import io.ebean.Query;
 import io.ebean.bean.CallStack;
-import io.ebean.bean.EntityBean;
 import io.ebean.bean.ObjectGraphNode;
 import io.ebean.bean.PersistenceContext;
 import io.ebean.event.readaudit.ReadEvent;
@@ -16,7 +15,6 @@ import io.ebean.plugin.BeanType;
 import io.ebeaninternal.server.autotune.ProfilingListener;
 import io.ebeaninternal.server.core.SpiOrmQueryRequest;
 import io.ebeaninternal.server.deploy.BeanDescriptor;
-import io.ebeaninternal.server.deploy.BeanProperty;
 import io.ebeaninternal.server.deploy.BeanPropertyAssocMany;
 import io.ebeaninternal.server.deploy.TableJoin;
 import io.ebeaninternal.server.query.CancelableQuery;
@@ -855,5 +853,5 @@ public interface SpiQuery<T> extends Query<T>, TxnProfileEventCodes {
   /**
    * Handles load errors.
    */
-  void handleLoadError(EntityBean bean, BeanProperty prop, String fullName, Exception e);
+  void handleLoadError(String fullName, Exception e);
 }
diff --git a/src/main/java/io/ebeaninternal/server/deploy/DbReadContext.java b/src/main/java/io/ebeaninternal/server/deploy/DbReadContext.java
index 09e257a93..8f42ad02d 100644
--- a/src/main/java/io/ebeaninternal/server/deploy/DbReadContext.java
+++ b/src/main/java/io/ebeaninternal/server/deploy/DbReadContext.java
@@ -94,6 +94,6 @@ public interface DbReadContext {
   /**
    * Handles a load error on given property.
    */
-  void handleLoadError(EntityBean bean, BeanProperty prop, String fullName, Exception e);
+  void handleLoadError(String fullName, Exception e);
 
 }
diff --git a/src/main/java/io/ebeaninternal/server/deploy/DynamicPropertyAggregationFormula.java b/src/main/java/io/ebeaninternal/server/deploy/DynamicPropertyAggregationFormula.java
index efc45759a..394266489 100644
--- a/src/main/java/io/ebeaninternal/server/deploy/DynamicPropertyAggregationFormula.java
+++ b/src/main/java/io/ebeaninternal/server/deploy/DynamicPropertyAggregationFormula.java
@@ -52,9 +52,8 @@ class DynamicPropertyAggregationFormula extends DynamicPropertyBase {
     Object value;
     try {
       value = scalarType.read(sqlBeanLoad.ctx().getDataReader());
-
     } catch (Exception e) {
-      sqlBeanLoad.ctx().handleLoadError(null, asTarget, fullName, e);
+      sqlBeanLoad.ctx().handleLoadError(fullName, e);
       return;
     }
     if (asTarget != null) {
diff --git a/src/main/java/io/ebeaninternal/server/query/CQuery.java b/src/main/java/io/ebeaninternal/server/query/CQuery.java
index d142e4e1d..6f7645c40 100644
--- a/src/main/java/io/ebeaninternal/server/query/CQuery.java
+++ b/src/main/java/io/ebeaninternal/server/query/CQuery.java
@@ -21,7 +21,6 @@ import io.ebeaninternal.server.core.SpiOrmQueryRequest;
 import io.ebeaninternal.server.deploy.BeanCollectionHelp;
 import io.ebeaninternal.server.deploy.BeanCollectionHelpFactory;
 import io.ebeaninternal.server.deploy.BeanDescriptor;
-import io.ebeaninternal.server.deploy.BeanProperty;
 import io.ebeaninternal.server.deploy.BeanPropertyAssocMany;
 import io.ebeaninternal.server.deploy.DbReadContext;
 import io.ebeaninternal.server.type.DataBind;
@@ -817,8 +816,8 @@ public class CQuery<T> implements DbReadContext, CancelableQuery, SpiProfileTran
   }
 
   @Override
-  public void handleLoadError(EntityBean bean, BeanProperty prop, String fullName, Exception e) {
-    query.handleLoadError(bean, prop, fullName, e);
+  public void handleLoadError(String fullName, Exception e) {
+    query.handleLoadError(fullName, e);
   }
 
   public Set<String> getDependentTables() {
diff --git a/src/main/java/io/ebeaninternal/server/query/SqlBeanLoad.java b/src/main/java/io/ebeaninternal/server/query/SqlBeanLoad.java
index c6557a5e0..56fe4d4cd 100644
--- a/src/main/java/io/ebeaninternal/server/query/SqlBeanLoad.java
+++ b/src/main/java/io/ebeaninternal/server/query/SqlBeanLoad.java
@@ -87,7 +87,7 @@ public class SqlBeanLoad {
 
     } catch (Exception e) {
       bean._ebean_getIntercept().setLoadError(prop.getPropertyIndex(), e);
-      ctx.handleLoadError(bean, prop, prop.getFullBeanName(), e);
+      ctx.handleLoadError(prop.getFullBeanName(), e);
       return prop.getValue(bean);
     }
   }
diff --git a/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java b/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java
index 09d7d6e75..c113ae3ac 100644
--- a/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java
+++ b/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java
@@ -25,7 +25,6 @@ import io.ebean.Transaction;
 import io.ebean.UpdateQuery;
 import io.ebean.Version;
 import io.ebean.bean.CallStack;
-import io.ebean.bean.EntityBean;
 import io.ebean.bean.ObjectGraphNode;
 import io.ebean.bean.ObjectGraphOrigin;
 import io.ebean.bean.PersistenceContext;
@@ -47,7 +46,6 @@ import io.ebeaninternal.api.SpiQuerySecondary;
 import io.ebeaninternal.server.autotune.ProfilingListener;
 import io.ebeaninternal.server.core.SpiOrmQueryRequest;
 import io.ebeaninternal.server.deploy.BeanDescriptor;
-import io.ebeaninternal.server.deploy.BeanProperty;
 import io.ebeaninternal.server.deploy.BeanPropertyAssocMany;
 import io.ebeaninternal.server.deploy.InheritInfo;
 import io.ebeaninternal.server.deploy.TableJoin;
@@ -57,6 +55,7 @@ import io.ebeaninternal.server.query.CancelableQuery;
 import io.ebeaninternal.server.query.NativeSqlQueryPlanKey;
 import io.ebeaninternal.server.rawsql.SpiRawSql;
 
+import javax.persistence.PersistenceException;
 import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -141,6 +140,8 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
 
   private String lazyLoadManyPath;
 
+  private boolean allowLoadErrors;
+
   /**
    * Flag set for report/DTO beans when we may choose to explicitly include the Id property.
    */
@@ -329,8 +330,10 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
   @Override
   public String profileEventId() {
     switch (mode) {
-      case LAZYLOAD_BEAN: return FIND_ONE_LAZY;
-      case LAZYLOAD_MANY: return FIND_MANY_LAZY;
+      case LAZYLOAD_BEAN:
+        return FIND_ONE_LAZY;
+      case LAZYLOAD_MANY:
+        return FIND_MANY_LAZY;
       default:
         return type.profileEventId();
     }
@@ -343,7 +346,7 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
 
   @Override
   public Query<T> setProfileId(int profileId) {
-    this.profileId = (short)profileId;
+    this.profileId = (short) profileId;
     return this;
   }
 
@@ -409,6 +412,12 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
     this.asOfBaseTable = true;
   }
 
+  @Override
+  public DefaultOrmQuery<T> setAllowLoadErrors() {
+    this.allowLoadErrors = true;
+    return this;
+  }
+
   @Override
   public void incrementAsOfTableCount() {
     asOfTableCount++;
@@ -463,7 +472,7 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
 
   @Override
   public DefaultOrmQuery<T> setRawSql(RawSql rawSql) {
-    this.rawSql = (SpiRawSql)rawSql;
+    this.rawSql = (SpiRawSql) rawSql;
     return this;
   }
 
@@ -775,6 +784,7 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
     copy.baseTable = baseTable;
     copy.rootTableAlias = rootTableAlias;
     copy.distinct = distinct;
+    copy.allowLoadErrors = allowLoadErrors;
     copy.timeout = timeout;
     copy.mapKey = mapKey;
     copy.id = id;
@@ -1088,6 +1098,9 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
     if (distinct) {
       sb.append(",dist:");
     }
+    if (allowLoadErrors) {
+      sb.append(",allowLoadErrors:");
+    }
     if (disableLazyLoading) {
       sb.append(",disLazy:");
     }
@@ -1194,6 +1207,7 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
       beanDescriptor.appendOrderById(this);
     }
   }
+
   /**
    * Calculate a hash based on the bind values used in the query.
    * <p>
@@ -1278,7 +1292,7 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
 
   @Override
   public boolean isBeanCacheGet() {
-    return useBeanCache.isGet() && beanDescriptor.isBeanCaching() ;
+    return useBeanCache.isGet() && beanDescriptor.isBeanCaching();
   }
 
   @Override
@@ -1337,7 +1351,7 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
 
   @Override
   public DefaultOrmQuery<T> select(FetchGroup fetchGroup) {
-    this.detail = ((SpiFetchGroup)fetchGroup).detail();
+    this.detail = ((SpiFetchGroup) fetchGroup).detail();
     return this;
   }
 
@@ -1969,8 +1983,10 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
   }
 
   @Override
-  public void handleLoadError(EntityBean bean, BeanProperty prop, String fullName, Exception e) {
-    server.getServerConfig().getLoadErrorHandler().handleLoadError(bean, prop, fullName, e);
+  public void handleLoadError(String fullName, Exception e) {
+    if (!allowLoadErrors) {
+      throw new PersistenceException("Error loading on " + fullName, e);
+    }
   }
 
   @Override
diff --git a/src/test/java/org/tests/json/TestDbJson_List.java b/src/test/java/org/tests/json/TestDbJson_List.java
index 00c7b1820..af4bf86ec 100644
--- a/src/test/java/org/tests/json/TestDbJson_List.java
+++ b/src/test/java/org/tests/json/TestDbJson_List.java
@@ -5,12 +5,12 @@ import io.ebean.Ebean;
 import io.ebean.annotation.ForPlatform;
 import io.ebean.annotation.Platform;
 import io.ebean.text.TextException;
-
-import org.tests.model.json.EBasicJsonList;
-import org.tests.model.json.PlainBean;
 import org.ebeantest.LoggedSqlCollector;
 import org.junit.Test;
+import org.tests.model.json.EBasicJsonList;
+import org.tests.model.json.PlainBean;
 
+import javax.persistence.PersistenceException;
 import java.util.ArrayList;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -18,7 +18,10 @@ import java.util.Map;
 import java.util.Set;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
 public class TestDbJson_List extends BaseTestCase {
 
@@ -173,30 +176,48 @@ public class TestDbJson_List extends BaseTestCase {
 
   @ForPlatform(Platform.H2)
   @Test
-  public void find_corrupt_json() {
-
+  public void find_corrupt_json_using_setAllowLoadErrors() {
 
     EBasicJsonList bean = new EBasicJsonList();
 
     PlainBean plainBean = new PlainBean();
     plainBean.setName("Blubb");
-    bean.getBeanMap().put("bla", plainBean );
+    bean.getBeanMap().put("bla", plainBean);
 
     Ebean.save(bean);
 
+    // set some invalid JSON content into DB
     Ebean.update(EBasicJsonList.class)
-    .set("beanMap", "blabla")
-    .where().eq("id", bean.getId())
-    .update();
+      .set("beanMap", "blabla")
+      .where().eq("id", bean.getId())
+      .update();
 
+    try {
+      // a normal query fails due to invalid JSON content
+      Ebean.find(EBasicJsonList.class)
+        .setId(bean.getId())
+        .findOne();
+
+      // never get here
+      assertTrue(false);
+
+    } catch (PersistenceException e) {
+      // query fails due to error loading invalid JSON content
+      assertThat(e.getMessage()).contains("beanMap");
+    }
+
+    bean = Ebean.find(EBasicJsonList.class)
+      .setId(bean.getId())
+      .setAllowLoadErrors() // allow invalid JSON content
+      .findOne();
 
-    bean = Ebean.find(EBasicJsonList.class, bean.getId());
     Map<String, Exception> errors = server().getBeanState(bean).getLoadErrors();
 
     assertThat(errors).containsKey("beanMap").hasSize(1);
     assertThat(errors.values().iterator().next())
       .isInstanceOf(TextException.class)
       .hasMessageContaining("blabla");
-    //assertThat(bean.getBeanMap().get("bla").getName()).isEqualTo("Blubb");
+
+    Ebean.delete(bean);
   }
 }
diff --git a/src/test/resources/ebean.properties b/src/test/resources/ebean.properties
index 6efadcc56..cd6b5c07c 100644
--- a/src/test/resources/ebean.properties
+++ b/src/test/resources/ebean.properties
@@ -26,7 +26,6 @@ ebean.collectQueryPlans=true
 
 ebean.autoReadOnlyDataSource=true
 
-ebean.failOnLoadError=false
 #ebean.persistBatch=NONE
 
 #ebean.h2.idType=SEQUENCE