From 9a02566a615b135083b889883044eeb70bdeb8e6 Mon Sep 17 00:00:00 2001 From: Marshall Massengill Date: Tue, 4 Aug 2026 11:02:31 -0400 Subject: [PATCH 1/3] NMS-20161: Apply criteria fetch modes after the distinct rewrite HibernateCriteriaVisitor.visitFetch() set fetch modes directly on m_criteria, but getCriteria() then implements distinct() by demoting that criteria to an id-only subquery and building a new outer criteria. The fetch modes went with the subquery, where Hibernate ignores them anyway once a projection is set, so any criteria combining fetch(EAGER) with distinct() silently got no join. For v1 /rest/alarms this left OnmsAlarm.lastEvent as a lazy proxy read in a separate statement per alarm, during hydration and after the alarm rows had been read. An event deleted in that window by auto-clean reduction raises ObjectNotFoundException, which OnmsAlarm.setLastEvent swallows, so the broken proxy escapes the DAO and fails later inside the serializer, truncating the response body. Hold the fetch modes and apply them to the outer criteria after the rewrite, the way m_orders already is. --- .../hibernate/HibernateCriteriaConverter.java | 22 ++++- .../HibernateCriteriaConverterIT.java | 93 +++++++++++++++++++ 2 files changed, 110 insertions(+), 5 deletions(-) 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..f4042bdf0ad7 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,8 +23,10 @@ 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; @@ -127,6 +129,8 @@ public static class HibernateCriteriaVisitor extends AbstractCriteriaVisitor { private Set m_criterions = new LinkedHashSet<>(); + private Map m_fetchModes = new LinkedHashMap<>(); + private boolean m_distinct = false; private Integer m_limit; @@ -154,7 +158,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 +177,10 @@ public DetachedCriteria getCriteria() { m_criteria = newCriteria; } + for (final Map.Entry fetchMode : m_fetchModes.entrySet()) { + m_criteria.setFetchMode(fetchMode.getKey(), fetchMode.getValue()); + } + for (final org.hibernate.criterion.Order order : m_orders) { m_criteria.addOrder(order); } @@ -229,18 +239,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..8e6d5587045f 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,28 @@ 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 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 +75,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 +137,83 @@ 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); + } + } + + /** + * 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; + } } From b61e7bb24a4082fd9fe7724073059b004fdd1764 Mon Sep 17 00:00:00 2001 From: Marshall Massengill Date: Tue, 4 Aug 2026 11:02:43 -0400 Subject: [PATCH 2/3] NMS-20161: Remove the unresolvable firstEvent fetch from the v1 alarm criteria OnmsAlarm has no firstEvent property. Its only OnmsEvent association is lastEvent; firstEventTime is a plain column. Hibernate ignores fetch modes whose path it cannot resolve, so this threw nothing before and throws nothing now, but the preceding commit makes these declarations reach the query, and a live fetch mode that resolves to nothing is a trap for the next reader. --- .../main/java/org/opennms/web/rest/v1/AlarmRestServiceBase.java | 1 - .../main/java/org/opennms/web/rest/v1/AlarmStatsRestService.java | 1 - 2 files changed, 2 deletions(-) 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 Date: Wed, 12 Aug 2026 14:53:56 -0400 Subject: [PATCH 3/3] NMS-20161: Keep to-many fetches out of the distinct outer criteria Applying the held fetch modes to the outer criteria makes them live for every association, not just the to-one ones the alarm criteria needs. A join fetch of a collection returns one outer row per element, which would undo the distinct() rewrite it is applied after and leave setMaxResults counting joined rows rather than entities. No caller fetches a collection today, but the declaration now reaches the query, so the trap is real. Ask the session factory whether the path is collection-valued and skip those fetches, leaving the association to load the way it did when the fetch modes still went to the subquery. Answering that needs a SessionFactory, so the two entry points that take a Session hand theirs to the visitor; the detached overloads, which nothing calls, still apply every fetch mode. --- .../hibernate/HibernateCriteriaConverter.java | 59 ++++++++++++++++++- .../HibernateCriteriaConverterIT.java | 59 +++++++++++++++++++ 2 files changed, 116 insertions(+), 2 deletions(-) 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 f4042bdf0ad7..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 @@ -31,19 +31,23 @@ 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; @@ -74,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); @@ -94,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); @@ -114,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 @@ -121,10 +131,15 @@ 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<>(); @@ -137,6 +152,14 @@ public static class HibernateCriteriaVisitor extends AbstractCriteriaVisitor { 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) @@ -178,6 +201,14 @@ public DetachedCriteria getCriteria() { } 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()); } @@ -188,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}. 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 8e6d5587045f..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 @@ -27,6 +27,8 @@ 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; @@ -204,6 +206,63 @@ public void testDistinctWithEagerFetchKeepsOrdering() { } } + /** + * 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