From e42c3d88345e6079ac2bfdc657e156dbbf73d433 Mon Sep 17 00:00:00 2001 From: Marshall Massengill Date: Tue, 11 Aug 2026 13:04:07 -0400 Subject: [PATCH 1/2] NMS-20204: Reduce per-event overhead in the event translator EventTranslatorConfigFactory repeated most of its work on every event. Value specs exposed matches() and getResult() as separate calls, and TranslationMapping.translate() invoked both, so each value was resolved twice per event. For a sql value that meant two connection checkouts, two prepared statements and two round trips, and nested values were evaluated three times. ValueSpec now exposes a single evaluate() that returns whether the value matched along with the value to assign. EvaluationResult carries the two separately rather than collapsing into an Optional, because a sql lookup that finds a row with a null column is a match whose value is null, which must not fall through to the assignment default. The matches regex was recompiled on every evaluation, and a "~"-prefixed parameter name was recompiled once per parm scanned via String.matches. Both are now compiled once when the value spec is constructed. translate() cloned the event before running any assignment, so a mapping that rejected the event still paid for the clone. Assignments are now resolved against the source event first, which is safe because value specs only ever read the source event, and the clone happens only once the mapping is known to match. The early exit on the first assignment that neither matches nor has a default is preserved, so a rejecting mapping does no more work than before. cloneEvent() deep-copied through a Java serialization round trip, which spent most of its time re-writing class descriptors: 52 us/op against 0.74 us/op for a field copy through the immutable event model. Event does not implement IEvent, so the copy goes via ImmutableMapper. Both mappers cover all 34 of Event's fields. m_translationSpecs is now volatile, since update() clears it while translateEvent() reads it without synchronization. No configuration, schema or interface changes. --- .../config/EventTranslatorConfigFactory.java | 292 +++++++----------- .../config/EventTranslatorCloneEventTest.java | 194 ++++++++++++ .../config/EventTranslatorSqlValueTest.java | 194 ++++++++++++ 3 files changed, 507 insertions(+), 173 deletions(-) create mode 100644 opennms-config/src/test/java/org/opennms/netmgt/config/EventTranslatorCloneEventTest.java create mode 100644 opennms-config/src/test/java/org/opennms/netmgt/config/EventTranslatorSqlValueTest.java diff --git a/opennms-config/src/main/java/org/opennms/netmgt/config/EventTranslatorConfigFactory.java b/opennms-config/src/main/java/org/opennms/netmgt/config/EventTranslatorConfigFactory.java index 04abea2de68f..51aefb14b063 100644 --- a/opennms-config/src/main/java/org/opennms/netmgt/config/EventTranslatorConfigFactory.java +++ b/opennms-config/src/main/java/org/opennms/netmgt/config/EventTranslatorConfigFactory.java @@ -23,19 +23,14 @@ import java.beans.PropertyEditorSupport; import java.beans.PropertyVetoException; -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; import java.io.File; import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; -import java.io.ObjectInputStream; -import java.io.ObjectOutputStream; import java.io.Reader; import java.sql.SQLException; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.regex.Matcher; @@ -57,6 +52,7 @@ import org.opennms.netmgt.config.translator.EventTranslatorConfiguration; import org.opennms.netmgt.config.translator.Mapping; import org.opennms.netmgt.config.translator.Value; +import org.opennms.netmgt.events.api.model.ImmutableMapper; import org.opennms.netmgt.xml.event.Event; import org.opennms.netmgt.xml.event.Parm; import org.slf4j.Logger; @@ -89,7 +85,8 @@ public final class EventTranslatorConfigFactory implements EventTranslatorConfig */ private EventTranslatorConfiguration m_config; - private List m_translationSpecs; + /** Volatile because update() clears it while translateEvent() reads it unsynchronized. */ + private volatile List m_translationSpecs; /** * This member is set to true if the configuration file has been loaded. @@ -360,20 +357,28 @@ class TranslationMapping { } public Event translate(Event srcEvent) { - Event targetEvent = cloneEvent(srcEvent); - - for (AssignmentSpec assignSpec : getAssignmentSpecs()) { - if (assignSpec.matches(srcEvent)) { - assignSpec.apply(srcEvent, targetEvent); + final List assignmentSpecs = getAssignmentSpecs(); + + /* Resolving before cloning is safe because value specs only ever read the source + * event, and it keeps a mapping that rejects the event from paying for a clone. */ + final String[] values = new String[assignmentSpecs.size()]; + for (int i = 0; i < values.length; i++) { + final AssignmentSpec assignSpec = assignmentSpecs.get(i); + final EvaluationResult result = assignSpec.evaluate(srcEvent); + if (result.matched()) { + values[i] = result.value(); + } else if (assignSpec.getAssignment().hasDefault()) { + values[i] = assignSpec.getAssignment().getDefault(); } else { - if (assignSpec.getAssignment().hasDefault()) { - assignSpec.setValue(targetEvent, assignSpec.getAssignment().getDefault()); - } else { - return null; - } + return null; } } + final Event targetEvent = cloneEvent(srcEvent); + for (int i = 0; i < values.length; i++) { + assignmentSpecs.get(i).setValue(targetEvent, values[i]); + } + targetEvent.setSource(TRANSLATOR_NAME); return targetEvent; } @@ -426,14 +431,10 @@ abstract class AssignmentSpec { private Assignment m_assignment; private ValueSpec m_valueSpec; AssignmentSpec(Assignment assignment) { - m_assignment = assignment; + m_assignment = assignment; m_valueSpec = null; // lazy init } - public void apply(Event srcEvent, Event targetEvent) { - setValue(targetEvent, getValueSpec().getResult(srcEvent)); - } - private Assignment getAssignment() { return m_assignment; } protected String getAttributeName() { return getAssignment().getName(); } @@ -451,8 +452,9 @@ private ValueSpec getValueSpec() { m_valueSpec = constructValueSpec(); return m_valueSpec; } - boolean matches(Event e) { - return getValueSpec().matches(e); + + EvaluationResult evaluate(Event srcEvent) { + return getValueSpec().evaluate(srcEvent); } } @@ -521,12 +523,47 @@ else if ("sql".equals(val.getType())) return new ValueSpecUnspecified(); } + /** + * Outcome of evaluating a {@link ValueSpec} against an event: either no match, or a match + * carrying a value. A matched value may legitimately be null (a SQL lookup that found a row + * with a null column), so this cannot collapse into an Optional. + */ + static final class EvaluationResult { + private static final EvaluationResult NO_MATCH = new EvaluationResult(null, false); + + private final String m_value; + private final boolean m_matched; - abstract class ValueSpec { + private EvaluationResult(String value, boolean matched) { + m_value = value; + m_matched = matched; + } + + static EvaluationResult noMatch() { + return NO_MATCH; + } + + static EvaluationResult of(String value) { + return new EvaluationResult(value, true); + } + + boolean matched() { + return m_matched; + } + + String value() { + return m_value; + } + } - public abstract boolean matches(Event e); + abstract class ValueSpec { - public abstract String getResult(Event srcEvent); + /** + * Evaluates this value against the source event exactly once, yielding both whether it + * matched and the value to assign. Callers must not re-evaluate to obtain the value: + * for sql values each evaluation is a database round trip. + */ + public abstract EvaluationResult evaluate(Event srcEvent); } class ConstantValueSpec extends ValueSpec { @@ -536,20 +573,13 @@ public ConstantValueSpec(Value constant) { m_constant = constant; } - @Override - public boolean matches(Event e) { + public EvaluationResult evaluate(Event srcEvent) { if (m_constant.getMatches().isPresent()) { - LOG.warn("ConstantValueSpec.matches: matches not allowed for constant value."); + LOG.warn("ConstantValueSpec.evaluate: matches not allowed for constant value."); throw new IllegalStateException("Illegal to use matches with constant type values"); } - return true; - } - - - @Override - public String getResult(Event srcEvent) { - return m_constant.getResult(); + return EvaluationResult.of(m_constant.getResult()); } } @@ -557,14 +587,9 @@ public String getResult(Event srcEvent) { class ValueSpecUnspecified extends ValueSpec { @Override - public boolean matches(Event e) { + public EvaluationResult evaluate(Event srcEvent) { // TODO: this should probably throw an exception since it makes no sense - return true; - } - - @Override - public String getResult(Event srcEvent) { - return "value unspecified"; + return EvaluationResult.of("value unspecified"); } } @@ -592,130 +617,63 @@ private List constructNestedValues() { } @Override - public boolean matches(Event e) { - for (ValueSpec nestedVal : getNestedValues()) { - if (!nestedVal.matches(e)) - return false; - } - - Query query = createQuery(e); - int rowCount = query.execute(); - - if (rowCount < 1) { - LOG.info("No results found for query {}. No match.", query.reproduceStatement()); - return false; - } - - return true; - } - - private class Query { - SingleResultQuerier m_querier; - Object[] m_args; - - Query(SingleResultQuerier querier, Object[] args) { - m_querier = querier; - m_args = Arrays.copyOf(args, args.length); - } - - public int getRowCount() { - return m_querier.getCount(); - } - - public int execute() { - m_querier.execute(m_args); - return getRowCount(); - } - - public String reproduceStatement() { - return m_querier.reproduceStatement(m_args); - } - - public Object getResult() { - return m_querier.getResult(); - } - - } - - public Query createQuery(Event srcEvent) { - Object[] args = new Object[getNestedValues().size()]; - SingleResultQuerier querier = new SingleResultQuerier(m_dbConnFactory, m_val.getResult()); + public EvaluationResult evaluate(Event srcEvent) { + final List nestedValues = getNestedValues(); + final Object[] args = new Object[nestedValues.size()]; for (int i = 0; i < args.length; i++) { - args[i] = (getNestedValues().get(i)).getResult(srcEvent); + final EvaluationResult nested = nestedValues.get(i).evaluate(srcEvent); + if (!nested.matched()) { + return EvaluationResult.noMatch(); + } + args[i] = nested.value(); } - return new Query(querier, args); - } + final SingleResultQuerier querier = new SingleResultQuerier(m_dbConnFactory, m_val.getResult()); + querier.execute(args); - @Override - public String getResult(Event srcEvent) { - Query query = createQuery(srcEvent); - query.execute(); - if (query.getRowCount() < 1) { - LOG.info("No results found for query {}. Returning null", query.reproduceStatement()); - return null; - } - else { - Object result = query.getResult(); - LOG.debug("getResult: result of single result querier is: {}", result); - if (result != null) { - return result.toString(); - } else { - return null; - } + if (querier.getCount() < 1) { + LOG.info("No results found for query {}. No match.", querier.reproduceStatement(args)); + return EvaluationResult.noMatch(); } + + final Object result = querier.getResult(); + LOG.debug("evaluate: result of single result querier is: {}", result); + return EvaluationResult.of(result == null ? null : result.toString()); } } abstract class AttributeValueSpec extends ValueSpec { Value m_val; - AttributeValueSpec(Value val) { m_val = val; } - - @Override - public boolean matches(Event e) { - - String attributeValue = getAttributeValue(e); - if (attributeValue == null) { - LOG.debug("AttributeValueSpec.matches: Event attributeValue doesn't match because attributeValue itself is null"); - return false; - } - - if (!m_val.getMatches().isPresent()) { - LOG.debug("AttributeValueSpec.matches: Event attributeValue: {} matches because pattern is null", attributeValue); - return true; - } - - Pattern p = Pattern.compile(m_val.getMatches().get()); - Matcher m = p.matcher(attributeValue); + /** Compiled once at construction; null when the value has no 'matches' attribute. */ + private final Pattern m_pattern; - LOG.debug("AttributeValueSpec.matches: Event attributeValue: {} {} pattern: {}", attributeValue, (m.matches()? "matches" : "doesn't match"), m_val.getMatches()); - if (m.matches()) { - return true; - } else { - return false; - } + AttributeValueSpec(Value val) { + m_val = val; + m_pattern = val.getMatches().map(Pattern::compile).orElse(null); } @Override - public String getResult(Event srcEvent) { - if (!m_val.getMatches().isPresent()) return m_val.getResult(); - - String attributeValue = getAttributeValue(srcEvent); - + public EvaluationResult evaluate(Event srcEvent) { + final String attributeValue = getAttributeValue(srcEvent); if (attributeValue == null) { - throw new TranslationFailedException("failed to match null against '"+m_val.getMatches().get()+"' for attribute "+getAttributeName()); + LOG.debug("AttributeValueSpec.evaluate: no match because attribute {} is null", getAttributeName()); + return EvaluationResult.noMatch(); } - Pattern p = Pattern.compile(m_val.getMatches().get()); - final Matcher m = p.matcher(attributeValue); - if (!m.matches()) { - throw new TranslationFailedException("failed to match "+attributeValue+" against '"+m_val.getMatches().get()+"' for attribute "+getAttributeName()); + if (m_pattern == null) { + LOG.debug("AttributeValueSpec.evaluate: Event attributeValue: {} matches because pattern is null", attributeValue); + return EvaluationResult.of(m_val.getResult()); } - MatchTable matches = new MatchTable(m); + final Matcher m = m_pattern.matcher(attributeValue); + if (!m.matches()) { + LOG.debug("AttributeValueSpec.evaluate: Event attributeValue: {} doesn't match pattern: {}", attributeValue, m_pattern); + return EvaluationResult.noMatch(); + } - return PropertiesUtils.substitute(m_val.getResult(), matches); + LOG.debug("AttributeValueSpec.evaluate: Event attributeValue: {} matches pattern: {}", attributeValue, m_pattern); + return EvaluationResult.of(PropertiesUtils.substitute(m_val.getResult(), new MatchTable(m))); } public String getAttributeName() { return m_val.getName().orElse(null); } @@ -777,7 +735,16 @@ private BeanWrapper getBeanWrapper(Event e) { } class ParameterValueSpec extends AttributeValueSpec { - ParameterValueSpec(Value val) { super(val); } + /** A '~' prefix makes the name a regex matched against each parm name; null otherwise. */ + private final Pattern m_namePattern; + + ParameterValueSpec(Value val) { + super(val); + final String attrName = val.getName().orElse(null); + m_namePattern = (attrName != null && attrName.startsWith("~")) + ? Pattern.compile(StringUtils.removeStart(attrName, "~")) + : null; + } @Override public String getAttributeValue(Event e) { @@ -790,10 +757,8 @@ public String getAttributeValue(Event e) { return (parm.getValue() == null ? "" : parm.getValue().getContent()); } - String trimmedAttrName = StringUtils.removeStart(attrName, "~"); - - if (attrName.startsWith("~") && (parm.getParmName().matches(trimmedAttrName))) { - LOG.debug("getAttributeValue: eventParm name: '{} matches translation parameter name expression: ' {}", trimmedAttrName, parm.getParmName()); + if (m_namePattern != null && m_namePattern.matcher(parm.getParmName()).matches()) { + LOG.debug("getAttributeValue: eventParm name: '{} matches translation parameter name expression: ' {}", parm.getParmName(), m_namePattern); return (parm.getValue() == null ? "" : parm.getValue().getContent()); } } @@ -809,27 +774,8 @@ public String getAttributeValue(Event e) { * @return a {@link org.opennms.netmgt.xml.event.Event} object. */ public static Event cloneEvent(Event orig) { - Event copy = null; - try { - // Write the object out to a byte array - ByteArrayOutputStream bos = new ByteArrayOutputStream(1024); - ObjectOutputStream out = new ObjectOutputStream(bos); - out.writeObject(orig); - out.flush(); - out.close(); - - // Make an input stream from the byte array and read - // a copy of the object back in. - ObjectInputStream in = new ObjectInputStream(new ByteArrayInputStream(bos.toByteArray())); - copy = (Event)in.readObject(); - } - catch(IOException e) { - LOG.error("Exception cloning event", e); - } - catch(ClassNotFoundException cnfe) { - LOG.error("Exception cloning event", cnfe); - } - return copy; - } + // Event doesn't implement IEvent, so the deep copy has to go via the immutable model. + return Event.copyFrom(ImmutableMapper.fromMutableEvent(orig)); + } } diff --git a/opennms-config/src/test/java/org/opennms/netmgt/config/EventTranslatorCloneEventTest.java b/opennms-config/src/test/java/org/opennms/netmgt/config/EventTranslatorCloneEventTest.java new file mode 100644 index 000000000000..3ccb47313f0d --- /dev/null +++ b/opennms-config/src/test/java/org/opennms/netmgt/config/EventTranslatorCloneEventTest.java @@ -0,0 +1,194 @@ +/* + * Licensed to The OpenNMS Group, Inc (TOG) under one or more + * contributor license agreements. See the LICENSE.md file + * distributed with this work for additional information + * regarding copyright ownership. + * + * TOG licenses this file to You under the GNU Affero General + * Public License Version 3 (the "License") or (at your option) + * any later version. You may not use this file except in + * compliance with the License. You may obtain a copy of the + * License at: + * + * https://www.gnu.org/licenses/agpl-3.0.txt + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied. See the License for the specific + * language governing permissions and limitations under the + * License. + */ +package org.opennms.netmgt.config; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertNull; + +import java.util.Date; + +import org.junit.Test; +import org.opennms.core.xml.JaxbUtils; +import org.opennms.netmgt.events.api.model.ImmutableMapper; +import org.opennms.netmgt.xml.event.AlarmData; +import org.opennms.netmgt.xml.event.Autoaction; +import org.opennms.netmgt.xml.event.Correlation; +import org.opennms.netmgt.xml.event.Event; +import org.opennms.netmgt.xml.event.Logmsg; +import org.opennms.netmgt.xml.event.Parm; +import org.opennms.netmgt.xml.event.Snmp; +import org.opennms.netmgt.xml.event.Value; + +/** + * Guards the fidelity of {@link EventTranslatorConfigFactory#cloneEvent}. Marshalled XML is the + * assertion because it covers every persisted field at once. + */ +public class EventTranslatorCloneEventTest { + + @Test + public void testClonePreservesEveryFieldOfAPopulatedEvent() { + final Event original = populatedEvent(); + + final Event clone = EventTranslatorConfigFactory.cloneEvent(original); + + assertNotNull(clone); + assertNotSame(original, clone); + assertEquals(JaxbUtils.marshal(original), JaxbUtils.marshal(clone)); + } + + /** + * EventTranslator.onEvent runs Event.copyFrom before calling translateEvent, so a parm-less + * event reaches cloneEvent with its parm collection already normalized to empty rather than + * null. Cloning that shape must not perturb it either way. + */ + @Test + public void testCloneOfAParmlessEventMatchesWhatTheDaemonDelivers() { + final Event parmless = new Event(); + parmless.setUei("uei.opennms.org/test/noParms"); + parmless.setSource("test"); + parmless.setTime(new Date(1700000000000L)); + + final Event asDeliveredByOnEvent = Event.copyFrom(ImmutableMapper.fromMutableEvent(parmless)); + + final Event clone = EventTranslatorConfigFactory.cloneEvent(asDeliveredByOnEvent); + + assertNotNull(clone); + assertEquals(JaxbUtils.marshal(asDeliveredByOnEvent), JaxbUtils.marshal(clone)); + } + + /** Mutating the clone must not reach back into the source event. */ + @Test + public void testCloneIsDeepEnoughToMutateIndependently() { + final Event original = populatedEvent(); + final String originalXml = JaxbUtils.marshal(original); + + final Event clone = EventTranslatorConfigFactory.cloneEvent(original); + clone.setUei("uei.opennms.org/translated/somethingElse"); + clone.setSeverity("Critical"); + clone.getParmCollection().get(0).getValue().setContent("mutated"); + clone.getLogmsg().setContent("mutated"); + clone.getSnmp().setCommunity("mutated"); + clone.addParm(parm("extraParm", "extraValue")); + + assertEquals(originalXml, JaxbUtils.marshal(original)); + } + + /** + * translate() relies on these being clearable on the clone so eventd recomputes them from + * eventconf after translation (NMS-4038). + */ + @Test + public void testClonedFieldsThatTranslationClearsAreIndependent() { + final Event original = populatedEvent(); + + final Event clone = EventTranslatorConfigFactory.cloneEvent(original); + clone.setAlarmData(null); + clone.setSeverity(null); + clone.setDescr(null); + clone.setSnmp(null); + + assertNotNull(original.getAlarmData()); + assertNotNull(original.getSeverity()); + assertNotNull(original.getDescr()); + assertNotNull(original.getSnmp()); + } + + @Test + public void testCloneOfNullIsNull() { + assertNull(EventTranslatorConfigFactory.cloneEvent(null)); + } + + private static Event populatedEvent() { + final Event event = new Event(); + event.setUuid("6a1b0e5c-0000-0000-0000-000000000001"); + event.setDbid(1234L); + event.setDistPoller("00000000-0000-0000-0000-000000000000"); + event.setCreationTime(new Date(1700000000000L)); + event.setMasterStation("master"); + event.setUei("uei.opennms.org/generic/traps/SNMP_Link_Down"); + event.setSource("trapd"); + event.setNodeid(42L); + event.setTime(new Date(1700000000000L)); + event.setHost("router-01.example.com"); + event.setInterface("192.168.1.1"); + event.setSnmphost("192.168.1.1"); + event.setService("SNMP"); + event.setDescr("A linkDown trap was received."); + event.setSeverity("Minor"); + event.setPathoutage("192.168.1.254"); + event.setOperinstruct("Check the interface."); + event.setIfIndex(2); + event.setIfAlias("uplink-to-core"); + event.setMouseovertext("linkDown"); + + final Snmp snmp = new Snmp(); + snmp.setId(".1.3.6.1.4.1.9"); + snmp.setVersion("v2c"); + snmp.setCommunity("public"); + snmp.setGeneric(2); + snmp.setSpecific(0); + snmp.setTimeStamp(1700000000000L); + event.setSnmp(snmp); + + final Logmsg logmsg = new Logmsg(); + logmsg.setContent("A linkDown trap was received from interface 2 on node 42."); + logmsg.setDest("logndisplay"); + logmsg.setNotify(Boolean.TRUE); + event.setLogmsg(logmsg); + + final Correlation correlation = new Correlation(); + correlation.setState("on"); + correlation.setPath("pathOutage"); + event.setCorrelation(correlation); + + final Autoaction autoaction = new Autoaction(); + autoaction.setContent("echo linkDown"); + autoaction.setState("on"); + event.addAutoaction(autoaction); + + event.addLoggroup("linkEvents"); + + final AlarmData alarmData = new AlarmData(); + alarmData.setReductionKey("%uei%:%nodeid%:%parm[ifIndex]%"); + alarmData.setAlarmType(1); + alarmData.setAutoClean(false); + event.setAlarmData(alarmData); + + event.addParm(parm(".1.3.6.1.2.1.2.2.1.1.2", "2")); + event.addParm(parm(".1.3.6.1.2.1.2.2.1.7.2", "1")); + event.addParm(parm(".1.3.6.1.2.1.2.2.1.8.2", "2")); + + return event; + } + + private static Parm parm(final String name, final String content) { + final Value value = new Value(); + value.setContent(content); + + final Parm parm = new Parm(); + parm.setParmName(name); + parm.setValue(value); + return parm; + } +} diff --git a/opennms-config/src/test/java/org/opennms/netmgt/config/EventTranslatorSqlValueTest.java b/opennms-config/src/test/java/org/opennms/netmgt/config/EventTranslatorSqlValueTest.java new file mode 100644 index 000000000000..7185a7a91e92 --- /dev/null +++ b/opennms-config/src/test/java/org/opennms/netmgt/config/EventTranslatorSqlValueTest.java @@ -0,0 +1,194 @@ +/* + * Licensed to The OpenNMS Group, Inc (TOG) under one or more + * contributor license agreements. See the LICENSE.md file + * distributed with this work for additional information + * regarding copyright ownership. + * + * TOG licenses this file to You under the GNU Affero General + * Public License Version 3 (the "License") or (at your option) + * any later version. You may not use this file except in + * compliance with the License. You may obtain a copy of the + * License at: + * + * https://www.gnu.org/licenses/agpl-3.0.txt + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied. See the License for the specific + * language governing permissions and limitations under the + * License. + */ +package org.opennms.netmgt.config; + +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.io.ByteArrayInputStream; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.List; + +import javax.sql.DataSource; + +import org.junit.Before; +import org.junit.Test; +import org.opennms.netmgt.xml.event.Event; +import org.opennms.netmgt.xml.event.Parm; +import org.opennms.netmgt.xml.event.Value; + +/** + * Covers the evaluation of type="sql" translation values: how many times the + * statement is issued per event, and how a matched-but-null column differs from no match. + */ +public class EventTranslatorSqlValueTest { + + private static final String LINK_DOWN_UEI = "uei.opennms.org/generic/traps/SNMP_Link_Down"; + private static final String IF_INDEX_OID = ".1.3.6.1.2.1.2.2.1.1.2"; + + private Connection m_connection; + private DataSource m_dataSource; + + @Before + public void setUp() throws SQLException { + m_connection = mock(Connection.class); + m_dataSource = mock(DataSource.class); + when(m_dataSource.getConnection()).thenReturn(m_connection); + } + + /** Deciding whether a value matches and fetching its result must share one round trip. */ + @Test + public void testSqlValueIsQueriedOnlyOncePerEvent() throws Exception { + stubQueryResult("eth0"); + + final List translated = translate(translationConfig(null), linkDownEvent(1, 2)); + + assertEquals(1, translated.size()); + assertEquals("eth0", parmValue(translated.get(0), "ifName")); + verify(m_connection, times(1)).prepareStatement(anyString()); + } + + @Test + public void testSqlValueFindingNoRowRejectsTheMappingWithASingleQuery() throws Exception { + stubEmptyQueryResult(); + + assertEquals(0, translate(translationConfig(null), linkDownEvent(1, 2)).size()); + verify(m_connection, times(1)).prepareStatement(anyString()); + } + + @Test + public void testDefaultIsUsedWhenSqlValueFindsNoRow() throws Exception { + stubEmptyQueryResult(); + + final List translated = translate(translationConfig("unknown"), linkDownEvent(1, 2)); + + assertEquals(1, translated.size()); + assertEquals("unknown", parmValue(translated.get(0), "ifName")); + } + + /** + * A row whose column is null is still a match, so the default must not kick in. Collapsing the + * evaluation result into an Optional would silently turn this case into the default. + */ + @Test + public void testNullColumnIsAMatchRatherThanAFallbackToTheDefault() throws Exception { + stubQueryResult(null); + + final List translated = translate(translationConfig("unknown"), linkDownEvent(1, 2)); + + assertEquals(1, translated.size()); + assertEquals("", parmValue(translated.get(0), "ifName")); + } + + @Test + public void testEventWithoutTheExpectedParmDoesNotReachTheDatabase() throws Exception { + final Event event = linkDownEvent(1, 2); + event.getParmCollection().get(0).setParmName("someOtherParm"); + + assertEquals(0, translate(translationConfig(null), event).size()); + verify(m_connection, times(0)).prepareStatement(anyString()); + } + + private List translate(final String config, final Event event) throws Exception { + final InputStream stream = new ByteArrayInputStream(config.getBytes(StandardCharsets.UTF_8)); + return new EventTranslatorConfigFactory(stream, m_dataSource).translateEvent(event); + } + + private void stubQueryResult(final Object columnValue) throws SQLException { + stubStatement(true, columnValue); + } + + private void stubEmptyQueryResult() throws SQLException { + stubStatement(false, null); + } + + /** + * Hands out a fresh ResultSet per execution, so re-running the query yields the same rows. + * Query counts are therefore asserted by verifying prepareStatement, not inferred from a + * result set that happens to be exhausted. + */ + private void stubStatement(final boolean hasRow, final Object columnValue) throws SQLException { + final PreparedStatement statement = mock(PreparedStatement.class); + when(statement.executeQuery()).thenAnswer(invocation -> { + final ResultSet resultSet = mock(ResultSet.class); + when(resultSet.next()).thenReturn(hasRow, false); + when(resultSet.getObject(1)).thenReturn(columnValue); + return resultSet; + }); + when(m_connection.prepareStatement(anyString())).thenReturn(statement); + } + + private static String parmValue(final Event event, final String parmName) { + for (final Parm parm : event.getParmCollection()) { + if (parmName.equals(parm.getParmName())) { + return parm.getValue() == null ? null : parm.getValue().getContent(); + } + } + return null; + } + + private static Event linkDownEvent(final long nodeId, final int ifIndex) { + final Value value = new Value(); + value.setContent(String.valueOf(ifIndex)); + + final Parm parm = new Parm(); + parm.setParmName(IF_INDEX_OID); + parm.setValue(value); + + final Event event = new Event(); + event.setUei(LINK_DOWN_UEI); + event.setNodeid(nodeId); + event.addParm(parm); + return event; + } + + /** Mirrors the shipped link-down translation: one sql value fed by a field and a '~' regex parm. */ + private static String translationConfig(final String ifNameDefault) { + return "\n" + + "\n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + "\n"; + } +} From 1c9c81edc35b708a7412650df2470f4dabed5c66 Mon Sep 17 00:00:00 2001 From: Marshall Massengill Date: Wed, 12 Aug 2026 15:41:18 -0400 Subject: [PATCH 2/2] NMS-20204: Address review findings in the event translator Read m_translationSpecs into a local before the null check. Returning the field directly hands back null when update() clears it in between, and translateEvent() iterates the result. Two threads racing a reload may now each construct a list, which is cheaper than putting a lock on the per-event path. Fix the misplaced quotes in the two getAttributeValue() debug messages. --- .../config/EventTranslatorConfigFactory.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/opennms-config/src/main/java/org/opennms/netmgt/config/EventTranslatorConfigFactory.java b/opennms-config/src/main/java/org/opennms/netmgt/config/EventTranslatorConfigFactory.java index 51aefb14b063..421cb0cfac52 100644 --- a/opennms-config/src/main/java/org/opennms/netmgt/config/EventTranslatorConfigFactory.java +++ b/opennms-config/src/main/java/org/opennms/netmgt/config/EventTranslatorConfigFactory.java @@ -283,10 +283,15 @@ public List translateEvent(Event e) { } private List getTranslationSpecs() { - if (m_translationSpecs == null) - m_translationSpecs = constructTranslationSpecs(); + // Read the field once. Returning it directly would hand back null when update() + // clears it after the check, and translateEvent() iterates the result. + List specs = m_translationSpecs; + if (specs == null) { + specs = constructTranslationSpecs(); + m_translationSpecs = specs; + } - return m_translationSpecs; + return specs; } private List constructTranslationSpecs() { @@ -753,12 +758,12 @@ public String getAttributeValue(Event e) { for (Parm parm : e.getParmCollection()) { if (parm.getParmName().equals(attrName)) { - LOG.debug("getAttributeValue: eventParm name: '{} equals translation parameter name: ' {}", attrName, parm.getParmName()); + LOG.debug("getAttributeValue: eventParm name: '{}' equals translation parameter name: '{}'", attrName, parm.getParmName()); return (parm.getValue() == null ? "" : parm.getValue().getContent()); } if (m_namePattern != null && m_namePattern.matcher(parm.getParmName()).matches()) { - LOG.debug("getAttributeValue: eventParm name: '{} matches translation parameter name expression: ' {}", parm.getParmName(), m_namePattern); + LOG.debug("getAttributeValue: eventParm name: '{}' matches translation parameter name expression: '{}'", parm.getParmName(), m_namePattern); return (parm.getValue() == null ? "" : parm.getValue().getContent()); } }