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/06/23 10:12:32 UTC
[GitHub] [nifi] bejancsaba opened a new pull request, #6149: NIFI-9908 C2 refactor and test coverage
bejancsaba opened a new pull request, #6149:
URL: https://github.com/apache/nifi/pull/6149
<!-- Licensed to the Apache Software Foundation (ASF) under one or more -->
<!-- contributor license agreements. See the NOTICE file distributed with -->
<!-- this work for additional information regarding copyright ownership. -->
<!-- The ASF licenses this file to You under the Apache License, Version 2.0 -->
<!-- (the "License"); you may not use this file except in compliance with -->
<!-- the License. You may obtain a copy of the License at -->
<!-- http://www.apache.org/licenses/LICENSE-2.0 -->
<!-- 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. -->
# Summary
[NIFI-9908](https://issues.apache.org/jira/browse/NIFI-9908) After the C2 change was merged a few opportunities for improvement were identified along with proper test coverage added for the essential classes in the C2 module. If needed there is room for extending the coverage but for now this should cover the critical parts.
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [ ] Pull Request based on current revision of the `main` branch
- [x] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [x] Build completed using `mvn clean install -P contrib-check`
- [x] JDK 8
- [ ] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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
[GitHub] [nifi] bejancsaba commented on a diff in pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
bejancsaba commented on code in PR #6149:
URL: https://github.com/apache/nifi/pull/6149#discussion_r906717336
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/operation/UpdateConfigurationOperationHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service.operation;
+
+import static org.apache.commons.lang3.StringUtils.EMPTY;
+import static org.apache.nifi.c2.client.service.operation.UpdateConfigurationOperationHandler.LOCATION;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Optional;
+import java.util.function.Function;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.FlowIdHolder;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.apache.nifi.c2.protocol.api.C2OperationState;
+import org.apache.nifi.c2.protocol.api.OperandType;
+import org.apache.nifi.c2.protocol.api.OperationType;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class UpdateConfigurationOperationHandlerTest {
+
+ private static final String FLOW_ID = "flowId";
+ private static final String OPERATION_ID = "operationId";
+ private static final Map<String, String> CORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "/path/for/the/" + FLOW_ID);
+ private static final Map<String, String> INCORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "incorrect/location");
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private FlowIdHolder flowIdHolder;
+
+ @Test
+ void testUpdateConfigurationOperationHandlerCreateSuccess() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+
+ assertEquals(OperationType.UPDATE, handler.getOperationType());
+ assertEquals(OperandType.CONFIGURATION, handler.getOperandType());
+ }
+
+ @Test
+ void testHandleThrowsExceptionForIncorrectArg() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+ C2Operation operation = new C2Operation();
+ operation.setArgs(INCORRECT_LOCATION_MAP);
+
+ IllegalStateException exception = assertThrows(IllegalStateException.class, () -> handler.handle(operation));
+
+ assertEquals("Could not get flow id from the provided URL", exception.getMessage());
Review Comment:
As we are already matching the thrown exception I removed this assertion it made it more fragile with not much added value.
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/operation/UpdateConfigurationOperationHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service.operation;
+
+import static org.apache.commons.lang3.StringUtils.EMPTY;
+import static org.apache.nifi.c2.client.service.operation.UpdateConfigurationOperationHandler.LOCATION;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Optional;
+import java.util.function.Function;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.FlowIdHolder;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.apache.nifi.c2.protocol.api.C2OperationState;
+import org.apache.nifi.c2.protocol.api.OperandType;
+import org.apache.nifi.c2.protocol.api.OperationType;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class UpdateConfigurationOperationHandlerTest {
+
+ private static final String FLOW_ID = "flowId";
+ private static final String OPERATION_ID = "operationId";
+ private static final Map<String, String> CORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "/path/for/the/" + FLOW_ID);
+ private static final Map<String, String> INCORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "incorrect/location");
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private FlowIdHolder flowIdHolder;
+
+ @Test
+ void testUpdateConfigurationOperationHandlerCreateSuccess() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+
+ assertEquals(OperationType.UPDATE, handler.getOperationType());
+ assertEquals(OperandType.CONFIGURATION, handler.getOperandType());
+ }
+
+ @Test
+ void testHandleThrowsExceptionForIncorrectArg() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+ C2Operation operation = new C2Operation();
+ operation.setArgs(INCORRECT_LOCATION_MAP);
+
+ IllegalStateException exception = assertThrows(IllegalStateException.class, () -> handler.handle(operation));
+
+ assertEquals("Could not get flow id from the provided URL", exception.getMessage());
+ }
+
+ @Test
+ void testHandleReturnsNotAppliedWithNoContent() {
+ when(flowIdHolder.getFlowId()).thenReturn("dummy");
Review Comment:
applied
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6149:
URL: https://github.com/apache/nifi/pull/6149#discussion_r906383862
##########
c2/c2-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2HttpClient.java:
##########
@@ -91,6 +92,45 @@ public Optional<C2HeartbeatResponse> publishHeartbeat(C2Heartbeat heartbeat) {
return serializer.serialize(heartbeat).flatMap(this::sendHeartbeat);
}
+ @Override
+ public Optional<byte[]> retrieveUpdateContent(String flowUpdateUrl) {
+ final Request.Builder requestBuilder = new Request.Builder()
+ .get()
+ .url(flowUpdateUrl);
+ final Request request = requestBuilder.build();
+
+ Optional<ResponseBody> body;
+ try (Response response = httpClientReference.get().newCall(request).execute()) {
+ int code = response.code();
+ body = Optional.ofNullable(response.body());
+
+ if (code >= HTTP_STATUS_BAD_REQUEST) {
+ StringBuilder messageBuilder = new StringBuilder(String.format("Configuration retrieval failed: HTTP %d", code));
+ body.map(Object::toString).ifPresent(messageBuilder::append);
+ throw new C2ServerException(messageBuilder.toString());
+ }
+
+ if (!body.isPresent()) {
+ logger.warn("No body returned when pulling a new configuration");
+ return Optional.empty();
+ }
+
+ return Optional.of(body.get().bytes());
+ } catch (Exception e) {
+ logger.warn("Configuration retrieval failed", e);
+ return Optional.empty();
+ }
+ }
+
+ @Override
+ public void acknowledgeOperation(C2OperationAck operationAck) {
+ logger.info("Performing acknowledgement request to {} for operation {}", clientConfig.getC2AckUrl(), operationAck.getOperationId());
Review Comment:
Thanks, that sounds good.
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6149:
URL: https://github.com/apache/nifi/pull/6149#discussion_r906384862
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.matching;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.apache.nifi.c2.client.http.C2HttpClient.HTTP_STATUS_BAD_REQUEST;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.junit5.WireMockTest;
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.client.api.C2Serializer;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@WireMockTest
+@ExtendWith(MockitoExtension.class)
+public class C2HttpClientTest {
+
+ private static final String HEARTBEAT_PATH = "/c2/heartbeat";
+ private static final String UPDATE_PATH = "/c2/update";
+ private static final String ACK_PATH = "/c2/acknowledge";
+ private static final int HTTP_STATUS_OK = 200;
+
+ @Mock
+ private C2ClientConfig c2ClientConfig;
+
+ @Mock
+ private C2Serializer serializer;
+
+ @InjectMocks
+ private C2HttpClient c2HttpClient;
+
+ @Test
+ void shouldReturnC2HeartbeatResponse(WireMockRuntimeInfo wmRuntimeInfo) {
+ // given
Review Comment:
Thanks for making the adjustments.
--
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
[GitHub] [nifi] bejancsaba commented on a diff in pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
bejancsaba commented on code in PR #6149:
URL: https://github.com/apache/nifi/pull/6149#discussion_r906327336
##########
c2/c2-client-bundle/c2-client-http/pom.xml:
##########
@@ -47,5 +47,32 @@ limitations under the License.
<groupId>com.squareup.okhttp3</groupId>
<artifactId>logging-interceptor</artifactId>
</dependency>
+
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-engine</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-junit-jupiter</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.github.tomakehurst</groupId>
+ <artifactId>wiremock-jre8</artifactId>
+ <version>2.33.2</version>
+ <scope>test</scope>
+ </dependency>
Review Comment:
Thanks moved to MockWebServer
--
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
[GitHub] [nifi] exceptionfactory closed pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
exceptionfactory closed pull request #6149: NIFI-9908 C2 refactor and test coverage
URL: https://github.com/apache/nifi/pull/6149
--
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
[GitHub] [nifi] bejancsaba commented on a diff in pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
bejancsaba commented on code in PR #6149:
URL: https://github.com/apache/nifi/pull/6149#discussion_r906717272
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/C2ClientServiceTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service;
+
+import static org.mockito.ArgumentMatchers.any;
+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.util.List;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.model.RuntimeInfoWrapper;
+import org.apache.nifi.c2.client.service.operation.C2OperationService;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class C2ClientServiceTest {
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private C2HeartbeatFactory c2HeartbeatFactory;
+
+ @Mock
+ private C2OperationService operationService;
+
+ @InjectMocks
+ private C2ClientService c2ClientService;
+
+ @Test
+ void testSendHeartbeatAndAckWhenOperationPresent() {
+ C2Heartbeat heartbeat = mock(C2Heartbeat.class);
+ when(c2HeartbeatFactory.create(any())).thenReturn(heartbeat);
+ C2HeartbeatResponse hbResponse = new C2HeartbeatResponse();
+ hbResponse.setRequestedOperations(generateOperation(1));
+ when(client.publishHeartbeat(heartbeat)).thenReturn(Optional.of(hbResponse));
+ when(operationService.handleOperation(any())).thenReturn(Optional.of(new C2OperationAck()));
+
+ c2ClientService.sendHeartbeat(mock(RuntimeInfoWrapper.class));
+
+ verify(c2HeartbeatFactory, times(1)).create(any());
Review Comment:
As there are mocks in this class which expect different times than 1 I thought it helps readability and consistency (even if it is more verbose) to explicitly state even the 1-s, however this is subjective so I removed as requested.
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6149:
URL: https://github.com/apache/nifi/pull/6149#discussion_r904997394
##########
c2/c2-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2ServerException.java:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import java.io.IOException;
+
Review Comment:
It would be helpful to add a class-level comment describing the basic purpose of this exception class.
##########
c2/c2-client-bundle/c2-client-http/pom.xml:
##########
@@ -47,5 +47,32 @@ limitations under the License.
<groupId>com.squareup.okhttp3</groupId>
<artifactId>logging-interceptor</artifactId>
</dependency>
+
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-engine</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-junit-jupiter</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.github.tomakehurst</groupId>
+ <artifactId>wiremock-jre8</artifactId>
+ <version>2.33.2</version>
+ <scope>test</scope>
+ </dependency>
Review Comment:
Instead of introducing a new dependency on WireMock, and the Java 8 dependency, recommend using OkHttp [MockWebServer](https://github.com/square/okhttp/tree/master/mockwebserver), which is already used for testing `InvokeHTTP` and `HttpNotificationService`, among other components.
##########
c2/c2-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2HttpClient.java:
##########
@@ -91,6 +92,45 @@ public Optional<C2HeartbeatResponse> publishHeartbeat(C2Heartbeat heartbeat) {
return serializer.serialize(heartbeat).flatMap(this::sendHeartbeat);
}
+ @Override
+ public Optional<byte[]> retrieveUpdateContent(String flowUpdateUrl) {
Review Comment:
Although not required, it is helpful to use the `final` keyword on method arguments.
```suggestion
public Optional<byte[]> retrieveUpdateContent(final String flowUpdateUrl) {
```
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.matching;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.apache.nifi.c2.client.http.C2HttpClient.HTTP_STATUS_BAD_REQUEST;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.junit5.WireMockTest;
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.client.api.C2Serializer;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@WireMockTest
+@ExtendWith(MockitoExtension.class)
+public class C2HttpClientTest {
+
+ private static final String HEARTBEAT_PATH = "/c2/heartbeat";
+ private static final String UPDATE_PATH = "/c2/update";
+ private static final String ACK_PATH = "/c2/acknowledge";
+ private static final int HTTP_STATUS_OK = 200;
+
+ @Mock
+ private C2ClientConfig c2ClientConfig;
+
+ @Mock
+ private C2Serializer serializer;
+
+ @InjectMocks
+ private C2HttpClient c2HttpClient;
Review Comment:
As mentioned on the dependencies, instead of injecting mocks, OkHttp MockWebServer supports testing actual HTTP communication.
##########
c2/c2-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2HttpClient.java:
##########
@@ -91,6 +92,45 @@ public Optional<C2HeartbeatResponse> publishHeartbeat(C2Heartbeat heartbeat) {
return serializer.serialize(heartbeat).flatMap(this::sendHeartbeat);
}
+ @Override
+ public Optional<byte[]> retrieveUpdateContent(String flowUpdateUrl) {
+ final Request.Builder requestBuilder = new Request.Builder()
+ .get()
+ .url(flowUpdateUrl);
+ final Request request = requestBuilder.build();
+
+ Optional<ResponseBody> body;
+ try (Response response = httpClientReference.get().newCall(request).execute()) {
+ int code = response.code();
+ body = Optional.ofNullable(response.body());
+
+ if (code >= HTTP_STATUS_BAD_REQUEST) {
Review Comment:
Is there a reason for checking this specific code as the minimum, as opposed to calling `Response.isSuccessful()`?
##########
c2/c2-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2HttpClient.java:
##########
@@ -91,6 +92,45 @@ public Optional<C2HeartbeatResponse> publishHeartbeat(C2Heartbeat heartbeat) {
return serializer.serialize(heartbeat).flatMap(this::sendHeartbeat);
}
+ @Override
+ public Optional<byte[]> retrieveUpdateContent(String flowUpdateUrl) {
+ final Request.Builder requestBuilder = new Request.Builder()
+ .get()
+ .url(flowUpdateUrl);
+ final Request request = requestBuilder.build();
+
+ Optional<ResponseBody> body;
+ try (Response response = httpClientReference.get().newCall(request).execute()) {
+ int code = response.code();
+ body = Optional.ofNullable(response.body());
+
+ if (code >= HTTP_STATUS_BAD_REQUEST) {
+ StringBuilder messageBuilder = new StringBuilder(String.format("Configuration retrieval failed: HTTP %d", code));
+ body.map(Object::toString).ifPresent(messageBuilder::append);
+ throw new C2ServerException(messageBuilder.toString());
+ }
+
+ if (!body.isPresent()) {
+ logger.warn("No body returned when pulling a new configuration");
+ return Optional.empty();
+ }
+
+ return Optional.of(body.get().bytes());
+ } catch (Exception e) {
+ logger.warn("Configuration retrieval failed", e);
+ return Optional.empty();
+ }
+ }
+
+ @Override
+ public void acknowledgeOperation(C2OperationAck operationAck) {
+ logger.info("Performing acknowledgement request to {} for operation {}", clientConfig.getC2AckUrl(), operationAck.getOperationId());
Review Comment:
Should this be logged at the info level? It can be helpful to delimit log arguments. What do you think of the following?
```suggestion
logger.info("Acknowledging Operation [{}] C2 URL [{}]", operationAck.getOperationId(). clientConfig.getC2AckUrl());
```
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.matching;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.apache.nifi.c2.client.http.C2HttpClient.HTTP_STATUS_BAD_REQUEST;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.junit5.WireMockTest;
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.client.api.C2Serializer;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@WireMockTest
+@ExtendWith(MockitoExtension.class)
+public class C2HttpClientTest {
+
+ private static final String HEARTBEAT_PATH = "/c2/heartbeat";
+ private static final String UPDATE_PATH = "/c2/update";
+ private static final String ACK_PATH = "/c2/acknowledge";
+ private static final int HTTP_STATUS_OK = 200;
+
+ @Mock
+ private C2ClientConfig c2ClientConfig;
+
+ @Mock
+ private C2Serializer serializer;
+
+ @InjectMocks
+ private C2HttpClient c2HttpClient;
+
+ @Test
+ void shouldReturnC2HeartbeatResponse(WireMockRuntimeInfo wmRuntimeInfo) {
+ // given
+ C2HeartbeatResponse hbResponse = new C2HeartbeatResponse();
+ stubFor(WireMock.post(HEARTBEAT_PATH)
+ .willReturn(aResponse().withBody("dummyResponseBody")));
+
+ given(serializer.serialize(any(C2Heartbeat.class))).willReturn(Optional.of("Heartbeat"));
+ given(serializer.deserialize(any(), any())).willReturn(Optional.of(hbResponse));
+ given(c2ClientConfig.getC2Url()).willReturn(wmRuntimeInfo.getHttpBaseUrl() + HEARTBEAT_PATH);
+
+ // when
+ Optional<C2HeartbeatResponse> response = c2HttpClient.publishHeartbeat(new C2Heartbeat());
+
+ // then
+ assertTrue(response.isPresent());
+ assertEquals(response.get(), hbResponse);
+ }
+
+ @Test
+ void shouldReturnEmptyInCaseOfCommunicationIssue() {
+ // given
+ given(serializer.serialize(any(C2Heartbeat.class))).willReturn(Optional.of("Heartbeat"));
+ given(c2ClientConfig.getC2Url()).willReturn("http://localhost/incorrectPath");
+
+ // when
+ Optional<C2HeartbeatResponse> response = c2HttpClient.publishHeartbeat(new C2Heartbeat());
+
+ // then
+ assertFalse(response.isPresent());
+ }
+
+ @Test
+ void shouldThrowExceptionForInvalidKeystoreFilenameAtInitialization() {
+ // given
+ given(c2ClientConfig.getKeystoreFilename()).willReturn("incorrectKeystoreFilename");
+
+ // when
+ IllegalStateException exception = assertThrows(IllegalStateException.class, () -> new C2HttpClient(c2ClientConfig, serializer));
+
+ // then
+ assertEquals("OkHttp TLS configuration failed", exception.getMessage());
Review Comment:
In general, it is best to avoid test an exact exception message, since that should not be part of the method contract. If the purpose is to test for a specific scenario, recommend either testing that the message contains a keyword, such as `TLS`, or using a different exception type.
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.matching;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.apache.nifi.c2.client.http.C2HttpClient.HTTP_STATUS_BAD_REQUEST;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
Review Comment:
In general, NiFi tests do not follow the BDD pattern, so recommend refactoring to avoid the use of this class and method.
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.matching;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.apache.nifi.c2.client.http.C2HttpClient.HTTP_STATUS_BAD_REQUEST;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.junit5.WireMockTest;
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.client.api.C2Serializer;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@WireMockTest
+@ExtendWith(MockitoExtension.class)
+public class C2HttpClientTest {
+
+ private static final String HEARTBEAT_PATH = "/c2/heartbeat";
+ private static final String UPDATE_PATH = "/c2/update";
+ private static final String ACK_PATH = "/c2/acknowledge";
+ private static final int HTTP_STATUS_OK = 200;
+
+ @Mock
+ private C2ClientConfig c2ClientConfig;
+
+ @Mock
+ private C2Serializer serializer;
+
+ @InjectMocks
+ private C2HttpClient c2HttpClient;
+
+ @Test
+ void shouldReturnC2HeartbeatResponse(WireMockRuntimeInfo wmRuntimeInfo) {
Review Comment:
In general, most unit test methods begin with `test`, followed by the name of the primary method being tested. This is not an absolute requirement, but the general pattern.
##########
c2/c2-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2HttpClient.java:
##########
@@ -91,6 +92,45 @@ public Optional<C2HeartbeatResponse> publishHeartbeat(C2Heartbeat heartbeat) {
return serializer.serialize(heartbeat).flatMap(this::sendHeartbeat);
}
+ @Override
+ public Optional<byte[]> retrieveUpdateContent(String flowUpdateUrl) {
+ final Request.Builder requestBuilder = new Request.Builder()
+ .get()
+ .url(flowUpdateUrl);
+ final Request request = requestBuilder.build();
+
+ Optional<ResponseBody> body;
+ try (Response response = httpClientReference.get().newCall(request).execute()) {
+ int code = response.code();
+ body = Optional.ofNullable(response.body());
+
+ if (code >= HTTP_STATUS_BAD_REQUEST) {
+ StringBuilder messageBuilder = new StringBuilder(String.format("Configuration retrieval failed: HTTP %d", code));
+ body.map(Object::toString).ifPresent(messageBuilder::append);
+ throw new C2ServerException(messageBuilder.toString());
+ }
+
+ if (!body.isPresent()) {
+ logger.warn("No body returned when pulling a new configuration");
+ return Optional.empty();
+ }
+
+ return Optional.of(body.get().bytes());
+ } catch (Exception e) {
+ logger.warn("Configuration retrieval failed", e);
+ return Optional.empty();
+ }
Review Comment:
For clarity, it would be helpful to refactor this method to have a single return statement.
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.matching;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.apache.nifi.c2.client.http.C2HttpClient.HTTP_STATUS_BAD_REQUEST;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.junit5.WireMockTest;
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.client.api.C2Serializer;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@WireMockTest
+@ExtendWith(MockitoExtension.class)
+public class C2HttpClientTest {
+
+ private static final String HEARTBEAT_PATH = "/c2/heartbeat";
+ private static final String UPDATE_PATH = "/c2/update";
+ private static final String ACK_PATH = "/c2/acknowledge";
+ private static final int HTTP_STATUS_OK = 200;
+
+ @Mock
+ private C2ClientConfig c2ClientConfig;
+
+ @Mock
+ private C2Serializer serializer;
+
+ @InjectMocks
+ private C2HttpClient c2HttpClient;
+
+ @Test
+ void shouldReturnC2HeartbeatResponse(WireMockRuntimeInfo wmRuntimeInfo) {
+ // given
Review Comment:
The given/when/then comments should be removed. They do not provide explanation of behavior, and simply separating test method elements with newlines is usually sufficient.
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/C2ClientServiceTest.java:
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import java.util.List;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.model.RuntimeInfoWrapper;
+import org.apache.nifi.c2.client.service.operation.C2OperationService;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class C2ClientServiceTest {
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private C2HeartbeatFactory c2HeartbeatFactory;
+
+ @Mock
+ private C2OperationService operationService;
+
+ @InjectMocks
+ private C2ClientService c2ClientService;
+
+ @Test
+ public void shouldSendHeartbeatAndAckWhenOperationPresent() {
Review Comment:
The other new test class did not use the `public` modifier on test methods, so it would be good to be consistent. In general, new JUnit 5 tests can be more concise, omitting the `public` modifier. However, while JUnit 4 tests still remain, there will be a mix of approaches.
##########
c2/c2-client-bundle/c2-client-http/pom.xml:
##########
@@ -47,5 +47,32 @@ limitations under the License.
<groupId>com.squareup.okhttp3</groupId>
<artifactId>logging-interceptor</artifactId>
</dependency>
+
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-engine</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-junit-jupiter</artifactId>
+ <scope>test</scope>
+ </dependency>
Review Comment:
These dependencies are included in the default Maven configuration for all modules, so they do not necessarily need to be declared.
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6149:
URL: https://github.com/apache/nifi/pull/6149#discussion_r906392796
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.matching;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.apache.nifi.c2.client.http.C2HttpClient.HTTP_STATUS_BAD_REQUEST;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.junit5.WireMockTest;
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.client.api.C2Serializer;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@WireMockTest
+@ExtendWith(MockitoExtension.class)
+public class C2HttpClientTest {
+
+ private static final String HEARTBEAT_PATH = "/c2/heartbeat";
+ private static final String UPDATE_PATH = "/c2/update";
+ private static final String ACK_PATH = "/c2/acknowledge";
+ private static final int HTTP_STATUS_OK = 200;
+
+ @Mock
+ private C2ClientConfig c2ClientConfig;
+
+ @Mock
+ private C2Serializer serializer;
+
+ @InjectMocks
+ private C2HttpClient c2HttpClient;
Review Comment:
Thanks for making the adjustments, injecting the mocked components is fine, and the change to use MockWebServer looks good.
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6149:
URL: https://github.com/apache/nifi/pull/6149#discussion_r906396485
##########
c2/c2-client-bundle/c2-client-service/src/main/java/org/apache/nifi/c2/client/service/operation/UpdateConfigurationOperationHandler.java:
##########
@@ -36,8 +38,9 @@
public class UpdateConfigurationOperationHandler implements C2OperationHandler {
private static final Logger logger = LoggerFactory.getLogger(UpdateConfigurationOperationHandler.class);
+ private static final Pattern FLOW_ID_PATTERN = Pattern.compile("/.*/.*/.*/(.*)/?.*");
Review Comment:
This regular expression pattern is not optimal given the multiple uses of greedy matching using `.*`. Does the Flow ID follow a standard structure, such as UUID? If so, it would be better to search of a UUID structure. If not, adjusting the pattern matching to be more specific for path elements, using something like `[^/]+?` to find all characters except for a forward slash, and including the question mark for non-greedy matching, would be better.
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/C2HeartbeatFactoryTest.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.client.service.model.RuntimeInfoWrapper;
+import org.apache.nifi.c2.protocol.api.AgentRepositories;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.FlowQueueStatus;
+import org.apache.nifi.c2.protocol.component.api.RuntimeManifest;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class C2HeartbeatFactoryTest {
+
+ private static final String AGENT_CLASS = "agentClass";
+ private static final String FLOW_ID = "flowId";
+
+ @Mock
+ private C2ClientConfig clientConfig;
+
+ @Mock
+ private FlowIdHolder flowIdHolder;
+
+ @InjectMocks
+ private C2HeartbeatFactory c2HeartbeatFactory;
+
+ @TempDir
+ private File tempDir;
+
+ @BeforeEach
+ public void setup() {
+ when(clientConfig.getConfDirectory()).thenReturn(tempDir.getAbsolutePath());
+ }
+
+ @Test
+ void testCreateHeartbeat() {
+ when(flowIdHolder.getFlowId()).thenReturn(FLOW_ID);
+ when(clientConfig.getAgentClass()).thenReturn(AGENT_CLASS);
+
+ C2Heartbeat heartbeat = c2HeartbeatFactory.create(mock(RuntimeInfoWrapper.class));
+
+ assertEquals(FLOW_ID, heartbeat.getFlowId());
+ assertEquals(AGENT_CLASS, heartbeat.getAgentClass());
+ }
+
+ @Test
+ void testCreateGeneratesAgentAndDeviceIdIfNotPresent() {
+ C2Heartbeat heartbeat = c2HeartbeatFactory.create(mock(RuntimeInfoWrapper.class));
+
+ assertNotNull(heartbeat.getAgentId());
+ assertNotNull(heartbeat.getDeviceId());
+ }
+
+ @Test
+ void testCreatePopulatesFromRuntimeInfoWrapper() {
+ AgentRepositories repos = new AgentRepositories();
+ RuntimeManifest manifest = new RuntimeManifest();
+ Map<String, FlowQueueStatus> queueStatus = new HashMap<>();
+
+ C2Heartbeat heartbeat = c2HeartbeatFactory.create(new RuntimeInfoWrapper(repos, manifest, queueStatus));
+
+ assertEquals(repos, heartbeat.getAgentInfo().getStatus().getRepositories());
+ assertEquals(manifest, heartbeat.getAgentInfo().getAgentManifest());
+ assertEquals(queueStatus, heartbeat.getFlowInfo().getQueues());
+ }
+
+ @Test
+ void testCreateThrowsExceptionWhenConfDirNotSet() {
+ when(clientConfig.getConfDirectory()).thenReturn("dummy");
Review Comment:
Use of the word `dummy` should be avoided. Some other placeholder, or even something like `String.class.getSimpleName()` would better.
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/C2ClientServiceTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service;
+
+import static org.mockito.ArgumentMatchers.any;
+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.util.List;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.model.RuntimeInfoWrapper;
+import org.apache.nifi.c2.client.service.operation.C2OperationService;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class C2ClientServiceTest {
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private C2HeartbeatFactory c2HeartbeatFactory;
+
+ @Mock
+ private C2OperationService operationService;
+
+ @InjectMocks
+ private C2ClientService c2ClientService;
+
+ @Test
+ void testSendHeartbeatAndAckWhenOperationPresent() {
+ C2Heartbeat heartbeat = mock(C2Heartbeat.class);
+ when(c2HeartbeatFactory.create(any())).thenReturn(heartbeat);
+ C2HeartbeatResponse hbResponse = new C2HeartbeatResponse();
+ hbResponse.setRequestedOperations(generateOperation(1));
+ when(client.publishHeartbeat(heartbeat)).thenReturn(Optional.of(hbResponse));
+ when(operationService.handleOperation(any())).thenReturn(Optional.of(new C2OperationAck()));
+
+ c2ClientService.sendHeartbeat(mock(RuntimeInfoWrapper.class));
Review Comment:
Changing `RuntimeInfoWrapper` to a `Mock` annotated member variable would simplify this and other test references.
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/operation/UpdateConfigurationOperationHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service.operation;
+
+import static org.apache.commons.lang3.StringUtils.EMPTY;
+import static org.apache.nifi.c2.client.service.operation.UpdateConfigurationOperationHandler.LOCATION;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Optional;
+import java.util.function.Function;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.FlowIdHolder;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.apache.nifi.c2.protocol.api.C2OperationState;
+import org.apache.nifi.c2.protocol.api.OperandType;
+import org.apache.nifi.c2.protocol.api.OperationType;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class UpdateConfigurationOperationHandlerTest {
+
+ private static final String FLOW_ID = "flowId";
+ private static final String OPERATION_ID = "operationId";
+ private static final Map<String, String> CORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "/path/for/the/" + FLOW_ID);
+ private static final Map<String, String> INCORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "incorrect/location");
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private FlowIdHolder flowIdHolder;
+
+ @Test
+ void testUpdateConfigurationOperationHandlerCreateSuccess() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+
+ assertEquals(OperationType.UPDATE, handler.getOperationType());
+ assertEquals(OperandType.CONFIGURATION, handler.getOperandType());
+ }
+
+ @Test
+ void testHandleThrowsExceptionForIncorrectArg() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+ C2Operation operation = new C2Operation();
+ operation.setArgs(INCORRECT_LOCATION_MAP);
+
+ IllegalStateException exception = assertThrows(IllegalStateException.class, () -> handler.handle(operation));
+
+ assertEquals("Could not get flow id from the provided URL", exception.getMessage());
+ }
+
+ @Test
+ void testHandleReturnsNotAppliedWithNoContent() {
+ when(flowIdHolder.getFlowId()).thenReturn("dummy");
+ when(client.retrieveUpdateContent(any())).thenReturn(Optional.empty());
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(client, flowIdHolder, null);
+ C2Operation operation = new C2Operation();
+ operation.setArgs(CORRECT_LOCATION_MAP);
+
+ C2OperationAck response = handler.handle(operation);
+
+ assertEquals(EMPTY, response.getOperationId());
+ assertEquals(C2OperationState.OperationState.NOT_APPLIED, response.getOperationState().getState());
+ }
+
+ @Test
+ void testHandleReturnsNotAppliedWithContentApplyIssues() {
+ Function<byte[], Boolean> failedToUpdate = x -> false;
+ when(flowIdHolder.getFlowId()).thenReturn("dummy");
Review Comment:
```suggestion
when(flowIdHolder.getFlowId()).thenReturn(FLOW_ID);
```
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/C2ClientServiceTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service;
+
+import static org.mockito.ArgumentMatchers.any;
+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.util.List;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.model.RuntimeInfoWrapper;
+import org.apache.nifi.c2.client.service.operation.C2OperationService;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class C2ClientServiceTest {
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private C2HeartbeatFactory c2HeartbeatFactory;
+
+ @Mock
+ private C2OperationService operationService;
+
+ @InjectMocks
+ private C2ClientService c2ClientService;
+
+ @Test
+ void testSendHeartbeatAndAckWhenOperationPresent() {
+ C2Heartbeat heartbeat = mock(C2Heartbeat.class);
+ when(c2HeartbeatFactory.create(any())).thenReturn(heartbeat);
+ C2HeartbeatResponse hbResponse = new C2HeartbeatResponse();
+ hbResponse.setRequestedOperations(generateOperation(1));
+ when(client.publishHeartbeat(heartbeat)).thenReturn(Optional.of(hbResponse));
+ when(operationService.handleOperation(any())).thenReturn(Optional.of(new C2OperationAck()));
+
+ c2ClientService.sendHeartbeat(mock(RuntimeInfoWrapper.class));
+
+ verify(c2HeartbeatFactory, times(1)).create(any());
Review Comment:
The default `verify()` method with a single argument defaults to `times(1)`, so this reference and others could be simplified to remove `times(1)`.
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import okhttp3.mockwebserver.MockResponse;
+import okhttp3.mockwebserver.MockWebServer;
+import okhttp3.mockwebserver.RecordedRequest;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.apache.nifi.c2.serializer.C2Serializer;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class C2HttpClientTest {
+
+ private static final String HEARTBEAT_PATH = "c2/heartbeat";
+ private static final String UPDATE_PATH = "c2/update";
+ private static final String ACK_PATH = "c2/acknowledge";
+ private static final int HTTP_STATUS_OK = 200;
+ private static final int HTTP_STATUS_BAD_REQUEST = 400;
+
+ @Mock
+ private C2ClientConfig c2ClientConfig;
+
+ @Mock
+ private C2Serializer serializer;
+
+ @InjectMocks
+ private C2HttpClient c2HttpClient;
+
+ private MockWebServer mockWebServer;
+
+ private String baseUrl;
+
+ @BeforeEach
+ public void startServer() {
+ mockWebServer = new MockWebServer();
+ baseUrl = mockWebServer.url("/").newBuilder().host("localhost").build().toString();
+ }
+
+ @AfterEach
+ public void shutdownServer() throws IOException {
+ mockWebServer.shutdown();
+ }
+
+ @Test
+ void testPublishHeartbeatSuccess() throws InterruptedException {
+ C2HeartbeatResponse hbResponse = new C2HeartbeatResponse();
+ mockWebServer.enqueue(new MockResponse().setBody("dummyResponseBody"));
Review Comment:
Use of `dummy` should be avoided, recommend changing to simply `responseBody`.
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/operation/UpdateConfigurationOperationHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service.operation;
+
+import static org.apache.commons.lang3.StringUtils.EMPTY;
+import static org.apache.nifi.c2.client.service.operation.UpdateConfigurationOperationHandler.LOCATION;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Optional;
+import java.util.function.Function;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.FlowIdHolder;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.apache.nifi.c2.protocol.api.C2OperationState;
+import org.apache.nifi.c2.protocol.api.OperandType;
+import org.apache.nifi.c2.protocol.api.OperationType;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class UpdateConfigurationOperationHandlerTest {
+
+ private static final String FLOW_ID = "flowId";
+ private static final String OPERATION_ID = "operationId";
+ private static final Map<String, String> CORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "/path/for/the/" + FLOW_ID);
+ private static final Map<String, String> INCORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "incorrect/location");
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private FlowIdHolder flowIdHolder;
+
+ @Test
+ void testUpdateConfigurationOperationHandlerCreateSuccess() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+
+ assertEquals(OperationType.UPDATE, handler.getOperationType());
+ assertEquals(OperandType.CONFIGURATION, handler.getOperandType());
+ }
+
+ @Test
+ void testHandleThrowsExceptionForIncorrectArg() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+ C2Operation operation = new C2Operation();
+ operation.setArgs(INCORRECT_LOCATION_MAP);
+
+ IllegalStateException exception = assertThrows(IllegalStateException.class, () -> handler.handle(operation));
+
+ assertEquals("Could not get flow id from the provided URL", exception.getMessage());
Review Comment:
Recommend adjusting this check to avoid testing for the exact exception message.
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/operation/UpdateConfigurationOperationHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service.operation;
+
+import static org.apache.commons.lang3.StringUtils.EMPTY;
+import static org.apache.nifi.c2.client.service.operation.UpdateConfigurationOperationHandler.LOCATION;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Optional;
+import java.util.function.Function;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.FlowIdHolder;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.apache.nifi.c2.protocol.api.C2OperationState;
+import org.apache.nifi.c2.protocol.api.OperandType;
+import org.apache.nifi.c2.protocol.api.OperationType;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class UpdateConfigurationOperationHandlerTest {
+
+ private static final String FLOW_ID = "flowId";
+ private static final String OPERATION_ID = "operationId";
+ private static final Map<String, String> CORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "/path/for/the/" + FLOW_ID);
+ private static final Map<String, String> INCORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "incorrect/location");
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private FlowIdHolder flowIdHolder;
+
+ @Test
+ void testUpdateConfigurationOperationHandlerCreateSuccess() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+
+ assertEquals(OperationType.UPDATE, handler.getOperationType());
+ assertEquals(OperandType.CONFIGURATION, handler.getOperandType());
+ }
+
+ @Test
+ void testHandleThrowsExceptionForIncorrectArg() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+ C2Operation operation = new C2Operation();
+ operation.setArgs(INCORRECT_LOCATION_MAP);
+
+ IllegalStateException exception = assertThrows(IllegalStateException.class, () -> handler.handle(operation));
+
+ assertEquals("Could not get flow id from the provided URL", exception.getMessage());
+ }
+
+ @Test
+ void testHandleReturnsNotAppliedWithNoContent() {
+ when(flowIdHolder.getFlowId()).thenReturn("dummy");
Review Comment:
```suggestion
when(flowIdHolder.getFlowId()).thenReturn(FLOW_ID);
```
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/operation/UpdateConfigurationOperationHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service.operation;
+
+import static org.apache.commons.lang3.StringUtils.EMPTY;
+import static org.apache.nifi.c2.client.service.operation.UpdateConfigurationOperationHandler.LOCATION;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Optional;
+import java.util.function.Function;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.FlowIdHolder;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.apache.nifi.c2.protocol.api.C2OperationState;
+import org.apache.nifi.c2.protocol.api.OperandType;
+import org.apache.nifi.c2.protocol.api.OperationType;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class UpdateConfigurationOperationHandlerTest {
+
+ private static final String FLOW_ID = "flowId";
+ private static final String OPERATION_ID = "operationId";
+ private static final Map<String, String> CORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "/path/for/the/" + FLOW_ID);
+ private static final Map<String, String> INCORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "incorrect/location");
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private FlowIdHolder flowIdHolder;
+
+ @Test
+ void testUpdateConfigurationOperationHandlerCreateSuccess() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+
+ assertEquals(OperationType.UPDATE, handler.getOperationType());
+ assertEquals(OperandType.CONFIGURATION, handler.getOperandType());
+ }
+
+ @Test
+ void testHandleThrowsExceptionForIncorrectArg() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+ C2Operation operation = new C2Operation();
+ operation.setArgs(INCORRECT_LOCATION_MAP);
+
+ IllegalStateException exception = assertThrows(IllegalStateException.class, () -> handler.handle(operation));
+
+ assertEquals("Could not get flow id from the provided URL", exception.getMessage());
+ }
+
+ @Test
+ void testHandleReturnsNotAppliedWithNoContent() {
+ when(flowIdHolder.getFlowId()).thenReturn("dummy");
+ when(client.retrieveUpdateContent(any())).thenReturn(Optional.empty());
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(client, flowIdHolder, null);
+ C2Operation operation = new C2Operation();
+ operation.setArgs(CORRECT_LOCATION_MAP);
+
+ C2OperationAck response = handler.handle(operation);
+
+ assertEquals(EMPTY, response.getOperationId());
+ assertEquals(C2OperationState.OperationState.NOT_APPLIED, response.getOperationState().getState());
+ }
+
+ @Test
+ void testHandleReturnsNotAppliedWithContentApplyIssues() {
+ Function<byte[], Boolean> failedToUpdate = x -> false;
+ when(flowIdHolder.getFlowId()).thenReturn("dummy");
+ when(client.retrieveUpdateContent(any())).thenReturn(Optional.of("content".getBytes()));
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(client, flowIdHolder, failedToUpdate);
+ C2Operation operation = new C2Operation();
+ operation.setIdentifier(OPERATION_ID);
+ operation.setArgs(CORRECT_LOCATION_MAP);
+
+ C2OperationAck response = handler.handle(operation);
+
+ assertEquals(OPERATION_ID, response.getOperationId());
+ assertEquals(C2OperationState.OperationState.NOT_APPLIED, response.getOperationState().getState());
+ }
+
+ @Test
+ void testHandleReturnsFullyApplied() {
+ Function<byte[], Boolean> successUpdate = x -> true;
+ when(flowIdHolder.getFlowId()).thenReturn("dummy");
Review Comment:
This reference and others could be replaced with a static `FLOW_ID`:
```suggestion
when(flowIdHolder.getFlowId()).thenReturn(FLOW_ID);
```
--
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
[GitHub] [nifi] bejancsaba commented on pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
bejancsaba commented on PR #6149:
URL: https://github.com/apache/nifi/pull/6149#issuecomment-1166350542
@exceptionfactory thanks for the new suggestions I addressed (I hope) everything that was requested. Theer is only one outstanding item. Of course I don't want to spend too much time on it. I'm accepting this is how it is done by the community just wanted to take one last stab at it:
> As far use use of the `final` keyword, it is strongly preferred as it provides a quick indicator that method parameters cannot and will not be changed during method processing. Although languages like Kotlin make this the default behavior, Java requires the keyword to enforce the behavior, so it is best to apply it across the board.
Thanks for the explanation. I know there could be a lot of pros and cons raised and there would be plenty of articles to be found across the internet justifying each point. My opinion is that even if we declare each method parameter final that just tells us that the reference won't be reassigned inside the method (which doesn't have any affect on the caller's reference) however even the final "annotated" object's state / properties can be changed if the object is not immutable. Based on the code I saw immutability is not followed from top to bottom and I thought adding final for every parameter is noisy (makes the code longer) and misleading as you can change the object inside the function as you wish you just can't reassign the reference to it which would have affect only inside the scope of the function anyway.
Of course if this is a must follow constraint in the NiFi codebase then there is not much to talk about I will add it everywhere just wanted to explain why I think it is counter productive.
--
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
[GitHub] [nifi] bejancsaba commented on a diff in pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
bejancsaba commented on code in PR #6149:
URL: https://github.com/apache/nifi/pull/6149#discussion_r905391497
##########
c2/c2-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2HttpClient.java:
##########
@@ -91,6 +92,45 @@ public Optional<C2HeartbeatResponse> publishHeartbeat(C2Heartbeat heartbeat) {
return serializer.serialize(heartbeat).flatMap(this::sendHeartbeat);
}
+ @Override
+ public Optional<byte[]> retrieveUpdateContent(String flowUpdateUrl) {
Review Comment:
I saw that "final" is heavily utilised in NiFi codebase. I understand the intention just think that it is more noisy than how useful it is in majority of the cases. So my question is how strongly you feel about this? If not too strongly I wouldn't add final everywhere in the c2 module which is pretty self contained. If this is more like a rule for consistency then I will go ahead and update.
##########
c2/c2-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2HttpClient.java:
##########
@@ -91,6 +92,45 @@ public Optional<C2HeartbeatResponse> publishHeartbeat(C2Heartbeat heartbeat) {
return serializer.serialize(heartbeat).flatMap(this::sendHeartbeat);
}
+ @Override
+ public Optional<byte[]> retrieveUpdateContent(String flowUpdateUrl) {
+ final Request.Builder requestBuilder = new Request.Builder()
+ .get()
+ .url(flowUpdateUrl);
+ final Request request = requestBuilder.build();
+
+ Optional<ResponseBody> body;
+ try (Response response = httpClientReference.get().newCall(request).execute()) {
+ int code = response.code();
+ body = Optional.ofNullable(response.body());
+
+ if (code >= HTTP_STATUS_BAD_REQUEST) {
+ StringBuilder messageBuilder = new StringBuilder(String.format("Configuration retrieval failed: HTTP %d", code));
+ body.map(Object::toString).ifPresent(messageBuilder::append);
+ throw new C2ServerException(messageBuilder.toString());
+ }
+
+ if (!body.isPresent()) {
+ logger.warn("No body returned when pulling a new configuration");
+ return Optional.empty();
+ }
+
+ return Optional.of(body.get().bytes());
+ } catch (Exception e) {
+ logger.warn("Configuration retrieval failed", e);
+ return Optional.empty();
+ }
Review Comment:
Done
##########
c2/c2-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2HttpClient.java:
##########
@@ -91,6 +92,45 @@ public Optional<C2HeartbeatResponse> publishHeartbeat(C2Heartbeat heartbeat) {
return serializer.serialize(heartbeat).flatMap(this::sendHeartbeat);
}
+ @Override
+ public Optional<byte[]> retrieveUpdateContent(String flowUpdateUrl) {
+ final Request.Builder requestBuilder = new Request.Builder()
+ .get()
+ .url(flowUpdateUrl);
+ final Request request = requestBuilder.build();
+
+ Optional<ResponseBody> body;
+ try (Response response = httpClientReference.get().newCall(request).execute()) {
+ int code = response.code();
+ body = Optional.ofNullable(response.body());
+
+ if (code >= HTTP_STATUS_BAD_REQUEST) {
+ StringBuilder messageBuilder = new StringBuilder(String.format("Configuration retrieval failed: HTTP %d", code));
+ body.map(Object::toString).ifPresent(messageBuilder::append);
+ throw new C2ServerException(messageBuilder.toString());
+ }
+
+ if (!body.isPresent()) {
+ logger.warn("No body returned when pulling a new configuration");
+ return Optional.empty();
+ }
+
+ return Optional.of(body.get().bytes());
+ } catch (Exception e) {
+ logger.warn("Configuration retrieval failed", e);
+ return Optional.empty();
+ }
+ }
+
+ @Override
+ public void acknowledgeOperation(C2OperationAck operationAck) {
+ logger.info("Performing acknowledgement request to {} for operation {}", clientConfig.getC2AckUrl(), operationAck.getOperationId());
Review Comment:
Changed the log structure as suggested but I would keep the level on info if that is ok. Operation acknowledgement is super rare and it usually indicates state / behaviour change on the agent side so can be really helpful in following what is happening.
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.matching;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.apache.nifi.c2.client.http.C2HttpClient.HTTP_STATUS_BAD_REQUEST;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.junit5.WireMockTest;
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.client.api.C2Serializer;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@WireMockTest
+@ExtendWith(MockitoExtension.class)
+public class C2HttpClientTest {
+
+ private static final String HEARTBEAT_PATH = "/c2/heartbeat";
+ private static final String UPDATE_PATH = "/c2/update";
+ private static final String ACK_PATH = "/c2/acknowledge";
+ private static final int HTTP_STATUS_OK = 200;
+
+ @Mock
+ private C2ClientConfig c2ClientConfig;
+
+ @Mock
+ private C2Serializer serializer;
+
+ @InjectMocks
+ private C2HttpClient c2HttpClient;
+
+ @Test
+ void shouldReturnC2HeartbeatResponse(WireMockRuntimeInfo wmRuntimeInfo) {
Review Comment:
Usually there are multiple tests covering the same method for different cases. I like prefixing it with "should" as it forces you to continue with a short "explanation" regarding what is being validated here while the method name can be really vague. Also "test" is implied by the Test annotation above :) Anyway I understand that the convention is different here so I updated all tests in this spirit just wanted to give some context why I created them like it.
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.matching;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.apache.nifi.c2.client.http.C2HttpClient.HTTP_STATUS_BAD_REQUEST;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.junit5.WireMockTest;
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.client.api.C2Serializer;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@WireMockTest
+@ExtendWith(MockitoExtension.class)
+public class C2HttpClientTest {
+
+ private static final String HEARTBEAT_PATH = "/c2/heartbeat";
+ private static final String UPDATE_PATH = "/c2/update";
+ private static final String ACK_PATH = "/c2/acknowledge";
+ private static final int HTTP_STATUS_OK = 200;
+
+ @Mock
+ private C2ClientConfig c2ClientConfig;
+
+ @Mock
+ private C2Serializer serializer;
+
+ @InjectMocks
+ private C2HttpClient c2HttpClient;
+
+ @Test
+ void shouldReturnC2HeartbeatResponse(WireMockRuntimeInfo wmRuntimeInfo) {
+ // given
Review Comment:
I like the comments as mostly on the given side there could be multiple sections that belong together so are worth separating from others by new line so in my opinion this structure supports readability but this is very subjective so I updated all tests ass requested to follow conventions.
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.matching;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.apache.nifi.c2.client.http.C2HttpClient.HTTP_STATUS_BAD_REQUEST;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
Review Comment:
Updated tests to follow NiFi conventions.
##########
c2/c2-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2HttpClient.java:
##########
@@ -91,6 +92,45 @@ public Optional<C2HeartbeatResponse> publishHeartbeat(C2Heartbeat heartbeat) {
return serializer.serialize(heartbeat).flatMap(this::sendHeartbeat);
}
+ @Override
+ public Optional<byte[]> retrieveUpdateContent(String flowUpdateUrl) {
+ final Request.Builder requestBuilder = new Request.Builder()
+ .get()
+ .url(flowUpdateUrl);
+ final Request request = requestBuilder.build();
+
+ Optional<ResponseBody> body;
+ try (Response response = httpClientReference.get().newCall(request).execute()) {
+ int code = response.code();
+ body = Optional.ofNullable(response.body());
+
+ if (code >= HTTP_STATUS_BAD_REQUEST) {
Review Comment:
Neat! I was not aware of this function. Iitial implementation was not okhttp based I think so it made sense then. It is more readable with this. Thanks
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.matching;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.apache.nifi.c2.client.http.C2HttpClient.HTTP_STATUS_BAD_REQUEST;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.junit5.WireMockTest;
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.client.api.C2Serializer;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@WireMockTest
+@ExtendWith(MockitoExtension.class)
+public class C2HttpClientTest {
+
+ private static final String HEARTBEAT_PATH = "/c2/heartbeat";
+ private static final String UPDATE_PATH = "/c2/update";
+ private static final String ACK_PATH = "/c2/acknowledge";
+ private static final int HTTP_STATUS_OK = 200;
+
+ @Mock
+ private C2ClientConfig c2ClientConfig;
+
+ @Mock
+ private C2Serializer serializer;
+
+ @InjectMocks
+ private C2HttpClient c2HttpClient;
Review Comment:
Maybe I'm missing something here. We are injecting mocks to the C2HttpClient (e.g.: C2ClientConfig and C2Serializer) but regardless the calls made are actual http calls. Anyway I moved to MockWebServer as requested.
##########
c2/c2-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2ServerException.java:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import java.io.IOException;
+
Review Comment:
Done
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.matching;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static org.apache.nifi.c2.client.http.C2HttpClient.HTTP_STATUS_BAD_REQUEST;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.junit5.WireMockTest;
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.client.api.C2Serializer;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@WireMockTest
+@ExtendWith(MockitoExtension.class)
+public class C2HttpClientTest {
+
+ private static final String HEARTBEAT_PATH = "/c2/heartbeat";
+ private static final String UPDATE_PATH = "/c2/update";
+ private static final String ACK_PATH = "/c2/acknowledge";
+ private static final int HTTP_STATUS_OK = 200;
+
+ @Mock
+ private C2ClientConfig c2ClientConfig;
+
+ @Mock
+ private C2Serializer serializer;
+
+ @InjectMocks
+ private C2HttpClient c2HttpClient;
+
+ @Test
+ void shouldReturnC2HeartbeatResponse(WireMockRuntimeInfo wmRuntimeInfo) {
+ // given
+ C2HeartbeatResponse hbResponse = new C2HeartbeatResponse();
+ stubFor(WireMock.post(HEARTBEAT_PATH)
+ .willReturn(aResponse().withBody("dummyResponseBody")));
+
+ given(serializer.serialize(any(C2Heartbeat.class))).willReturn(Optional.of("Heartbeat"));
+ given(serializer.deserialize(any(), any())).willReturn(Optional.of(hbResponse));
+ given(c2ClientConfig.getC2Url()).willReturn(wmRuntimeInfo.getHttpBaseUrl() + HEARTBEAT_PATH);
+
+ // when
+ Optional<C2HeartbeatResponse> response = c2HttpClient.publishHeartbeat(new C2Heartbeat());
+
+ // then
+ assertTrue(response.isPresent());
+ assertEquals(response.get(), hbResponse);
+ }
+
+ @Test
+ void shouldReturnEmptyInCaseOfCommunicationIssue() {
+ // given
+ given(serializer.serialize(any(C2Heartbeat.class))).willReturn(Optional.of("Heartbeat"));
+ given(c2ClientConfig.getC2Url()).willReturn("http://localhost/incorrectPath");
+
+ // when
+ Optional<C2HeartbeatResponse> response = c2HttpClient.publishHeartbeat(new C2Heartbeat());
+
+ // then
+ assertFalse(response.isPresent());
+ }
+
+ @Test
+ void shouldThrowExceptionForInvalidKeystoreFilenameAtInitialization() {
+ // given
+ given(c2ClientConfig.getKeystoreFilename()).willReturn("incorrectKeystoreFilename");
+
+ // when
+ IllegalStateException exception = assertThrows(IllegalStateException.class, () -> new C2HttpClient(c2ClientConfig, serializer));
+
+ // then
+ assertEquals("OkHttp TLS configuration failed", exception.getMessage());
Review Comment:
Agreed, updated
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/C2ClientServiceTest.java:
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import java.util.List;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.model.RuntimeInfoWrapper;
+import org.apache.nifi.c2.client.service.operation.C2OperationService;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class C2ClientServiceTest {
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private C2HeartbeatFactory c2HeartbeatFactory;
+
+ @Mock
+ private C2OperationService operationService;
+
+ @InjectMocks
+ private C2ClientService c2ClientService;
+
+ @Test
+ public void shouldSendHeartbeatAndAckWhenOperationPresent() {
Review Comment:
Removed
##########
c2/c2-client-bundle/c2-client-http/pom.xml:
##########
@@ -47,5 +47,32 @@ limitations under the License.
<groupId>com.squareup.okhttp3</groupId>
<artifactId>logging-interceptor</artifactId>
</dependency>
+
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-engine</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-junit-jupiter</artifactId>
+ <scope>test</scope>
+ </dependency>
Review Comment:
Was not aware, removed. Thanks.
--
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
[GitHub] [nifi] bejancsaba commented on a diff in pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
bejancsaba commented on code in PR #6149:
URL: https://github.com/apache/nifi/pull/6149#discussion_r906717346
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/operation/UpdateConfigurationOperationHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service.operation;
+
+import static org.apache.commons.lang3.StringUtils.EMPTY;
+import static org.apache.nifi.c2.client.service.operation.UpdateConfigurationOperationHandler.LOCATION;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Optional;
+import java.util.function.Function;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.FlowIdHolder;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.apache.nifi.c2.protocol.api.C2OperationState;
+import org.apache.nifi.c2.protocol.api.OperandType;
+import org.apache.nifi.c2.protocol.api.OperationType;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class UpdateConfigurationOperationHandlerTest {
+
+ private static final String FLOW_ID = "flowId";
+ private static final String OPERATION_ID = "operationId";
+ private static final Map<String, String> CORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "/path/for/the/" + FLOW_ID);
+ private static final Map<String, String> INCORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "incorrect/location");
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private FlowIdHolder flowIdHolder;
+
+ @Test
+ void testUpdateConfigurationOperationHandlerCreateSuccess() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+
+ assertEquals(OperationType.UPDATE, handler.getOperationType());
+ assertEquals(OperandType.CONFIGURATION, handler.getOperandType());
+ }
+
+ @Test
+ void testHandleThrowsExceptionForIncorrectArg() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+ C2Operation operation = new C2Operation();
+ operation.setArgs(INCORRECT_LOCATION_MAP);
+
+ IllegalStateException exception = assertThrows(IllegalStateException.class, () -> handler.handle(operation));
+
+ assertEquals("Could not get flow id from the provided URL", exception.getMessage());
+ }
+
+ @Test
+ void testHandleReturnsNotAppliedWithNoContent() {
+ when(flowIdHolder.getFlowId()).thenReturn("dummy");
+ when(client.retrieveUpdateContent(any())).thenReturn(Optional.empty());
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(client, flowIdHolder, null);
+ C2Operation operation = new C2Operation();
+ operation.setArgs(CORRECT_LOCATION_MAP);
+
+ C2OperationAck response = handler.handle(operation);
+
+ assertEquals(EMPTY, response.getOperationId());
+ assertEquals(C2OperationState.OperationState.NOT_APPLIED, response.getOperationState().getState());
+ }
+
+ @Test
+ void testHandleReturnsNotAppliedWithContentApplyIssues() {
+ Function<byte[], Boolean> failedToUpdate = x -> false;
+ when(flowIdHolder.getFlowId()).thenReturn("dummy");
Review Comment:
applied
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/operation/UpdateConfigurationOperationHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service.operation;
+
+import static org.apache.commons.lang3.StringUtils.EMPTY;
+import static org.apache.nifi.c2.client.service.operation.UpdateConfigurationOperationHandler.LOCATION;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Optional;
+import java.util.function.Function;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.FlowIdHolder;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.apache.nifi.c2.protocol.api.C2OperationState;
+import org.apache.nifi.c2.protocol.api.OperandType;
+import org.apache.nifi.c2.protocol.api.OperationType;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class UpdateConfigurationOperationHandlerTest {
+
+ private static final String FLOW_ID = "flowId";
+ private static final String OPERATION_ID = "operationId";
+ private static final Map<String, String> CORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "/path/for/the/" + FLOW_ID);
+ private static final Map<String, String> INCORRECT_LOCATION_MAP = Collections.singletonMap(LOCATION, "incorrect/location");
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private FlowIdHolder flowIdHolder;
+
+ @Test
+ void testUpdateConfigurationOperationHandlerCreateSuccess() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+
+ assertEquals(OperationType.UPDATE, handler.getOperationType());
+ assertEquals(OperandType.CONFIGURATION, handler.getOperandType());
+ }
+
+ @Test
+ void testHandleThrowsExceptionForIncorrectArg() {
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(null, null, null);
+ C2Operation operation = new C2Operation();
+ operation.setArgs(INCORRECT_LOCATION_MAP);
+
+ IllegalStateException exception = assertThrows(IllegalStateException.class, () -> handler.handle(operation));
+
+ assertEquals("Could not get flow id from the provided URL", exception.getMessage());
+ }
+
+ @Test
+ void testHandleReturnsNotAppliedWithNoContent() {
+ when(flowIdHolder.getFlowId()).thenReturn("dummy");
+ when(client.retrieveUpdateContent(any())).thenReturn(Optional.empty());
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(client, flowIdHolder, null);
+ C2Operation operation = new C2Operation();
+ operation.setArgs(CORRECT_LOCATION_MAP);
+
+ C2OperationAck response = handler.handle(operation);
+
+ assertEquals(EMPTY, response.getOperationId());
+ assertEquals(C2OperationState.OperationState.NOT_APPLIED, response.getOperationState().getState());
+ }
+
+ @Test
+ void testHandleReturnsNotAppliedWithContentApplyIssues() {
+ Function<byte[], Boolean> failedToUpdate = x -> false;
+ when(flowIdHolder.getFlowId()).thenReturn("dummy");
+ when(client.retrieveUpdateContent(any())).thenReturn(Optional.of("content".getBytes()));
+ UpdateConfigurationOperationHandler handler = new UpdateConfigurationOperationHandler(client, flowIdHolder, failedToUpdate);
+ C2Operation operation = new C2Operation();
+ operation.setIdentifier(OPERATION_ID);
+ operation.setArgs(CORRECT_LOCATION_MAP);
+
+ C2OperationAck response = handler.handle(operation);
+
+ assertEquals(OPERATION_ID, response.getOperationId());
+ assertEquals(C2OperationState.OperationState.NOT_APPLIED, response.getOperationState().getState());
+ }
+
+ @Test
+ void testHandleReturnsFullyApplied() {
+ Function<byte[], Boolean> successUpdate = x -> true;
+ when(flowIdHolder.getFlowId()).thenReturn("dummy");
Review Comment:
applied
--
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
[GitHub] [nifi] bejancsaba commented on a diff in pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
bejancsaba commented on code in PR #6149:
URL: https://github.com/apache/nifi/pull/6149#discussion_r906717169
##########
c2/c2-client-bundle/c2-client-http/src/test/java/org/apache/nifi/c2/client/http/C2HttpClientTest.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.http;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+import okhttp3.mockwebserver.MockResponse;
+import okhttp3.mockwebserver.MockWebServer;
+import okhttp3.mockwebserver.RecordedRequest;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.apache.nifi.c2.serializer.C2Serializer;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class C2HttpClientTest {
+
+ private static final String HEARTBEAT_PATH = "c2/heartbeat";
+ private static final String UPDATE_PATH = "c2/update";
+ private static final String ACK_PATH = "c2/acknowledge";
+ private static final int HTTP_STATUS_OK = 200;
+ private static final int HTTP_STATUS_BAD_REQUEST = 400;
+
+ @Mock
+ private C2ClientConfig c2ClientConfig;
+
+ @Mock
+ private C2Serializer serializer;
+
+ @InjectMocks
+ private C2HttpClient c2HttpClient;
+
+ private MockWebServer mockWebServer;
+
+ private String baseUrl;
+
+ @BeforeEach
+ public void startServer() {
+ mockWebServer = new MockWebServer();
+ baseUrl = mockWebServer.url("/").newBuilder().host("localhost").build().toString();
+ }
+
+ @AfterEach
+ public void shutdownServer() throws IOException {
+ mockWebServer.shutdown();
+ }
+
+ @Test
+ void testPublishHeartbeatSuccess() throws InterruptedException {
+ C2HeartbeatResponse hbResponse = new C2HeartbeatResponse();
+ mockWebServer.enqueue(new MockResponse().setBody("dummyResponseBody"));
Review Comment:
removed
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/C2ClientServiceTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service;
+
+import static org.mockito.ArgumentMatchers.any;
+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.util.List;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.nifi.c2.client.api.C2Client;
+import org.apache.nifi.c2.client.service.model.RuntimeInfoWrapper;
+import org.apache.nifi.c2.client.service.operation.C2OperationService;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.C2HeartbeatResponse;
+import org.apache.nifi.c2.protocol.api.C2Operation;
+import org.apache.nifi.c2.protocol.api.C2OperationAck;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class C2ClientServiceTest {
+
+ @Mock
+ private C2Client client;
+
+ @Mock
+ private C2HeartbeatFactory c2HeartbeatFactory;
+
+ @Mock
+ private C2OperationService operationService;
+
+ @InjectMocks
+ private C2ClientService c2ClientService;
+
+ @Test
+ void testSendHeartbeatAndAckWhenOperationPresent() {
+ C2Heartbeat heartbeat = mock(C2Heartbeat.class);
+ when(c2HeartbeatFactory.create(any())).thenReturn(heartbeat);
+ C2HeartbeatResponse hbResponse = new C2HeartbeatResponse();
+ hbResponse.setRequestedOperations(generateOperation(1));
+ when(client.publishHeartbeat(heartbeat)).thenReturn(Optional.of(hbResponse));
+ when(operationService.handleOperation(any())).thenReturn(Optional.of(new C2OperationAck()));
+
+ c2ClientService.sendHeartbeat(mock(RuntimeInfoWrapper.class));
Review Comment:
changed
--
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
[GitHub] [nifi] bejancsaba commented on a diff in pull request #6149: NIFI-9908 C2 refactor and test coverage
Posted by GitBox <gi...@apache.org>.
bejancsaba commented on code in PR #6149:
URL: https://github.com/apache/nifi/pull/6149#discussion_r906717150
##########
c2/c2-client-bundle/c2-client-service/src/main/java/org/apache/nifi/c2/client/service/operation/UpdateConfigurationOperationHandler.java:
##########
@@ -36,8 +38,9 @@
public class UpdateConfigurationOperationHandler implements C2OperationHandler {
private static final Logger logger = LoggerFactory.getLogger(UpdateConfigurationOperationHandler.class);
+ private static final Pattern FLOW_ID_PATTERN = Pattern.compile("/.*/.*/.*/(.*)/?.*");
Review Comment:
Unfortunately it is not enforced anywhere for the flow id to be a guid also in the current setup there could be multiple guids on the path so I went with your enhanced matching proposal. thanks.
##########
c2/c2-client-bundle/c2-client-service/src/test/java/org/apache/nifi/c2/client/service/C2HeartbeatFactoryTest.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.nifi.c2.client.service;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.nifi.c2.client.C2ClientConfig;
+import org.apache.nifi.c2.client.service.model.RuntimeInfoWrapper;
+import org.apache.nifi.c2.protocol.api.AgentRepositories;
+import org.apache.nifi.c2.protocol.api.C2Heartbeat;
+import org.apache.nifi.c2.protocol.api.FlowQueueStatus;
+import org.apache.nifi.c2.protocol.component.api.RuntimeManifest;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class C2HeartbeatFactoryTest {
+
+ private static final String AGENT_CLASS = "agentClass";
+ private static final String FLOW_ID = "flowId";
+
+ @Mock
+ private C2ClientConfig clientConfig;
+
+ @Mock
+ private FlowIdHolder flowIdHolder;
+
+ @InjectMocks
+ private C2HeartbeatFactory c2HeartbeatFactory;
+
+ @TempDir
+ private File tempDir;
+
+ @BeforeEach
+ public void setup() {
+ when(clientConfig.getConfDirectory()).thenReturn(tempDir.getAbsolutePath());
+ }
+
+ @Test
+ void testCreateHeartbeat() {
+ when(flowIdHolder.getFlowId()).thenReturn(FLOW_ID);
+ when(clientConfig.getAgentClass()).thenReturn(AGENT_CLASS);
+
+ C2Heartbeat heartbeat = c2HeartbeatFactory.create(mock(RuntimeInfoWrapper.class));
+
+ assertEquals(FLOW_ID, heartbeat.getFlowId());
+ assertEquals(AGENT_CLASS, heartbeat.getAgentClass());
+ }
+
+ @Test
+ void testCreateGeneratesAgentAndDeviceIdIfNotPresent() {
+ C2Heartbeat heartbeat = c2HeartbeatFactory.create(mock(RuntimeInfoWrapper.class));
+
+ assertNotNull(heartbeat.getAgentId());
+ assertNotNull(heartbeat.getDeviceId());
+ }
+
+ @Test
+ void testCreatePopulatesFromRuntimeInfoWrapper() {
+ AgentRepositories repos = new AgentRepositories();
+ RuntimeManifest manifest = new RuntimeManifest();
+ Map<String, FlowQueueStatus> queueStatus = new HashMap<>();
+
+ C2Heartbeat heartbeat = c2HeartbeatFactory.create(new RuntimeInfoWrapper(repos, manifest, queueStatus));
+
+ assertEquals(repos, heartbeat.getAgentInfo().getStatus().getRepositories());
+ assertEquals(manifest, heartbeat.getAgentInfo().getAgentManifest());
+ assertEquals(queueStatus, heartbeat.getFlowInfo().getQueues());
+ }
+
+ @Test
+ void testCreateThrowsExceptionWhenConfDirNotSet() {
+ when(clientConfig.getConfDirectory()).thenReturn("dummy");
Review Comment:
applied
--
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