NIFI-6872: support download flow #3931

jsferner · 2019-12-12T19:25:48Z

NIFI-6872:

Added UI versioned flow supportsDownload functionality with download flow menu item
Added VersionsResource endpoint for downloading versioned flow with registry-related info removed (not currently exposed in the UI since decision was made to support any flow for download, whether versioned or not)
Added ProcessGroupResource endpoint for downloading current flow with registry-related info removed
Added StandardNifiServiceFacade functionality for downloading both current and versioned flow
Added XmlTransient markers on variables introduced by Instantiated model classes so they do not appear in serialized download
Updated NiFiRegistryFlowMapper.mapParameterContexts to handle mapping nested parameter contexts for use in producing a complete VersionedFlowSnapshot
Added ability for NiFiRegistryFlowMapper to map nested process groups ignoring versioning for use in producing a complete VersionedFlowSnapshot
Added unit tests where helpful

- Added UI versioned flow supportsDownload functionality with download flow menu item - Added VersionsResource endpoint for downloading versioned flow with registry-related info removed - Added ProcessGroupResource endpoint for downloading current flow with registry-related info removed - Added StandardNifiServiceFacade functionality for downloading both current and versioned flow - Added XmlTransient markers on variables introduced by Instantiated model classes so they do not appear in serialized download - Updated NiFiRegistryFlowMapper.mapParameterContexts to handle mapping nested parameter contexts for use in producing a complete VersionedFlowSnapshot - Added ability for NiFiRegistryFlowMapper to map nested process groups ignoring versioning for use in producing a complete VersionedFlowSnapshot - Added unit tests where helpful

markap14

Hey @jsferner thanks for the contribution! This is awesome! I made a few minor stylistic comments but those are not a really big deal. You can ignore them if you disagree. The only thing I think needs to be changed is the comment made in the StandardNiFiServiceFacade regarding mapping of the Collection to a Map, which can contain duplicate keys and cause an IllegalStateException. Thanks for an awesome contribution!

markap14 · 2019-12-13T15:16:30Z

...amework-core/src/main/java/org/apache/nifi/registry/flow/mapping/NiFiRegistryFlowMapper.java

-        }
-    }
+        // apply registry versioning according to the lambda below
+        // NOTE: lambda refers to registry client and map descendant boolean which will not change during recursion


This might read more clearly if this lambda were instead created as a method: boolean isMapDescendants(ProcessGroup processGroup, VersionedProcessGroup versionedGroup) and then referenced here as return mapGroup(group, serviceProvider, this::isMapDescendants). I think this would avoid having a lengthy lambda and obviate the need for documentation explaining the point of the lambda.

I was going to make the lambda a separate method but it actually would require taking in 2 more arguments because it's currently referencing those arguments locally where it's defined inline (registryClient and mapDescendantVersionedFlows). The values of these args don't change during recursion, they just get passed down through so it made more sense to just reference them in the lambda rather than expect them to also be passed in when it's called. Does that make sense?

markap14 · 2019-12-13T15:26:28Z

...amework-core/src/main/java/org/apache/nifi/registry/flow/mapping/NiFiRegistryFlowMapper.java

-        final Set<VersionedParameter> parameters = context.getParameters().values().stream()
-            .map(this::mapParameter)
-            .collect(Collectors.toSet());
+    public Collection<VersionedParameterContext> mapParameterContexts(final ProcessGroup processGroup, final boolean mapDescendantVersionedFlows) {


Am thinking out loud a bit here. I don't yet fully understand the entire context around where this will be used. But it seems to me anywhere that we would want a Collection of VersionedParameterContext it would probably make sense to restrict it to a Set<VersionedParameterContext>. Because we can certainly have the case where descendant Process Groups will reference the same ParameterContext as a parent/ancestor Process Group. I don't think we would actually want the context in the Collection multiple times, would we?

Agreed, moving everything to a Map to begin with, as we discussed

markap14 · 2019-12-13T15:29:50Z

...ork-core/src/test/java/org/apache/nifi/registry/flow/mapping/NiFiRegistryFlowMapperTest.java

+                                             final boolean includeRemoteProcessGroup, final boolean includeVersionControlInfo,
+                                             final List<ProcessGroup> childProcessGroups) {
+        final String processGroupId = UUID.randomUUID().toString();
+        final ProcessGroup processGroup = mock(ProcessGroup.class);


It looks like there's quite a lot of mocking going on here for the ProcessGroup. There already exists a MockProcessGroup class that may simplify this quite a bit.

MockProcessGroup was limited in what it was implementing already so I couldn't use it without adding a ton more implementation. Since this mapper exercises almost all the elements of a process group, it was going to take quite a bit to make the MockProcessGroup usable.

markap14 · 2019-12-13T15:32:49Z

...ork-core/src/test/java/org/apache/nifi/registry/flow/mapping/NiFiRegistryFlowMapperTest.java

+        final String expectedGroupIdentifier = flowMapper.getGroupId(processGroup.getProcessGroupIdentifier());
+
+        // verify process group fields
+        assertEquals(versionedProcessGroup.getName(), processGroup.getName());


Not a big deal, but I think all of these asserts are passing the arguments backwards. The signature is assertEquals(Object expected, Object actual) but it looks like all of these assertions are made such that the actual value is passed before the expected value.

Agreed, updating all

markap14 · 2019-12-13T15:38:32Z

...ework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/StandardNiFiServiceFacade.java

+        // Create a complete (include descendant flows) map of parameter contexts
+        final Collection<VersionedParameterContext> parameterContexts =
+                mapper.mapParameterContexts(processGroup, true);
+        final Map<String, VersionedParameterContext> parameterContextMap = parameterContexts.stream()


See comment above, about the creation of Collection<VersionedParameterContext> with regards to changing to a Set<VersionedParameterContext>. As this is currently written, if we have Group A with Parameter Context A, and a child group, Group B, also with Parameter Context A, this will result in the Collection<VersionedParameterContext> containing two VersionedParameterContext objects with the same name. This line of code will now throw an IllegalStateException because it will result in the Map that is being formed having duplicate keys.

Agreed, moving everything to a Map as we discussed

markap14 · 2019-12-13T15:58:48Z

Also noticed that Travis flagged 1 checkstyle violation:
[WARNING] src/main/java/org/apache/nifi/registry/flow/mapping/NiFiRegistryFlowMapper.java:[115,83] (blocks) LeftCurly: '{' at column 83 should have line break after.

jsferner · 2019-12-13T18:22:29Z

Fixed checkstyle failure

mcgilman · 2019-12-16T13:56:28Z

Will also have a look to review the front end changes...

mcgilman · 2019-12-16T16:18:43Z

...k-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-flow-version.js

+         *
+         * @param processGroupId
+         */
+        downloadFlowVersion: function (processGroupId) {


This does not appear to be invoked. I'd recommend removing it as it would need to be updated to support a One Time Password if we wanted to leave it in place.

mcgilman · 2019-12-16T16:21:52Z

...mework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-actions.js

+            }
+
+            if (processGroupId !== null) {
+                window.open('../nifi-api/process-groups/' + encodeURIComponent(processGroupId) + '/download');


For when the user is authenticated and leveraging a token that is presented in a HTTP header, we need to update this endpoint to utilize a One Time Password which is included in a query parameter. This is necessary because the code that adds the token to the header down not execute within the new window from the window.open call.

Please check out provenance download or content download for an example.

- Updated mapParameterContext to return a Map to handle uniqueness of contexts by name since ultimately everything converted it to a map anyway. The VersionedParameterContext class from the registry model doesn't support hashcode/equals currently so returning a Set wouldn't work. - Updated assert calls to put expected value as first parameter and actual as second parameter - Added one time password (OTP) support for flow download endpoint to support non cert based authentication

markap14 · 2019-12-18T18:25:45Z

Thanks for the update @jsferner ! The code looks good from my perspective. I haven't had a chance yet to test with the update, though. Once I can test and verify behavior I'm a +1. Or if you have a chance to verify behavior, @mcgilman, feel free to merge it in if you're happy with the update as well.

mcgilman · 2019-12-18T21:39:35Z

Thanks @jsferner for the PR. This has been merged to master!
Thanks @markap14 for the review help!

- Added UI versioned flow supportsDownload functionality with download flow menu item - Added VersionsResource endpoint for downloading versioned flow with registry-related info removed - Added ProcessGroupResource endpoint for downloading current flow with registry-related info removed - Added StandardNifiServiceFacade functionality for downloading both current and versioned flow - Added XmlTransient markers on variables introduced by Instantiated model classes so they do not appear in serialized download - Updated NiFiRegistryFlowMapper.mapParameterContexts to handle mapping nested parameter contexts for use in producing a complete VersionedFlowSnapshot - Added ability for NiFiRegistryFlowMapper to map nested process groups ignoring versioning for use in producing a complete VersionedFlowSnapshot - Added unit tests where helpful NIFI-6872: PR response... - Updated mapParameterContext to return a Map to handle uniqueness of contexts by name since ultimately everything converted it to a map anyway. The VersionedParameterContext class from the registry model doesn't support hashcode/equals currently so returning a Set wouldn't work. - Updated assert calls to put expected value as first parameter and actual as second parameter - Added one time password (OTP) support for flow download endpoint to support non cert based authentication This closes apache#3931

jsferner force-pushed the NIFI-6872 branch from a8c5241 to 980e034 Compare December 12, 2019 19:46

markap14 requested changes Dec 13, 2019

View reviewed changes

mcgilman reviewed Dec 16, 2019

View reviewed changes

jsferner force-pushed the NIFI-6872 branch from eb6af53 to dc815e6 Compare December 17, 2019 18:06

asfgit closed this in 0588830 Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-6872: support download flow #3931

NIFI-6872: support download flow #3931

jsferner commented Dec 12, 2019

markap14 left a comment

markap14 Dec 13, 2019

jsferner Dec 13, 2019

markap14 Dec 13, 2019

jsferner Dec 13, 2019

markap14 Dec 13, 2019

jsferner Dec 13, 2019

markap14 Dec 13, 2019

jsferner Dec 13, 2019

markap14 Dec 13, 2019

jsferner Dec 13, 2019

markap14 commented Dec 13, 2019

jsferner commented Dec 13, 2019

mcgilman commented Dec 16, 2019

mcgilman Dec 16, 2019

mcgilman Dec 16, 2019

markap14 commented Dec 18, 2019

mcgilman commented Dec 18, 2019

NIFI-6872: support download flow #3931

NIFI-6872: support download flow #3931

Conversation

jsferner commented Dec 12, 2019

markap14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markap14 commented Dec 13, 2019

jsferner commented Dec 13, 2019

mcgilman commented Dec 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markap14 commented Dec 18, 2019

mcgilman commented Dec 18, 2019