NMS-20161: Apply criteria fetch modes after the distinct rewrite for v1 /rest/alarms - #8762
Open
marshallmassengill wants to merge 3 commits into
Open
NMS-20161: Apply criteria fetch modes after the distinct rewrite for v1 /rest/alarms #8762marshallmassengill wants to merge 3 commits into
marshallmassengill wants to merge 3 commits into
Conversation
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.
… 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.
marshallmassengill
force-pushed
the
mm/NMS-20161-smoke
branch
from
August 7, 2026 19:14
5b4ec33 to
b61e7bb
Compare
marshallmassengill
requested review from
cgorantla,
christianpape,
dino2gnt,
indigo423 and
mershad-manesh
August 7, 2026 19:16
Contributor
|
Seems low risk, and since you already did the work to fix it... |
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.
marshallmassengill
dismissed stale reviews from dino2gnt and christianpape
via
August 12, 2026 18:54
8422d94
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I'm a bit torn on this one. A customer reported it and this does seem to resolve the issue but I'm not sure if it's actually needed with the v2 API. I'd like feedback on this at the very least. It's probably worth fixing but this isn't urgent.
I also want to see if CI goes green for the full ITs.
Assisted by Anthropic Claude Opus 5.
External References