tpalfy commented on code in PR #10079:
URL: https://github.com/apache/nifi/pull/10079#discussion_r2198064947
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/java/org/apache/nifi/web/StandardNiFiServiceFacadeTest.java:
##########
@@ -1076,13 +1076,90 @@ public void testGetRuleViolationsForGroupIsRecursive()
throws Exception {
grandChildRuleViolation1, grandChildRuleViolation2,
grandChildRuleViolation3
));
+ when(processGroupDAO.getProcessGroup("root")).thenReturn(processGroup);
+ reset(ruleViolationsManager);
+ when(ruleViolationsManager.isEmpty()).thenReturn(false);
+
when(ruleViolationsManager.getAllRuleViolations()).thenReturn(expected);
+
+ // WHEN
+ Collection<RuleViolation> actual =
serviceFacade.getRuleViolationStream(processGroup.getIdentifier()).collect(Collectors.toSet());
+
+ // THEN
+ assertEquals(expected, actual);
+ }
+
+ @Test
+ public void testGetRuleViolationsEmpty() throws Exception {
+ // GIVEN
+ String groupId = "groupId";
+
+ ProcessGroup processGroup = mockProcessGroup(
+ groupId,
+ Arrays.asList(),
+ Arrays.asList()
+ );
+
+ Collection<RuleViolation> expected = new HashSet<>(List.of());
+
+ when(ruleViolationsManager.isEmpty()).thenReturn(true);
+
// WHEN
Collection<RuleViolation> actual =
serviceFacade.getRuleViolationStream(processGroup.getIdentifier()).collect(Collectors.toSet());
// THEN
assertEquals(expected, actual);
}
+ @Test
+ public void testGetRuleViolationsForGroupIsRecursive() throws Exception {
+ // GIVEN
+ int ruleViolationCounter = 0;
+
+ String groupId = "groupId";
+ String childGroupId = "childGroupId";
+ String grandChildGroupId = "grandChildGroupId";
+
+ RuleViolation ruleViolation1 = createRuleViolation(groupId,
ruleViolationCounter++);
+ RuleViolation ruleViolation2 = createRuleViolation(groupId,
ruleViolationCounter++);
+
+ RuleViolation childRuleViolation1 = createRuleViolation(childGroupId,
ruleViolationCounter++);
+ RuleViolation childRuleViolation2 = createRuleViolation(childGroupId,
ruleViolationCounter++);
+
+ RuleViolation grandChildRuleViolation1 =
createRuleViolation(grandChildGroupId, ruleViolationCounter++);
+ RuleViolation grandChildRuleViolation2 =
createRuleViolation(grandChildGroupId, ruleViolationCounter++);
+ RuleViolation grandChildRuleViolation3 =
createRuleViolation(grandChildGroupId, ruleViolationCounter++);
+
+ ProcessGroup grandChildProcessGroup = mockProcessGroup(
+ grandChildGroupId,
+ Collections.emptyList(),
+ Arrays.asList(grandChildRuleViolation1,
grandChildRuleViolation2, grandChildRuleViolation3)
+ );
+ ProcessGroup childProcessGroup = mockProcessGroup(
+ childGroupId,
+ Arrays.asList(grandChildProcessGroup),
+ Arrays.asList(childRuleViolation1, childRuleViolation2)
+ );
+ ProcessGroup processGroup = mockProcessGroup(
+ groupId,
+ Arrays.asList(childProcessGroup),
+ Arrays.asList(ruleViolation1, ruleViolation2)
+ );
+
+ Collection<RuleViolation> expected = new HashSet<>(Arrays.asList(
+ childRuleViolation1, childRuleViolation2,
+ grandChildRuleViolation1, grandChildRuleViolation2,
grandChildRuleViolation3
+ ));
+
+
when(processGroupDAO.getProcessGroup(groupId)).thenReturn(processGroup);
+ when(ruleViolationsManager.isEmpty()).thenReturn(false);
Review Comment:
```suggestion
```
These are not needed. The first is taken care of by the `mockProcessGroup`
call.
The second is default behaviour for mocks. Just increases boilerplate.
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/StandardNiFiServiceFacade.java:
##########
@@ -6846,18 +6846,25 @@ public FlowAnalysisResultEntity
getFlowAnalysisResult(String processGroupId) {
}
public Stream<RuleViolation> getRuleViolationStream(String processGroupId)
{
- ProcessGroup processGroup =
processGroupDAO.getProcessGroup(processGroupId);
-
- Collection<RuleViolation> ruleViolations =
ruleViolationsManager.getRuleViolationsForGroup(processGroupId);
-
- Stream<RuleViolation> ruleViolationStreamForGroupAndAllChildren =
Stream.concat(
- ruleViolations.stream(),
- processGroup.getProcessGroups().stream()
- .map(ProcessGroup::getIdentifier)
- .flatMap(this::getRuleViolationStream)
- );
-
- return ruleViolationStreamForGroupAndAllChildren;
+ if (ruleViolationsManager.isEmpty())
Review Comment:
Do we need the comments? The code looks clear enough. (Maintaining comments
is not only a hassle but doomed to degradation. Even local variables (with
descriptive names) are better in that regard because - being part of the code -
they are less likely to degrade.)
I personally prefer a single return statement but not a dealbreaker for me.
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/java/org/apache/nifi/web/StandardNiFiServiceFacadeTest.java:
##########
@@ -1076,13 +1076,90 @@ public void testGetRuleViolationsForGroupIsRecursive()
throws Exception {
grandChildRuleViolation1, grandChildRuleViolation2,
grandChildRuleViolation3
));
+ when(processGroupDAO.getProcessGroup("root")).thenReturn(processGroup);
+ reset(ruleViolationsManager);
+ when(ruleViolationsManager.isEmpty()).thenReturn(false);
+
when(ruleViolationsManager.getAllRuleViolations()).thenReturn(expected);
+
+ // WHEN
+ Collection<RuleViolation> actual =
serviceFacade.getRuleViolationStream(processGroup.getIdentifier()).collect(Collectors.toSet());
Review Comment:
Shouldn't we have a case for when we call `getRuleViolationStream` with
`FlowManager.ROOT_GROUP_ID_ALIAS` instead of an ID?
```java
// WHEN
Collection<RuleViolation> actual =
serviceFacade.getRuleViolationStream(FlowManager.ROOT_GROUP_ID_ALIAS).collect(Collectors.toSet());
```
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/java/org/apache/nifi/web/StandardNiFiServiceFacadeTest.java:
##########
@@ -1076,13 +1076,90 @@ public void testGetRuleViolationsForGroupIsRecursive()
throws Exception {
grandChildRuleViolation1, grandChildRuleViolation2,
grandChildRuleViolation3
));
+ when(processGroupDAO.getProcessGroup("root")).thenReturn(processGroup);
Review Comment:
```suggestion
when(processGroupDAO.getProcessGroup(FlowManager.ROOT_GROUP_ID_ALIAS)).thenReturn(processGroup);
```
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/java/org/apache/nifi/web/StandardNiFiServiceFacadeTest.java:
##########
@@ -1076,13 +1076,90 @@ public void testGetRuleViolationsForGroupIsRecursive()
throws Exception {
grandChildRuleViolation1, grandChildRuleViolation2,
grandChildRuleViolation3
));
+ when(processGroupDAO.getProcessGroup("root")).thenReturn(processGroup);
+ reset(ruleViolationsManager);
+ when(ruleViolationsManager.isEmpty()).thenReturn(false);
Review Comment:
These are not needed I think.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
