[GitHub] [nifi] turcsanyip commented on a diff in pull request #7246: NIFI-11535: Transfer ConnectWebsocket HTTP header flowfile to relatio…

["turcsanyip (via GitHub)" <gi...@apache.org>]

turcsanyip commented on code in PR #7246:
URL: https://github.com/apache/nifi/pull/7246#discussion_r1194007689


##########
nifi-nar-bundles/nifi-websocket-bundle/nifi-websocket-processors/src/main/java/org/apache/nifi/processors/websocket/AbstractWebSocketGatewayProcessor.java:
##########
@@ -77,6 +77,18 @@ public abstract class AbstractWebSocketGatewayProcessor extends AbstractSessionF
             .description("The WebSocket binary message output")
             .build();
 
+    public static final Relationship REL_SUCCESS = new Relationship.Builder()
+            .name("success")
+            .description("Connection HTTP header attributes in case of successful connection")
+            .autoTerminateDefault(true)
+            .build();
+
+    public static final Relationship REL_FAILURE = new Relationship.Builder()
+            .name("failure")
+            .description("Connection HTTP header attributes in case of connection failure")
+            .autoTerminateDefault(true)
+            .build();
+

Review Comment:
   "Connection HTTP header attributes" is a bit unclear in this context and other config parameters can be passed also. I would suggest the following descriptions:
   ```suggestion
       public static final Relationship REL_SUCCESS = new Relationship.Builder()
               .name("success")
               .description("FlowFile holding connection configuration attributes (like URL or HTTP headers) in case of successful connection")
               .autoTerminateDefault(true)
               .build();
   
       public static final Relationship REL_FAILURE = new Relationship.Builder()
               .name("failure")
               .description("FlowFile holding connection configuration attributes (like URL or HTTP headers) in case of connection failure")
               .autoTerminateDefault(true)
               .build();
   
   ```



##########
nifi-nar-bundles/nifi-websocket-bundle/nifi-websocket-processors/src/test/java/org/apache/nifi/processors/websocket/TestConnectWebSocket.java:
##########
@@ -162,10 +154,44 @@ void testDynamicUrlsParsedFromFlowFileAndAbleToConnect() throws InitializationEx
         final List<MockFlowFile> flowFilesForRelationship = runner.getFlowFilesForRelationship(ConnectWebSocket.REL_CONNECTED);
         assertEquals(1, flowFilesForRelationship.size());
 
+        final List<MockFlowFile> flowFilesForSuccess = runner.getFlowFilesForRelationship(ConnectWebSocket.REL_SUCCESS);
+        assertEquals(1, flowFilesForSuccess.size());
+
         runner.stop();
         webSocketListener.stop();
     }
 
+    @Test
+    void testDynamicUrlsTransferredToFailure() throws InitializationException {

Review Comment:
   The following test name would describe the test scenario more straightforward:
   ```suggestion
       void testDynamicUrlsParsedFromFlowFileButNotAbleToConnect() throws InitializationException {
   ```



##########
nifi-nar-bundles/nifi-websocket-bundle/nifi-websocket-processors/src/test/java/org/apache/nifi/processors/websocket/TestConnectWebSocket.java:
##########
@@ -162,10 +154,44 @@ void testDynamicUrlsParsedFromFlowFileAndAbleToConnect() throws InitializationEx
         final List<MockFlowFile> flowFilesForRelationship = runner.getFlowFilesForRelationship(ConnectWebSocket.REL_CONNECTED);
         assertEquals(1, flowFilesForRelationship.size());
 
+        final List<MockFlowFile> flowFilesForSuccess = runner.getFlowFilesForRelationship(ConnectWebSocket.REL_SUCCESS);
+        assertEquals(1, flowFilesForSuccess.size());
+
         runner.stop();
         webSocketListener.stop();
     }
 
+    @Test
+    void testDynamicUrlsTransferredToFailure() throws InitializationException {
+        final TestRunner runner = TestRunners.newTestRunner(ConnectWebSocket.class);
+
+        final String serviceId = "ws-service";
+        final String endpointId = "client-1";
+
+        MockFlowFile flowFile = getFlowFile();
+        runner.enqueue(flowFile);
+
+        JettyWebSocketClient service = new JettyWebSocketClient();
+
+
+        runner.addControllerService(serviceId, service);
+        runner.setProperty(service, JettyWebSocketClient.WS_URI, "ws://localhost/12345");

Review Comment:
   No need to set a fake URL because the failure comes due to ListenWebSocket not started.
   ```suggestion
           runner.setProperty(service, JettyWebSocketClient.WS_URI, "ws://localhost/${dynamicUrlPart}");
   ```



-- 
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