nandorsoma commented on code in PR #6046:
URL: https://github.com/apache/nifi/pull/6046#discussion_r878108907
##########
nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/processors/SendTrapSNMP.java:
##########
@@ -120,32 +123,53 @@ public void init(ProcessContext context) {
@Override
public void onTrigger(final ProcessContext context, final ProcessSession
processSession) {
final FlowFile flowFile = processSession.get();
- if (flowFile != null) {
- try {
- final int snmpVersion =
SNMPUtils.getVersion(context.getProperty(BasicProperties.SNMP_VERSION).getValue());
- if (SnmpConstants.version1 == snmpVersion) {
- V1TrapConfiguration v1TrapConfiguration =
V1TrapConfiguration.builder()
-
.enterpriseOid(context.getProperty(V1TrapProperties.ENTERPRISE_OID).evaluateAttributeExpressions(flowFile).getValue())
-
.agentAddress(context.getProperty(V1TrapProperties.AGENT_ADDRESS).evaluateAttributeExpressions(flowFile).getValue())
-
.genericTrapType(context.getProperty(V1TrapProperties.GENERIC_TRAP_TYPE).evaluateAttributeExpressions(flowFile).getValue())
-
.specificTrapType(context.getProperty(V1TrapProperties.SPECIFIC_TRAP_TYPE).evaluateAttributeExpressions(flowFile).getValue())
- .build();
- snmpHandler.sendTrap(flowFile.getAttributes(),
v1TrapConfiguration);
- } else {
- V2TrapConfiguration v2TrapConfiguration = new
V2TrapConfiguration(
-
context.getProperty(V2TrapProperties.TRAP_OID_VALUE).evaluateAttributeExpressions(flowFile).getValue()
- );
- snmpHandler.sendTrap(flowFile.getAttributes(),
v2TrapConfiguration);
- }
+ final Map<String, String> attributes = new HashMap<>(
+ Optional.ofNullable(flowFile)
+ .map(FlowFile::getAttributes)
+ .orElse(Collections.emptyMap())
+ );
+
+ try {
+ final int snmpVersion =
SNMPUtils.getVersion(context.getProperty(BasicProperties.SNMP_VERSION).getValue());
+ if (SnmpConstants.version1 == snmpVersion) {
+
+ final String enterpriseOid =
context.getProperty(V1TrapProperties.ENTERPRISE_OID).evaluateAttributeExpressions(flowFile).getValue();
+ final String agentAddress =
context.getProperty(V1TrapProperties.AGENT_ADDRESS).evaluateAttributeExpressions(flowFile).getValue();
+ final String genericTrapType =
context.getProperty(V1TrapProperties.GENERIC_TRAP_TYPE).evaluateAttributeExpressions(flowFile).getValue();
+ final String specificTrapType =
context.getProperty(V1TrapProperties.SPECIFIC_TRAP_TYPE).evaluateAttributeExpressions(flowFile).getValue();
+ V1TrapConfiguration v1TrapConfiguration =
V1TrapConfiguration.builder()
+ .enterpriseOid(enterpriseOid)
+ .agentAddress(agentAddress)
+ .genericTrapType(genericTrapType)
+ .specificTrapType(specificTrapType)
+ .build();
+ attributes.put("agentAddress", agentAddress);
+ attributes.put("enterpriseOid", enterpriseOid);
+ attributes.put("genericTrapType", genericTrapType);
+ attributes.put("specificTrapType", specificTrapType);
+ snmpHandler.sendTrap(attributes, v1TrapConfiguration);
+ } else {
+ final String trapOidValue =
context.getProperty(V2TrapProperties.TRAP_OID_VALUE).evaluateAttributeExpressions(flowFile).getValue();
+ V2TrapConfiguration v2TrapConfiguration = new
V2TrapConfiguration(trapOidValue);
+ attributes.put("trapOidValue", trapOidValue);
+ snmpHandler.sendTrap(attributes, v2TrapConfiguration);
+ }
+ if (flowFile == null) {
+ FlowFile outgoingFlowFile = processSession.create();
+ processSession.putAllAttributes(outgoingFlowFile, attributes);
+ processSession.transfer(outgoingFlowFile, REL_SUCCESS);
+ } else {
+ processSession.putAllAttributes(flowFile, attributes);
processSession.transfer(flowFile, REL_SUCCESS);
- } catch (IOException e) {
- getLogger().error("Failed to send request to the agent. Check
if the agent supports the used version.", e);
- processSession.transfer(processSession.penalize(flowFile),
REL_FAILURE);
- } catch (IllegalArgumentException e) {
- getLogger().error("Invalid trap configuration.", e);
- processSession.transfer(processSession.penalize(flowFile),
REL_FAILURE);
}
+ } catch (IOException e) {
+ getLogger().error("Failed to send request to the agent. Check if
the agent supports the used version.", e);
Review Comment:
I was able to send a request to a non existent port and the flowfile was
routed to success. Are we sure that execution jumps to this line if it failed
to send the request? Or... Can it be that the processor fails when it uses
incorrect protocol version because it gets a response about that, but it
doesn't do anything when it doesn't receive any response which could indicate a
more severe error.
##########
nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/test/java/org/apache/nifi/snmp/processors/GetSNMPIT.java:
##########
@@ -73,58 +69,47 @@ public class GetSNMPIT {
registerManagedObjects(v3TestAgent);
}
- @Parameterized.Parameters
- public static Collection<Object[]> data() {
- return Arrays.asList(new Object[][]{
- {v1TestAgent, v1TestRunnerFactory},
- {v2cTestAgent, v2cTestRunnerFactory},
- {v3TestAgent, v3TestRunnerFactory}
- });
+ private static Stream<Arguments> provideArguments() {
+ return Stream.of(
+ Arguments.of(v1TestAgent, v1TestRunnerFactory),
+ Arguments.of(v2cTestAgent, v2cTestRunnerFactory),
+ Arguments.of(v3TestAgent, v3TestRunnerFactory)
+ );
}
- private final TestAgent testAgent;
- private final SNMPTestRunnerFactory testRunnerFactory;
-
- public GetSNMPIT(final TestAgent testAgent, final SNMPTestRunnerFactory
testRunnerFactory) {
- this.testAgent = testAgent;
- this.testRunnerFactory = testRunnerFactory;
- }
-
- @Before
- public void setUp() throws IOException {
+ @ParameterizedTest
+ @MethodSource("provideArguments")
+ void testSnmpGet(TestAgent testAgent, SNMPTestRunnerFactory
testRunnerFactory) throws IOException {
testAgent.start();
- }
-
- @After
- public void tearDown() {
- testAgent.stop();
- testAgent.unregister();
- }
-
- @Test
- public void testSnmpGet() {
-
final TestRunner runner =
testRunnerFactory.createSnmpGetTestRunner(testAgent.getPort(), READ_ONLY_OID_1,
GET);
runner.run();
final MockFlowFile successFF =
runner.getFlowFilesForRelationship(GetSNMP.REL_SUCCESS).get(0);
assertNotNull(successFF);
assertEquals(READ_ONLY_OID_VALUE_1,
successFF.getAttribute(SNMPUtils.SNMP_PROP_PREFIX + READ_ONLY_OID_1 +
SNMPUtils.SNMP_PROP_DELIMITER + "4"));
+ testAgent.stop();
Review Comment:
Same what I wrote at SNMPRequestIT.
##########
nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/test/java/org/apache/nifi/snmp/processors/SetSNMPIT.java:
##########
@@ -65,43 +61,26 @@ public class SetSNMPIT {
registerManagedObjects(v3TestAgent);
}
- @Parameterized.Parameters
- public static Collection<Object[]> data() {
- return Arrays.asList(new Object[][]{
- {v1TestAgent, v1TestRunnerFactory},
- {v2cTestAgent, v2cTestRunnerFactory},
- {v3TestAgent, v3TestRunnerFactory}
- });
- }
-
- private final TestAgent testAgent;
- private final SNMPTestRunnerFactory testRunnerFactory;
-
- public SetSNMPIT(final TestAgent testAgent, final SNMPTestRunnerFactory
testRunnerFactory) {
- this.testAgent = testAgent;
- this.testRunnerFactory = testRunnerFactory;
+ private static Stream<Arguments> provideArguments() {
+ return Stream.of(
+ Arguments.of(v1TestAgent, v1TestRunnerFactory),
+ Arguments.of(v2cTestAgent, v2cTestRunnerFactory),
+ Arguments.of(v3TestAgent, v3TestRunnerFactory)
+ );
}
- @Before
- public void setUp() throws IOException {
+ @ParameterizedTest
+ @MethodSource("provideArguments")
+ void testSnmpSet(TestAgent testAgent, SNMPTestRunnerFactory
testRunnerFactory) throws IOException {
testAgent.start();
- }
-
- @After
- public void tearDown() {
- testAgent.stop();
- testAgent.unregister();
- }
-
-
- @Test
- public void testSnmpSet() {
final TestRunner runner =
testRunnerFactory.createSnmpSetTestRunner(testAgent.getPort(), TEST_OID,
TEST_OID_VALUE);
runner.run();
final MockFlowFile successFF =
runner.getFlowFilesForRelationship(SetSNMP.REL_SUCCESS).get(0);
assertNotNull(successFF);
assertEquals(TEST_OID_VALUE,
successFF.getAttribute(SNMPUtils.SNMP_PROP_PREFIX + TEST_OID +
SNMPUtils.SNMP_PROP_DELIMITER + "4"));
+ testAgent.stop();
Review Comment:
Same what I wrote at SNMPRequestIT.
##########
nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/test/java/org/apache/nifi/snmp/operations/SNMPRequestIT.java:
##########
@@ -102,60 +101,69 @@ public class SNMPRequestIT {
registerManagedObjects(v3TestAgent);
}
- @Before
- public void initAgent() throws IOException {
- agent.start();
- }
-
- @After
+ @AfterEach
public void tearDown() {
- agent.stop();
- agent.unregister();
snmpResourceHandler.close();
}
- @Parameterized.Parameters
- public static Collection<Object[]> data() {
- return Arrays.asList(new Object[][]{
- {SnmpConstants.version1, snmpV1ConfigurationFactory,
v1TestAgent, NO_SUCH_NAME, NO_SUCH_NAME, NO_SUCH_NAME, NO_SUCH_NAME},
- {SnmpConstants.version2c, snmpv2cConfigurationFactory,
v2cTestAgent, NOT_WRITABLE, NO_ACCESS, NO_SUCH_OBJECT, UNABLE_TO_CREATE_OBJECT},
- {SnmpConstants.version3, snmpv3ConfigurationFactory,
v3TestAgent, NOT_WRITABLE, NO_ACCESS, NO_SUCH_OBJECT, UNABLE_TO_CREATE_OBJECT}
- });
+ private static Stream<Arguments> provideBasicArguments() {
+ return Stream.of(
+ Arguments.of(SnmpConstants.version1,
snmpV1ConfigurationFactory, v1TestAgent),
+ Arguments.of(SnmpConstants.version2c,
snmpv2cConfigurationFactory, v2cTestAgent),
+ Arguments.of(SnmpConstants.version3,
snmpv3ConfigurationFactory, v3TestAgent)
+ );
+ }
+
+ private static Stream<Arguments> provideCannotSetReadOnlyOidArguments() {
+ return Stream.of(
+ Arguments.of(SnmpConstants.version1,
snmpV1ConfigurationFactory, v1TestAgent, NO_SUCH_NAME),
+ Arguments.of(SnmpConstants.version2c,
snmpv2cConfigurationFactory, v2cTestAgent, NOT_WRITABLE),
+ Arguments.of(SnmpConstants.version3,
snmpv3ConfigurationFactory, v3TestAgent, NOT_WRITABLE)
+ );
+ }
+
+ private static Stream<Arguments>
provideCannotModifyOidStatusMessageArguments() {
+ return Stream.of(
+ Arguments.of(SnmpConstants.version1,
snmpV1ConfigurationFactory, v1TestAgent, NO_SUCH_NAME),
+ Arguments.of(SnmpConstants.version2c,
snmpv2cConfigurationFactory, v2cTestAgent, NO_ACCESS),
+ Arguments.of(SnmpConstants.version3,
snmpv3ConfigurationFactory, v3TestAgent, NO_ACCESS)
+ );
}
- private final int version;
- private final SNMPConfigurationFactory snmpConfigurationFactory;
- private final TestAgent agent;
- private final String cannotSetReadOnlyOidStatusMessage;
- private final String cannotModifyOidStatusMessage;
- private final String getInvalidOidStatusMessage;
- private final String setInvalidOidStatusMessage;
-
- public SNMPRequestIT(final int version, final SNMPConfigurationFactory
snmpConfigurationFactory, final TestAgent agent,
- final String cannotSetReadOnlyOidStatusMessage, final
String cannotModifyOidStatusMessage,
- final String getInvalidOidStatusMessage, final String
setInvalidOidStatusMessage) {
- this.version = version;
- this.snmpConfigurationFactory = snmpConfigurationFactory;
- this.agent = agent;
- this.cannotSetReadOnlyOidStatusMessage =
cannotSetReadOnlyOidStatusMessage;
- this.cannotModifyOidStatusMessage = cannotModifyOidStatusMessage;
- this.getInvalidOidStatusMessage = getInvalidOidStatusMessage;
- this.setInvalidOidStatusMessage = setInvalidOidStatusMessage;
+ private static Stream<Arguments>
provideGetInvalidOidStatusMessageArguments() {
+ return Stream.of(
+ Arguments.of(SnmpConstants.version1,
snmpV1ConfigurationFactory, v1TestAgent, NO_SUCH_NAME),
+ Arguments.of(SnmpConstants.version2c,
snmpv2cConfigurationFactory, v2cTestAgent, NO_SUCH_OBJECT),
+ Arguments.of(SnmpConstants.version3,
snmpv3ConfigurationFactory, v3TestAgent, NO_SUCH_OBJECT)
+ );
+ }
+
+ private static Stream<Arguments>
provideSetInvalidOidStatusMessageArguments() {
+ return Stream.of(
+ Arguments.of(SnmpConstants.version1,
snmpV1ConfigurationFactory, v1TestAgent, NO_SUCH_NAME),
+ Arguments.of(SnmpConstants.version2c,
snmpv2cConfigurationFactory, v2cTestAgent, UNABLE_TO_CREATE_OBJECT),
+ Arguments.of(SnmpConstants.version3,
snmpv3ConfigurationFactory, v3TestAgent, UNABLE_TO_CREATE_OBJECT)
+ );
}
- @Test
- public void testSuccessfulSnmpGet() throws IOException {
+ @ParameterizedTest
+ @MethodSource("provideBasicArguments")
+ void testSuccessfulSnmpGet(int version, SNMPConfigurationFactory
snmpConfigurationFactory, TestAgent agent) throws IOException {
+ agent.start();
final SNMPConfiguration snmpConfiguration =
snmpConfigurationFactory.createSnmpGetSetConfiguration(agent.getPort());
snmpResourceHandler =
SNMPFactoryProvider.getFactory(version).createSNMPResourceHandler(snmpConfiguration);
final GetSNMPHandler getSNMPHandler = new
GetSNMPHandler(snmpResourceHandler);
final SNMPSingleResponse response =
getSNMPHandler.get(READ_ONLY_OID_1);
assertEquals(READ_ONLY_OID_VALUE_1,
response.getVariableBindings().get(0).getVariable());
assertEquals(SUCCESS, response.getErrorStatusText());
-
+ agent.stop();
Review Comment:
With the original version it was guaranteed that the agent will be stopped
and unregistered when an error happens. With the current version I think this
won't happen.
##########
nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/test/java/org/apache/nifi/snmp/configuration/V1TrapConfigurationTest.java:
##########
@@ -49,29 +44,32 @@ public void testMembersAreSetCorrectly() {
}
@Test
- public void testRequireNonNullEnterpriseOid() {
- exceptionRule.expect(IllegalArgumentException.class);
- exceptionRule.expectMessage("Enterprise OID must be specified.");
- V1TrapConfiguration.builder()
- .agentAddress(AGENT_ADDRESS)
- .genericTrapType(String.valueOf(GENERIC_TRAP_TYPE))
- .specificTrapType(String.valueOf(SPECIFIC_TRAP_TYPE))
- .build();
+ void testRequireNonNullEnterpriseOid() {
+ final IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, () ->
+ V1TrapConfiguration.builder()
+ .agentAddress(AGENT_ADDRESS)
+ .genericTrapType(String.valueOf(GENERIC_TRAP_TYPE))
+ .specificTrapType(String.valueOf(SPECIFIC_TRAP_TYPE))
+ .build()
+ );
+ assertEquals("Enterprise OID must be specified.",
exception.getMessage());
Review Comment:
Could you extract the exception message to a static final variable in the
configuration class? This way this message can be deduplicated which leads to a
more robust testing. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
