#1863 - L2 cache miss on natural key cache when multiple properties that include a ManyToOne

src/main/java/io/ebeaninternal/api/NaturalKeyEntry.java

@@ -2,6 +2,7 @@ package io.ebeaninternal.api;
 
 
 import io.ebean.Pairs;
+import io.ebeaninternal.server.deploy.BeanNaturalKey;
 
 import java.util.HashMap;
 import java.util.List;
@@ -19,7 +20,7 @@ public class NaturalKeyEntry {
   /**
    * Used when query query just has a series of EQ expressions (no IN clause).
    */
-  public NaturalKeyEntry(String[] naturalKey, List<NaturalKeyEq> eqList) {
+  NaturalKeyEntry(BeanNaturalKey naturalKey, List<NaturalKeyEq> eqList) {
     load(eqList);
     this.key = calculateKey(naturalKey);
   }
@@ -27,7 +28,7 @@ public class NaturalKeyEntry {
   /**
    * Create when query uses an IN clause.
    */
-  public NaturalKeyEntry(String[] naturalKey, List<NaturalKeyEq> eqList, String inProperty, Object inValue) {
+  NaturalKeyEntry(BeanNaturalKey naturalKey, List<NaturalKeyEq> eqList, String inProperty, Object inValue) {
     load(eqList);
     if (inProperty != null) {
       map.put(inProperty, inValue);
@@ -39,7 +40,7 @@ public class NaturalKeyEntry {
   /**
    * Create when query uses an IN PAIRS clause.
    */
-  public NaturalKeyEntry(String[] naturalKey, List<NaturalKeyEq> eqList,
+  NaturalKeyEntry(BeanNaturalKey naturalKey, List<NaturalKeyEq> eqList,
                          String inMapProperty0, String inMapProperty1, Pairs.Entry pair) {
     load(eqList);
     map.put(inMapProperty0, pair.getA());
@@ -56,18 +57,8 @@ public class NaturalKeyEntry {
     }
   }
 
-  private Object calculateKey(String[] naturalKey) {
-
-    if (naturalKey.length == 1) {
-      return map.get(naturalKey[0]);
-    }
-
-    StringBuilder sb = new StringBuilder();
-    for (String key : naturalKey) {
-      sb.append(map.get(key)).append(";");
-    }
-
-    return sb.toString();
+  private Object calculateKey(BeanNaturalKey naturalKey) {
+    return naturalKey.calculateKey(map);
   }
 
   /**
@@ -80,7 +71,7 @@ public class NaturalKeyEntry {
   /**
    * Return the inValue (used to remove from IN clause of original query).
    */
-  public Object getInValue() {
+  Object getInValue() {
     return inValue;
   }
 }

src/main/java/io/ebeaninternal/api/NaturalKeyQueryData.java

@@ -1,6 +1,7 @@
 package io.ebeaninternal.api;
 
 import io.ebean.Pairs;
+import io.ebeaninternal.server.deploy.BeanNaturalKey;
 
 import java.util.ArrayList;
 import java.util.HashSet;
@@ -12,7 +13,7 @@ import java.util.Set;
  */
 public class NaturalKeyQueryData<T> {
 
-  private final String[] naturalKey;
+  private final BeanNaturalKey naturalKey;
 
   /**
    * Only one of IN or IN PAIRS is allowed.
@@ -34,18 +35,12 @@ public class NaturalKeyQueryData<T> {
 
   private int hitCount;
 
-  public NaturalKeyQueryData(String[] naturalKey) {
+  public NaturalKeyQueryData(BeanNaturalKey naturalKey) {
     this.naturalKey = naturalKey;
   }
 
   private boolean matchProperty(String propName) {
-
-    for (String key : naturalKey) {
-      if (key.equals(propName)) {
-        return true;
-      }
-    }
-    return false;
+    return naturalKey.matchProperty(propName);
   }
 
   /**
@@ -132,13 +127,8 @@ public class NaturalKeyQueryData<T> {
    * Return true if the properties match the natural key properties.
    */
   private boolean matchProperties() {
-    if (naturalKey.length == 1) {
-      // simple single property case
-      if (inProperty != null) {
-        return inProperty.equals(naturalKey[0]);
-      } else {
-        return eqList.get(0).property.equals(naturalKey[0]);
-      }
+    if (naturalKey.isSingleProperty()) {
+      naturalKey.matchSingleProperty((inProperty != null) ? inProperty : eqList.get(0).property);
     }
 
     // multiple properties case
@@ -157,27 +147,17 @@ public class NaturalKeyQueryData<T> {
         exprProps.add(eq.property);
       }
     }
-    if (exprProps.size() != naturalKey.length) {
-      return false;
-    }
-    for (String key : naturalKey) {
-      if (!exprProps.remove(key)) {
-        return false;
-      }
-    }
-
-    return exprProps.isEmpty();
+    return naturalKey.matchMultiProperties(exprProps);
   }
 
   /**
    * Check that all the natural key properties are defined.
    */
   private boolean expressionCount() {
-
     int defined = (inValues == null) ? 0 : 1;
     defined += (inPairs == null) ? 0 : 2;
     defined += (eqList == null) ? 0 : eqList.size();
-    return defined == naturalKey.length;
+    return defined == naturalKey.length();
   }
 
   /**

src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java

@@ -375,6 +375,8 @@ public class BeanDescriptor<T> implements BeanType<T>, STreeType {
   private final BeanProperty[] propertiesGenUpdate;
   private final List<BeanProperty[]> propertiesUnique = new ArrayList<>();
 
+  private BeanNaturalKey beanNaturalKey;
+
   /**
    * The bean class name or the table name for MapBeans.
    */
@@ -752,6 +754,18 @@ public class BeanDescriptor<T> implements BeanType<T>, STreeType {
       softDeleteByIdSql = null;
       softDeleteByIdInSql = null;
     }
+    initNaturalKey();
+  }
+
+  private void initNaturalKey() {
+    final String[] naturalKey = cacheHelp.getNaturalKey();
+    if (naturalKey != null && naturalKey.length != 0) {
+      BeanProperty[] props = new BeanProperty[naturalKey.length];
+      for (int i = 0; i < naturalKey.length; i++) {
+        props[i] = getBeanProperty(naturalKey[i]);
+      }
+      this.beanNaturalKey = new BeanNaturalKey(naturalKey, props);
+    }
   }
 
   private boolean hasCircularImportedId() {
@@ -1322,10 +1336,10 @@ public class BeanDescriptor<T> implements BeanType<T>, STreeType {
   }
 
   /**
-   * Return the natural key properties.
+   * Return the natural key.
    */
-  public String[] getNaturalKey() {
-    return cacheHelp.getNaturalKey();
+  public BeanNaturalKey getNaturalKey() {
+    return beanNaturalKey;
   }
 
   /**

src/main/java/io/ebeaninternal/server/deploy/BeanNaturalKey.java

@@ -0,0 +1,80 @@
+package io.ebeaninternal.server.deploy;
+
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Natural key for a bean type.
+ */
+public class BeanNaturalKey {
+
+  private final String[] naturalKey;
+  private final BeanProperty[] props;
+
+  BeanNaturalKey(String[] naturalKey, BeanProperty[] props) {
+    this.naturalKey = naturalKey;
+    this.props = props;
+  }
+
+  public int length() {
+    return naturalKey.length;
+  }
+
+  /**
+   * Return true if the property name is part of the natural key.
+   */
+  public boolean matchProperty(String propName) {
+    for (String key : naturalKey) {
+      if (key.equals(propName)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  /**
+   * Return true if this is a single property natural key.
+   */
+  public boolean isSingleProperty() {
+    return props.length == 1;
+  }
+
+  /**
+   * Return true if the given propertyName is our natural key property.
+   */
+  public boolean matchSingleProperty(String propertyName) {
+    return naturalKey[0].equals(propertyName);
+  }
+
+  /**
+   * Return true if all the properties match our natural key.
+   */
+  public boolean matchMultiProperties(Set<String> expressionProperties) {
+    if (expressionProperties.size() != naturalKey.length) {
+      return false;
+    }
+    for (String key : naturalKey) {
+      if (!expressionProperties.remove(key)) {
+        return false;
+      }
+    }
+    return expressionProperties.isEmpty();
+  }
+
+  /**
+   * Return the cache key given the bind values.
+   *
+   * @param map The bind values for the properties.
+   */
+  public Object calculateKey(Map<String, Object> map) {
+    if (naturalKey.length == 1) {
+      return map.get(naturalKey[0]);
+    }
+
+    StringBuilder sb = new StringBuilder();
+    for (BeanProperty prop : props) {
+      sb.append(prop.naturalKeyVal(map)).append(";");
+    }
+    return sb.toString();
+  }
+}

src/main/java/io/ebeaninternal/server/deploy/BeanProperty.java

@@ -856,6 +856,13 @@ public class BeanProperty implements ElPropertyValue, Property, STreeProperty {
     setValue(bean, cacheData);
   }
 
+  /**
+   * Return the cache key value for this property.
+   */
+  Object naturalKeyVal(Map<String, Object> values) {
+    return values.get(name);
+  }
+
   @Override
   public Object getVal(Object bean) {
     return getValue((EntityBean) bean);

src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocOne.java

@@ -210,6 +210,14 @@ public class BeanPropertyAssocOne<T> extends BeanPropertyAssoc<T> implements STr
     }
   }
 
+  Object naturalKeyVal(Map<String, Object> values) {
+    EntityBean bean = (EntityBean) values.get(name);
+    if (bean == null) {
+      return null;
+    }
+    return targetIdBinder.cacheKeyFromBean(bean);
+  }
+
   @Override
   public ElPropertyValue buildElPropertyValue(String propName, String remainder, ElPropertyChainBuilder chain, boolean propertyDeploy) {
 

src/main/java/io/ebeaninternal/server/deploy/id/IdBinder.java

@@ -213,4 +213,8 @@ public interface IdBinder {
    */
   String cacheKey(Object idValue);
 
+  /**
+   * Return a key to use for bean caches given the bean.
+   */
+  String cacheKeyFromBean(EntityBean bean);
 }

src/main/java/io/ebeaninternal/server/deploy/id/IdBinderEmbedded.java

@@ -501,4 +501,8 @@ public final class IdBinderEmbedded implements IdBinder {
     return sb.toString();
   }
 
+  @Override
+  public String cacheKeyFromBean(EntityBean bean) {
+    return cacheKey(embIdProperty.getValue(bean));
+  }
 }

src/main/java/io/ebeaninternal/server/deploy/id/IdBinderEmpty.java

@@ -182,4 +182,8 @@ public final class IdBinderEmpty implements IdBinder {
     return null;
   }
 
+  @Override
+  public String cacheKeyFromBean(EntityBean bean) {
+    return null;
+  }
 }

src/main/java/io/ebeaninternal/server/deploy/id/IdBinderSimple.java

@@ -268,4 +268,9 @@ public final class IdBinderSimple implements IdBinder {
     return scalarType.format(value);
   }
 
+  @Override
+  public String cacheKeyFromBean(EntityBean bean) {
+    final Object value = idProperty.getValue(bean);
+    return scalarType.format(value);
+  }
 }

src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java

@@ -48,6 +48,7 @@ import io.ebeaninternal.api.SpiTransaction;
 import io.ebeaninternal.server.autotune.ProfilingListener;
 import io.ebeaninternal.server.core.SpiOrmQueryRequest;
 import io.ebeaninternal.server.deploy.BeanDescriptor;
+import io.ebeaninternal.server.deploy.BeanNaturalKey;
 import io.ebeaninternal.server.deploy.BeanPropertyAssocMany;
 import io.ebeaninternal.server.deploy.InheritInfo;
 import io.ebeaninternal.server.deploy.TableJoin;
@@ -771,8 +772,8 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
     if (whereExpressions == null) {
       return null;
     }
-    String[] naturalKey = beanDescriptor.getNaturalKey();
-    if (naturalKey == null || naturalKey.length == 0) {
+    BeanNaturalKey naturalKey = beanDescriptor.getNaturalKey();
+    if (naturalKey == null) {
       return null;
     }
 

src/test/java/io/ebean/BaseTestCase.java

@@ -2,6 +2,7 @@ package io.ebean;
 
 import io.ebean.annotation.PersistBatch;
 import io.ebean.annotation.Platform;
+import io.ebean.cache.ServerCacheStatistics;
 import io.ebean.config.dbplatform.IdType;
 import io.ebean.meta.MetaTimedMetric;
 import io.ebean.meta.MetricType;
@@ -76,6 +77,10 @@ public abstract class BaseTestCase {
     }
   }
 
+  protected void clearAllL2Cache() {
+    server().getServerCacheManager().clearAll();
+  }
+
   protected void resetAllMetrics() {
     server().getMetaInfoManager().resetAllMetrics();
   }
@@ -221,6 +226,10 @@ public abstract class BaseTestCase {
     return spiEbeanServer().getBeanDescriptor(cls);
   }
 
+  protected <T> ServerCacheStatistics getBeanCacheStats(Class<T> cls, boolean reset) {
+    return server().getServerCacheManager().getBeanCache(cls).getStatistics(reset);
+  }
+
   protected Platform platform() {
     return spiEbeanServer().getDatabasePlatform().getPlatform();
   }

src/test/java/org/tests/model/basic/cache/OCacheBase.java

@@ -0,0 +1,33 @@
+package org.tests.model.basic.cache;
+
+import io.ebean.Model;
+
+import javax.persistence.Id;
+import javax.persistence.MappedSuperclass;
+import javax.persistence.Version;
+
+@MappedSuperclass
+public class OCacheBase extends Model {
+
+  @Id
+  private long id;
+
+  @Version
+  private long version;
+
+  public long getId() {
+    return id;
+  }
+
+  public void setId(long id) {
+    this.id = id;
+  }
+
+  public long getVersion() {
+    return version;
+  }
+
+  public void setVersion(long version) {
+    this.version = version;
+  }
+}

src/test/java/org/tests/model/basic/cache/OCachedApp.java

@@ -0,0 +1,22 @@
+package org.tests.model.basic.cache;
+
+import io.ebean.annotation.Cache;
+
+import javax.persistence.Entity;
+import javax.persistence.UniqueConstraint;
+
+@Cache(naturalKey = "appName")
+@Entity
+@UniqueConstraint(columnNames = "app_name")
+public class OCachedApp extends OCacheBase {
+
+  private final String appName;
+
+  public OCachedApp(String appName) {
+    this.appName = appName;
+  }
+
+  public String getAppName() {
+    return appName;
+  }
+}

src/test/java/org/tests/model/basic/cache/OCachedAppDetail.java

@@ -0,0 +1,31 @@
+package org.tests.model.basic.cache;
+
+import io.ebean.annotation.Cache;
+
+import javax.persistence.Entity;
+import javax.persistence.ManyToOne;
+import javax.persistence.UniqueConstraint;
+
+@Cache(naturalKey = {"app", "detail"})
+@Entity
+@UniqueConstraint(columnNames = {"app_id", "detail"})
+public class OCachedAppDetail extends OCacheBase {
+
+  @ManyToOne(optional = false)
+  private final OCachedApp app;
+
+  private final String detail;
+
+  public OCachedAppDetail(OCachedApp app, String detail) {
+    this.app = app;
+    this.detail = detail;
+  }
+
+  public OCachedApp getApp() {
+    return app;
+  }
+
+  public String getDetail() {
+    return detail;
+  }
+}

src/test/java/org/tests/model/basic/cache/TestNatKeyCacheWithForeignKey.java

@@ -0,0 +1,126 @@
+package org.tests.model.basic.cache;
+
+import io.ebean.BaseTestCase;
+import io.ebean.DB;
+import io.ebean.cache.ServerCacheStatistics;
+import org.ebeantest.LoggedSqlCollector;
+import org.junit.Test;
+
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class TestNatKeyCacheWithForeignKey extends BaseTestCase {
+
+  private static boolean seededData;
+
+  private static final OCachedApp app0 = new OCachedApp("app0");
+  private static final OCachedApp app1 = new OCachedApp("app1");
+
+  private ServerCacheStatistics getStats() {
+    return getBeanCacheStats(OCachedAppDetail.class, true);
+  }
+
+  @Test
+  public void test_findOne() {
+
+    setupData();
+    clearAllL2Cache();
+
+    final OCachedAppDetail found0 = findDetail(app0, "detail0");
+    assertThat(found0).isNotNull();
+    assertThat(getStats().getHitCount()).isEqualTo(0);
+
+    resetAllMetrics();
+
+    LoggedSqlCollector.start();
+
+    final OCachedAppDetail found1 = findDetail(app0, "detail0");
+    assertThat(found1).isNotNull();
+
+    final List<String> sql = LoggedSqlCollector.stop();
+    assertThat(sql).as("Expected cache hit, no SQL query expected").isEmpty();
+    assertThat(getStats().getHitCount()).isEqualTo(1);
+  }
+
+  private OCachedAppDetail findDetail(OCachedApp app, String detail) {
+
+    return DB.find(OCachedAppDetail.class)
+      .where()
+      .eq("app", app)
+      .eq("detail", detail)
+      .findOne();
+  }
+
+
+  @Test
+  public void test_findList_details_expect_hitNatKeyCache() {
+
+    setupData();
+    clearAllL2Cache();
+
+    final List<OCachedAppDetail> result0 = findListDetails(app0, "detail0", "detail1");
+    assertThat(result0).hasSize(2);
+    assertThat(getStats().getHitCount()).isEqualTo(0);
+
+    LoggedSqlCollector.start();
+    final List<OCachedAppDetail> result1 = findListDetails(app0, "detail0", "detail1");
+    assertThat(result1).hasSize(2);
+
+    final List<String> sql = LoggedSqlCollector.stop();
+    assertThat(sql).as("Expected cache hit, no SQL query expected").isEmpty();
+    assertThat(getStats().getHitCount()).isEqualTo(2);
+  }
+
+  private List<OCachedAppDetail> findListDetails(OCachedApp app, String... details) {
+
+    return DB.find(OCachedAppDetail.class)
+      .setUseCache(true)
+      .where()
+      .eq("app", app)
+      .in("detail", details)
+      .findList();
+  }
+
+  @Test
+  public void test_findList_foreignKey_expect_hitNatKeyCache() {
+
+    setupData();
+    clearAllL2Cache();
+
+    final List<OCachedAppDetail> result0 = findListApps("detail0", app0, app1);
+    assertThat(result0).hasSize(2);
+    assertThat(getStats().getHitCount()).isEqualTo(0);
+
+    LoggedSqlCollector.start();
+    final List<OCachedAppDetail> result1 = findListApps("detail0", app0, app1);
+    assertThat(result1).hasSize(2);
+
+    final List<String> sql = LoggedSqlCollector.stop();
+    assertThat(sql).as("Expected cache hit, no SQL query expected").isEmpty();
+    assertThat(getStats().getHitCount()).isEqualTo(2);
+  }
+
+  private List<OCachedAppDetail> findListApps(String detail, OCachedApp... apps) {
+
+    return DB.find(OCachedAppDetail.class)
+      .setUseCache(true)
+      .where()
+      .in("app", apps)
+      .eq("detail", detail)
+      .findList();
+  }
+
+  private static void setupData() {
+
+    if (!seededData) {
+      seededData = true;
+      app0.save();
+      app1.save();
+      new OCachedAppDetail(app0, "detail0").save();
+      new OCachedAppDetail(app0, "detail1").save();
+      new OCachedAppDetail(app1, "detail0").save();
+      new OCachedAppDetail(app1, "detail1").save();
+    }
+  }
+}