NMS-20161: Apply criteria fetch modes after the distinct rewrite for v1 /rest/alarms

opennms-dao/src/main/java/org/opennms/netmgt/dao/hibernate/HibernateCriteriaConverter.java

@@ -23,25 +23,31 @@
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
 
 import org.hibernate.FetchMode;
+import org.hibernate.HibernateException;
 import org.hibernate.LockMode;
 import org.hibernate.Session;
+import org.hibernate.SessionFactory;
 import org.hibernate.criterion.Criterion;
 import org.hibernate.criterion.DetachedCriteria;
 import org.hibernate.criterion.Junction;
 import org.hibernate.criterion.Projections;
 import org.hibernate.criterion.SimpleExpression;
 import org.hibernate.criterion.Subqueries;
+import org.hibernate.metadata.ClassMetadata;
 import org.hibernate.type.FloatType;
 import org.hibernate.type.IntegerType;
 import org.hibernate.type.LongType;
 import org.hibernate.type.StringType;
 import org.hibernate.type.TimestampType;
+import org.hibernate.type.Type;
 import org.opennms.core.criteria.AbstractCriteriaVisitor;
 import org.opennms.core.criteria.Alias;
 import org.opennms.core.criteria.Criteria;
@@ -72,12 +78,14 @@
 import org.opennms.core.criteria.restrictions.RestrictionVisitor;
 import org.opennms.core.criteria.restrictions.SqlRestriction;
 import org.opennms.netmgt.dao.api.CriteriaConverter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import com.google.common.base.Strings;
 
 public class HibernateCriteriaConverter implements CriteriaConverter<DetachedCriteria> {
     public org.hibernate.Criteria convert(final Criteria criteria, final Session session) {
-        final HibernateCriteriaVisitor visitor = new HibernateCriteriaVisitor();
+        final HibernateCriteriaVisitor visitor = new HibernateCriteriaVisitor(session.getSessionFactory());
         criteria.visit(visitor);
 
         return visitor.getCriteria(session);
@@ -92,7 +100,7 @@ public DetachedCriteria convert(final Criteria criteria) {
     }
 
     public org.hibernate.Criteria convertForCount(final Criteria criteria, final Session session) {
-        final HibernateCriteriaVisitor visitor = new CountHibernateCriteriaVisitor();
+        final HibernateCriteriaVisitor visitor = new CountHibernateCriteriaVisitor(session.getSessionFactory());
         criteria.visit(visitor);
 
         return visitor.getCriteria(session);
@@ -112,27 +120,46 @@ public void visitOrder(final Order order) {
     }
 
     public static class CountHibernateCriteriaVisitor extends HibernateCriteriaVisitor {
+        public CountHibernateCriteriaVisitor(final SessionFactory sessionFactory) {
+            super(sessionFactory);
+        }
+
         @Override
         public void visitOrder(final Order order) {
             // skip order-by when converting for count
         }
     }
 
     public static class HibernateCriteriaVisitor extends AbstractCriteriaVisitor {
+        private static final Logger LOG = LoggerFactory.getLogger(HibernateCriteriaVisitor.class);
+
         private DetachedCriteria m_criteria;
 
         private Class<?> m_class;
 
+        /** Null when the criteria is converted without a session; see isToMany(). */
+        private final SessionFactory m_sessionFactory;
+
         private Set<org.hibernate.criterion.Order> m_orders = new LinkedHashSet<>();
 
         private Set<org.hibernate.criterion.Criterion> m_criterions = new LinkedHashSet<>();
 
+        private Map<String, FetchMode> m_fetchModes = new LinkedHashMap<>();
+
         private boolean m_distinct = false;
 
         private Integer m_limit;
 
         private Integer m_offset;
 
+        public HibernateCriteriaVisitor() {
+            this(null);
+        }
+
+        public HibernateCriteriaVisitor(final SessionFactory sessionFactory) {
+            m_sessionFactory = sessionFactory;
+        }
+
         public org.hibernate.Criteria getCriteria(final Session session) {
             final org.hibernate.Criteria hibernateCriteria = getCriteria().getExecutableCriteria(session);
             if (m_limit != null)
@@ -154,7 +181,9 @@ public DetachedCriteria getCriteria() {
             /*
              * By implementing distinct() as a subquery, we lose the ability to sort the
              * results on any of the aliased columns. See bug NMS-7830 for more details.
-             * 
+             * Orders and fetch modes are therefore applied to the outer criteria below,
+             * after the rewrite has replaced m_criteria. See bug NMS-20161.
+             *
              * @see http://issues.opennms.org/browse/NMS-7830
              */
             if (m_distinct) {
@@ -171,13 +200,49 @@ public DetachedCriteria getCriteria() {
                 m_criteria = newCriteria;
             }
 
+            for (final Map.Entry<String, FetchMode> fetchMode : m_fetchModes.entrySet()) {
+                if (m_distinct && FetchMode.JOIN.equals(fetchMode.getValue()) && isToMany(fetchMode.getKey())) {
+                    // joining a to-many association yields one outer row per element, which
+                    // would undo the distinct() rewrite above and make limit/offset count
+                    // rows rather than entities
+                    LOG.warn("Ignoring the eager fetch of '{}' on {}: a to-many association cannot be join-fetched by a distinct() criteria.",
+                             fetchMode.getKey(), m_class.getName());
+                    continue;
+                }
+                m_criteria.setFetchMode(fetchMode.getKey(), fetchMode.getValue());
+            }
+
             for (final org.hibernate.criterion.Order order : m_orders) {
                 m_criteria.addOrder(order);
             }
 
             return m_criteria;
         }
 
+        /**
+         * Whether an association path on the root entity is collection-valued.
+         * Returns false when there is no session factory to ask, and for paths
+         * Hibernate cannot resolve, since it ignores fetch modes for those anyway.
+         */
+        private boolean isToMany(final String path) {
+            if (m_sessionFactory == null) {
+                return false;
+            }
+
+            final ClassMetadata metadata = m_sessionFactory.getClassMetadata(m_class);
+            if (metadata == null) {
+                return false;
+            }
+
+            try {
+                final Type type = metadata.getPropertyType(path);
+                return type != null && type.isCollectionType();
+            } catch (final HibernateException e) {
+                LOG.debug("Unable to determine the type of '{}' on {}.", path, m_class.getName(), e);
+                return false;
+            }
+        }
+
         /**
          * If {@code rootAlias} is null, then Hibernate will use a default
          * alias of {@code this}.
@@ -229,18 +294,20 @@ public void visitAlias(final Alias alias) {
 
         @Override
         public void visitFetch(final Fetch fetch) {
+            // held rather than applied here, because the distinct rewrite in
+            // getCriteria() replaces the criteria these would be set on
             switch (fetch.getFetchType()) {
             case DEFAULT:
-                m_criteria.setFetchMode(fetch.getAttribute(), FetchMode.DEFAULT);
+                m_fetchModes.put(fetch.getAttribute(), FetchMode.DEFAULT);
                 break;
             case EAGER:
-                m_criteria.setFetchMode(fetch.getAttribute(), FetchMode.JOIN);
+                m_fetchModes.put(fetch.getAttribute(), FetchMode.JOIN);
                 break;
             case LAZY:
-                m_criteria.setFetchMode(fetch.getAttribute(), FetchMode.SELECT);
+                m_fetchModes.put(fetch.getAttribute(), FetchMode.SELECT);
                 break;
             default:
-                m_criteria.setFetchMode(fetch.getAttribute(), FetchMode.DEFAULT);
+                m_fetchModes.put(fetch.getAttribute(), FetchMode.DEFAULT);
                 break;
             }
         }

opennms-dao/src/test/java/org/opennms/netmgt/dao/hibernate/HibernateCriteriaConverterIT.java

@@ -22,20 +22,30 @@
 package org.opennms.netmgt.dao.hibernate;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 
 import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
 
+import org.hibernate.SessionFactory;
+import org.hibernate.proxy.HibernateProxy;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.opennms.core.criteria.Alias.JoinType;
 import org.opennms.core.criteria.CriteriaBuilder;
+import org.opennms.core.criteria.Fetch.FetchType;
 import org.opennms.core.spring.BeanUtils;
 import org.opennms.core.test.MockLogAppender;
 import org.opennms.core.test.OpenNMSJUnit4ClassRunner;
 import org.opennms.core.test.db.annotations.JUnitTemporaryDatabase;
 import org.opennms.netmgt.dao.DatabasePopulator;
+import org.opennms.netmgt.dao.api.AlarmDao;
 import org.opennms.netmgt.dao.api.NodeDao;
+import org.opennms.netmgt.model.OnmsAlarm;
 import org.opennms.netmgt.model.OnmsCriteria;
 import org.opennms.netmgt.model.OnmsNode;
 import org.opennms.test.JUnitConfigurationEnvironment;
@@ -67,6 +77,12 @@ public class HibernateCriteriaConverterIT implements InitializingBean {
     @Autowired
     NodeDao m_nodeDao;
 
+    @Autowired
+    AlarmDao m_alarmDao;
+
+    @Autowired
+    SessionFactory m_sessionFactory;
+
     @Override
     public void afterPropertiesSet() throws Exception {
         BeanUtils.assertAutowiring(this);
@@ -123,4 +139,140 @@ public void testDistinctQuery() {
         assertEquals(1, nodes.size());
         assertEquals(Integer.valueOf(1), nodes.get(0).getId());
     }
+
+    /**
+     * Mirrors the v1 /rest/alarms criteria: an eager fetch alongside distinct().
+     * The distinct rewrite replaces the criteria object, so fetch modes have to
+     * be applied afterwards or the association falls back to a lazy proxy that
+     * is read in a separate statement. See NMS-20161.
+     */
+    @Test
+    @JUnitTemporaryDatabase
+    public void testDistinctPreservesEagerFetch() {
+        final CriteriaBuilder cb = alarmCriteriaBuilder();
+        cb.distinct();
+
+        // the populated alarm and its event must not already be in the session,
+        // or a lazy association would resolve from the first-level cache
+        m_sessionFactory.getCurrentSession().clear();
+
+        final List<OnmsAlarm> alarms = m_alarmDao.findMatching(cb.toCriteria());
+        assertEquals(1, alarms.size());
+
+        final OnmsAlarm alarm = alarms.get(0);
+        assertNotNull(alarm.getLastEvent());
+        assertFalse("lastEvent should be join-fetched, not a lazy proxy",
+                    alarm.getLastEvent() instanceof HibernateProxy);
+    }
+
+    /**
+     * Applying the fetch modes to the outer criteria must not reintroduce the
+     * duplicate rows that distinct() is there to collapse.
+     */
+    @Test
+    @JUnitTemporaryDatabase
+    public void testDistinctWithEagerFetchStillDeduplicates() {
+        m_sessionFactory.getCurrentSession().clear();
+        final List<OnmsAlarm> notDistinct = m_alarmDao.findMatching(alarmCriteriaBuilder().toCriteria());
+
+        final CriteriaBuilder cb = alarmCriteriaBuilder();
+        cb.distinct();
+        m_sessionFactory.getCurrentSession().clear();
+        final List<OnmsAlarm> distinct = m_alarmDao.findMatching(cb.toCriteria());
+
+        assertTrue("the to-many join should multiply the single alarm into several rows",
+                   notDistinct.size() > 1);
+        assertEquals(1, distinct.size());
+    }
+
+    /**
+     * Ordering is applied to the outer criteria after the distinct rewrite
+     * (NMS-7830); the fetch modes must not disturb that.
+     */
+    @Test
+    @JUnitTemporaryDatabase
+    public void testDistinctWithEagerFetchKeepsOrdering() {
+        final CriteriaBuilder cb = new CriteriaBuilder(OnmsNode.class);
+        cb.fetch("assetRecord", FetchType.EAGER);
+        cb.alias("ipInterfaces", "ipInterface", JoinType.LEFT_JOIN);
+        cb.orderBy("label").desc();
+        cb.distinct();
+
+        final List<OnmsNode> nodes = m_nodeDao.findMatching(cb.toCriteria());
+        assertEquals(6, nodes.size());
+        for (int i = 1; i < nodes.size(); i++) {
+            assertFalse("nodes should be ordered by label descending",
+                        nodes.get(i - 1).getLabel().compareTo(nodes.get(i).getLabel()) < 0);
+        }
+    }
+
+    /**
+     * A to-many association cannot be join-fetched by a distinct() criteria: the
+     * join would return one outer row per element and undo the rewrite. Such a
+     * fetch is dropped, leaving the association to load lazily as it did before
+     * fetch modes reached the outer criteria.
+     */
+    @Test
+    @JUnitTemporaryDatabase
+    public void testDistinctDropsToManyEagerFetch() {
+        final CriteriaBuilder cb = new CriteriaBuilder(OnmsNode.class);
+        cb.fetch("ipInterfaces", FetchType.EAGER);
+        cb.distinct();
+
+        final List<OnmsNode> nodes = m_nodeDao.findMatching(cb.toCriteria());
+        assertEquals(6, nodes.size());
+        assertFalse("the interfaces should still be reachable, just not join-fetched",
+                    nodes.get(0).getIpInterfaces().isEmpty());
+    }
+
+    /**
+     * limit() becomes setMaxResults() on the outer criteria, so it counts rows.
+     * Dropping the to-many fetch is what keeps those rows one-per-entity.
+     */
+    @Test
+    @JUnitTemporaryDatabase
+    public void testDistinctWithToManyEagerFetchStillPages() {
+        final CriteriaBuilder cb = new CriteriaBuilder(OnmsNode.class);
+        cb.fetch("ipInterfaces", FetchType.EAGER);
+        cb.orderBy("label").desc();
+        cb.distinct();
+        cb.limit(2);
+
+        final List<OnmsNode> nodes = m_nodeDao.findMatching(cb.toCriteria());
+        assertEquals(2, nodes.size());
+        assertEquals("limit should count nodes, not joined interface rows", 2, idsOf(nodes).size());
+    }
+
+    /**
+     * Only the distinct() path drops the fetch. Without it, a to-many join fetch
+     * multiplies the rows as it always has.
+     */
+    @Test
+    @JUnitTemporaryDatabase
+    public void testToManyEagerFetchSurvivesWithoutDistinct() {
+        final CriteriaBuilder cb = new CriteriaBuilder(OnmsNode.class);
+        cb.fetch("ipInterfaces", FetchType.EAGER);
+
+        final List<OnmsNode> nodes = m_nodeDao.findMatching(cb.toCriteria());
+        assertEquals(6, idsOf(nodes).size());
+        assertTrue("the join fetch should return one row per interface",
+                   nodes.size() > idsOf(nodes).size());
+    }
+
+    private Set<Integer> idsOf(final List<OnmsNode> nodes) {
+        return nodes.stream().map(OnmsNode::getId).collect(Collectors.toSet());
+    }
+
+    /**
+     * The to-many join on node.ipInterfaces is what makes distinct()
+     * load-bearing here: without it the single alarm comes back once per
+     * interface.
+     */
+    private CriteriaBuilder alarmCriteriaBuilder() {
+        final CriteriaBuilder cb = new CriteriaBuilder(OnmsAlarm.class);
+        cb.fetch("lastEvent", FetchType.EAGER);
+        cb.alias("node", "node", JoinType.LEFT_JOIN);
+        cb.alias("node.ipInterfaces", "ipInterface", JoinType.LEFT_JOIN);
+        return cb;
+    }
 }

opennms-webapp-rest/src/main/java/org/opennms/web/rest/v1/AlarmRestServiceBase.java

@@ -62,7 +62,6 @@ protected CriteriaBuilder getCriteriaBuilder(final MultivaluedMap<String, String
 
     	final CriteriaBuilder cb = new CriteriaBuilder(OnmsAlarm.class);
 
-    	cb.fetch("firstEvent", FetchType.EAGER);
         cb.fetch("lastEvent", FetchType.EAGER);
 
         cb.alias("node", "node", JoinType.LEFT_JOIN);

opennms-webapp-rest/src/main/java/org/opennms/web/rest/v1/AlarmStatsRestService.java

@@ -174,7 +174,6 @@ protected static CriteriaBuilder getCriteriaBuilder(final OnmsSeverity severity)
             builder.eq("severity", severity);
         }
 
-        builder.fetch("firstEvent", FetchType.EAGER);
         builder.fetch("lastEvent", FetchType.EAGER);
 
         builder.alias("node", "node", JoinType.LEFT_JOIN);