gresockj commented on a change in pull request #5086:
URL: https://github.com/apache/nifi/pull/5086#discussion_r637882602



##########
File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/SnippetUtils.java
##########
@@ -299,62 +315,10 @@ public FlowSnippetDTO populateFlowSnippet(final Snippet 
snippet, final boolean r
         return snippetDto;
     }
 
-    /**
-     * Finds all Controller Services that are referenced in the given Process 
Group (and child Process Groups, recursively), and
-     * adds them to the given servicesByGroup map
-     *
-     * @param group the Process Group to start from
-     * @param dto the DTO representation of the Process Group
-     * @param allServicesReferenced a Set of all Controller Service DTO's that 
have already been referenced; used to dedupe services
-     * @param contentsByGroup a Map of Process Group ID to the Process Group's 
contents
-     * @param highestGroupId the UUID of the 'highest' process group in the 
snippet
-     */
-    private void addControllerServices(final ProcessGroup group, final 
ProcessGroupDTO dto, final Set<ControllerServiceDTO> allServicesReferenced,
-        final boolean includeControllerServices, final Set<String> 
visitedGroupIds, final Map<String, FlowSnippetDTO> contentsByGroup, final 
String highestGroupId) {
-
-        final FlowSnippetDTO contents = dto.getContents();
-        contentsByGroup.put(dto.getId(), contents);
-        if (contents == null) {
-            return;
-        }
-
-        // include this group in the ancestry for this snippet, services only 
get included if the includeControllerServices
-        // flag is set or if the service is defined within this groups 
hierarchy within the snippet
-        visitedGroupIds.add(group.getIdentifier());
-
-        for (final ProcessorNode procNode : group.getProcessors()) {
-            // Include all referenced services that are not already included 
in this snippet.
-            
getControllerServices(procNode.getEffectivePropertyValues()).stream()
-                .filter(allServicesReferenced::add)
-                .filter(svc -> includeControllerServices || 
visitedGroupIds.contains(svc.getParentGroupId()))
-                .forEach(svc -> {
-                    final String svcGroupId = svc.getParentGroupId();
-                    final String destinationGroupId = 
contentsByGroup.containsKey(svcGroupId) ? svcGroupId : highestGroupId;
-                    svc.setParentGroupId(destinationGroupId);
-                    final FlowSnippetDTO snippetDto = 
contentsByGroup.get(destinationGroupId);
-                    if (snippetDto != null) {
-                        Set<ControllerServiceDTO> services = 
snippetDto.getControllerServices();
-                        if (services == null) {
-                            
snippetDto.setControllerServices(Collections.singleton(svc));
-                        } else {
-                            services.add(svc);
-                            snippetDto.setControllerServices(services);
-                        }
-                    }
-                });
-        }
-
-        // Map child process group ID to the child process group for easy 
lookup
-        final Map<String, ProcessGroupDTO> childGroupMap = 
contents.getProcessGroups().stream()
-            .collect(Collectors.toMap(ComponentDTO::getId, childGroupDto -> 
childGroupDto));
-
-        for (final ProcessGroup childGroup : group.getProcessGroups()) {
-            final ProcessGroupDTO childDto = 
childGroupMap.get(childGroup.getIdentifier());
-            if (childDto == null) {
-                continue;
-            }
-
-            addControllerServices(childGroup, childDto, allServicesReferenced, 
includeControllerServices, visitedGroupIds, contentsByGroup, highestGroupId);
+    private void fillContentsByGroupMap(ProcessGroupDTO processGroup, 
Map<String, FlowSnippetDTO> contentByGroupMap) {
+        for (ProcessGroupDTO group: 
processGroup.getContents().getProcessGroups()) {

Review comment:
       final group

##########
File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/SnippetUtils.java
##########
@@ -235,23 +234,41 @@ public FlowSnippetDTO populateFlowSnippet(final Snippet 
snippet, final boolean r
         }
 
         // add any process groups
-        final Set<ProcessGroupDTO> processGroups = new LinkedHashSet<>();
-        if (!snippet.getProcessGroups().isEmpty()) {
-            for (final String childGroupId : 
snippet.getProcessGroups().keySet()) {
-                final ProcessGroup childGroup = 
processGroup.getProcessGroup(childGroupId);
-                if (childGroup == null) {
-                    throw new IllegalStateException("A process group in this 
snippet could not be found.");
-                }
-
-                final ProcessGroupDTO childGroupDto = 
dtoFactory.createProcessGroupDto(childGroup, recurse);
-                processGroups.add(childGroupDto);
-
-                // maintain a listing of visited groups starting with each 
group in the snippet. this is used to determine
-                // whether a referenced controller service should be included 
in the resulting snippet. if the service is
-                // defined at groupId or one of it's ancestors, its considered 
outside of this snippet and will only be included
-                // when the includeControllerServices is set to true. this 
happens above when considering the processors in this snippet
-                final Set<String> visitedGroupIds = new HashSet<>();
-                addControllerServices(childGroup, childGroupDto, 
allServicesReferenced, includeControllerServices, visitedGroupIds, 
contentsByGroup, processGroup.getIdentifier());
+        final ProcessGroupDTO highestProcessGroupDTO = 
dtoFactory.createProcessGroupDto(processGroup, recurse);
+        final Set<ProcessGroupDTO> processGroups = 
highestProcessGroupDTO.getContents().getProcessGroups();
+        fillContentsByGroupMap(highestProcessGroupDTO, contentsByGroup);
+
+        // maintain a listing of visited groups starting with each group in 
the snippet. this is used to determine

Review comment:
       For style purposes, can we capitalize each sentence?

##########
File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/SnippetUtils.java
##########
@@ -299,62 +315,10 @@ public FlowSnippetDTO populateFlowSnippet(final Snippet 
snippet, final boolean r
         return snippetDto;
     }
 
-    /**
-     * Finds all Controller Services that are referenced in the given Process 
Group (and child Process Groups, recursively), and
-     * adds them to the given servicesByGroup map
-     *
-     * @param group the Process Group to start from
-     * @param dto the DTO representation of the Process Group
-     * @param allServicesReferenced a Set of all Controller Service DTO's that 
have already been referenced; used to dedupe services
-     * @param contentsByGroup a Map of Process Group ID to the Process Group's 
contents
-     * @param highestGroupId the UUID of the 'highest' process group in the 
snippet
-     */
-    private void addControllerServices(final ProcessGroup group, final 
ProcessGroupDTO dto, final Set<ControllerServiceDTO> allServicesReferenced,
-        final boolean includeControllerServices, final Set<String> 
visitedGroupIds, final Map<String, FlowSnippetDTO> contentsByGroup, final 
String highestGroupId) {
-
-        final FlowSnippetDTO contents = dto.getContents();
-        contentsByGroup.put(dto.getId(), contents);
-        if (contents == null) {
-            return;
-        }
-
-        // include this group in the ancestry for this snippet, services only 
get included if the includeControllerServices
-        // flag is set or if the service is defined within this groups 
hierarchy within the snippet
-        visitedGroupIds.add(group.getIdentifier());
-
-        for (final ProcessorNode procNode : group.getProcessors()) {
-            // Include all referenced services that are not already included 
in this snippet.
-            
getControllerServices(procNode.getEffectivePropertyValues()).stream()
-                .filter(allServicesReferenced::add)
-                .filter(svc -> includeControllerServices || 
visitedGroupIds.contains(svc.getParentGroupId()))
-                .forEach(svc -> {
-                    final String svcGroupId = svc.getParentGroupId();
-                    final String destinationGroupId = 
contentsByGroup.containsKey(svcGroupId) ? svcGroupId : highestGroupId;
-                    svc.setParentGroupId(destinationGroupId);
-                    final FlowSnippetDTO snippetDto = 
contentsByGroup.get(destinationGroupId);
-                    if (snippetDto != null) {
-                        Set<ControllerServiceDTO> services = 
snippetDto.getControllerServices();
-                        if (services == null) {
-                            
snippetDto.setControllerServices(Collections.singleton(svc));
-                        } else {
-                            services.add(svc);
-                            snippetDto.setControllerServices(services);
-                        }
-                    }
-                });
-        }
-
-        // Map child process group ID to the child process group for easy 
lookup
-        final Map<String, ProcessGroupDTO> childGroupMap = 
contents.getProcessGroups().stream()
-            .collect(Collectors.toMap(ComponentDTO::getId, childGroupDto -> 
childGroupDto));
-
-        for (final ProcessGroup childGroup : group.getProcessGroups()) {
-            final ProcessGroupDTO childDto = 
childGroupMap.get(childGroup.getIdentifier());
-            if (childDto == null) {
-                continue;
-            }
-
-            addControllerServices(childGroup, childDto, allServicesReferenced, 
includeControllerServices, visitedGroupIds, contentsByGroup, highestGroupId);
+    private void fillContentsByGroupMap(ProcessGroupDTO processGroup, 
Map<String, FlowSnippetDTO> contentByGroupMap) {

Review comment:
       We can declare these parameters as final

##########
File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/SnippetUtils.java
##########
@@ -235,23 +234,41 @@ public FlowSnippetDTO populateFlowSnippet(final Snippet 
snippet, final boolean r
         }
 
         // add any process groups
-        final Set<ProcessGroupDTO> processGroups = new LinkedHashSet<>();
-        if (!snippet.getProcessGroups().isEmpty()) {
-            for (final String childGroupId : 
snippet.getProcessGroups().keySet()) {
-                final ProcessGroup childGroup = 
processGroup.getProcessGroup(childGroupId);
-                if (childGroup == null) {
-                    throw new IllegalStateException("A process group in this 
snippet could not be found.");
-                }
-
-                final ProcessGroupDTO childGroupDto = 
dtoFactory.createProcessGroupDto(childGroup, recurse);
-                processGroups.add(childGroupDto);
-
-                // maintain a listing of visited groups starting with each 
group in the snippet. this is used to determine
-                // whether a referenced controller service should be included 
in the resulting snippet. if the service is
-                // defined at groupId or one of it's ancestors, its considered 
outside of this snippet and will only be included
-                // when the includeControllerServices is set to true. this 
happens above when considering the processors in this snippet
-                final Set<String> visitedGroupIds = new HashSet<>();
-                addControllerServices(childGroup, childGroupDto, 
allServicesReferenced, includeControllerServices, visitedGroupIds, 
contentsByGroup, processGroup.getIdentifier());
+        final ProcessGroupDTO highestProcessGroupDTO = 
dtoFactory.createProcessGroupDto(processGroup, recurse);
+        final Set<ProcessGroupDTO> processGroups = 
highestProcessGroupDTO.getContents().getProcessGroups();
+        fillContentsByGroupMap(highestProcessGroupDTO, contentsByGroup);
+
+        // maintain a listing of visited groups starting with each group in 
the snippet. this is used to determine
+        // whether a referenced controller service should be included in the 
resulting snippet. if the service is
+        // defined at groupId or one of it's ancestors, its considered outside 
of this snippet and will only be included
+        // when the includeControllerServices is set to true. this happens 
above when considering the processors in this snippet
+        final Set<String> visitedGroupIds = new HashSet<>();
+        for (String groupIdentifier : contentsByGroup.keySet()) {

Review comment:
       Let's declare groupIdentifier as final




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

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to