diff --git a/opennms-dao/src/main/java/org/opennms/netmgt/dao/hibernate/HibernateCriteriaConverter.java b/opennms-dao/src/main/java/org/opennms/netmgt/dao/hibernate/HibernateCriteriaConverter.java index 4781bafc3052..7a4f3b237b3f 100644 --- a/opennms-dao/src/main/java/org/opennms/netmgt/dao/hibernate/HibernateCriteriaConverter.java +++ b/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 { 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,6 +120,10 @@ 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 @@ -119,20 +131,35 @@ public void visitOrder(final Order order) { } 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 m_orders = new LinkedHashSet<>(); private Set m_criterions = new LinkedHashSet<>(); + private Map 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,6 +200,18 @@ public DetachedCriteria getCriteria() { m_criteria = newCriteria; } + for (final Map.Entry 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); } @@ -178,6 +219,30 @@ public DetachedCriteria getCriteria() { 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; } } diff --git a/opennms-dao/src/test/java/org/opennms/netmgt/dao/hibernate/HibernateCriteriaConverterIT.java b/opennms-dao/src/test/java/org/opennms/netmgt/dao/hibernate/HibernateCriteriaConverterIT.java index 609b973c2ae1..ce3c2b3c7ec9 100644 --- a/opennms-dao/src/test/java/org/opennms/netmgt/dao/hibernate/HibernateCriteriaConverterIT.java +++ b/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 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 notDistinct = m_alarmDao.findMatching(alarmCriteriaBuilder().toCriteria()); + + final CriteriaBuilder cb = alarmCriteriaBuilder(); + cb.distinct(); + m_sessionFactory.getCurrentSession().clear(); + final List 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 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 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 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 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 idsOf(final List 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; + } } diff --git a/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v1/AlarmRestServiceBase.java b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v1/AlarmRestServiceBase.java index b9827d1cfa94..ac596af40446 100644 --- a/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v1/AlarmRestServiceBase.java +++ b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v1/AlarmRestServiceBase.java @@ -62,7 +62,6 @@ protected CriteriaBuilder getCriteriaBuilder(final MultivaluedMap