You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/05/20 13:22:23 UTC
[GitHub] [nifi] nandorsoma commented on a diff in pull request #6046: NIFI-10019: SendTrapSNMP works without flowfile, upgraded to JUnit5
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org